From c07e5d4992c35382a2e502cb305c28a6d8300820 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 14 Dec 2016 15:31:35 +0000 Subject: [PATCH 1/7] Improve the performance of MemberEventListSummary - The MessagePanel now uses the same key for the MELS instances rendered so that entirely new instances are not created, they are simply passed new props (namely when new events arrive). - MELS itself now uses `shouldComponentUpdate` so that it only updates if it is given a different number of events to previous or if it is toggled to expand. --- src/components/structures/MessagePanel.js | 8 +- .../views/elements/MemberEventListSummary.js | 158 ++++++++++-------- 2 files changed, 95 insertions(+), 71 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 53bf560673..a2e6c1bc7e 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -296,6 +296,12 @@ module.exports = React.createClass({ // Wrap consecutive member events in a ListSummary if (isMembershipChange(mxEv)) { let ts1 = mxEv.getTs(); + // Ensure that the key of the MemberEventListSummary does not change with new + // member events. This will prevent it from being re-created necessarily, and + // instead will allow new props to be provided. In turn, the shouldComponentUpdate + // method on MELS can be used to prevent unnecessary renderings. + // `prevEvent` at this point is a non-member event or null. + const key = "memberlistsummary-" + (prevEvent ? prevEvent.getId() : ""); if (this._wantsDateSeparator(prevEvent, ts1)) { let dateSeparator =
    • ; @@ -332,7 +338,7 @@ module.exports = React.createClass({ ret.push( {eventTiles} diff --git a/src/components/views/elements/MemberEventListSummary.js b/src/components/views/elements/MemberEventListSummary.js index 69bcd7c203..50fd93f01c 100644 --- a/src/components/views/elements/MemberEventListSummary.js +++ b/src/components/views/elements/MemberEventListSummary.js @@ -42,7 +42,7 @@ module.exports = React.createClass({ return { summaryLength: 3, threshold: 3, - avatarsMaxLength: 5 + avatarsMaxLength: 5, }; }, @@ -144,88 +144,106 @@ module.exports = React.createClass({ ); }, - render: function() { - let summary = null; - - // Reorder events so that joins come before leaves - let eventsToRender = this.props.events; - - // 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) => { - let userEvents = eventsToRender.filter((e) => { - return e.getSender() === userId; - }); - - // NB: These may be the same event, in which case the lastEvent is used - // because prev_content should != content - let firstEvent = userEvents[0]; - let lastEvent = userEvents[userEvents.length - 1]; - - // Membership BEFORE eventsToRender - let previousMembership = firstEvent.getPrevContent().membership || "leave"; - - // Otherwise, if the last membership event differs from previousMembership, - // use that. - if (previousMembership !== lastEvent.getContent().membership) { - filteredEvents.push(lastEvent); - } - } + shouldComponentUpdate: function(nextProps, nextState) { + return ( + nextProps.events.length !== this.props.events.length || + nextState.expanded !== this.state.expanded ); + }, - let joinAndLeft = (eventsToRender.length - filteredEvents.length) / 2; - if (joinAndLeft <= 0 || joinAndLeft % 1 !== 0) { - joinAndLeft = null; - } - - let joinEvents = filteredEvents.filter((ev) => { - return ev.event.content.membership === 'join'; - }); - - let leaveEvents = filteredEvents.filter((ev) => { - return ev.event.content.membership === 'leave'; - }); - + render: function() { + let eventsToRender = this.props.events; let fewEvents = eventsToRender.length < this.props.threshold; let expanded = this.state.expanded || fewEvents; - let expandedEvents = null; + let expandedEvents = null; if (expanded) { expandedEvents = this.props.children; } - let avatars = this._renderAvatars(joinEvents.concat(leaveEvents)); - - let toggleButton = null; - let summaryContainer = null; - if (!fewEvents) { - summary = this._renderSummary(joinEvents, leaveEvents); - toggleButton = ( - - {expanded ? 'collapse' : 'expand'} - - ); - let plural = (joinEvents.length + leaveEvents.length > 0) ? 'others' : 'users'; - let noun = (joinAndLeft === 1 ? 'user' : plural); - - summaryContainer = ( -
    -
    - - {avatars} - - - {summary}{joinAndLeft ? joinAndLeft + ' ' + noun + ' joined and left' : ''} -   - {toggleButton} -
    + if (fewEvents) { + return ( +
    + {expandedEvents}
    ); } + // Map user IDs to the first and last member events in eventsToRender for each user + let userEvents = { + // $userId : {first : e0, last : e1} + }; + + eventsToRender.forEach((e) => { + const userId = e.getSender(); + // Initialise a user's events + if (!userEvents[userId]) { + userEvents[userId] = {first: null, last: null}; + } + if (!userEvents[userId].first) { + userEvents[userId].first = e; + } else { + userEvents[userId].last = e; + } + }); + + // Populate the join/leave event arrays with events that represent what happened + // overall to a user's membership. If no events are added to either array for a + // particular user, they will be considered a user that "joined and left". + let joinEvents = []; + let leaveEvents = []; + let joinedAndLeft = 0; + let senders = Object.keys(userEvents); + senders.forEach( + (userId) => { + let firstEvent = userEvents[userId].first; + let lastEvent = userEvents[userId].last; + // Only one membership event was recorded for this userId + if (!lastEvent) { + lastEvent = firstEvent; + } + + // Membership BEFORE eventsToRender + let previousMembership = firstEvent.getPrevContent().membership || "leave"; + + // If the last membership event differs from previousMembership, use that. + if (previousMembership !== lastEvent.getContent().membership) { + if (lastEvent.event.content.membership === 'join') { + joinEvents.push(lastEvent); + } else if (lastEvent.event.content.membership === 'leave') { + leaveEvents.push(lastEvent); + } + } else { + // Increment the number of users whose membership change was nil overall + joinedAndLeft++; + } + } + ); + + let avatars = this._renderAvatars(joinEvents.concat(leaveEvents)); + let summary = this._renderSummary(joinEvents, leaveEvents); + let toggleButton = ( + + {expanded ? 'collapse' : 'expand'} + + ); + let plural = (joinEvents.length + leaveEvents.length > 0) ? 'others' : 'users'; + let noun = (joinedAndLeft === 1 ? 'user' : plural); + + let summaryContainer = ( +
    +
    + + {avatars} + + + {summary}{joinedAndLeft ? joinedAndLeft + ' ' + noun + ' joined and left' : ''} +   + {toggleButton} +
    +
    + ); + return (
    {summaryContainer} From 86739e7d1e72478c59bdaa029ba62e1c9fd1143f Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 15 Dec 2016 13:15:00 +0000 Subject: [PATCH 2/7] Simplify handling of only one member event --- src/components/views/elements/MemberEventListSummary.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/components/views/elements/MemberEventListSummary.js b/src/components/views/elements/MemberEventListSummary.js index 50fd93f01c..7811044fda 100644 --- a/src/components/views/elements/MemberEventListSummary.js +++ b/src/components/views/elements/MemberEventListSummary.js @@ -182,9 +182,8 @@ module.exports = React.createClass({ } if (!userEvents[userId].first) { userEvents[userId].first = e; - } else { - userEvents[userId].last = e; } + userEvents[userId].last = e; }); // Populate the join/leave event arrays with events that represent what happened @@ -198,10 +197,6 @@ module.exports = React.createClass({ (userId) => { let firstEvent = userEvents[userId].first; let lastEvent = userEvents[userId].last; - // Only one membership event was recorded for this userId - if (!lastEvent) { - lastEvent = firstEvent; - } // Membership BEFORE eventsToRender let previousMembership = firstEvent.getPrevContent().membership || "leave"; From e7564f4dc531a76726d47d0c236dfcde9a78c96a Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 15 Dec 2016 13:16:36 +0000 Subject: [PATCH 3/7] Spelling --- src/components/structures/MessagePanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index a2e6c1bc7e..edd227b58e 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -297,7 +297,7 @@ module.exports = React.createClass({ if (isMembershipChange(mxEv)) { let ts1 = mxEv.getTs(); // Ensure that the key of the MemberEventListSummary does not change with new - // member events. This will prevent it from being re-created necessarily, and + // member events. This will prevent it from being re-created unnecessarily, and // instead will allow new props to be provided. In turn, the shouldComponentUpdate // method on MELS can be used to prevent unnecessary renderings. // `prevEvent` at this point is a non-member event or null. From 55f85befc1c3e512cb63477e08ea884bf8a99778 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 15 Dec 2016 14:43:44 +0000 Subject: [PATCH 4/7] Allow component to update if currently expanded or if about to collapse --- src/components/views/elements/MemberEventListSummary.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/components/views/elements/MemberEventListSummary.js b/src/components/views/elements/MemberEventListSummary.js index 7811044fda..27c50fe97c 100644 --- a/src/components/views/elements/MemberEventListSummary.js +++ b/src/components/views/elements/MemberEventListSummary.js @@ -145,9 +145,13 @@ module.exports = React.createClass({ }, shouldComponentUpdate: function(nextProps, nextState) { + // Update if + // - The number of summarised events has changed + // - or if the summary is currently expanded + // - or if the summary is about to toggle to become collapsed return ( nextProps.events.length !== this.props.events.length || - nextState.expanded !== this.state.expanded + this.state.expanded || nextState.expanded ); }, From 88aeb6417e8d233e041096ffc0096f9eed879f5d Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 15 Dec 2016 18:23:54 +0000 Subject: [PATCH 5/7] Use the first member event ID or "initial" in the MELS key --- src/components/structures/MessagePanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index edd227b58e..9eacca2139 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -301,7 +301,7 @@ module.exports = React.createClass({ // instead will allow new props to be provided. In turn, the shouldComponentUpdate // method on MELS can be used to prevent unnecessary renderings. // `prevEvent` at this point is a non-member event or null. - const key = "memberlistsummary-" + (prevEvent ? prevEvent.getId() : ""); + const key = "memberlistsummary-" + (prevEvent ? mxEv.getId() : "initial"); if (this._wantsDateSeparator(prevEvent, ts1)) { let dateSeparator =
    • ; From 6b52b247e729e586134e8be34bfadd8c7643beac Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 15 Dec 2016 18:26:41 +0000 Subject: [PATCH 6/7] Update comment on MELS key --- src/components/structures/MessagePanel.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 9eacca2139..c681f5105d 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -300,8 +300,11 @@ module.exports = React.createClass({ // member events. This will prevent it from being re-created unnecessarily, and // instead will allow new props to be provided. In turn, the shouldComponentUpdate // method on MELS can be used to prevent unnecessary renderings. - // `prevEvent` at this point is a non-member event or null. - const key = "memberlistsummary-" + (prevEvent ? mxEv.getId() : "initial"); + // + // Whilst back-paginating with a MELS at the top of the panel, prevEvent will be null, + // so use the key "membereventlistsummary-initial". Otherwise, use the ID of the first + // membership event, which will not change during forward pagination. + const key = "membereventlistsummary-" + (prevEvent ? mxEv.getId() : "initial"); if (this._wantsDateSeparator(prevEvent, ts1)) { let dateSeparator =
    • ; From 7475056bb443a0da4e88dc5082643ed28564e4d4 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 15 Dec 2016 18:33:13 +0000 Subject: [PATCH 7/7] MELS component should update if there are fewEvents, effectively expanding the summary --- src/components/views/elements/MemberEventListSummary.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/components/views/elements/MemberEventListSummary.js b/src/components/views/elements/MemberEventListSummary.js index 27c50fe97c..bff2ce3d05 100644 --- a/src/components/views/elements/MemberEventListSummary.js +++ b/src/components/views/elements/MemberEventListSummary.js @@ -149,9 +149,11 @@ module.exports = React.createClass({ // - The number of summarised events has changed // - or if the summary is currently expanded // - or if the summary is about to toggle to become collapsed + // - or if there are fewEvents, meaning the child eventTiles are shown as-is return ( nextProps.events.length !== this.props.events.length || - this.state.expanded || nextState.expanded + this.state.expanded || nextState.expanded || + nextProps.events.length < this.props.threshold ); },