Summarise state events after room creation #3433
Conversation
| Tests don't support hooks, needs changes |
This comment has been minimized.
This comment has been minimized.
| } | ||
| | ||
| const summarisedEvents = []; // Don't add m.room.create here as we don't want it inside the summary | ||
| for (;i + 1 < this.props.events.length; i++) { |
There was a problem hiding this comment.
Hmm, you've copied the pattern of advancing the outer for-loop counter in an inner for-loop from below, but it's a bit gnarly. I think this will only work because m.room.create events will only ever be at the start - otherwise it would break for an m.room.create after an aggregated member list because we'd have already done that check for a different event by the time we got to the second summarise loop. Not to mention that it makes the outer for loop rather long.
Would like at least a comment on the above assumption that makes this work.
There was a problem hiding this comment.
Not to mention that it makes the outer for loop rather long.
Agreed, but I don't see any quick wins here without rewriting the outer loop entirely
otherwise it would break for an m.room.create after an aggregated member list because we'd have already done that check for a different event by the time we got to the second summarise loop
I can't picture this failure mode
Which check do you mean by
that check
There was a problem hiding this comment.
Sorry - by, 'that check' I mean if (mxEv.getType() === "m.room.create") { - where if we're processing member events (if (isMembershipChange(mxEv) && wantTile) {) and find the end of them, we'll then not run the if (mxEv.getType() === "m.room.create") { check on the event after this member events. This happens to be OK because m.room.create events will never come after member events, but I can see someone c+ping this loop again for different event types.
There was a problem hiding this comment.
I agree this method is getting very long. Perhaps a good quick win would be to put both the room-create-collapse loop and the membership-collapse loop each in their own method?
There was a problem hiding this comment.
@bwindels can't put them cleanly in their own method due to their side-effect on the iterator of their parent loop, unless the method signature would be something like
i = groupX(i, events) which is not a very readable thing
| } | ||
| | ||
| const summarisedEvents = []; // Don't add m.room.create here as we don't want it inside the summary | ||
| for (;i + 1 < this.props.events.length; i++) { |
There was a problem hiding this comment.
I agree this method is getting very long. Perhaps a good quick win would be to put both the room-create-collapse loop and the membership-collapse loop each in their own method?
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
bwindels force-pushed the t3chguy/nvl/react16/EventListSummary branch from 1193294 to f2d7379 Compare 
| Rebased to resolve conflicts |
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
| I think @turt2live mentioned that pulling in base changes from develop is preferred to rebasing as the latter discards the effort of prior reviews |
3 participants
Fixes element-hq/element-web#8830
Fixes element-hq/element-web#1269
(blocked until post-privacy release or until we feel comfortable with React 16 internals not bricking our app)