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

T3chguy/hide join part (attempt) 2 #1243

Merged
merged 28 commits into from Jul 26, 2017

Conversation

Projects
None yet
4 participants
@t3chguy
Collaborator

t3chguy commented Jul 21, 2017

Ignore for now, using to easier follow Jenkins because tests never seem to fail on my Windows machines...

Replaces #1104 - Maybe... Diff seems near-identical

Kegsay and others added some commits May 31, 2017

WIP join/part hiding
- Doesn't work with MELS
- Doesn't work with read markers
- Doesn't work with jumping to events

Shelving this for now as I fix some of this mess.
Merge branches 'develop' and 'kegan/hide-join-part' of github.com:mat…
…rix-org/matrix-react-sdk into kegan/hide-join-part

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

# Conflicts:
#	src/components/structures/UserSettings.js
make hide joins/parts work for MELS and highlighted/permalink event
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
modernize imports
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
pass user settings from above
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
try to make hiding events work with read markers
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
make hiding redactions use new pattern
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
remove debug and add comment
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@matrixbot

This comment has been minimized.

Member

matrixbot commented Jul 21, 2017

Can one of the admins verify this patch?

first attempt at stubbing tests
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

@t3chguy t3chguy force-pushed the t3chguy/hide-join-part-2 branch from c34da8a to f036fd1 Jul 21, 2017

@t3chguy t3chguy changed the base branch from master to develop Jul 21, 2017

t3chguy added some commits Jul 21, 2017

re-add core logic
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
re-add Unread shouldHideEvent code
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
attempt to incorporate more code #1
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
attempt to incorporate more code #2
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
fix logic ordering
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
remove comments/commented code
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
unrevert most of the logic.
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
unrevert some more...
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
fix comment and remove unused const
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
re-add i18n string
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
add algo to ensure that RM always moves forwards
this is needed so that if a client which does not hide any events
sets and RM at bottom of timeline, then riot-web which hides events
sets the RM it'd set it at X-N where X is bottom and N is the amount of
hidden events at bottom of the timeline, this way now an RM will
fall through to the hidden events below a seen event.

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

@t3chguy t3chguy requested a review from lukebarnard1 Jul 22, 2017

fix for loop - check happens before pre-inc so cause issues at end of TL
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

@t3chguy t3chguy force-pushed the t3chguy/hide-join-part-2 branch from 688e48d to 8e66e64 Jul 22, 2017

@t3chguy

This comment has been minimized.

Collaborator

t3chguy commented Jul 22, 2017

Does not fix the broken visible var hoisting luke verified, that'll need to be done in another PR

@t3chguy t3chguy referenced this pull request Jul 22, 2017

Closed

Hide Joins/Parts #1104

// 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 DateSeperator is inserted.
const ret = this._getTilesForEvent(e, e, last);

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 24, 2017

Contributor

Is it OK if last is not computed for e but instead for the first event in the MELS?

This comment has been minimized.

@t3chguy

t3chguy Jul 25, 2017

Collaborator

Hmm, that change was Kegan's - fc93517#diff-dbc0a683dd11c32adf8c9532eb9ebb27R386
I'll figure out why its needed

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 25, 2017

Contributor

I would guess that it's needed for when you've expanded the MELS at the front of the timeline, (i.e. the most recent events are member events) and the last event should be indicated with last=true but last is calculated with i where i is the index of the first event in the MELS. I think last should be recalculated for each event in the summary (so within the inner for that iterates i).

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

can't do it within that for-loop because it doesn't use the last value, the map afterwards does, so it'd set the last value to the same for all events in MELS as the last event in the MELS

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

best way I can see is changing it from lastShownEventIndex to lastShownEvent and then just checking it against the event itself, other way is to recalculate last inside that interior for loop and then if last && index === summarisedEvents.length - 1 (where index is the index passed to the mapper function)

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

Ended up implementing in the former manner:
image

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

(memory pointer check)

@@ -1023,7 +1020,14 @@ var TimelinePanel = React.createClass({
var boundingRect = node.getBoundingClientRect();
if ((allowPartial && boundingRect.top < wrapperRect.bottom) ||
(!allowPartial && boundingRect.bottom < wrapperRect.bottom)) {
return i;
let latestReadEventIndex = i;
// Place the RM at a hidden event below the latest seen event (if exists)

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 24, 2017

Contributor

nit: if it exists

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 24, 2017

Contributor

Actually this is confusing, the event is just the latest read visible event... event (if exists) should be probably be visible event

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

done

}
// this only applies to joins/leaves not invites/kicks/bans
return membership === 'join' || (membership === 'leave' && ev.getStateKey() === ev.getSender());

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 24, 2017

Contributor

What about unbans and invites? An unban = ban to join, invite acception = invite to join?

This comment has been minimized.

@t3chguy

t3chguy Jul 25, 2017

Collaborator

/me votes for a helper in js-sdk to figure out what a member event does...

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 25, 2017

Contributor

Different apps may feel differently about how to group certain events and hiding joins/parts is a good example of how an app treats fairly arbitrary events specially.

Anyway, either the logic or the comment above needs to change; if the intention is to return true for accepting invites (type of join) and being unbanned (type of join), then this should be mentioned in the comment.

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

done

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

by helper I meant a thing that took an event and spat out an enum val describing its effect 'ban'/'kick'/'unban'/'leave'/'join' etc

return membership === 'join' || (membership === 'leave' && ev.getStateKey() === ev.getSender());
}
export default function(ev, syncedSettings) {

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 24, 2017

Contributor

You could just import UserSettingsStore here instead of importing it in the files that use shouldHideEvent?

This comment has been minimized.

@t3chguy

t3chguy Jul 24, 2017

Collaborator

See #1104 (comment)
plus getSyncedSettings has to json parse so doing it once per event is nasty.

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 25, 2017

Contributor

Fair enough!

t3chguy added some commits Jul 26, 2017

change lastShownEventIndex -> lastShownEvent since some places
do not have the same i reference (namely MELS generation)
this way a member event at bottom of MELS (if is the last event
in the timeline will have last set appropriately)
@t3chguy

This comment has been minimized.

Collaborator

t3chguy commented Jul 26, 2017

I believe that is everything in Luke's review done

// this only applies to joins/leaves not invites/kicks/bans
return membership === 'join' || (membership === 'leave' && ev.getStateKey() === ev.getSender());
const isJoin = membership === 'join' && prevMembership !== 'ban';

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 26, 2017

Contributor

This will be true if prevMembership is invite, so if the intention is to return true for invitation acceptations, then the comment above needs to be updated.

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

updated comment @lukebarnard1

@@ -1021,7 +1021,7 @@ var TimelinePanel = React.createClass({
if ((allowPartial && boundingRect.top < wrapperRect.bottom) ||
(!allowPartial && boundingRect.bottom < wrapperRect.bottom)) {
let latestReadEventIndex = i;
// Place the RM at a hidden event below the latest seen event (if exists)
// Place the RM at a hidden event below the latest visible event.
// to prevent RM going up the timeline between clients which do not hide the same events.
for (let j = i + 1; j < this.state.events.length; j++) {
if (messagePanel._shouldShowEvent(this.state.events[j])) break;

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 26, 2017

Contributor

We never check if i itself is visible, if it is surely we can return i immediately? In other words, let j = i;.

This comment has been minimized.

@t3chguy

t3chguy Jul 26, 2017

Collaborator

we know i is visible because its in the DOM
we want the last non-visible event to put the rm on
(last non-visible but still before the next visible, so it trickles down the timeline)

@@ -1021,7 +1021,7 @@ var TimelinePanel = React.createClass({
if ((allowPartial && boundingRect.top < wrapperRect.bottom) ||
(!allowPartial && boundingRect.bottom < wrapperRect.bottom)) {
let latestReadEventIndex = i;
// Place the RM at a hidden event below the latest seen event (if exists)
// Place the RM at a hidden event below the latest visible event.
// to prevent RM going up the timeline between clients which do not hide the same events.

This comment has been minimized.

@lukebarnard1

lukebarnard1 Jul 26, 2017

Contributor

The wording still seems confusing. How about something like "if the RM is on an event that is not visible, find the first event that is visible and place the RM on the event before it."

@lukebarnard1 lukebarnard1 merged commit c4f049e into develop Jul 26, 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/hide-join-part-2 branch Dec 8, 2017

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