Improve UX for Jitsi by adding local echo for widgets #2035

dbkr · 2018-07-03T10:25:48Z

Show a spinner while we wait for widgets to be added
Hide widgets while they're pending deletion
Don't put another jitsi widget into the room if there's already
one pending
Add an error if removing a widget fails
Wait longer for widgets (at least while matrix.org is slow)

Feedback welcome on the architecture of WidgetEchoStore, in particular how we pass in the current room widgets so the store isn't pulling these from a different place, implying that it would be responsible for emitting events when they change.

* Show a spinner while we wait for widgets to be deleted * Hide widgets while they're pending deletion * Don't put another jitsi widget into the room if there's already one pending

dbkr · 2018-07-03T10:28:45Z

src/stores/RoomViewStore.js

@@ -1,6 +1,6 @@
 /*
 Copyright 2017 Vector Creations Ltd
-Copyright 2017 New Vector Ltd
+Copyright 2017, 2018 New Vector Ltd


Also up the timeout because matrix.org is that slow

lukebarnard1

Code-wise/commenting-wise needs a few changes.

Architecturally, I've already advised on this but I'll try and comment objectively on this implementation specifically:

Requiring both widget utils and the widget store to be able to render the view smells a bit like high coupling.
It seems odd to have to provide an array of current widgets to get the state for rendering. If the state for rendering was already in the store, the view would be simpler.
Overall I can't help but notice that this could be written very simply as a Flux store.

lukebarnard1 · 2018-07-05T09:33:15Z

src/actions/MatrixActionCreators.js

+ * @returns {EventDecryptedAction} an action of type `MatrixActions.Event.decrypted`.
+ */
+function createRoomStateEventsAction(matrixClient, event, state) {
+    return { action: 'MatrixActions.RoomState.events', event, state };


This is unused.

lukebarnard1 · 2018-07-05T09:47:13Z

src/stores/WidgetEchoStore.js

+     * @param {MatrixEvent[]} currentRoomWidgets Current widgets for the room
+     * @returns {MatrixEvent[]} List of widgets in the room, minus any pending removal
+     */
+    getEchoedRoomWidgets(room, currentRoomWidgets) {


Looks like we only need the roomId?

lukebarnard1 · 2018-07-05T09:49:09Z

src/stores/WidgetEchoStore.js

+    /**
+     * Gets the widgets for a room, substracting those that are pending deletion.
+     * Widgets that are pending addition are not included, since widgets are
+     * represted as MatrixEvents, so to do this we'd have to create fake MatrixEvents,


In theory, you could have a class that represents widget data within an event which doesn't care about MatrixEvents. Then you'd just need instantiate one of those as per usual (not a fake one).

yep, true, it just means changing everything else to add that layer of abstraction

lukebarnard1 · 2018-07-05T10:01:37Z

src/stores/WidgetEchoStore.js

+
+        for (const w of currentRoomWidgets) {
+            const widgetId = w.getStateKey();
+            if (roomEchoState && roomEchoState[widgetId] && Object.keys(roomEchoState[widgetId]).length === 0) {


Including roomEchoState is redundant here because Object.assign({}, ...) should at least return an object.

It's not so clear why Object.keys are checked for length. Why would the echo state contain an object {}?

And sometimes it's this, but other times it's === {}. Good to be consistent.

ah yep.

Why would the echo state contain an object {}

In the case where it's been deleted locally - I should doc this.

lukebarnard1 · 2018-07-05T10:05:33Z

src/stores/WidgetEchoStore.js

+
+        this._roomWidgetEcho = {
+            // roomId: {
+            //     widgetId: [object]


I think it's worth commenting what object is stored here.

lukebarnard1 · 2018-07-05T13:19:31Z

src/stores/WidgetEchoStore.js

+
+        for (const w of currentRoomWidgets) {
+            const widgetId = w.getStateKey();
+            delete roomEchoState[widgetId];


It's not totally clear to me why the current room widgets are removed from the echo state, only for us to calculate a result and return that to the caller. Why not have the caller decide that currentRoomWidgets by nature are not pending?

Have added some comments - I think the reason for the above is just again to take logic out of the view.

I think this comes back to where the state gets calculated and I think I'm too tunnel-visioned on flux to be at ease with another flow...

lukebarnard1 · 2018-07-05T13:28:27Z

src/stores/WidgetEchoStore.js

+        }
+    }
+
+    roomHasPendingWidgets(room, currentRoomWidgets) {


Perhaps a single function that has an optional type? called roomHasPendingWidgets?

yeah, seemed nicer to add a wrapper function though.

lukebarnard1 · 2018-07-05T14:06:59Z

src/utils/WidgetUtils.js

@@ -19,6 +19,10 @@ import MatrixClientPeg from '../MatrixClientPeg';
 import SdkConfig from "../SdkConfig";
 import dis from '../dispatcher';
 import * as url from "url";
+import WidgetEchoStore from '../stores/WidgetEchoStore';
+
+// How long we wait for the state event echo to come back from the server


I think it's worth commenting what happens when the timeout elapses.

lukebarnard1 · 2018-07-05T14:10:48Z

src/CallHandler.js

+    if (WidgetEchoStore.roomHasPendingWidgetsOfType(room, currentRoomWidgets, 'jitsi')) {
+        const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");
+
+        Modal.createTrackedDialog('Already have pending Jitsi Widget', '', ErrorDialog, {


Using an internal tracking ID that is not clearly related to what is shown in the modal might be confusing.

lukebarnard1 · 2018-07-05T14:19:09Z

src/stores/WidgetEchoStore.js

+        if (this._roomWidgetEcho[room.roomId] === undefined) this._roomWidgetEcho[room.roomId] = {};
+
+        this._roomWidgetEcho[room.roomId][widgetId] = state;
+        this.emit('updateRoomWidgetEcho');


This is the only type of event emitted, why not "update" or smth?

As it wasn't used in the end

lukebarnard1 · 2018-07-06T09:36:08Z

src/CallHandler.js

        const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog");

-        Modal.createTrackedDialog('Already have pending Jitsi Widget', '', ErrorDialog, {
+        Modal.createTrackedDialog('Call being placed', '', ErrorDialog, {


On paper this looks like we're tracking a call being placed. I would suggest "Call already in progress".

lukebarnard1 · 2018-07-06T09:37:25Z

src/stores/WidgetEchoStore.js

-            //} else if (roomEchoState && roomEchoState[widgetId]) {
-            //    echoedWidgets.push(roomEchoState[widgetId]);
+            // If there's no echo, or the echo still has a widget present, show the *old* widget
+            // we don't include widgets that have changed for the same rason we don't include new ones


typo ("rason")

Also, what is that reason?

lukebarnard1 · 2018-07-06T09:38:05Z

src/stores/WidgetEchoStore.js

-            //    echoedWidgets.push(roomEchoState[widgetId]);
+            // If there's no echo, or the echo still has a widget present, show the *old* widget
+            // we don't include widgets that have changed for the same rason we don't include new ones
+            if (!roomEchoState[widgetId] || Object.keys(roomEchoState[widgetId]).length !== 0) {


Strongly suggest negating this instead of having an empty if clause.

lukebarnard1 · 2018-07-06T09:40:11Z

src/stores/WidgetEchoStore.js

+
+        for (const w of currentRoomWidgets) {
+            const widgetId = w.getStateKey();
+            delete roomEchoState[widgetId];


I think this comes back to where the state gets calculated and I think I'm too tunnel-visioned on flux to be at ease with another flow...

lukebarnard1 · 2018-07-06T09:41:14Z

src/utils/WidgetUtils.js

@@ -22,6 +22,7 @@ import * as url from "url";
 import WidgetEchoStore from '../stores/WidgetEchoStore';

 // How long we wait for the state event echo to come back from the server
+// before the promise is rejected


Which promise though?

bwindels · 2018-07-17T16:49:21Z

What should we do with this? I'm fine with not doing the improvements Luke suggested, even though I feel I can't really judge whether or not it's the right thing to do, but we should move forward or abandon this change, as it's already conflicting. Anyone else is also welcome to pitch in of course :)

dbkr · 2018-07-18T08:16:04Z

Well, the experience without doing anything is terrible since there is zero feedback when pressing the call buttons, so I don't think doing nothing is an option. IMO, we should probably go with this for now, rather than push the release back further to rewrite this.

dbkr added 3 commits July 3, 2018 11:16

Do some level of local echo for widgets

8b64ddc

* Show a spinner while we wait for widgets to be deleted * Hide widgets while they're pending deletion * Don't put another jitsi widget into the room if there's already one pending

Lint

3199e68

Merge remote-tracking branch 'origin/develop' into dbkr/widget_echo

13530e5

dbkr requested a review from a team July 3, 2018 10:25

i18n

9d41b87

dbkr commented Jul 3, 2018

View reviewed changes

dbkr removed the request for review from a team July 3, 2018 10:37

dbkr changed the title ~~Improve UX for Jitsi by adding local echo for widgets~~ [WIP] Improve UX for Jitsi by adding local echo for widgets Jul 3, 2018

Add error dialog if widget remove fails

c665ab8

Also up the timeout because matrix.org is that slow

dbkr changed the title ~~[WIP] Improve UX for Jitsi by adding local echo for widgets~~ Improve UX for Jitsi by adding local echo for widgets Jul 3, 2018

dbkr requested a review from a team July 3, 2018 10:57

lukebarnard1 suggested changes Jul 5, 2018

View reviewed changes

dbkr added 11 commits July 5, 2018 18:22

Revert 8b64ddc for MatrixActionCreators

5f2e2ef

As it wasn't used in the end

Just pass the roomId into WidgetEchoStore

eb552e5

Consistency, redundant check & doc

1f2d279

More doc

fb7ce9d

Make if clearer

7e2b559

Remove unhelpful commented code

8c10dc1

Remove redundant check

3f60300

comments

658ac73

more comments

c26b300

Use more helpful dialog tracking title

3f20eb9

Simplify event name as it's the only one

b6f854a

lukebarnard1 suggested changes Jul 6, 2018

View reviewed changes

dbkr added 3 commits July 16, 2018 13:18

PR feedback

0f32c3a

Merge remote-tracking branch 'origin/develop' into dbkr/widget_echo

4c6419a

lint

983dc3a

Merge remote-tracking branch 'origin/develop' into dbkr/widget_echo

7aab6fa

lint

68c46a6

dbkr mentioned this pull request Jul 18, 2018

Widgets behave a bit strangely if you close the add widget dialog before it completes adding the widget element-hq/element-web#7057

Closed

bwindels self-requested a review July 19, 2018 10:50

bwindels approved these changes Jul 19, 2018

View reviewed changes

bwindels merged commit 10e4a4f into develop Jul 19, 2018

bwindels deleted the dbkr/widget_echo branch July 19, 2018 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve UX for Jitsi by adding local echo for widgets #2035

Improve UX for Jitsi by adding local echo for widgets #2035

dbkr commented Jul 3, 2018 •

edited

Loading

dbkr Jul 3, 2018

lukebarnard1 left a comment

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 5, 2018

dbkr Jul 5, 2018

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 5, 2018

dbkr Jul 5, 2018 •

edited

Loading

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 5, 2018

dbkr Jul 5, 2018

lukebarnard1 Jul 6, 2018

lukebarnard1 Jul 5, 2018

dbkr Jul 5, 2018

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 5, 2018

lukebarnard1 Jul 6, 2018

lukebarnard1 Jul 6, 2018

lukebarnard1 Jul 6, 2018

lukebarnard1 Jul 6, 2018

lukebarnard1 Jul 6, 2018

lukebarnard1 Jul 6, 2018

bwindels commented Jul 17, 2018 •

edited

Loading

dbkr commented Jul 18, 2018

Improve UX for Jitsi by adding local echo for widgets #2035

Improve UX for Jitsi by adding local echo for widgets #2035

Conversation

dbkr commented Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

lukebarnard1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwindels commented Jul 17, 2018 • edited Loading

dbkr commented Jul 18, 2018

dbkr commented Jul 3, 2018 •

edited

Loading

dbkr Jul 5, 2018 •

edited

Loading

bwindels commented Jul 17, 2018 •

edited

Loading