Overhaul MELS to deal with causality, kicks, etc. #613

lukebarnard1 · 2017-01-12T19:00:39Z

The MELS can now deal with arbitrary sequences of transitions per user, where a transition is a change in membership. A transition can be joined, left, invite_reject, invite_withdrawal, invited, banned, unbanned or kicked.

Repeated segments (modulo 1 and 2), such as joined,left,joined,left,joined will be handled and will be rendered as " ... and 10 others joined and left 2 times and then joined". The repeated segments are assumed to be at the beginning of the sequence. This could be improved to handle arbitrary repeated sequences.

The MELS can now deal with arbitrary sequences of transitions per user, where a transition is a change in membership. A transition can be joined, left, invite_reject, invite_withdrawal, invited, banned, unbanned or kicked. Repeated segments (modulo 1 and 2), such as joined,left,joined,left,joined will be handled and will be rendered as " ... and 10 others joined and left 2 times and then joined". The repeated segments are assumed to be at the beginning of the sequence. This could be improved to handle arbitrary repeated sequences.

richvdh

This needs some tests. There are lots of edge cases here, and it's going to be very easy to break one edge case while fixing another and not notice it. Given most of the logic is just generating a string, it should be easy enough to do without having to deal with React. You might consider a MemberEventListSummaryGenerator which takes the config properties in its constructor and has a generate method which takes the list of events and returns a (string) summary?

The ordering of methods seems a bit arbitrary. Where possible, it's nice to be able to read a file from top to bottom with higher-level functions earlier, and the functions they call later. So, for example, _renderCommaSeparatedList should be after all the functions that call it.

richvdh · 2017-01-13T10:16:13Z

src/components/views/elements/MemberEventListSummary.js

+                    aggregate[seq] = [];
+                }
+
+                // Assumes display names are unique


that's an awful assumption. Can you aggregate by userId here, and then turn into displayname later by doing a room.getMember(userId).name later? Not sure how well that will cope with people who leave, though.

Actually, using target.name works. Well it works as much as EventTiles already do.

richvdh · 2017-01-13T10:31:22Z

src/components/views/elements/MemberEventListSummary.js

@@ -157,6 +191,32 @@ module.exports = React.createClass({
        );
    },

+    _getTransition: function(e) {


shouldComponentUpdate is now in the middle of the render logic. suggest moving it to the top. Normal ordering is:

react lifecycle methods

internal methods

render

richvdh · 2017-01-13T10:41:13Z

src/components/views/elements/MemberEventListSummary.js

-        let joinedAndLeft = 0;
-        let senders = Object.keys(userEvents);
-        senders.forEach(
+        // A map of agregate type to arrays of display names. Each aggregate type


This won't (aiui) preserve ordering. Per element-hq/element-web#2856 (comment), I am unconvinced that is correct.

True. I think it will suffice to order by the first event of each user?

richvdh · 2017-01-13T11:30:36Z

src/components/views/elements/MemberEventListSummary.js

-        if (remaining > 0) {
-            //  name1, name2, name3, and 100 others
-            return names + ', ' + lastName + ', and ' + remaining + ' others';
+    _renderCommaSeparatedList(items, disableAnd) {


rather than disableAnd, it might be preferable to have a itemLimit param, and move the 'and x others' logic in here - this making it reusable.

const remaining = itemLimit === undefined ? 0 : Math.max(items.length - itemLimit, 0); if (items.length === 0) { return ""; } else if (items.length === 1) { return items[0]; } else if (remaining) { items = items.slice(0, itemLimit); const other = " other" + (remaining > 1 ? "s" : ""); return items.join(', ') + ' and ' + remaining + other; } else { let last = items.pop(); return items.join(', ') + ' and ' + last; }

richvdh · 2017-01-13T11:49:07Z

src/components/views/elements/MemberEventListSummary.js

+
+            for (let modulus = 1; modulus <= 2; modulus++) {
+                // Sequences that are repeating through modulus transitions will be truncated
+                if (this._isRepeatedSequence(describedTransitions, modulus)) {


I don't think that looking for a pattern repeating throughout the whole of the transitions is the right approach. My understanding of matthew's intentions was that we would look specifically for pairs of 'join, leave' or 'leave, join' and treat them as special transitions. YMMV though.

If you push the transitions "joined","left" together to form "joined_and_left" as a transition, say, then you could reduce the problem from modulo 2 to modulo 1. Then you could just look for possibly multiple contiguous blocks of transitions during any part of the sequence (instead of just the beginning). The only advantage the current implementation has over that is that it works with transitions in general. So a repeated "joined","kicked","joined","kicked" would be compressed. Otherwise you'd have to special case a "joined" next to a "kicked" as well...

But I'm guessing join/leave/join/leave is so common that it warrants a special transition?

…ransitions Transition pairs joined,left and left,joined are now transformed into single meta-transitions "joined_and_left" and "left_and_joined" respectively. These are described as "joined and left", "left and rejoined". Treat consecutive sequences of transitions as repetitions, and handle any arbitrary repetitions of transitions: ...,joined,left,joined,left,joined,left,... is canonicalised into ...,joined_and_left, joined_and_left, joined_and_left,... which is truncated and described as ... , joined and left 3 times, ... This also works if there are multiple consecutive sequences separated by other transitions: ..., banned, banned, banned, joined, unbanned, unbanned, unbanned,... becomes ... was banned 3 times, joined, was unbanned 3 times ...

richvdh

Good work on the tests! I still think it would have been easier to factor out a separate non-react thing which generates the summaries, but this is fine.

Generally looking pretty good!

richvdh · 2017-01-17T15:41:25Z

test/components/views/elements/MemberEventListSummary-test.js

+const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary');
+
+const testUtils = require('../../../test-utils');
+describe.only('MemberEventListSummary', function() {


why .only?

richvdh · 2017-01-17T15:47:52Z

test/components/views/elements/MemberEventListSummary-test.js

+        testUtils.beforeEach(this);
+        sandbox = testUtils.stubClient();
+        parentDiv = document.createElement('div');
+        document.body.appendChild(parentDiv);


you need to remove this at the end of the test.

(In fact, there is no need to add it at all unless you want to observe it in the test browser or interact with it via the browser - react will render happily to a detached DOM node and it will probably be faster).

richvdh · 2017-01-17T15:52:03Z

test/components/views/elements/MemberEventListSummary-test.js

+        });
+    };
+
+    const generateMembershipEvent = (eventId, parameters) => {


there's something very similar to this in test-utils (mkMembership). Could you use that to save reinventing this wheel?

With some adjustments, that sounds good 👍

richvdh · 2017-01-17T15:54:53Z

test/components/views/elements/MemberEventListSummary-test.js

+            userId = userIdTemplate.replace('$', i);
+            events.forEach((e) => {
+                e.userId = userId;
+                return e;


richvdh · 2017-01-17T15:58:33Z

test/components/views/elements/MemberEventListSummary-test.js

+    let sandbox;
+    let parentDiv;
+
+    const generateTiles = (events) => {


could you add some comments to these helper functions?

richvdh · 2017-01-17T17:11:05Z

src/components/views/elements/MemberEventListSummary.js

+                    aggregateIndices[seq] = -1;
+                }
+
+                if (aggregate[seq].indexOf(displayName) === -1) {


I think if two different Matthews have the same aggregate sequence, then we ought to show it as "Matthew and one other joined", not just "Matthew joined". (IOW: you need to remove this condition).

Yep, that works.

richvdh · 2017-01-17T17:20:12Z

src/components/views/elements/MemberEventListSummary.js

+    _renderSummary: function(eventAggregates, orderedTransitionSequences) {
+        let summaries = orderedTransitionSequences.map((transitions) => {
+            let nameList = this._renderNameList(eventAggregates[transitions]);
+            let plural = eventAggregates[transitions].length > 1;


plural what?

I think this would be clearer if you factored out let userNames = eventAggregates[transitions] and used it here and above.

richvdh · 2017-01-17T17:20:40Z

src/components/views/elements/MemberEventListSummary.js

-        // movement is nil, then neither joinSummary nor leaveSummary will be
-        // truthy, so return null.
-        if (!joinSummary && !leaveSummary) {
+            let repeats = 1;


these look redundant?

richvdh · 2017-01-17T17:28:26Z

src/components/views/elements/MemberEventListSummary.js

-    _renderSummary: function(joinEvents, leaveEvents) {
-        let joiners = this._renderNameList(joinEvents);
-        let leavers = this._renderNameList(leaveEvents);
+    _getDescriptionForTransition(t, plural, repeats) {


could do with some more comments explaining what these helpers do, and what their params are. For example, the fact that plural is a boolean flag meaning that more than one user was affected would be very useful knowledge here.

richvdh · 2017-01-17T17:29:07Z

src/components/views/elements/MemberEventListSummary.js

-        let joiners = this._renderNameList(joinEvents);
-        let leavers = this._renderNameList(leaveEvents);
+    _getDescriptionForTransition(t, plural, repeats) {
+        let beConjugated = plural ? "were" : "was";


this is going to be hilarious to i18nify.

…matrix-org/matrix-react-sdk into luke/fix-join-part-collapsing-causality Conflicts: src/components/views/elements/MemberEventListSummary.js

Conflicts: src/components/structures/MessagePanel.js

richvdh · 2017-01-24T16:09:00Z

test/components/views/elements/MemberEventListSummary-test.js

-        };
+        });
+        // Override random event ID
+        e.event.event_id = eventId;


why is this necessary, ooi?

Because when the tests for expanded events are done, the shallow equality tests require a list of React instances (

matrix-react-sdk/test/components/views/elements/MemberEventListSummary-test.js

Line 134 in bd10696

<div className="event_tile" key="event0">Expanded membership</div>,

)

And the keys of these must match those generated in generateTiles, so they need to be constant, not random.

ok. Could you add this as a comment?

richvdh

some remaining grumbling, mostly about comments

richvdh · 2017-01-24T16:11:38Z

test/components/views/elements/MemberEventListSummary-test.js

@@ -4,12 +4,14 @@ const ReactDOM = require("react-dom");
 const ReactTestUtils = require('react-addons-test-utils');
 const sdk = require('matrix-react-sdk');
 const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary');
+const jssdk = require('matrix-js-sdk');
+const MatrixEvent = jssdk.MatrixEvent;

 const testUtils = require('../../../test-utils');
 describe.only('MemberEventListSummary', function() {


still .only ? whyyyyyyyyyyyy?

oops, I'll get rid - it was just to isolate the test so that the others wouldn't run

richvdh · 2017-01-24T16:12:43Z

src/components/views/elements/MemberEventListSummary.js

+     * Canonicalise an array of transitions into an array of transitions and how many times
+     * they are repeated consecutively.
+     *
+     * An array of 123 "joined_and_left" transitions, would result in:


richvdh · 2017-01-24T16:12:47Z

src/components/views/elements/MemberEventListSummary.js

+     * }, ... ]
+     * ```
+     * @param {string[]} transitions the array of transitions to transform.
+     * @returns {object[]} an array of truncated transitions.


richvdh · 2017-01-24T16:46:38Z

src/components/views/elements/MemberEventListSummary.js

+    },
+
+    /**
+     * Enumerate a given membership event, `e`, where `getContent().membership` has


I don't understand this description. How can you enumerate one thing?

You keep using that word. I do not think it means what you think it means.

I think I meant 'label', I'll use that instead

richvdh · 2017-01-24T16:50:34Z

src/components/views/elements/MemberEventListSummary.js

+     * ```
+     * @param {string[]} transitions the array of transitions to transform.
+     * @returns {object[]} an array of truncated transitions.
+     */
    _getTruncatedTransitions: function(transitions) {


I think this is a poor name for this. It implies that you're dropping transitions off the list, which isn't the case.

How about something like _coalesceRepeatedTransitions or sth?

"Truncated" appears in other places in this file too (including in the jsdoc). Remember to update them.

Agreed that truncated is poor and that to coalesce the events sounds better.

richvdh · 2017-01-24T16:51:42Z

src/components/views/elements/MemberEventListSummary.js

+
+    /**
+     * Canonicalise an array of transitions into an array of transitions and how many times
+     * they are repeated consecutively.


richvdh · 2017-01-25T10:53:39Z

Ok. You can also set KARMA_TEST_FILE in the environment to limit the runner to one file without having to remember to fix the source.

…

On 25/01/17 09:12, Luke Barnard wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In test/components/views/elements/MemberEventListSummary-test.js <#613>: > @@ -4,12 +4,14 @@ const ReactDOM = require("react-dom"); const ReactTestUtils = require('react-addons-test-utils'); const sdk = require('matrix-react-sdk'); const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary'); +const jssdk = require('matrix-js-sdk'); +const MatrixEvent = jssdk.MatrixEvent; const testUtils = require('../../../test-utils'); describe.only('MemberEventListSummary', function() { oops, I'll get rid - it was just to isolate the test so that the others wouldn't run — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#613>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU1VNH6e46qfCw0J1gEpzxwASWIs-Caks5rVxHzgaJpZM4LiHIv>.

richvdh

LGTM!

lukebarnard1 · 2017-01-25T15:23:06Z

lukebarnard1 assigned richvdh Jan 12, 2017

richvdh mentioned this pull request Jan 13, 2017

collapsed join/part messages don't respect causality element-hq/element-web#2856

Closed

richvdh suggested changes Jan 13, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh Jan 13, 2017

Luke Barnard and others added 9 commits January 13, 2017 16:40

Order names by order of first events for users

77ae041

Refactor renderCommaSeparated for reuse

fb68fff

Move shouldComponentUpdate

4be444d

Order sequences by occurance of the first event in each sequence

a79dc88

Use pre-calculated displaynames to handle dupes

5ab287f

Remove comment

aa6e168

Modified desc for invitation rejections, withdrawals

45655f4

Add test for MemberEventListSummary

ade7c65

richvdh assigned richvdh and unassigned lukebarnard1 Jan 17, 2017

richvdh suggested changes Jan 17, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh Jan 17, 2017

lukebarnard1 and others added 10 commits January 17, 2017 18:53

Remove duplicate test

49f2b9d

Remove pointless length guard

9574a0b

Move functions around, remove redundancies, add docs

3ba9f50

Refactor a few things and document everything

484549e

Move aggregation code to dedicated function

5dd1512

Refactor and document test helpers.

78e2c78

Remove parentDiv from tests

867a532

Remove done, const instead of var for requiers

41d2697

Merge branch 'luke/fix-join-part-collapsing-causality' of github.com:…

d690677

…matrix-org/matrix-react-sdk into luke/fix-join-part-collapsing-causality Conflicts: src/components/views/elements/MemberEventListSummary.js

Merge branch 'develop' into luke/fix-join-part-collapsing-causality

bd10696

Conflicts: src/components/structures/MessagePanel.js

lukebarnard1 assigned richvdh and unassigned lukebarnard1 Jan 24, 2017

richvdh reviewed Jan 24, 2017

View reviewed changes

richvdh suggested changes Jan 24, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh Jan 24, 2017

Luke Barnard added 4 commits January 25, 2017 09:12

Get rid of .only

e9719b1

Document _getCanonicalTransitions

3b8b2cf

Rename truncated->coalesced

f8e4681

Enumerate->label

8091cf7

lukebarnard1 assigned richvdh and unassigned lukebarnard1 Jan 25, 2017

Luke Barnard added 2 commits January 25, 2017 10:39

Improve comment

d5edf26

A lot of linting

24e9478

lukebarnard1 assigned lukebarnard1 and unassigned richvdh Jan 25, 2017

Luke Barnard added 2 commits January 25, 2017 11:05

Much linting

b887d5b

Fix _renderCommaSeparatedList

f9ca2a8

lukebarnard1 assigned richvdh and unassigned lukebarnard1 Jan 25, 2017

richvdh approved these changes Jan 25, 2017

View reviewed changes

richvdh merged commit 9cadc2f into develop Jan 25, 2017

Overhaul MELS to deal with causality, kicks, etc. #613

Overhaul MELS to deal with causality, kicks, etc. #613

Conversation

lukebarnard1 commented Jan 12, 2017

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jan 25, 2017 via email

richvdh left a comment

Choose a reason for hiding this comment

lukebarnard1 commented Jan 25, 2017

lukebarnard1 Jan 16, 2017 •

edited

Loading

lukebarnard1 Jan 13, 2017 •

edited

Loading

lukebarnard1 Jan 16, 2017 •

edited

Loading

lukebarnard1 Jan 18, 2017 •

edited

Loading