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

Fix: roomlist reordering lags #2612

Merged
merged 8 commits into from
Feb 13, 2019
Merged

Fix: roomlist reordering lags #2612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f348555
replace ratelimitedfunc with lodash impl
bwindels Feb 7, 2019
d2dd1ba
run search after you've stopped typing for 200ms instead of every 500ms
bwindels Feb 7, 2019
6f9aa9c
Merge branch 'develop' into bwindels/roomlistlag
bwindels Feb 8, 2019
b6aa72d
update RoomTile notificationCount through props so updates are consis…
bwindels Feb 11, 2019
d069f4a
not the cause of the bug, but this seems wrong
bwindels Feb 11, 2019
fbc0bbf
clear hover flag after 1000ms of not moving the mouse
bwindels Feb 11, 2019
ab6535b
lint
bwindels Feb 11, 2019
7c72fdd
Merge branch 'develop' into bwindels/roomlistlag
bwindels Feb 11, 2019
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/Unread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ module.exports = {
return false;
} else if (ev.getType() == 'm.call.answer' || ev.getType() == 'm.call.hangup') {
return false;
} else if (ev.getType == 'm.room.message' && ev.getContent().msgtype == 'm.notify') {
} else if (ev.getType() == 'm.room.message' && ev.getContent().msgtype == 'm.notify') {
bwindels marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
const EventTile = sdk.getComponent('rooms.EventTile');
Expand Down
1 change: 1 addition & 0 deletions src/components/structures/RoomSubList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ const RoomSubList = React.createClass({
collapsed={this.props.collapsed || false}
unread={Unread.doesRoomHaveUnreadMessages(room)}
highlight={room.getUnreadNotificationCount('highlight') > 0 || this.props.isInvite}
notificationCount={room.getUnreadNotificationCount()}
isInvite={this.props.isInvite}
refreshSubList={this._updateSubListCount}
incomingCall={null}
Expand Down
11 changes: 4 additions & 7 deletions src/components/structures/SearchBox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { _t } from '../../languageHandler';
import { KeyCode } from '../../Keyboard';
import sdk from '../../index';
import dis from '../../dispatcher';
import rate_limited_func from '../../ratelimitedfunc';
import { debounce } from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

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

I thought throttle closer matched what we had? Or is there a reason to prefer the different behaviour (I think thge difference being whether it keep executing the thing every n seconds on sustained activity).

Copy link
Member

Choose a reason for hiding this comment

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

From reading the documentation, debounce seems like the thing we want here with the trailing edge later defined. If someone were to roll their face on the keyboard, we probably want to debounce that rather than rate limit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, what Travis said. Feels faster in practice as well, as the delay can be smaller.

turt2live marked this conversation as resolved.
Show resolved Hide resolved
import AccessibleButton from '../../components/views/elements/AccessibleButton';

module.exports = React.createClass({
Expand Down Expand Up @@ -67,12 +67,9 @@ module.exports = React.createClass({
this.onSearch();
},

onSearch: new rate_limited_func(
function() {
this.props.onSearch(this.refs.search.value);
},
500,
),
onSearch: debounce(function() {
this.props.onSearch(this.refs.search.value);
}, 200, {trailing: true}),

_onKeyDown: function(ev) {
switch (ev.keyCode) {
Expand Down
32 changes: 28 additions & 4 deletions src/components/views/rooms/RoomList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.

'use strict';
import SettingsStore from "../../../settings/SettingsStore";
import Timer from "../../../utils/Timer";

const React = require("react");
const ReactDOM = require("react-dom");
Expand All @@ -41,6 +42,7 @@ import {Resizer} from '../../../resizer';
import {Layout, Distributor} from '../../../resizer/distributors/roomsublist2';
const HIDE_CONFERENCE_CHANS = true;
const STANDARD_TAGS_REGEX = /^(m\.(favourite|lowpriority|server_notice)|im\.vector\.fake\.(invite|recent|direct|archived))$/;
const HOVER_MOVE_TIMEOUT = 1000;

function labelForTagName(tagName) {
if (tagName.startsWith('u.')) return tagName.slice(2);
Expand Down Expand Up @@ -73,6 +75,7 @@ module.exports = React.createClass({

getInitialState: function() {

this._hoverClearTimer = null;
this._subListRefs = {
// key => RoomSubList ref
};
Expand All @@ -95,7 +98,7 @@ module.exports = React.createClass({
// update overflow indicators
this._checkSubListsOverflow();
// don't store height for collapsed sublists
if(!this.collapsedState[key]) {
if (!this.collapsedState[key]) {
this.subListSizes[key] = size;
window.localStorage.setItem("mx_roomlist_sizes",
JSON.stringify(this.subListSizes));
Expand Down Expand Up @@ -357,11 +360,32 @@ module.exports = React.createClass({
this.forceUpdate();
},

onMouseEnter: function(ev) {
this.setState({hover: true});
onMouseMove: async function(ev) {
if (!this._hoverClearTimer) {
this.setState({hover: true});
this._hoverClearTimer = new Timer(HOVER_MOVE_TIMEOUT);
this._hoverClearTimer.start();
let finished = true;
try {
await this._hoverClearTimer.finished();
} catch (err) {
finished = false;
}
this._hoverClearTimer = null;
if (finished) {
this.setState({hover: false});
this._delayedRefreshRoomList();
}
} else {
this._hoverClearTimer.restart();
}
},

onMouseLeave: function(ev) {
if (this._hoverClearTimer) {
this._hoverClearTimer.abort();
this._hoverClearTimer = null;
}
this.setState({hover: false});

// Refresh the room list just in case the user missed something.
Expand Down Expand Up @@ -774,7 +798,7 @@ module.exports = React.createClass({

return (
<div ref={this._collectResizeContainer} className="mx_RoomList"
onMouseEnter={this.onMouseEnter} onMouseLeave={this.onMouseLeave}>
onMouseMove={this.onMouseMove} onMouseLeave={this.onMouseLeave}>
{ subListComponents }
</div>
);
Expand Down
11 changes: 1 addition & 10 deletions src/components/views/rooms/RoomTile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,6 @@ module.exports = React.createClass({
return statusUser._unstable_statusMessage;
},

onRoomTimeline: function(ev, room) {
if (room !== this.props.room) return;
this.setState({
notificationCount: this.props.room.getUnreadNotificationCount(),
});
},

onRoomName: function(room) {
if (room !== this.props.room) return;
this.setState({
Expand Down Expand Up @@ -159,7 +152,6 @@ module.exports = React.createClass({

componentWillMount: function() {
MatrixClientPeg.get().on("accountData", this.onAccountData);
MatrixClientPeg.get().on("Room.timeline", this.onRoomTimeline);
MatrixClientPeg.get().on("Room.name", this.onRoomName);
ActiveRoomObserver.addListener(this.props.room.roomId, this._onActiveRoomChange);
this.dispatcherRef = dis.register(this.onAction);
Expand All @@ -179,7 +171,6 @@ module.exports = React.createClass({
const cli = MatrixClientPeg.get();
if (cli) {
MatrixClientPeg.get().removeListener("accountData", this.onAccountData);
MatrixClientPeg.get().removeListener("Room.timeline", this.onRoomTimeline);
MatrixClientPeg.get().removeListener("Room.name", this.onRoomName);
}
ActiveRoomObserver.removeListener(this.props.room.roomId, this._onActiveRoomChange);
Expand Down Expand Up @@ -306,7 +297,7 @@ module.exports = React.createClass({

render: function() {
const isInvite = this.props.room.getMyMembership() === "invite";
const notificationCount = this.state.notificationCount;
const notificationCount = this.props.notificationCount;
// var highlightCount = this.props.room.getUnreadNotificationCount("highlight");

const notifBadges = notificationCount > 0 && this._shouldShowNotifBadge();
Expand Down
60 changes: 17 additions & 43 deletions src/ratelimitedfunc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,54 +20,28 @@ limitations under the License.
* to update the interface once for all of them.
*
* Note that the function must not take arguments, since the args
* could be different for each invocarion of the function.
* could be different for each invocation of the function.
*
* The returned function has a 'cancelPendingCall' property which can be called
* on unmount or similar to cancel any pending update.
*/
module.exports = function(f, minIntervalMs) {
this.lastCall = 0;
this.scheduledCall = undefined;

const self = this;
const wrapper = function() {
const now = Date.now();

if (self.lastCall < now - minIntervalMs) {
f.apply(this);
// get the time again now the function has finished, so if it
// took longer than the delay time to execute, it doesn't
// immediately become eligible to run again.
self.lastCall = Date.now();
} else if (self.scheduledCall === undefined) {
self.scheduledCall = setTimeout(
() => {
self.scheduledCall = undefined;
f.apply(this);
// get time again as per above
self.lastCall = Date.now();
},
(self.lastCall + minIntervalMs) - now,
);
}
};

// add the cancelPendingCall property
wrapper.cancelPendingCall = function() {
if (self.scheduledCall) {
clearTimeout(self.scheduledCall);
self.scheduledCall = undefined;
}
import { throttle } from "lodash";
Copy link
Member

Choose a reason for hiding this comment

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

oh, it's throttle here - is there a reason to use different ones in different places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the behaviour we had in place here was the same as throttle, so I preserved that.
For a search field, debounce is usually better as you run the search code less and you can have a smaller delay.


export default function ratelimitedfunc(fn, time) {
const throttledFn = throttle(fn, time, {
leading: true,
trailing: true,
});
const _bind = throttledFn.bind;
throttledFn.bind = function() {
const boundFn = _bind.apply(throttledFn, arguments);
boundFn.cancelPendingCall = throttledFn.cancelPendingCall;
return boundFn;
};

// make sure that cancelPendingCall is copied when react rebinds the
// wrapper
const _bind = wrapper.bind;
wrapper.bind = function() {
const rebound = _bind.apply(this, arguments);
rebound.cancelPendingCall = wrapper.cancelPendingCall;
return rebound;
throttledFn.cancelPendingCall = function() {
throttledFn.cancel();
};

return wrapper;
};
return throttledFn;
}