From b1fd5f1ad530a2d1f3eb38cfa4f13aa073a31ea3 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Thu, 4 Aug 2022 16:33:59 +0530 Subject: [PATCH 1/9] Do not fill gap when offline --- src/domain/session/room/timeline/tiles/GapTile.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index 6caa4b9b..e2e5b041 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -16,6 +16,8 @@ limitations under the License. import {SimpleTile} from "./SimpleTile.js"; import {UpdateAction} from "../UpdateAction.js"; +import {ConnectionError} from "../../../../../matrix/error.js"; +import {ConnectionStatus} from "../../../../../matrix/net/Reconnector"; export class GapTile extends SimpleTile { constructor(entry, options) { @@ -29,6 +31,7 @@ export class GapTile extends SimpleTile { async fill() { if (!this._loading && !this._entry.edgeReached) { this._loading = true; + this._error = null; this.emitChange("isLoading"); try { await this._room.fillGap(this._entry, 10); @@ -55,7 +58,15 @@ export class GapTile extends SimpleTile { let canFillMore; this._siblingChanged = false; do { - canFillMore = await this.fill(); + try { + canFillMore = await this.fill(); + } + catch (e) { + if (e instanceof ConnectionError) { + await this.options.client.reconnector.connectionStatus.waitFor(status => status === ConnectionStatus.Online).promise; + canFillMore = true; + } + } depth = depth + 1; } while (depth < 10 && !this._siblingChanged && canFillMore && !this.isDisposed); } From d01a95aae3da88577fab0b74d209b3ae2460cffd Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Thu, 4 Aug 2022 16:37:28 +0530 Subject: [PATCH 2/9] UI improvements --- .../session/room/timeline/tiles/GapTile.js | 5 ++++- .../web/ui/css/themes/element/timeline.css | 9 ++++++++ .../web/ui/session/room/timeline/GapView.js | 22 +++++++++++++++---- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index e2e5b041..9bebb7b5 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -111,8 +111,11 @@ export class GapTile extends SimpleTile { get error() { if (this._error) { + if (this._error instanceof ConnectionError) { + return { message: "Waiting for reconnection", showSpinner: true }; + } const dir = this._entry.prev_batch ? "previous" : "next"; - return `Could not load ${dir} messages: ${this._error.message}`; + return { message: `Could not load ${dir} messages: ${this._error.message}`, showSpinner: false }; } return null; } diff --git a/src/platform/web/ui/css/themes/element/timeline.css b/src/platform/web/ui/css/themes/element/timeline.css index 43c57d19..47f9a365 100644 --- a/src/platform/web/ui/css/themes/element/timeline.css +++ b/src/platform/web/ui/css/themes/element/timeline.css @@ -422,3 +422,12 @@ only loads when the top comes into view*/ .GapView.isAtTop { padding: 52px 20px 12px 20px; } + +.GapView__container { + display: flex; + align-items: center; +} + +.GapView__container .spinner { + margin-right: 10px; +} diff --git a/src/platform/web/ui/session/room/timeline/GapView.js b/src/platform/web/ui/session/room/timeline/GapView.js index db6cda59..03f87fcf 100644 --- a/src/platform/web/ui/session/room/timeline/GapView.js +++ b/src/platform/web/ui/session/room/timeline/GapView.js @@ -29,10 +29,24 @@ export class GapView extends TemplateView { isLoading: vm => vm.isLoading, isAtTop: vm => vm.isAtTop, }; - return t.li({className}, [ - spinner(t), - t.div(vm => vm.isLoading ? vm.i18n`Loading more messages …` : vm.i18n`Not loading!`), - t.if(vm => vm.error, t => t.strong(vm => vm.error)) + return t.li({ className }, [ + t.map(vm => vm.error, + (error, t, vm) => { + let elements; + if (error) { + elements = [t.strong(() => error.message)]; + if (error.showSpinner) { + elements.unshift(spinner(t)); + } + } + else if (vm.isLoading) { + elements = [spinner(t), t.span(vm.i18n`Loading more messages …`)]; + } + else { + elements = t.span(vm.i18n`Not loading!`); + } + return t.div({ className: "GapView__container" }, elements); + }) ]); } From d1c7a792b8cbe34cf4bc77f842e2038615fffe0c Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Sun, 14 Aug 2022 17:43:24 +0530 Subject: [PATCH 3/9] Await in fill method to prevent multiple errors --- .../session/room/timeline/tiles/GapTile.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index 9bebb7b5..65a01988 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -39,6 +39,17 @@ export class GapTile extends SimpleTile { console.error(`room.fillGap(): ${err.message}:\n${err.stack}`); this._error = err; this.emitChange("error"); + if (err instanceof ConnectionError) { + /* + 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(); + return true; + } // rethrow so caller of this method // knows not to keep calling this for now throw err; @@ -63,8 +74,8 @@ export class GapTile extends SimpleTile { } catch (e) { if (e instanceof ConnectionError) { - await this.options.client.reconnector.connectionStatus.waitFor(status => status === ConnectionStatus.Online).promise; - canFillMore = true; + // Don't increase depth because this gap fill was a noop + continue; } } depth = depth + 1; @@ -101,6 +112,10 @@ export class GapTile extends SimpleTile { } } + async _waitForReconnection() { + this.options.client.reconnector.connectionStatus.waitFor(status => status === ConnectionStatus.Online).promise; + } + get shape() { return "gap"; } From 4a62cdb8fbcd5ae3010cc6a485f6ea111af9eded Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Sun, 14 Aug 2022 17:52:19 +0530 Subject: [PATCH 4/9] Await the promise --- src/domain/session/room/timeline/tiles/GapTile.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index 65a01988..f41dd722 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -77,6 +77,9 @@ export class GapTile extends SimpleTile { // Don't increase depth because this gap fill was a noop continue; } + else { + canFillMore = false; + } } depth = depth + 1; } while (depth < 10 && !this._siblingChanged && canFillMore && !this.isDisposed); @@ -113,7 +116,7 @@ export class GapTile extends SimpleTile { } async _waitForReconnection() { - this.options.client.reconnector.connectionStatus.waitFor(status => status === ConnectionStatus.Online).promise; + await this.options.client.reconnector.connectionStatus.waitFor(status => status === ConnectionStatus.Online).promise; } get shape() { From 8e2838264f5fd0e8ba41a667c6da446de69bad97 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Mon, 15 Aug 2022 15:00:31 +0530 Subject: [PATCH 5/9] Run binding when isLoading changes --- src/domain/session/room/timeline/tiles/GapTile.js | 1 + src/platform/web/ui/session/room/timeline/GapView.js | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index f41dd722..3a7f7533 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -38,6 +38,7 @@ export class GapTile extends SimpleTile { } catch (err) { console.error(`room.fillGap(): ${err.message}:\n${err.stack}`); this._error = err; + this._loading = false; this.emitChange("error"); if (err instanceof ConnectionError) { /* diff --git a/src/platform/web/ui/session/room/timeline/GapView.js b/src/platform/web/ui/session/room/timeline/GapView.js index 03f87fcf..87afe503 100644 --- a/src/platform/web/ui/session/room/timeline/GapView.js +++ b/src/platform/web/ui/session/room/timeline/GapView.js @@ -30,16 +30,17 @@ export class GapView extends TemplateView { isAtTop: vm => vm.isAtTop, }; return t.li({ className }, [ - t.map(vm => vm.error, - (error, t, vm) => { + t.map(vm => vm.isLoading, + (isLoading, t, vm) => { let elements; + const error = vm.error; if (error) { elements = [t.strong(() => error.message)]; if (error.showSpinner) { elements.unshift(spinner(t)); } } - else if (vm.isLoading) { + else if (isLoading) { elements = [spinner(t), t.span(vm.i18n`Loading more messages …`)]; } else { From 220144898be390bbeeec68bcb5037f706c63a520 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Wed, 17 Aug 2022 13:13:20 +0530 Subject: [PATCH 6/9] Fix errors and simplify code --- .../session/room/timeline/tiles/GapTile.js | 15 ++++++---- .../web/ui/session/room/timeline/GapView.js | 30 ++++++++----------- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index 3a7f7533..7aae124d 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -26,21 +26,22 @@ export class GapTile extends SimpleTile { this._error = null; this._isAtTop = true; this._siblingChanged = false; + this._showSpinner = false; } async fill() { 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; - this._loading = false; - this.emitChange("error"); if (err instanceof ConnectionError) { + this.emitChange("error"); /* We need to wait for reconnection here rather than in notifyVisible() because when we return/throw here @@ -49,13 +50,13 @@ export class GapTile extends SimpleTile { resulting in multiple error entries in logs and elsewhere! */ await this._waitForReconnection(); - return true; } // 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; @@ -128,13 +129,17 @@ export class GapTile extends SimpleTile { return this._loading; } + get showSpinner() { + return this._showSpinner; + } + get error() { if (this._error) { if (this._error instanceof ConnectionError) { - return { message: "Waiting for reconnection", showSpinner: true }; + return "Waiting for reconnection"; } const dir = this._entry.prev_batch ? "previous" : "next"; - return { message: `Could not load ${dir} messages: ${this._error.message}`, showSpinner: false }; + return `Could not load ${dir} messages: ${this._error.message}`; } return null; } diff --git a/src/platform/web/ui/session/room/timeline/GapView.js b/src/platform/web/ui/session/room/timeline/GapView.js index 87afe503..e80321be 100644 --- a/src/platform/web/ui/session/room/timeline/GapView.js +++ b/src/platform/web/ui/session/room/timeline/GapView.js @@ -30,24 +30,18 @@ export class GapView extends TemplateView { isAtTop: vm => vm.isAtTop, }; return t.li({ className }, [ - t.map(vm => vm.isLoading, - (isLoading, t, vm) => { - let elements; - const error = vm.error; - if (error) { - elements = [t.strong(() => error.message)]; - if (error.showSpinner) { - elements.unshift(spinner(t)); - } - } - else if (isLoading) { - elements = [spinner(t), t.span(vm.i18n`Loading more messages …`)]; - } - else { - elements = t.span(vm.i18n`Not loading!`); - } - return t.div({ className: "GapView__container" }, elements); - }) + t.if(vm => vm.showSpinner, (t) => spinner(t)), + t.span(vm => { + if (vm.error) { + return vm.error; + } + else if (vm.isLoading) { + return "Loading"; + } + else { + return "Not Loading"; + } + }) ]); } From 98bd8cd624063e2b3458af87425269819caeca93 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Wed, 17 Aug 2022 13:19:11 +0530 Subject: [PATCH 7/9] Remove unused css --- src/platform/web/ui/css/themes/element/timeline.css | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/platform/web/ui/css/themes/element/timeline.css b/src/platform/web/ui/css/themes/element/timeline.css index 47f9a365..43c57d19 100644 --- a/src/platform/web/ui/css/themes/element/timeline.css +++ b/src/platform/web/ui/css/themes/element/timeline.css @@ -422,12 +422,3 @@ only loads when the top comes into view*/ .GapView.isAtTop { padding: 52px 20px 12px 20px; } - -.GapView__container { - display: flex; - align-items: center; -} - -.GapView__container .spinner { - margin-right: 10px; -} From e6f43d6f4f32717153eda313a77a04ff970d2bbd Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Fri, 19 Aug 2022 11:55:23 +0530 Subject: [PATCH 8/9] Put logic into VM --- src/domain/session/room/timeline/tiles/GapTile.js | 12 ++++++++++++ src/platform/web/ui/session/room/timeline/GapView.js | 12 +----------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index 7aae124d..56d05edd 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -143,6 +143,18 @@ export class GapTile extends SimpleTile { } return null; } + + get currentAction() { + if (this.error) { + return this.error; + } + else if (this.isLoading) { + return "Loading"; + } + else { + return "Not Loading"; + } + } } import {FragmentBoundaryEntry} from "../../../../../matrix/room/timeline/entries/FragmentBoundaryEntry.js"; diff --git a/src/platform/web/ui/session/room/timeline/GapView.js b/src/platform/web/ui/session/room/timeline/GapView.js index e80321be..4fc0e3d6 100644 --- a/src/platform/web/ui/session/room/timeline/GapView.js +++ b/src/platform/web/ui/session/room/timeline/GapView.js @@ -31,17 +31,7 @@ export class GapView extends TemplateView { }; return t.li({ className }, [ t.if(vm => vm.showSpinner, (t) => spinner(t)), - t.span(vm => { - if (vm.error) { - return vm.error; - } - else if (vm.isLoading) { - return "Loading"; - } - else { - return "Not Loading"; - } - }) + t.span(vm => vm.currentAction) ]); } From fdd6eb8fdc21f47e2b64f2b24e1e63f91e4e5417 Mon Sep 17 00:00:00 2001 From: RMidhunSuresh Date: Fri, 19 Aug 2022 11:55:44 +0530 Subject: [PATCH 9/9] Set boolean to true so that gapfill proceeds --- src/domain/session/room/timeline/tiles/GapTile.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/domain/session/room/timeline/tiles/GapTile.js b/src/domain/session/room/timeline/tiles/GapTile.js index 56d05edd..bb7d8086 100644 --- a/src/domain/session/room/timeline/tiles/GapTile.js +++ b/src/domain/session/room/timeline/tiles/GapTile.js @@ -76,6 +76,7 @@ export class GapTile extends SimpleTile { } catch (e) { if (e instanceof ConnectionError) { + canFillMore = true; // Don't increase depth because this gap fill was a noop continue; }