From eeb9abdf81729502ffd40e565308467d62572a62 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 18 Apr 2016 01:34:45 +0100 Subject: [PATCH 1/5] trailing whitespace --- src/components/structures/RoomView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 5563eaea83..ac848030af 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -293,7 +293,7 @@ module.exports = React.createClass({ // no change } else { - this.setState((state, props) => { + this.setState((state, props) => { return {numUnreadMessages: state.numUnreadMessages + 1}; }); } From b949e7868393ea64ab88f03e0099d6a0bec01af2 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 18 Apr 2016 01:35:40 +0100 Subject: [PATCH 2/5] Improve ordering of memberlist by absolutizing lastActive correctly Change ordering of memberlist to not try to compare lastActive of 'currentlyActive' users, as lastActive may will be a complete lie as it only gets updated when currentlyActive transitions to false (i think?) Remove order by online/idle/offline in favour of "currently active, ordered by power and then alphabetic name, followed by last active, followed by offline" Add commented-out code to track last-spoken-within-a-room ordering. Fix kludges due to SYJS-28 (depends on JS PR landing) --- src/components/views/rooms/EntityTile.js | 11 +- src/components/views/rooms/MemberList.js | 164 +++++++++++++++----- src/components/views/rooms/MemberTile.js | 10 +- src/components/views/rooms/PresenceLabel.js | 2 + 4 files changed, 135 insertions(+), 52 deletions(-) diff --git a/src/components/views/rooms/EntityTile.js b/src/components/views/rooms/EntityTile.js index 47e3fd3350..acc424b098 100644 --- a/src/components/views/rooms/EntityTile.js +++ b/src/components/views/rooms/EntityTile.js @@ -37,7 +37,8 @@ module.exports = React.createClass({ avatarJsx: React.PropTypes.any, // className: React.PropTypes.string, presenceState: React.PropTypes.string, - presenceActiveAgo: React.PropTypes.number, + presenceLastActiveAgo: React.PropTypes.number, + presenceLastTs: React.PropTypes.number, presenceCurrentlyActive: React.PropTypes.bool, showInviteButton: React.PropTypes.bool, shouldComponentUpdate: React.PropTypes.func, @@ -50,7 +51,8 @@ module.exports = React.createClass({ shouldComponentUpdate: function(nextProps, nextState) { return true; }, onClick: function() {}, presenceState: "offline", - presenceActiveAgo: -1, + presenceLastActiveAgo: 0, + presenceLastTs: 0, showInviteButton: false, suppressOnHover: false }; @@ -82,13 +84,16 @@ module.exports = React.createClass({ var nameEl; if (this.state.hover && !this.props.suppressOnHover) { + var activeAgo = this.props.presenceLastActiveAgo ? + (Date.now() - (this.props.presenceLastTs - this.props.presenceLastActiveAgo)) : -1; + mainClassName += " mx_EntityTile_hover"; var PresenceLabel = sdk.getComponent("rooms.PresenceLabel"); nameEl = (
{ this.props.name }
-
diff --git a/src/components/views/rooms/MemberList.js b/src/components/views/rooms/MemberList.js index 0597d897db..e2c94524ba 100644 --- a/src/components/views/rooms/MemberList.js +++ b/src/components/views/rooms/MemberList.js @@ -64,15 +64,19 @@ module.exports = React.createClass({ cli.on("RoomMember.name", this.onRoomMemberName); cli.on("RoomState.events", this.onRoomStateEvent); cli.on("Room", this.onRoom); // invites + cli.on("User.presence", this.onUserPresence); + // cli.on("Room.timeline", this.onRoomTimeline); }, componentWillUnmount: function() { - if (MatrixClientPeg.get()) { - MatrixClientPeg.get().removeListener("Room", this.onRoom); - MatrixClientPeg.get().removeListener("RoomState.members", this.onRoomStateMember); - MatrixClientPeg.get().removeListener("RoomMember.name", this.onRoomMemberName); - MatrixClientPeg.get().removeListener("User.presence", this.userPresenceFn); - MatrixClientPeg.get().removeListener("RoomState.events", this.onRoomStateEvent); + var cli = MatrixClientPeg.get(); + if (cli) { + cli.removeListener("RoomState.members", this.onRoomStateMember); + cli.removeListener("RoomMember.name", this.onRoomMemberName); + cli.removeListener("RoomState.events", this.onRoomStateEvent); + cli.removeListener("Room", this.onRoom); + cli.removeListener("User.presence", this.onUserPresence); + // cli.removeListener("Room.timeline", this.onRoomTimeline); } }, @@ -87,25 +91,45 @@ module.exports = React.createClass({ members: self.roomMembers() }); }, 50); + }, +/* + onRoomTimeline: function(ev, room, toStartOfTimeline, removed, data) { + // ignore anything but real-time updates at the end of the room: + // updates from pagination will happen when the paginate completes. + if (toStartOfTimeline || !data || !data.liveEvent) return; + + // treat any activity from a user as implicit presence to update the + // ordering of the list whenever someone says something. + // Except right now we're not tiebreaking "active now" users in this way + // so don't bother for now. + if (ev.getSender()) { + console.log("implicit presence from " + ev.getSender()); + + var tile = this.refs[ev.getSender()]; + if (tile) { + // work around a race where you might have a room member object + // before the user object exists. XXX: why does this ever happen? + var all_members = room.currentState.members; + var userId = ev.getSender(); + if (all_members[userId].user === null) { + all_members[userId].user = MatrixClientPeg.get().getUser(userId); + } + this._updateList(); // reorder the membership list + } + } + }, +*/ + + onUserPresence(event, user) { // Attach a SINGLE listener for global presence changes then locate the // member tile and re-render it. This is more efficient than every tile // evar attaching their own listener. - function updateUserState(event, user) { - // XXX: evil hack to track the age of this presence info. - // this should be removed once syjs-28 is resolved in the JS SDK itself. - user.lastPresenceTs = Date.now(); - - var tile = self.refs[user.userId]; - - if (tile) { - self._updateList(); // reorder the membership list - } + console.log("explicit presence from " + user.userId); + var tile = this.refs[user.userId]; + if (tile) { + this._updateList(); // reorder the membership list } - // FIXME: we should probably also reset 'lastActiveAgo' to zero whenever - // we see a typing notif from a user, as we don't get presence updates for those. - MatrixClientPeg.get().on("User.presence", updateUserState); - this.userPresenceFn = updateUserState; }, onRoom: function(room) { @@ -133,6 +157,7 @@ module.exports = React.createClass({ }, _updateList: new rate_limited_func(function() { + console.log("Updating memberlist"); this.memberDict = this.getMemberDict(); var self = this; @@ -266,7 +291,6 @@ module.exports = React.createClass({ var all_members = room.currentState.members; - // XXX: evil hack until SYJS-28 is fixed Object.keys(all_members).map(function(userId) { // work around a race where you might have a room member object // before the user object exists. This may or may not cause @@ -275,9 +299,8 @@ module.exports = React.createClass({ all_members[userId].user = MatrixClientPeg.get().getUser(userId); } - if (all_members[userId].user && !all_members[userId].user.lastPresenceTs) { - all_members[userId].user.lastPresenceTs = Date.now(); - } + // XXX: this user may have no lastPresenceTs value! + // the right solution here is to fix the race rather than leave it as 0 }); return all_members; @@ -288,7 +311,7 @@ module.exports = React.createClass({ var all_user_ids = Object.keys(all_members); var ConferenceHandler = CallHandler.getConferenceHandler(); - if (this.memberSort) all_user_ids.sort(this.memberSort); + all_user_ids.sort(this.memberSort(Date.now())); var to_display = []; var count = 0; @@ -325,27 +348,82 @@ module.exports = React.createClass({ }); }, - memberSort: function(userIdA, userIdB) { - var userA = this.memberDict[userIdA].user; - var userB = this.memberDict[userIdB].user; - - var presenceMap = { - online: 3, - unavailable: 2, - offline: 1 - }; - - var presenceOrdA = userA ? presenceMap[userA.presence] : 0; - var presenceOrdB = userB ? presenceMap[userB.presence] : 0; - - if (presenceOrdA != presenceOrdB) { - return presenceOrdB - presenceOrdA; + memberString: function(member) { + if (!member) { + return "(null)"; } + else { + return "(" + member.name + ", " + member.powerLevel + ", " + member.user.lastActiveAgo + ", " + member.user.currentlyActive + ")"; + } + }, - var lastActiveTsA = userA && userA.lastActiveAgo ? userA.lastPresenceTs - userA.lastActiveAgo : 0; - var lastActiveTsB = userB && userB.lastActiveAgo ? userB.lastPresenceTs - userB.lastActiveAgo : 0; + memberSort: function(now) { + return (userIdA, userIdB) => { + // order by last active, with "active now" first. + // ...and then by power + // ...and then alphabetically. + // We could tiebreak instead by "last recently spoken in this room" if we wanted to. - return lastActiveTsB - lastActiveTsA; + var memberA = this.memberDict[userIdA]; + var memberB = this.memberDict[userIdB]; + var userA = memberA.user; + var userB = memberB.user; + + if (!userA || !userB) { + console.log("comparing " + memberA.name + " user=" + memberA.user + " with " + memberB.name + " user=" + memberB.user); + } + + if (!userA && !userB) return 0; + if (userA && !userB) return -1; + if (!userA && userB) return 1; + + console.log("comparing " + this.memberString(memberA) + " and " + this.memberString(memberB)); + + if (userA.currentlyActive && userB.currentlyActive) { + console.log(memberA.name + " and " + memberB.name + " are both active"); + if (memberA.powerLevel === memberB.powerLevel) { + console.log(memberA + " and " + memberB + " have same power level"); + if (memberA.name && memberB.name) { + console.log("comparing names: " + memberA.name + " and " + memberB.name); + return memberA.name.localeCompare(memberB.name); + } + else { + return 0; + } + } + else { + console.log("comparing power: " + memberA.powerLevel + " and " + memberB.powerLevel); + return memberB.powerLevel - memberA.powerLevel; + } + } + + if (userA.currentlyActive && !userB.currentlyActive) return -1; + if (!userA.currentlyActive && userB.currentlyActive) return 1; + + // For now, let's just order things by timestamp. It's really annoying + // that a user disappears from sight just because they temporarily go offline + /* + var presenceMap = { + online: 3, + unavailable: 2, + offline: 1 + }; + + var presenceOrdA = userA ? presenceMap[userA.presence] : 0; + var presenceOrdB = userB ? presenceMap[userB.presence] : 0; + + if (presenceOrdA != presenceOrdB) { + return presenceOrdB - presenceOrdA; + } + */ + + var lastActiveTsA = userA && userA.lastActiveAgo ? (now - (userA.lastPresenceTs - userA.lastActiveAgo)) : Number.MAX_SAFE_INTEGER; + var lastActiveTsB = userB && userB.lastActiveAgo ? (now - (userB.lastPresenceTs - userB.lastActiveAgo)) : Number.MAX_SAFE_INTEGER; + + console.log("comparing ts: " + lastActiveTsA + " and " + lastActiveTsB); + + return lastActiveTsA - lastActiveTsB; + } }, onSearchQueryChanged: function(input) { diff --git a/src/components/views/rooms/MemberTile.js b/src/components/views/rooms/MemberTile.js index b19a215668..cf79394228 100644 --- a/src/components/views/rooms/MemberTile.js +++ b/src/components/views/rooms/MemberTile.js @@ -82,15 +82,13 @@ module.exports = React.createClass({ if (member.user) { this.user_last_modified_time = member.user.getLastModifiedTime(); - - // FIXME: make presence data update whenever User.presence changes... - active = member.user.lastActiveAgo ? - (Date.now() - (member.user.lastPresenceTs - member.user.lastActiveAgo)) : -1; } this.member_last_modified_time = member.getLastModifiedTime(); - + return ( - diff --git a/src/components/views/rooms/PresenceLabel.js b/src/components/views/rooms/PresenceLabel.js index ac0410c1f7..2ece4c771e 100644 --- a/src/components/views/rooms/PresenceLabel.js +++ b/src/components/views/rooms/PresenceLabel.js @@ -76,6 +76,8 @@ module.exports = React.createClass({ render: function() { if (this.props.activeAgo >= 0) { var ago = this.props.currentlyActive ? "now" : (this.getDuration(this.props.activeAgo) + " ago"); + // var ago = this.getDuration(this.props.activeAgo) + " ago"; + // if (this.props.currentlyActive) ago += " (now?)"; return (
{ this.getPrettyPresence(this.props.presenceState) } { ago } From b2d0950a4670e5b3a7e3e700f1d139d451282f98 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 18 Apr 2016 01:45:46 +0100 Subject: [PATCH 3/5] oops, remove debug logging --- src/components/views/rooms/MemberList.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/components/views/rooms/MemberList.js b/src/components/views/rooms/MemberList.js index e2c94524ba..d3f9d887af 100644 --- a/src/components/views/rooms/MemberList.js +++ b/src/components/views/rooms/MemberList.js @@ -104,7 +104,7 @@ module.exports = React.createClass({ // Except right now we're not tiebreaking "active now" users in this way // so don't bother for now. if (ev.getSender()) { - console.log("implicit presence from " + ev.getSender()); + // console.log("implicit presence from " + ev.getSender()); var tile = this.refs[ev.getSender()]; if (tile) { @@ -125,7 +125,7 @@ module.exports = React.createClass({ // Attach a SINGLE listener for global presence changes then locate the // member tile and re-render it. This is more efficient than every tile // evar attaching their own listener. - console.log("explicit presence from " + user.userId); + // console.log("explicit presence from " + user.userId); var tile = this.refs[user.userId]; if (tile) { this._updateList(); // reorder the membership list @@ -157,7 +157,7 @@ module.exports = React.createClass({ }, _updateList: new rate_limited_func(function() { - console.log("Updating memberlist"); + // console.log("Updating memberlist"); this.memberDict = this.getMemberDict(); var self = this; @@ -369,22 +369,22 @@ module.exports = React.createClass({ var userA = memberA.user; var userB = memberB.user; - if (!userA || !userB) { - console.log("comparing " + memberA.name + " user=" + memberA.user + " with " + memberB.name + " user=" + memberB.user); - } + // if (!userA || !userB) { + // console.log("comparing " + memberA.name + " user=" + memberA.user + " with " + memberB.name + " user=" + memberB.user); + // } if (!userA && !userB) return 0; if (userA && !userB) return -1; if (!userA && userB) return 1; - console.log("comparing " + this.memberString(memberA) + " and " + this.memberString(memberB)); + // console.log("comparing " + this.memberString(memberA) + " and " + this.memberString(memberB)); if (userA.currentlyActive && userB.currentlyActive) { - console.log(memberA.name + " and " + memberB.name + " are both active"); + // console.log(memberA.name + " and " + memberB.name + " are both active"); if (memberA.powerLevel === memberB.powerLevel) { - console.log(memberA + " and " + memberB + " have same power level"); + // console.log(memberA + " and " + memberB + " have same power level"); if (memberA.name && memberB.name) { - console.log("comparing names: " + memberA.name + " and " + memberB.name); + // console.log("comparing names: " + memberA.name + " and " + memberB.name); return memberA.name.localeCompare(memberB.name); } else { @@ -392,7 +392,7 @@ module.exports = React.createClass({ } } else { - console.log("comparing power: " + memberA.powerLevel + " and " + memberB.powerLevel); + // console.log("comparing power: " + memberA.powerLevel + " and " + memberB.powerLevel); return memberB.powerLevel - memberA.powerLevel; } } @@ -420,7 +420,7 @@ module.exports = React.createClass({ var lastActiveTsA = userA && userA.lastActiveAgo ? (now - (userA.lastPresenceTs - userA.lastActiveAgo)) : Number.MAX_SAFE_INTEGER; var lastActiveTsB = userB && userB.lastActiveAgo ? (now - (userB.lastPresenceTs - userB.lastActiveAgo)) : Number.MAX_SAFE_INTEGER; - console.log("comparing ts: " + lastActiveTsA + " and " + lastActiveTsB); + // console.log("comparing ts: " + lastActiveTsA + " and " + lastActiveTsB); return lastActiveTsA - lastActiveTsB; } From 60f92fd15bf0d91d9797f7bb6eda0e7e1778f252 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 18 Apr 2016 14:07:20 +0100 Subject: [PATCH 4/5] PR feedback --- src/components/views/rooms/MemberList.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/components/views/rooms/MemberList.js b/src/components/views/rooms/MemberList.js index d3f9d887af..c490a72e46 100644 --- a/src/components/views/rooms/MemberList.js +++ b/src/components/views/rooms/MemberList.js @@ -311,7 +311,7 @@ module.exports = React.createClass({ var all_user_ids = Object.keys(all_members); var ConferenceHandler = CallHandler.getConferenceHandler(); - all_user_ids.sort(this.memberSort(Date.now())); + all_user_ids.sort(this.memberSort); var to_display = []; var count = 0; @@ -357,8 +357,10 @@ module.exports = React.createClass({ } }, - memberSort: function(now) { - return (userIdA, userIdB) => { + // returns -1 if a < b. + // 0 if a and b are equivalent + // 1 if a > b. + memberSort: function(userIdA, userIdB) { // order by last active, with "active now" first. // ...and then by power // ...and then alphabetically. @@ -417,8 +419,8 @@ module.exports = React.createClass({ } */ - var lastActiveTsA = userA && userA.lastActiveAgo ? (now - (userA.lastPresenceTs - userA.lastActiveAgo)) : Number.MAX_SAFE_INTEGER; - var lastActiveTsB = userB && userB.lastActiveAgo ? (now - (userB.lastPresenceTs - userB.lastActiveAgo)) : Number.MAX_SAFE_INTEGER; + var lastActiveTsA = userA && userA.lastActiveAgo ? (userA.lastPresenceTs - userA.lastActiveAgo) : Number.MAX_SAFE_INTEGER; + var lastActiveTsB = userB && userB.lastActiveAgo ? (userB.lastPresenceTs - userB.lastActiveAgo) : Number.MAX_SAFE_INTEGER; // console.log("comparing ts: " + lastActiveTsA + " and " + lastActiveTsB); From a95c45eb9680517518edcaede18ef1d4b94712dd Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 18 Apr 2016 14:48:55 +0100 Subject: [PATCH 5/5] final(?) PR fixes --- src/components/views/rooms/MemberList.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/components/views/rooms/MemberList.js b/src/components/views/rooms/MemberList.js index c490a72e46..8f392aac95 100644 --- a/src/components/views/rooms/MemberList.js +++ b/src/components/views/rooms/MemberList.js @@ -357,9 +357,9 @@ module.exports = React.createClass({ } }, - // returns -1 if a < b. - // 0 if a and b are equivalent - // 1 if a > b. + // returns negative if a comes before b, + // returns 0 if a and b are equivalent in ordering + // returns positive if a comes after b. memberSort: function(userIdA, userIdB) { // order by last active, with "active now" first. // ...and then by power @@ -419,12 +419,12 @@ module.exports = React.createClass({ } */ - var lastActiveTsA = userA && userA.lastActiveAgo ? (userA.lastPresenceTs - userA.lastActiveAgo) : Number.MAX_SAFE_INTEGER; - var lastActiveTsB = userB && userB.lastActiveAgo ? (userB.lastPresenceTs - userB.lastActiveAgo) : Number.MAX_SAFE_INTEGER; + var lastActiveTsA = userA && userA.lastActiveTs ? userA.lastActiveTs : 0; + var lastActiveTsB = userB && userB.lastActiveTs ? userB.lastActiveTs : 0; // console.log("comparing ts: " + lastActiveTsA + " and " + lastActiveTsB); - return lastActiveTsA - lastActiveTsB; + return lastActiveTsB - lastActiveTsA; } },