From ce1623691e8bfcd5539634c69f6353356bd381e5 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Mar 2019 12:46:38 +0000 Subject: [PATCH] Fix instantly sending RRs 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 https://github.com/vector-im/riot-web/issues/9023 --- src/Lifecycle.js | 2 +- src/UserActivity.js | 157 +++++++++++++++------ src/components/structures/TimelinePanel.js | 8 +- test/UserActivity-test.js | 134 ++++++++++++++++++ 4 files changed, 250 insertions(+), 51 deletions(-) create mode 100644 test/UserActivity-test.js diff --git a/src/Lifecycle.js b/src/Lifecycle.js index 54ac605c65..e725e91044 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -439,7 +439,7 @@ async function startMatrixClient() { dis.dispatch({action: 'will_start_client'}, true); Notifier.start(); - UserActivity.start(); + UserActivity.sharedInstance().start(); Presence.start(); DMRoomMap.makeShared().start(); ActiveWidgetStore.start(); diff --git a/src/UserActivity.js b/src/UserActivity.js index b49bcf1a91..a4fba65f7f 100644 --- a/src/UserActivity.js +++ b/src/UserActivity.js @@ -23,16 +23,25 @@ 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': + * 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); @@ -40,48 +49,76 @@ class UserActivity { 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) { + 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; + 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 }); } @@ -89,39 +126,57 @@ class UserActivity { * 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 @@ -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(); } } + + async _runTimersUntilTimeout(attachedTimers, timeout) { + attachedTimers.forEach((t) => t.start()); + try { + await timeout.finished(); + } catch (_e) { /* aborted */ } + attachedTimers.forEach((t) => t.abort()); + } } - - -module.exports = new UserActivity(); diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 911499e314..ca2d01de04 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -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 @@ -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 */ } @@ -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 */ } diff --git a/test/UserActivity-test.js b/test/UserActivity-test.js new file mode 100644 index 0000000000..db15fe27c4 --- /dev/null +++ b/test/UserActivity-test.js @@ -0,0 +1,134 @@ +/* +Copyright 2019 New Vector Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import expect from 'expect'; +import lolex from 'lolex'; +import jest from 'jest-mock'; +import EventEmitter from 'events'; +import UserActivity from '../src/UserActivity'; + +class FakeDomEventEmitter extends EventEmitter { + addEventListener(what, l) { + this.on(what, l); + } + + removeEventListener(what, l) { + this.removeListener(what, l); + } +}; + +describe('UserActivity', function() { + let fakeWindow; + let fakeDocument; + let userActivity; + let clock; + + beforeEach(function() { + fakeWindow = new FakeDomEventEmitter(), + fakeDocument = new FakeDomEventEmitter(), + userActivity = new UserActivity(fakeWindow, fakeDocument); + userActivity.start(); + clock = lolex.install(); + }); + + afterEach(function() { + userActivity.stop(); + userActivity = null; + clock.uninstall(); + clock = null; + }); + + it('should return the same shared instance', function() { + expect(UserActivity.sharedInstance()).toBe(UserActivity.sharedInstance()); + }); + + it('should consider user inactive if no activity', function() { + expect(userActivity.userCurrentlyActive()).toBe(false); + }); + + it('should consider user not passive if no activity', function() { + expect(userActivity.userCurrentlyPassive()).toBe(false); + }); + + it('should not consider user active after activity if no window focus', function() { + fakeDocument.hasFocus = jest.fn().mockReturnValue(false); + + userActivity._onUserActivity({}); + expect(userActivity.userCurrentlyActive()).toBe(false); + expect(userActivity.userCurrentlyPassive()).toBe(false); + }); + + it('should consider user active shortly after activity', function() { + fakeDocument.hasFocus = jest.fn().mockReturnValue(true); + + userActivity._onUserActivity({}); + expect(userActivity.userCurrentlyActive()).toBe(true); + expect(userActivity.userCurrentlyPassive()).toBe(true); + clock.tick(200); + expect(userActivity.userCurrentlyActive()).toBe(true); + expect(userActivity.userCurrentlyPassive()).toBe(true); + }); + + it('should consider user not active after 10s of no activity', function() { + fakeDocument.hasFocus = jest.fn().mockReturnValue(true); + + userActivity._onUserActivity({}); + clock.tick(10000); + expect(userActivity.userCurrentlyActive()).toBe(false); + }); + + it('should consider user passive after 10s of no activity', function() { + fakeDocument.hasFocus = jest.fn().mockReturnValue(true); + + userActivity._onUserActivity({}); + clock.tick(10000); + expect(userActivity.userCurrentlyPassive()).toBe(true); + }); + + it('should not consider user passive after 10s if window un-focused', function() { + fakeDocument.hasFocus = jest.fn().mockReturnValue(true); + + userActivity._onUserActivity({}); + clock.tick(10000); + + fakeDocument.hasFocus = jest.fn().mockReturnValue(false); + fakeWindow.emit('blur', {}); + + expect(userActivity.userCurrentlyPassive()).toBe(false); + }); + + it('should not consider user passive after 3 mins', function() { + fakeDocument.hasFocus = jest.fn().mockReturnValue(true); + + userActivity._onUserActivity({}); + clock.tick(3 * 60 * 1000); + + expect(userActivity.userCurrentlyPassive()).toBe(false); + }); + + it('should extend timer on activity', function() { + fakeDocument.hasFocus = jest.fn().mockReturnValue(true); + + userActivity._onUserActivity({}); + clock.tick(1 * 60 * 1000); + userActivity._onUserActivity({}); + clock.tick(1 * 60 * 1000); + userActivity._onUserActivity({}); + clock.tick(1 * 60 * 1000); + + expect(userActivity.userCurrentlyPassive()).toBe(true); + }); +});