From 7777ef83dd194617e51c975484d09875354d03ac Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Fri, 10 Feb 2023 12:34:47 +0100 Subject: [PATCH] adopt errorViewModel in GapTile to report errors --- .../session/room/timeline/tiles/GapTile.js | 83 +++++++------------ .../web/ui/css/themes/element/error.css | 11 ++- .../web/ui/css/themes/element/timeline.css | 2 +- src/platform/web/ui/css/timeline.css | 4 +- .../web/ui/session/room/timeline/GapView.js | 12 ++- 5 files changed, 49 insertions(+), 63 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index 1e6bdd08..58f0b3a6 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -24,47 +24,40 @@ export class GapTile extends SimpleTile { constructor(entry, options) { super(entry, options); this._loading = false; - this._error = null; + this._waitingForConnection = false; this._isAtTop = true; this._siblingChanged = false; - this._showSpinner = false; } get needsDateSeparator() { return false; } - async fill() { + async fill(isRetrying = false) { if (!this._loading && !this._entry.edgeReached) { this._loading = true; - this._error = null; - this._showSpinner = true; this.emitChange("isLoading"); try { await this._room.fillGap(this._entry, 10); } catch (err) { - console.error(`room.fillGap(): ${err.message}:\n${err.stack}`); - this._error = err; if (err instanceof ConnectionError) { - this.emitChange("error"); - /* - We need to wait for reconnection here rather than in - notifyVisible() because when we return/throw here - this._loading is set to false and other queued invocations of - this method will succeed and attempt further room.fillGap() calls - - resulting in multiple error entries in logs and elsewhere! - */ await this._waitForReconnection(); + if (!isRetrying) { + // retry after the connection comes back + // if this wasn't already a retry after coming back online + return await this.fill(true); + } else { + return false; + } + } else { + this.reportError(err); + return false; } - // rethrow so caller of this method - // knows not to keep calling this for now - throw err; } finally { this._loading = false; - this._showSpinner = false; this.emitChange("isLoading"); } - return true; + return true; } return false; } @@ -76,19 +69,7 @@ export class GapTile extends SimpleTile { let canFillMore; this._siblingChanged = false; do { - try { - canFillMore = await this.fill(); - } - catch (e) { - if (e instanceof ConnectionError) { - canFillMore = true; - // Don't increase depth because this gap fill was a noop - continue; - } - else { - canFillMore = false; - } - } + canFillMore = await this.fill(); depth = depth + 1; } while (depth < 10 && !this._siblingChanged && canFillMore && !this.isDisposed); } @@ -124,7 +105,11 @@ export class GapTile extends SimpleTile { } async _waitForReconnection() { + this._waitingForConnection = true; + this.emitUpdate("status"); await this.options.client.reconnector.connectionStatus.waitFor(status => status === ConnectionStatus.Online).promise; + this._waitingForConnection = false; + this.emitUpdate("status"); } get shape() { @@ -136,29 +121,19 @@ export class GapTile extends SimpleTile { } get showSpinner() { - return this._showSpinner; + return this.isLoading || this._waitingForConnection; } - get error() { - if (this._error) { - if (this._error instanceof ConnectionError) { - return "Waiting for reconnection"; - } - const dir = this._entry.prev_batch ? "previous" : "next"; - return `Could not load ${dir} messages: ${this._error.message}`; - } - return null; - } - - get currentAction() { - if (this.error) { - return this.error; - } - else if (this.isLoading) { - return "Loading"; - } - else { - return "Not Loading"; + get status() { + const dir = this._entry.prev_batch ? "previous" : "next"; + if (this._waitingForConnection) { + return "Waiting for connection…"; + } else if (this.errorViewModel) { + return `Could not load ${dir} messages`; + } else if (this.isLoading) { + return "Loading more messages…"; + } else { + return "Gave up loading more messages"; } } } diff --git a/src/platform/web/ui/css/themes/element/error.css b/src/platform/web/ui/css/themes/element/error.css index ce252056..e0b945ca 100644 --- a/src/platform/web/ui/css/themes/element/error.css +++ b/src/platform/web/ui/css/themes/element/error.css @@ -4,9 +4,14 @@ margin: 16px; } -.ErrorView_inline { +.ErrorView.ErrorView_inline { color: var(--error-color); - margin: 4px; + margin: 4px 0; + padding: 4px 0; +} + +.ErrorView.ErrorView_inline > p { + margin: 0; } .ErrorView { @@ -48,4 +53,4 @@ .ErrorView_inline .ErrorView_close { background-image: url('icons/clear.svg?primary=text-color'); -} \ No newline at end of file +} diff --git a/src/platform/web/ui/css/themes/element/timeline.css b/src/platform/web/ui/css/themes/element/timeline.css index 8606400d..4a822605 100644 --- a/src/platform/web/ui/css/themes/element/timeline.css +++ b/src/platform/web/ui/css/themes/element/timeline.css @@ -411,7 +411,7 @@ only loads when the top comes into view*/ border-radius: 10px; } -.GapView > :not(:first-child) { +.GapView_container > :not(:first-child) { margin-left: 12px; } diff --git a/src/platform/web/ui/css/timeline.css b/src/platform/web/ui/css/timeline.css index b6d62c98..0932739c 100644 --- a/src/platform/web/ui/css/timeline.css +++ b/src/platform/web/ui/css/timeline.css @@ -52,11 +52,11 @@ limitations under the License. align-items: center; } -.GapView { +.GapView_container { display: flex; } -.GapView > :nth-child(2) { +.GapView_container > span { flex: 1; } diff --git a/src/platform/web/ui/session/room/timeline/GapView.js b/src/platform/web/ui/session/room/timeline/GapView.js index 4fc0e3d6..91181c09 100644 --- a/src/platform/web/ui/session/room/timeline/GapView.js +++ b/src/platform/web/ui/session/room/timeline/GapView.js @@ -16,6 +16,7 @@ limitations under the License. import {TemplateView} from "../../../general/TemplateView"; import {spinner} from "../../../common.js"; +import {ErrorView} from "../../../general/ErrorView"; export class GapView extends TemplateView { // ignore other argument @@ -23,15 +24,20 @@ export class GapView extends TemplateView { super(vm); } - render(t) { + render(t, vm) { const className = { GapView: true, isLoading: vm => vm.isLoading, isAtTop: vm => vm.isAtTop, }; return t.li({ className }, [ - t.if(vm => vm.showSpinner, (t) => spinner(t)), - t.span(vm => vm.currentAction) + t.div({class: "GapView_container"}, [ + t.if(vm => vm.showSpinner, (t) => spinner(t)), + t.span(vm => vm.status), + ]), + t.if(vm => !!vm.errorViewModel, t => { + return t.view(new ErrorView(vm.errorViewModel, {inline: true})); + }) ]); }