Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Redesign: restyle jump to first unread message & rework read marker logic #2322

Closed
wants to merge 22 commits into from

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Dec 4, 2018

element-hq/element-web#7591

image

The read marker logic is also changed, as https://github.com/vector-im/riot-web/issues/4134 has been a long outstanding issue, and making the feature look different without making it work well didn't make sense.

The read marker is now also updated if it is currently above the viewport:

  • if the marker is currently above the viewport, the timeout to move it is 30s.
  • if the marker is within the viewport, the timeout to move it is 10s.
    After the timeout elapses, the read marker is moved below the last event in the viewport when timeout elapses, as before.

The timeout is aborted when the user becomes inactive. The read marker is not touched, and the timer starts counting again once the user is active again. What is considered user activity changed a bit as well: the user is considered active when activating the tab/window, moving the mouse or scrollwheel, or using the keyboard. The user is considered inactive when blurring the window/tab, or 2 minutes have elapsed since the last interaction event was fired. This to prevent the read marker being updated when we know the user is not looking at the page.

Also added a Timer class using promises as the clear/setTimeout's were going to grow a bit unwieldy. Should be reusable for other timeout related usecases.

I'm not doing the badge count for now, as many rooms have gaps in their timeline so we can't give an accurate number of messages since the unread marker. Tried it and it looks confusing.

Parked the work for the badge on the bwindels/jumptofirstunread-badgecount branch (also on js-sdk) if we ever want to get back to it.

@bwindels bwindels requested review from a team and removed request for a team December 4, 2018 13:53
@bwindels
Copy link
Contributor Author

bwindels commented Dec 4, 2018

Needs more work actually, marking as not ready.

@bwindels bwindels force-pushed the bwindels/jumptofirstunread branch from d5267a8 to 82d5873 Compare December 5, 2018 12:04
@bwindels bwindels force-pushed the bwindels/jumptofirstunread branch from 82d5873 to 04e1542 Compare December 5, 2018 17:34
Make Timer revert back to not started state so you can't forget to clone it like was the case here.
and change timeout on timer when that changes
so allow the jump button to be visible
as it will immediately put it to the bottom with new logic
makes the useractivity logic more consistent then doing it in
timelinepanle when user switches away from the page.
@bwindels bwindels changed the title Redesign: restyle jump to first unread message Redesign: restyle jump to first unread message & rework read marker logic Dec 10, 2018
@bwindels bwindels requested a review from ara4n December 10, 2018 12:59
@bwindels bwindels removed the notready label Dec 10, 2018
@ara4n
Copy link
Member

ara4n commented Dec 11, 2018

this looks awesome! although i'm finding the size of patch a little hard to review (this is my 3rd time of trying to read through it but failing to track all the changes in my head).

It might be useful to know whether this is going to change the timing semantics for presence or typing notifs etc at all, or if it keeps backwards compatibility with the prior behaviour there?

Also, how does it feel? is 30s too short or too long for removing the off-screen-RM? I assume the 30s has to be consecutive seconds of activity in a given room?

If you start viewing a room, but then stop interacting with the app, does it stop the timer and only start counting again when you start being active again? (Sorry if this is clear from the code or the initial comment, but my brain is failing)

Ftr, I talked @AmandineLP through this yesterday and she has concerns about ever destroying off-screen bookmarks, although I think we should how this feels first before we creep it in future towards the full "track all RM bookmarks" solution.

@bwindels
Copy link
Contributor Author

this looks awesome! although i'm finding the size of patch a little hard to review (this is my 3rd time of trying to read through it but failing to track all the changes in my head).

I'm restructuring the commits with better commit messages so it's clearer. I went over several iterations and it show in the commit log I suppose :).

It might be useful to know whether this is going to change the timing semantics for presence or typing notifs etc at all, or if it keeps backwards compatibility with the prior behaviour there?

It should not, apart from the fact that you'll be marked on-line when you go back to your riot tab/window as well, without using mouse or keyboard in the page. This is collateral, but I thought an improvement, see below.

Also, how does it feel? is 30s too short or too long for removing the off-screen-RM? I assume the 30s has to be consecutive seconds of activity in a given room?

Well, you don't see the RM as it is moved from above the viewport to below it. So you're not really aware. From my brief testing, it does feel better than before, but I think there is room for experimentation here like, off-the-top-of-my-head: a timeout based on the amount of events between where the RM and where it would be moved to, or just a bigger value.

The 10s to move it when in view does feel a bit long, given that before it used to be 500ms.

If you start viewing a room, but then stop interacting with the app, does it stop the timer and only start counting again when you start being active again? (Sorry if this is clear from the code or the initial comment, but my brain is failing)

Yes & no; what is considered "user activity" is different now, but if that is interrupted, the timeout does reset. Before it meant the user has touched their hardware very recently. Now, it's an educated guess that the user is looking at app, still starting it by taking the keyboard and mouse queues from before, but also when you switch back to the tab or window. The activity ends with a timeout of 2 minutes (instead of 500ms), or switching away from the tab or window.

I did it this way because for the larger 30s timeout for the out-of-view RM you can't expect continuous hardware interaction, this is why Presence also ignored user_activity_end before. I could have implemented the RM timeout similar to Presence, but it needs to cancelled when you switch away from the window/tab. I sort of did it like this first, handling window/tab switching in TimelinePanel only for the RM but as it became more complicated, I built it in UserActivity itself, which sort of makes sense I think. It also has the bonus of marking you online when you switch back to riot...

Ftr, I talked @AmandineLP through this yesterday and she has concerns about ever destroying off-screen bookmarks, although I think we should how this feels first before we creep it in future towards the full "track all RM bookmarks" solution.

👍

@bwindels
Copy link
Contributor Author

Fixed Presence not being updated often enough btw, see f856a64.

@bwindels
Copy link
Contributor Author

@ara4n Abadoning this PR in favor of #2345 with cleaned up commit log.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants