Fix instantly sending RRs #2770

dbkr · 2019-03-08T12:52:24Z

Splits UserActivity into a tristate of 'active' (last < 1s), 'passive' (lasts a
couple of mins) and neither. Read receipts are sent when 'active', read markers
are sent while 'passive'.

Also fixed a document / window mix-up on the 'blur' handler.

Also adds a unit test for UserActivity because it's quite complex now
(and changes UserActivity to make it testable by accessing the singleton
via sharedInstance() rather than exporting it directly).

Fixes element-hq/element-web#9023

Splits UserActivity into a tristate of 'active' (last < 1s), 'passive' (lasts a couple of mins) and neither. Read receipts are sent when 'active', read markers are sent while 'passive'. Also fixed a document / window mix-up on the 'blur' handler. Also adds a unit test for UserActivity because it's quite complex now (and changes UserActivity to make it testable by accessing the singleton via sharedInstance() rather than exporting it directly). Fixes element-hq/element-web#9023

jryans

Thanks for working on this! 😁 It's a bit tricky to visualise how the sequence of timers winding other timers impacts the UX... 😅 Anyway, that's not really new from this PR.

I think a few of the comments would help clarify, so please take a look.

jryans · 2019-03-08T13:42:58Z

src/Lifecycle.js

@@ -439,7 +439,7 @@ async function startMatrixClient() {
    dis.dispatch({action: 'will_start_client'}, true);

    Notifier.start();
-    UserActivity.start();
+    UserActivity.sharedInstance().start();


I guess DMRoomMap below reveals another, slightly different convention for shared instances. In general, the approach of sharedInstance (to create one if it doesn't exist and return) without a separate getter seems simpler to me.

Maybe there's value in filing an issue to unify approaches for simplicity? The peg style is also related, though not quite the same. Anyway, might be worth having a consistent style, especially if want to add this kind of thing more for testing.

Good point, I'd forgotten about DMRoomMap's approach to singletons. Have filed element-hq/element-web#9090

src/UserActivity.js

jryans · 2019-03-08T13:48:10Z

src/UserActivity.js


 /**
 * This class watches for user activity (moving the mouse or pressing a key)
 * and starts/stops attached timers while the user is active.
+ *
+ * There are two classes of 'active' a user can be: 'active' and 'passive':


It's a bit strange naming-wise for there to classes of X, where one of the values is also X. So maybe something like:

Suggested change

* There are two classes of 'active' a user can be: 'active' and 'passive':

* There are two classes of 'focused' a user can be: 'active' and 'passive':

If you agree, we may also want to rename the class to UserFocusTracker or something...?

Mmm, 'focused' makes me think it's more just about tracking whether the window has focus. We could go with Bruno's 'activeActive' and 'passiveActive', or 'activeNow' and 'activeRecently'?

Hmm, activeNow vs. activeRecently sounds like the clearest so far to me.

Was just about to change this and then realised this would make functions called userCurrentlyActiveNow() and, even better, userCurrentlyActiveRecently(). I'll have a think...

Maybe userActiveNowish() and userActiveRecently()?

Yeah, I was thinking maybe just userActiveNow()

Right, that could work as well.

src/UserActivity.js

jryans · 2019-03-08T14:59:46Z

src/UserActivity.js

-        document.addEventListener("visibilitychange", this._onPageVisibilityChanged);
-        window.addEventListener("blur", this._onWindowBlurred);
-        window.addEventListener("focus", this._onUserActivity);
+        this._document.onmousedown = this._onUserActivity;


Maybe a good moment to consistently use addEventListener here as well, rather than a mix of aEL and on<event>?

ah yes, good point

jryans · 2019-03-08T18:14:20Z

src/UserActivity.js

-        this._document.onkeydown = this._onUserActivity;
+        this._document.addEventListener('mousedown', this._onUserActivity);
+        this._document.addEventListener('mousemove', this._onUserActivity);
+        this._document.addEventListener('keydown', this._onUserActivity);


Thanks for doing this! 😁 We should also change the ones in stop to call removeEventListener now.

jryans

A few things still to fix, I think. I'd like to see how the naming plays out.

jryans · 2019-03-08T18:17:07Z

(Clearing assignment following recent discussion to stop assigning PRs.)

also fix more indenting

jryans

There's still more to go here to match the new naming, I think. Overall though, I think we have good terminology now. 😁

jryans · 2019-03-11T17:32:32Z

src/UserActivity.js

-        this._activeTimeout = new Timer(CURRENTLY_ACTIVE_THRESHOLD_MS);
-        this._passiveTimeout = new Timer(CURRENTLY_PASSIVE_THRESHOLD_MS);
+        this._activeNowTimeout = new Timer(CURRENTLY_ACTIVE_THRESHOLD_MS);
+        this._activeRecentlyTimeout = new Timer(CURRENTLY_PASSIVE_THRESHOLD_MS);


These thresholds should likely also be renamed to match as well.

jryans · 2019-03-11T17:33:59Z

src/UserActivity.js

-     * See userCurrentlyActive() for what it means for a user to be 'active'.
+     * Runs the given timer while the user is 'active', aborting when the user is no longer
+     * considered currently active.
+     * See userActiveNow() for what it means for a user to be 'active'.
     * Can be called multiple times with the same already running timer, which is a NO-OP.
     * Can be called before the user becomes active, in which case it is only started
     * later on when the user does become active.
     * @param {Timer} timer the timer to use
     */
    timeWhileActive(timer) {


Hmm, shouldn't we carry the new naming through to this API as well? We'd then have timeWhileActiveNow and timeWhileActiveRecently.

src/components/structures/TimelinePanel.js

Co-Authored-By: dbkr <[email protected]>

…ix-react-sdk into dbkr/fix_instant_rrs_pt2

jryans

Looks great! 😁 Thanks for working on the naming changes. I think it's much easier to follow now.

dbkr requested a review from a team March 8, 2019 12:52

jryans self-assigned this Mar 8, 2019

spurious semicolon

61ebf4d

jryans suggested changes Mar 8, 2019

View reviewed changes

jryans assigned dbkr and unassigned jryans Mar 8, 2019

dbkr added 2 commits March 8, 2019 15:09

Fix call to stop()

7e424ce

PR feedback

b404d21

dbkr assigned jryans and unassigned dbkr Mar 8, 2019

jryans reviewed Mar 8, 2019

View reviewed changes

jryans suggested changes Mar 8, 2019

View reviewed changes

jryans removed their assignment Mar 8, 2019

Missed the removes

999ebe6

also fix more indenting

jryans mentioned this pull request Mar 8, 2019

Add tests to ensure resolved modules are singleton instances element-hq/element-web#9093

Closed

Rename

ce9f3d8

dbkr requested a review from jryans March 11, 2019 17:21

jryans suggested changes Mar 11, 2019

View reviewed changes

dbkr and others added 6 commits March 12, 2019 09:37

Rest of the naming changes

63d19a8

Rename

ea01853

Co-Authored-By: dbkr <[email protected]>

Rename

2d074d0

Co-Authored-By: dbkr <[email protected]>

Fix comment

08e21ff

Rename more things

374be0b

Merge branch 'dbkr/fix_instant_rrs_pt2' of github.com:matrix-org/matr…

9f4de60

…ix-react-sdk into dbkr/fix_instant_rrs_pt2

dbkr requested a review from jryans March 12, 2019 10:43

jryans approved these changes Mar 12, 2019

View reviewed changes

dbkr merged commit b39a7e0 into develop Mar 12, 2019

amstan mentioned this pull request Dec 26, 2020

Don't send push notifications to users who are clearly active on a device element-hq/element-meta#360

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix instantly sending RRs #2770

Fix instantly sending RRs #2770

dbkr commented Mar 8, 2019

jryans left a comment

jryans Mar 8, 2019

dbkr Mar 8, 2019

jryans Mar 8, 2019

dbkr Mar 8, 2019

jryans Mar 8, 2019

dbkr Mar 8, 2019

jryans Mar 8, 2019

dbkr Mar 11, 2019

jryans Mar 11, 2019

jryans Mar 8, 2019

dbkr Mar 8, 2019

jryans Mar 8, 2019

jryans left a comment

jryans commented Mar 8, 2019

jryans left a comment

jryans Mar 11, 2019

jryans Mar 11, 2019

jryans left a comment

	* There are two classes of 'active' a user can be: 'active' and 'passive':
	* There are two classes of 'focused' a user can be: 'active' and 'passive':

Fix instantly sending RRs #2770

Fix instantly sending RRs #2770

Conversation

dbkr commented Mar 8, 2019

jryans 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

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

jryans left a comment

Choose a reason for hiding this comment

jryans commented Mar 8, 2019

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment