From beecbc7cd720dc3e255220d314a79d9dd362c566 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 16 Nov 2016 14:42:30 +0000 Subject: [PATCH] Fix join/part collapsing regressions (#553) * Fix join/part collapsing regressions * Simplify loop * Explain e,e * Explain return null in _renderSummary * Kill it properly * Move . to _renderSummary * Only use the first and last events to decide whether a net change has occured * Do not sort events by TS before summarising * fix loop and comment * remove data-number-events * Better explanation comment in _renderSummary * Less tortuous comment --- src/components/structures/MessagePanel.js | 32 ++++++++++----- src/components/views/avatars/MemberAvatar.js | 4 +- .../views/elements/MemberEventListSummary.js | 41 ++++++++++++++----- 3 files changed, 53 insertions(+), 24 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index ee9d5f1ff4..1e0db2321d 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -229,6 +229,7 @@ module.exports = React.createClass({ _getEventTiles: function() { var EventTile = sdk.getComponent('rooms.EventTile'); + var DateSeparator = sdk.getComponent('messages.DateSeparator'); const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary'); this.eventNodes = {}; @@ -278,8 +279,8 @@ module.exports = React.createClass({ var isMembershipChange = (e) => e.getType() === 'm.room.member' - && ['join', 'leave'].indexOf(e.event.content.membership) !== -1 - && (!e.event.prev_content || e.event.content.membership !== e.event.prev_content.membership); + && ['join', 'leave'].indexOf(e.getContent().membership) !== -1 + && (!e.getPrevContent() || e.getContent().membership !== e.getPrevContent().membership); for (i = 0; i < this.props.events.length; i++) { var mxEv = this.props.events[i]; @@ -294,23 +295,32 @@ module.exports = React.createClass({ // Wrap consecutive member events in a ListSummary if (isMembershipChange(mxEv)) { - let summarisedEvents = [mxEv]; - i++; - for (;i < this.props.events.length; i++) { - let collapsedMxEv = this.props.events[i]; + let ts1 = mxEv.getTs(); - if (!isMembershipChange(collapsedMxEv)) { - i--; + if (this._wantsDateSeparator(prevEvent, ts1)) { + let dateSeparator =
    • ; + ret.push(dateSeparator); + } + + let summarisedEvents = [mxEv]; + for (;i + 1 < this.props.events.length; i++) { + let collapsedMxEv = this.props.events[i + 1]; + + if (!isMembershipChange(collapsedMxEv) || + this._wantsDateSeparator(this.props.events[i], collapsedMxEv.getTs())) { break; } summarisedEvents.push(collapsedMxEv); } - // At this point, i = this.props.events.length OR i = the index of the last - // MembershipChange in a sequence of MembershipChanges + // At this point, i = the index of the last event in the summary sequence let eventTiles = summarisedEvents.map( (e) => { - let ret = this._getTilesForEvent(prevEvent, e); + // In order to prevent DateSeparators from appearing in the expanded form + // of MemberEventListSummary, render each member event as if the previous + // one was itself. This way, the timestamp of the previous event === the + // timestamp of the current event, and no DateSeperator is inserted. + let ret = this._getTilesForEvent(e, e); prevEvent = e; return ret; } diff --git a/src/components/views/avatars/MemberAvatar.js b/src/components/views/avatars/MemberAvatar.js index 161c37ef55..1f6736138d 100644 --- a/src/components/views/avatars/MemberAvatar.js +++ b/src/components/views/avatars/MemberAvatar.js @@ -69,9 +69,9 @@ module.exports = React.createClass({ render: function() { var BaseAvatar = sdk.getComponent("avatars.BaseAvatar"); - var {member, onClick, ...otherProps} = this.props; + var {member, onClick, viewUserOnClick, ...otherProps} = this.props; - if (this.props.viewUserOnClick) { + if (viewUserOnClick) { onClick = () => { dispatcher.dispatch({ action: 'view_user', diff --git a/src/components/views/elements/MemberEventListSummary.js b/src/components/views/elements/MemberEventListSummary.js index f50f56ffb4..4a4e8bfd1e 100644 --- a/src/components/views/elements/MemberEventListSummary.js +++ b/src/components/views/elements/MemberEventListSummary.js @@ -107,10 +107,20 @@ module.exports = React.createClass({ ); } + + // The joinEvents and leaveEvents are representative of the net movement + // per-user, and so it is possible that the total net movement is nil, + // whilst there are some events in the expanded list. If the total net + // movement is nil, then neither joinSummary nor leaveSummary will be + // truthy, so return null. + if (!joinSummary && !leaveSummary) { + return null; + } + return ( {joinSummary}{joinSummary && leaveSummary?'; ':''} - {leaveSummary} + {leaveSummary}.  ); }, @@ -140,15 +150,24 @@ module.exports = React.createClass({ // Reorder events so that joins come before leaves let eventsToRender = this.props.events; - // Filter out those who joined, then left - let filteredEvents = eventsToRender.filter( - (e) => { - return eventsToRender.filter( - (e2) => { - return e.getSender() === e2.getSender() - && e.event.content.membership !== e2.event.content.membership; - } - ).length === 0; + // Create an array of events that are not "cancelled-out" by another + // A join of sender S is cancelled out by a leave of sender S etc. + let filteredEvents = []; + let senders = new Set(eventsToRender.map((e) => e.getSender())); + senders.forEach( + (userId) => { + // Only push the last event if it isn't the same membership as the first + + let userEvents = eventsToRender.filter((e) => { + return e.getSender() === userId; + }); + + let firstEvent = userEvents[0]; + let lastEvent = userEvents[userEvents.length - 1]; + + if (firstEvent.getContent().membership !== lastEvent.getContent().membership) { + filteredEvents.push(lastEvent); + } } ); @@ -194,7 +213,7 @@ module.exports = React.createClass({ {avatars} - {summary}{joinAndLeft? '. ' + joinAndLeft + ' ' + noun + ' joined and left' : ''} + {summary}{joinAndLeft ? joinAndLeft + ' ' + noun + ' joined and left' : ''}   {toggleButton}