New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change OOO so that MELS generation will continue over hidden events #1308

Merged
merged 2 commits into from Aug 18, 2017

Conversation

Projects
None yet
2 participants
@t3chguy
Collaborator

t3chguy commented Aug 16, 2017

Fixes vector-im/riot-web#4716

this fixes the scenario of N Member events, then an invisible event e.g. (m.room.aliases) then more Member events not forming a MELS or forming one on either side of the invisible event.. Pre hide-join-parts this is how it worked.

Signed-off-by: Michael Telatynski 7t3chguy@gmail.com

Change OOO so that MELS generation will continue over hidden events
this fixes the scenario of N Member events, then an invisible event
e.g. (m.room.aliases) then more Member events. Pre hide-join-parts
this is how it worked.

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@lukebarnard1

This comment has been minimized.

Contributor

lukebarnard1 commented Aug 18, 2017

But what happens if the RM is the invisible event? By the looks of it, the RM won't be considered "in" the MELS and it won't get rendered.

@t3chguy

This comment has been minimized.

Collaborator

t3chguy commented Aug 18, 2017

This was the case and worse from what I can tell pre-hide-join-parts but I'll look into getting around that:
https://github.com/matrix-org/matrix-react-sdk/blob/release-v0.9.7/src/components/structures/MessagePanel.js#L327-L340

catch hidden event being RM
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy

This comment has been minimized.

Collaborator

t3chguy commented Aug 18, 2017

@lukebarnard1 should be better now

@lukebarnard1

This comment has been minimized.

Contributor

lukebarnard1 commented Aug 18, 2017

Ah but what if the hidden event is not a membership event or needs a date separator? Putting the continue before that conditional break feels like it would break things.

@t3chguy

This comment has been minimized.

Collaborator

t3chguy commented Aug 18, 2017

if it needs a date separator then so will the next one or even if it doesn't, its not being shown in the timeline so why does it matter it needs a date sep?
if it isn't a member event then we don't want it to split a MELS in two
(potentially preveneting either MELS from forming due to min-events)

@t3chguy

This comment has been minimized.

Collaborator

t3chguy commented Aug 18, 2017

fwiw, my reasoning on the code

we need to mark RM-in-MELS iff
the event is the RM && either of:

  • MELS generation loop continued over the event and thus no other code will apply to it
  • The event was added to the MELS

so if the break; applies then we should not check if the event is RM, because the outer loop will determine that and we no longer care about that event (otherwise we'd get Two RMs for it)

@t3chguy

This comment has been minimized.

Collaborator

t3chguy commented Aug 18, 2017

I could reverse the logic, only break if the existing conditions hold and _shouldShowEvent(collapsedMxEv)
to have the break before the continue but the effect would be the same

@lukebarnard1

This comment has been minimized.

Contributor

lukebarnard1 commented Aug 18, 2017

if it needs a date separator then so will the next one

Will there be a next one? I think your reasoning is correct, but for this particular edge case, e.g. a membership event or an event that needs a date separator (i.e. true for the break) that isn't being shown for whatever reason (true for the continue before the break) I think it's broken. In this case you would skip over the thing that would otherwise cause the MELs to split in two...

Oh but we don't want it to split it in two.

Then it's fine. It's very very annoying how hard it is to reason about this code, I regret ever writing it like this 😇

@lukebarnard1 lukebarnard1 merged commit caff761 into develop Aug 18, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@t3chguy t3chguy deleted the t3chguy/mels_fix branch Dec 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment