From 4c5046afd5b55aa157afe052fa0ba041f92d8a81 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 11 Feb 2020 15:19:32 -0500 Subject: [PATCH 1/3] refactor event grouping into separate helper classes --- src/components/structures/MessagePanel.js | 430 +++++++++++++--------- 1 file changed, 248 insertions(+), 182 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index a13278cf68e..4f4136ed662 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -403,8 +403,6 @@ export default class MessagePanel extends React.Component { _getEventTiles() { const DateSeparator = sdk.getComponent('messages.DateSeparator'); - const EventListSummary = sdk.getComponent('views.elements.EventListSummary'); - const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary'); this.eventNodes = {}; @@ -447,199 +445,48 @@ export default class MessagePanel extends React.Component { this._readReceiptsByEvent = this._getReadReceiptsByShownEvent(); } + let grouper = null; + for (i = 0; i < this.props.events.length; i++) { const mxEv = this.props.events[i]; const eventId = mxEv.getId(); const last = (mxEv === lastShownEvent); - // Wrap initial room creation events into an EventListSummary - // Grouping only events sent by the same user that sent the `m.room.create` and only until - // the first non-state event or membership event which is not regarding the sender of the `m.room.create` event - const shouldGroup = (ev) => { - if (ev.getType() === "m.room.member" - && (ev.getStateKey() !== mxEv.getSender() || ev.getContent()["membership"] !== "join")) { - return false; - } - if (ev.isState() && ev.getSender() === mxEv.getSender()) { - return true; - } - return false; - }; - // events that we include in the group but then eject out and place - // above the group. - const shouldEject = (ev) => { - if (ev.getType() === "m.room.encryption") return true; - return false; - }; - if (mxEv.getType() === "m.room.create") { - let summaryReadMarker = null; - const ts1 = mxEv.getTs(); - - if (this._wantsDateSeparator(prevEvent, mxEv.getDate())) { - const dateSeparator =
    • ; - ret.push(dateSeparator); - } - - // If RM event is the first in the summary, append the RM after the summary - summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(mxEv.getId()); - - // If this m.room.create event should be shown (room upgrade) then show it before the summary - if (this._shouldShowEvent(mxEv)) { - // pass in the mxEv as prevEvent as well so no extra DateSeparator is rendered - ret.push(...this._getTilesForEvent(mxEv, mxEv, false)); - } - - const summarisedEvents = []; // Don't add m.room.create here as we don't want it inside the summary - const ejectedEvents = []; - for (;i + 1 < this.props.events.length; i++) { - const collapsedMxEv = this.props.events[i + 1]; - - // Ignore redacted/hidden member events - if (!this._shouldShowEvent(collapsedMxEv)) { - // If this hidden event is the RM and in or at end of a summary put RM after the summary. - summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); - continue; - } - - if (!shouldGroup(collapsedMxEv) || this._wantsDateSeparator(mxEv, collapsedMxEv.getDate())) { - break; - } - - // If RM event is in the summary, mark it as such and the RM will be appended after the summary. - summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); - - if (shouldEject(collapsedMxEv)) { - ejectedEvents.push(collapsedMxEv); - } else { - summarisedEvents.push(collapsedMxEv); - } - } - - // At this point, i = the index of the last event in the summary sequence - const eventTiles = summarisedEvents.map((e) => { - // In order to prevent DateSeparators from appearing in the expanded form - // of EventListSummary, 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 DateSeparator is inserted. - return this._getTilesForEvent(e, e, e === lastShownEvent); - }).reduce((a, b) => a.concat(b), []); - - for (const ejected of ejectedEvents) { - ret.push(...this._getTilesForEvent(mxEv, ejected, last)); + if (grouper) { + if (grouper.shouldGroup(mxEv)) { + grouper.add(mxEv); + continue; + } else { + // not part of group, so get the group tiles, close the + // group, and continue like a normal event + ret.push(...grouper.getTiles()); + prevEvent = grouper.getNewPrevEvent(); + grouper = null; } - - // Get sender profile from the latest event in the summary as the m.room.create doesn't contain one - const ev = this.props.events[i]; - ret.push( - { eventTiles } - ); - - if (summaryReadMarker) { - ret.push(summaryReadMarker); - } - - prevEvent = mxEv; - continue; } - const wantTile = this._shouldShowEvent(mxEv); - - // Wrap consecutive member events in a ListSummary, ignore if redacted - if (isMembershipChange(mxEv) && wantTile) { - let summaryReadMarker = null; - const 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 unnecessarily, and - // instead will allow new props to be provided. In turn, the shouldComponentUpdate - // method on MELS can be used to prevent unnecessary renderings. - // - // 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, mxEv.getDate())) { - const dateSeparator =
    • ; - ret.push(dateSeparator); + for (const Grouper of groupers) { + if (Grouper.canStartGroup(this, mxEv)) { + grouper = new Grouper(this, mxEv, prevEvent, lastShownEvent); } - - // If RM event is the first in the MELS, append the RM after MELS - summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(mxEv.getId()); - - const summarisedEvents = [mxEv]; - for (;i + 1 < this.props.events.length; i++) { - const collapsedMxEv = this.props.events[i + 1]; - - // Ignore redacted/hidden member events - if (!this._shouldShowEvent(collapsedMxEv)) { - // If this hidden event is the RM and in or at end of a MELS put RM after MELS. - summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); - continue; - } - - if (!isMembershipChange(collapsedMxEv) || - this._wantsDateSeparator(mxEv, collapsedMxEv.getDate())) { - break; - } - - // If RM event is in MELS mark it as such and the RM will be appended after MELS. - summaryReadMarker = summaryReadMarker || this._readMarkerForEvent(collapsedMxEv.getId()); - - summarisedEvents.push(collapsedMxEv); - } - - let highlightInMels = false; - - // At this point, i = the index of the last event in the summary sequence - let eventTiles = summarisedEvents.map((e) => { - if (e.getId() === this.props.highlightedEventId) { - highlightInMels = true; - } - // 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 DateSeparator is inserted. - return this._getTilesForEvent(e, e, e === lastShownEvent); - }).reduce((a, b) => a.concat(b), []); - - if (eventTiles.length === 0) { - eventTiles = null; - } - - ret.push( - { eventTiles } - ); - - if (summaryReadMarker) { - ret.push(summaryReadMarker); - } - - prevEvent = mxEv; - continue; } + if (!grouper) { + const wantTile = this._shouldShowEvent(mxEv); + if (wantTile) { + // make sure we unpack the array returned by _getTilesForEvent, + // otherwise react will auto-generate keys and we will end up + // replacing all of the DOM elements every time we paginate. + ret.push(...this._getTilesForEvent(prevEvent, mxEv, last)); + prevEvent = mxEv; + } - if (wantTile) { - // make sure we unpack the array returned by _getTilesForEvent, - // otherwise react will auto-generate keys and we will end up - // replacing all of the DOM elements every time we paginate. - ret.push(...this._getTilesForEvent(prevEvent, mxEv, last)); - prevEvent = mxEv; + const readMarker = this._readMarkerForEvent(eventId, i >= lastShownNonLocalEchoIndex); + if (readMarker) ret.push(readMarker); } + } - const readMarker = this._readMarkerForEvent(eventId, i >= lastShownNonLocalEchoIndex); - if (readMarker) ret.push(readMarker); + if (grouper) { + ret.push(...grouper.getTiles()); } return ret; @@ -950,3 +797,222 @@ export default class MessagePanel extends React.Component { ); } } + +/* Grouper classes determine when events can be grouped together in a summary. + * Groupers should have the following methods: + * - canStartGroup (static): determines if a new group should be started with the + * given event + * - shouldGroup: determines if the given event should be added to an existing group + * - add: adds an event to an existing group (should only be called if shouldGroup + * return true) + * - getTiles: returns the tiles that represent the group + * - getNewPrevEvent: returns the event that should be used as the new prevEvent + * when determining things such as whether a date separator is necessary + */ + +// Wrap initial room creation events into an EventListSummary +// Grouping only events sent by the same user that sent the `m.room.create` and only until +// the first non-state event or membership event which is not regarding the sender of the `m.room.create` event +class CreationGrouper { + static canStartGroup = function (panel, ev) { + return ev.getType() === "m.room.create"; + }; + + constructor(panel, createEvent, prevEvent, lastShownEvent) { + this.panel = panel; + this.createEvent = createEvent; + this.prevEvent = prevEvent; + this.lastShownEvent = lastShownEvent; + this.events = []; + // events that we include in the group but then eject out and place + // above the group. + this.ejectedEvents = []; + this.readMarker = panel._readMarkerForEvent(createEvent.getId()); + } + + shouldGroup(ev) { + const panel = this.panel; + const createEvent = this.createEvent; + if (!panel._shouldShowEvent(ev)) { + this.readMarker = this.readMarker || panel._readMarkerForEvent(ev.getId()); + return true; + } + if (panel._wantsDateSeparator(this.createEvent, ev.getDate())) { + return false; + } + if (ev.getType() === "m.room.member" + && (ev.getStateKey() !== createEvent.getSender() || ev.getContent()["membership"] !== "join")) { + return false; + } + if (ev.isState() && ev.getSender() === createEvent.getSender()) { + return true; + } + return false; + } + + add(ev) { + const panel = this.panel; + this.readMarker = this.readMarker || panel._readMarkerForEvent(ev.getId()); + if (!panel._shouldShowEvent(ev)) { + return; + } + if (ev.getType() === "m.room.encryption") { + this.ejectedEvents.push(ev); + } else { + this.events.push(ev); + } + } + + getTiles() { + const DateSeparator = sdk.getComponent('messages.DateSeparator'); + const EventListSummary = sdk.getComponent('views.elements.EventListSummary'); + + const panel = this.panel; + const ret = []; + const createEvent = this.createEvent; + const lastShownEvent = this.lastShownEvent; + + if (panel._wantsDateSeparator(this.prevEvent, createEvent.getDate())) { + const ts = createEvent.getTs(); + ret.push( +
    • + ) + } + + // If this m.room.create event should be shown (room upgrade) then show it before the summary + if (panel._shouldShowEvent(createEvent)) { + // pass in the createEvent as prevEvent as well so no extra DateSeparator is rendered + ret.push(...panel._getTilesForEvent(createEvent, createEvent, false)); + } + + for (const ejected of this.ejectedEvents) { + ret.push(...panel._getTilesForEvent( + createEvent, ejected, createEvent === lastShownEvent + )); + } + + const eventTiles = this.events.map((e) => { + // In order to prevent DateSeparators from appearing in the expanded form + // of EventListSummary, 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 DateSeparator is inserted. + return panel._getTilesForEvent(e, e, e === lastShownEvent); + }).reduce((a, b) => a.concat(b), []); + // Get sender profile from the latest event in the summary as the m.room.create doesn't contain one + const ev = this.events[this.events.length - 1]; + ret.push( + + { eventTiles } + + ); + + if (this.readMarker) { + ret.push(readMarker); + } + + return ret; + } + + getNewPrevEvent() { + return this.createEvent; + } +} + +// Wrap consecutive member events in a ListSummary, ignore if redacted +class MemberGrouper { + static canStartGroup = function (panel, ev) { + return panel._shouldShowEvent(ev) && isMembershipChange(ev); + } + + constructor(panel, ev, prevEvent, lastShownEvent) { + this.panel = panel; + this.readMarker = panel._readMarkerForEvent(ev.getId()); + this.events = [ev]; + this.prevEvent = prevEvent; + this.lastShownEvent = lastShownEvent; + } + + shouldGroup(ev) { + return isMembershipChange(ev); + } + + add(ev) { + this.readMarker = this.readMarker || this.panel._readMarkerForEvent(ev.getId()); + this.events.push(ev); + } + + getTiles() { + const DateSeparator = sdk.getComponent('messages.DateSeparator'); + const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary'); + + const panel = this.panel; + const lastShownEvent = lastShownEvent; + const ret = []; + + if (panel._wantsDateSeparator(this.prevEvent, this.events[0].getDate())) { + const ts = this.events[0].getTs(); + ret.push( +
    • + ); + } + + // Ensure that the key of the MemberEventListSummary does not change with new + // 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. + // + // 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-" + ( + this.prevEvent ? this.events[0].getId() : "initial" + ); + + let highlightInMels; + let eventTiles = this.events.map((e) => { + if (e.getId() === panel.props.highlightedEventId) { + highlightInMels = true; + } + // 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 DateSeparator is inserted. + return panel._getTilesForEvent(e, e, e === lastShownEvent); + }).reduce((a, b) => a.concat(b), []); + + if (eventTiles.length === 0) { + eventTiles = null; + } + + ret.push( + + { eventTiles } + + ); + + if (this.readMarker) { + ret.push(this.readMarker); + } + + return ret; + } + + getNewPrevEvent() { + return this.events[0]; + } +} + +// all the grouper classes that we use +const groupers = [CreationGrouper, MemberGrouper]; From be70ef44f81521b70f05cbb538edd918f61a1781 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 11 Feb 2020 15:34:14 -0500 Subject: [PATCH 2/3] lint --- src/components/structures/MessagePanel.js | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 4f4136ed662..5835b023330 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -402,8 +402,6 @@ export default class MessagePanel extends React.Component { }; _getEventTiles() { - const DateSeparator = sdk.getComponent('messages.DateSeparator'); - this.eventNodes = {}; let i; @@ -814,7 +812,7 @@ export default class MessagePanel extends React.Component { // Grouping only events sent by the same user that sent the `m.room.create` and only until // the first non-state event or membership event which is not regarding the sender of the `m.room.create` event class CreationGrouper { - static canStartGroup = function (panel, ev) { + static canStartGroup = function(panel, ev) { return ev.getType() === "m.room.create"; }; @@ -875,8 +873,8 @@ class CreationGrouper { if (panel._wantsDateSeparator(this.prevEvent, createEvent.getDate())) { const ts = createEvent.getTs(); ret.push( -
    • - ) +
    • , + ); } // If this m.room.create event should be shown (room upgrade) then show it before the summary @@ -887,7 +885,7 @@ class CreationGrouper { for (const ejected of this.ejectedEvents) { ret.push(...panel._getTilesForEvent( - createEvent, ejected, createEvent === lastShownEvent + createEvent, ejected, createEvent === lastShownEvent, )); } @@ -911,11 +909,11 @@ class CreationGrouper { })} > { eventTiles } - + , ); if (this.readMarker) { - ret.push(readMarker); + ret.push(this.readMarker); } return ret; @@ -928,7 +926,7 @@ class CreationGrouper { // Wrap consecutive member events in a ListSummary, ignore if redacted class MemberGrouper { - static canStartGroup = function (panel, ev) { + static canStartGroup = function(panel, ev) { return panel._shouldShowEvent(ev) && isMembershipChange(ev); } @@ -954,13 +952,13 @@ class MemberGrouper { const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary'); const panel = this.panel; - const lastShownEvent = lastShownEvent; + const lastShownEvent = this.lastShownEvent; const ret = []; if (panel._wantsDateSeparator(this.prevEvent, this.events[0].getDate())) { const ts = this.events[0].getTs(); ret.push( -
    • +
    • , ); } @@ -999,7 +997,7 @@ class MemberGrouper { startExpanded={highlightInMels} > { eventTiles } - + , ); if (this.readMarker) { From 908ca6b6efbdec47213e195fa2fc9d3ddb4a6bab Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 13 Feb 2020 17:25:54 -0500 Subject: [PATCH 3/3] add test for grouping room creation events --- .../structures/MessagePanel-test.js | 112 ++++++++++++++++++ test/test-utils.js | 4 +- 2 files changed, 114 insertions(+), 2 deletions(-) diff --git a/test/components/structures/MessagePanel-test.js b/test/components/structures/MessagePanel-test.js index 59917057a56..e6332cf7f8d 100644 --- a/test/components/structures/MessagePanel-test.js +++ b/test/components/structures/MessagePanel-test.js @@ -34,10 +34,15 @@ import Matrix from 'matrix-js-sdk'; const test_utils = require('../../test-utils'); const mockclock = require('../../mock-clock'); +import Adapter from "enzyme-adapter-react-16"; +import { configure, mount } from "enzyme"; + import Velocity from 'velocity-animate'; import MatrixClientContext from "../../../src/contexts/MatrixClientContext"; import RoomContext from "../../../src/contexts/RoomContext"; +configure({ adapter: new Adapter() }); + let client; const room = new Matrix.Room(); @@ -251,4 +256,111 @@ describe('MessagePanel', function() { }, 100); }, 100); }); + + it('should collapse creation events', function() { + const mkEvent = test_utils.mkEvent; + const mkMembership = test_utils.mkMembership; + const roomId = "!someroom"; + const alice = "@alice:example.org"; + const ts0 = Date.now(); + const events = [ + mkEvent({ + event: true, + type: "m.room.create", + room: roomId, + user: alice, + content: { + creator: alice, + room_version: "5", + predecessor: { + room_id: "!prevroom", + event_id: "$someevent", + }, + }, + ts: ts0, + }), + mkMembership({ + event: true, + room: roomId, + user: alice, + target: { + userId: alice, + name: "Alice", + getAvatarUrl: () => { + return "avatar.jpeg"; + }, + }, + ts: ts0 + 1, + mship: 'join', + name: 'Alice', + }), + mkEvent({ + event: true, + type: "m.room.join_rules", + room: roomId, + user: alice, + content: { + "join_rule": "invite" + }, + ts: ts0 + 2, + }), + mkEvent({ + event: true, + type: "m.room.history_visibility", + room: roomId, + user: alice, + content: { + "history_visibility": "invited", + }, + ts: ts0 + 3, + }), + mkEvent({ + event: true, + type: "m.room.encryption", + room: roomId, + user: alice, + content: { + "algorithm": "m.megolm.v1.aes-sha2", + }, + ts: ts0 + 4, + }), + mkMembership({ + event: true, + room: roomId, + user: alice, + skey: "@bob:example.org", + target: { + userId: "@bob:example.org", + name: "Bob", + getAvatarUrl: () => { + return "avatar.jpeg"; + }, + }, + ts: ts0 + 5, + mship: 'invite', + name: 'Bob', + }), + ]; + const res = mount( + , + ); + + // we expect that + // - the room creation event, the room encryption event, and Alice inviting Bob, + // should be outside of the room creation summary + // - all other events should be inside the room creation summary + + const tiles = res.find(sdk.getComponent('views.rooms.EventTile')); + + expect(tiles.at(0).props().mxEvent.getType()).toEqual("m.room.create"); + expect(tiles.at(1).props().mxEvent.getType()).toEqual("m.room.encryption"); + + const summaryTiles = res.find(sdk.getComponent('views.elements.EventListSummary')); + const summaryTile = summaryTiles.at(0); + + const summaryEventTiles = summaryTile.find(sdk.getComponent('views.rooms.EventTile')); + // every event except for the room creation, room encryption, and Bob's + // invite event should be in the event summary + expect(summaryEventTiles.length).toEqual(tiles.length - 3); + }); }); diff --git a/test/test-utils.js b/test/test-utils.js index fdd50a87928..d7aa9d5de90 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -51,7 +51,7 @@ export function createTestClient() { getUserId: jest.fn().mockReturnValue("@userId:matrix.rog"), getPushActionsForEvent: jest.fn(), - getRoom: jest.fn().mockReturnValue(mkStubRoom()), + getRoom: jest.fn().mockImplementation(mkStubRoom), getRooms: jest.fn().mockReturnValue([]), getVisibleRooms: jest.fn().mockReturnValue([]), getGroups: jest.fn().mockReturnValue([]), @@ -111,7 +111,7 @@ export function mkEvent(opts) { if (opts.skey) { event.state_key = opts.skey; } else if (["m.room.name", "m.room.topic", "m.room.create", "m.room.join_rules", - "m.room.power_levels", "m.room.topic", + "m.room.power_levels", "m.room.topic", "m.room.history_visibility", "m.room.encryption", "com.example.state"].indexOf(opts.type) !== -1) { event.state_key = ""; }