Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Fix instantly sending RRs #2770

Merged
merged 12 commits into from
Mar 12, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Lifecycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ async function startMatrixClient() {
dis.dispatch({action: 'will_start_client'}, true);

Notifier.start();
UserActivity.start();
UserActivity.sharedInstance().start();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Presence.start();
DMRoomMap.makeShared().start();
ActiveWidgetStore.start();
Expand Down
157 changes: 111 additions & 46 deletions src/UserActivity.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,105 +23,160 @@ import Timer from './utils/Timer';
// such as READ_MARKER_INVIEW_THRESHOLD_MS,
// READ_MARKER_OUTOFVIEW_THRESHOLD_MS,
// READ_RECEIPT_INTERVAL_MS in TimelinePanel
const CURRENTLY_ACTIVE_THRESHOLD_MS = 2 * 60 * 1000;
const CURRENTLY_ACTIVE_THRESHOLD_MS = 700;
const CURRENTLY_PASSIVE_THRESHOLD_MS = 2 * 60 * 1000;

/**
* 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':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe userActiveNowish() and userActiveRecently()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking maybe just userActiveNow()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that could work as well.

* see doc on the userCurrently* functions for what these mean.
*/
class UserActivity {
constructor() {
this._attachedTimers = [];
this._activityTimeout = new Timer(CURRENTLY_ACTIVE_THRESHOLD_MS);
export default class UserActivity {
constructor(windowObj, documentObj) {
this._window = windowObj;
this._document = documentObj;

this._attachedTimersActive = [];
this._attachedTimersPassive = [];
this._activeTimeout = new Timer(CURRENTLY_ACTIVE_THRESHOLD_MS);
this._passiveTimeout = new Timer(CURRENTLY_PASSIVE_THRESHOLD_MS);
this._onUserActivity = this._onUserActivity.bind(this);
this._onWindowBlurred = this._onWindowBlurred.bind(this);
this._onPageVisibilityChanged = this._onPageVisibilityChanged.bind(this);
this.lastScreenX = 0;
this.lastScreenY = 0;
}

static sharedInstance() {
if (global.mxUserActivity === undefined) {
global.mxUserActivity = new UserActivity(window, document);
}
return global.mxUserActivity;
}

/**
* Runs the given timer while the user is active, aborting when the user becomes inactive.
* Runs the given timer while the user is 'active', aborting when the user becomes 'passive'.
* See userCurrentlyActive() 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this._timeWhile(timer, this._attachedTimersActive);
if (this.userCurrentlyActive()) {
timer.start();
}
}

/**
* Runs the given timer while the user is 'active' or 'passive', aborting when the user becomes
* inactive.
* See userCurrentlyPassive() for what it means for a user to be 'passive'.
* 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
*/
timeWhilePassive(timer) {
this._timeWhile(timer, this._attachedTimersPassive);
if (this.userCurrentlyPassive()) {
timer.start();
}
}

_timeWhile(timer, attachedTimers) {
// important this happens first
const index = this._attachedTimers.indexOf(timer);
const index = attachedTimers.indexOf(timer);
if (index === -1) {
this._attachedTimers.push(timer);
attachedTimers.push(timer);
// remove when done or aborted
timer.finished().finally(() => {
const index = this._attachedTimers.indexOf(timer);
const index = attachedTimers.indexOf(timer);
if (index !== -1) { // should never be -1
this._attachedTimers.splice(index, 1);
attachedTimers.splice(index, 1);
}
// as we fork the promise here,
// avoid unhandled rejection warnings
}).catch((err) => {});
}
if (this.userCurrentlyActive()) {
timer.start();
}
}

/**
* Start listening to user activity
*/
start() {
document.onmousedown = this._onUserActivity;
document.onmousemove = this._onUserActivity;
document.onkeydown = this._onUserActivity;
document.addEventListener("visibilitychange", this._onPageVisibilityChanged);
window.addEventListener("blur", this._onWindowBlurred);
window.addEventListener("focus", this._onUserActivity);
this._document.onmousedown = this._onUserActivity;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, good point

this._document.onmousemove = this._onUserActivity;
this._document.onkeydown = this._onUserActivity;
this._document.addEventListener("visibilitychange", this._onPageVisibilityChanged);
this._window.addEventListener("blur", this._onWindowBlurred);
this._window.addEventListener("focus", this._onUserActivity);
// can't use document.scroll here because that's only the document
// itself being scrolled. Need to use addEventListener's useCapture.
// also this needs to be the wheel event, not scroll, as scroll is
// fired when the view scrolls down for a new message.
window.addEventListener('wheel', this._onUserActivity,
this._window.addEventListener('wheel', this._onUserActivity,
{ passive: true, capture: true });
}

/**
* Stop tracking user activity
*/
stop() {
document.onmousedown = undefined;
document.onmousemove = undefined;
document.onkeydown = undefined;
window.removeEventListener('wheel', this._onUserActivity,
this._document.onmousedown = undefined;
this._document.onmousemove = undefined;
this._document.onkeydown = undefined;
this._window.removeEventListener('wheel', this._onUserActivity,
{ passive: true, capture: true });

document.removeEventListener("visibilitychange", this._onPageVisibilityChanged);
document.removeEventListener("blur", this._onDocumentBlurred);
document.removeEventListener("focus", this._onUserActivity);
this._document.removeEventListener("visibilitychange", this._onPageVisibilityChanged);
this._window.removeEventListener("blur", this._onWindowBlurred);
this._window.removeEventListener("focus", this._onUserActivity);
}

/**
* Return true if there has been user activity very recently
* (ie. within a few seconds)
* @returns {boolean} true if user is currently/very recently active
* Return true if the user is currently 'active'
* A user is 'active' while they are interacting with the app and for a very short (<1s)
* time after that. This is intended to give a strong indication that the app has the
* user's attention at any given moment.
* @returns {boolean} true if user is currently 'active'
*/
userCurrentlyActive() {
return this._activityTimeout.isRunning();
return this._activeTimeout.isRunning();
}

/**
* Return true if the user is currently 'active' or 'passive'
* A user is 'passive' for a longer period of time (~2 mins) after they have been 'active' and
* while the app still has the focus. This is intended to indicate when the app may still have
* the user's attention (or they may have gone to make tea and left the window focused).
* @returns {boolean} true if user is currently 'active' or 'passive'
*/
userCurrentlyPassive() {
return this._passiveTimeout.isRunning();
}

_onPageVisibilityChanged(e) {
if (document.visibilityState === "hidden") {
this._activityTimeout.abort();
if (this._document.visibilityState === "hidden") {
this._activeTimeout.abort();
this._passiveTimeout.abort();
} else {
this._onUserActivity(e);
}
}

_onWindowBlurred() {
this._activityTimeout.abort();
this._activeTimeout.abort();
this._passiveTimeout.abort();
}

async _onUserActivity(event) {
_onUserActivity(event) {
// ignore anything if the window isn't focused
if (!this._document.hasFocus()) return;

if (event.screenX && event.type === "mousemove") {
if (event.screenX === this.lastScreenX && event.screenY === this.lastScreenY) {
// mouse hasn't actually moved
Expand All @@ -132,19 +187,29 @@ class UserActivity {
}

dis.dispatch({action: 'user_activity'});
if (!this._activityTimeout.isRunning()) {
this._activityTimeout.start();
if (!this._activeTimeout.isRunning()) {
this._activeTimeout.start();
dis.dispatch({action: 'user_activity_start'});
this._attachedTimers.forEach((t) => t.start());
try {
await this._activityTimeout.finished();
} catch (_e) { /* aborted */ }
this._attachedTimers.forEach((t) => t.abort());

this._runTimersUntilTimeout(this._attachedTimersActive, this._activeTimeout);
} else {
this._activityTimeout.restart();
this._activeTimeout.restart();
}
}
}

if (!this._passiveTimeout.isRunning()) {
this._passiveTimeout.start();

this._runTimersUntilTimeout(this._attachedTimersPassive, this._passiveTimeout);
} else {
this._passiveTimeout.restart();
}
}

module.exports = new UserActivity();
async _runTimersUntilTimeout(attachedTimers, timeout) {
attachedTimers.forEach((t) => t.start());
try {
await timeout.finished();
} catch (_e) { /* aborted */ }
attachedTimers.forEach((t) => t.abort());
}
}
8 changes: 4 additions & 4 deletions src/components/structures/TimelinePanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -451,12 +451,12 @@ var TimelinePanel = React.createClass({
//
// We ignore events we have sent ourselves; we don't want to see the
// read-marker when a remote echo of an event we have just sent takes
// more than the timeout on userCurrentlyActive.
// more than the timeout on userCurrentlyPassive.
//
const myUserId = MatrixClientPeg.get().credentials.userId;
const sender = ev.sender ? ev.sender.userId : null;
var callRMUpdated = false;
if (sender != myUserId && !UserActivity.userCurrentlyActive()) {
if (sender != myUserId && !UserActivity.sharedInstance().userCurrentlyPassive()) {
updatedState.readMarkerVisible = true;
} else if (lastEv && this.getReadMarkerPosition() === 0) {
// we know we're stuckAtBottom, so we can advance the RM
Expand Down Expand Up @@ -562,7 +562,7 @@ var TimelinePanel = React.createClass({
this._readMarkerActivityTimer = new Timer(initialTimeout);

while (this._readMarkerActivityTimer) { //unset on unmount
UserActivity.timeWhileActive(this._readMarkerActivityTimer);
UserActivity.sharedInstance().timeWhilePassive(this._readMarkerActivityTimer);
try {
await this._readMarkerActivityTimer.finished();
} catch(e) { continue; /* aborted */ }
Expand All @@ -574,7 +574,7 @@ var TimelinePanel = React.createClass({
updateReadReceiptOnUserActivity: async function() {
this._readReceiptActivityTimer = new Timer(READ_RECEIPT_INTERVAL_MS);
while (this._readReceiptActivityTimer) { //unset on unmount
UserActivity.timeWhileActive(this._readReceiptActivityTimer);
UserActivity.sharedInstance().timeWhileActive(this._readReceiptActivityTimer);
try {
await this._readReceiptActivityTimer.finished();
} catch(e) { continue; /* aborted */ }
Expand Down
Loading