Merge pull request #4059 from matrix-org/uhoreg/refactor_event_grouping

refactor event grouping into separate helper classes

src/components/structures/MessagePanel.js

@@ -413,10 +413,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 = {};
 
         let i;
@@ -458,189 +454,33 @@ 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
+            if (grouper) {
-            // Grouping only events sent by the same user that sent the `m.room.create` and only until
+                if (grouper.shouldGroup(mxEv)) {
-            // the first non-state event or membership event which is not regarding the sender of the `m.room.create` event
+                    grouper.add(mxEv);
-            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 = <li key={ts1+'~'}><DateSeparator key={ts1+'~'} ts={ts1} /></li>;
-                    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);
+                    // 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;
                 }
             }
 
-                // At this point, i = the index of the last event in the summary sequence
+            for (const Grouper of groupers) {
-                const eventTiles = summarisedEvents.map((e) => {
+                if (Grouper.canStartGroup(this, mxEv)) {
-                    // In order to prevent DateSeparators from appearing in the expanded form
+                    grouper = new Grouper(this, mxEv, prevEvent, lastShownEvent);
-                    // 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));
                 }
-
-                // 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(<EventListSummary
-                    key="roomcreationsummary"
-                    events={summarisedEvents}
-                    onToggle={this._onHeightChanged} // Update scroll state
-                    summaryMembers={[ev.sender]}
-                    summaryText={_t("%(creator)s created and configured the room.", {
-                        creator: ev.sender ? ev.sender.name : ev.getSender(),
-                    })}
-                >
-                    { eventTiles }
-                </EventListSummary>);
-
-                if (summaryReadMarker) {
-                    ret.push(summaryReadMarker);
             }
-
+            if (!grouper) {
-                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 = <li key={ts1+'~'}><DateSeparator key={ts1+'~'} ts={ts1} /></li>;
-                    ret.push(dateSeparator);
-                }
-
-                // 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(<MemberEventListSummary key={key}
-                    events={summarisedEvents}
-                    onToggle={this._onHeightChanged} // Update scroll state
-                    startExpanded={highlightInMels}
-                >
-                        { eventTiles }
-                </MemberEventListSummary>);
-
-                if (summaryReadMarker) {
-                    ret.push(summaryReadMarker);
-                }
-
-                prevEvent = mxEv;
-                continue;
-            }
-
                 if (wantTile) {
                     // make sure we unpack the array returned by _getTilesForEvent,
                     // otherwise react will auto-generate keys and we will end up
@@ -652,6 +492,11 @@ export default class MessagePanel extends React.Component {
                 const readMarker = this._readMarkerForEvent(eventId, i >= lastShownNonLocalEchoIndex);
                 if (readMarker) ret.push(readMarker);
             }
+        }
+
+        if (grouper) {
+            ret.push(...grouper.getTiles());
+        }
 
         return ret;
     }
@@ -961,3 +806,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(
+                <li key={ts+'~'}><DateSeparator key={ts+'~'} ts={ts} /></li>,
+            );
+        }
+
+        // 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(
+            <EventListSummary
+                 key="roomcreationsummary"
+                 events={this.events}
+                 onToggle={panel._onHeightChanged} // Update scroll state
+                 summaryMembers={[ev.sender]}
+                 summaryText={_t("%(creator)s created and configured the room.", {
+                     creator: ev.sender ? ev.sender.name : ev.getSender(),
+                 })}
+            >
+                 { eventTiles }
+            </EventListSummary>,
+        );
+
+        if (this.readMarker) {
+            ret.push(this.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 = this.lastShownEvent;
+        const ret = [];
+
+        if (panel._wantsDateSeparator(this.prevEvent, this.events[0].getDate())) {
+            const ts = this.events[0].getTs();
+            ret.push(
+                <li key={ts+'~'}><DateSeparator key={ts+'~'} ts={ts} /></li>,
+            );
+        }
+
+        // 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(
+            <MemberEventListSummary key={key}
+                 events={this.events}
+                 onToggle={panel._onHeightChanged} // Update scroll state
+                 startExpanded={highlightInMels}
+            >
+                 { eventTiles }
+            </MemberEventListSummary>,
+        );
+
+        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];

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(
+            <WrappedMessagePanel className="cls" events={events} />,
+        );
+
+        // 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);
+    });
 });

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 = "";
     }