Skip to content
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.

Sign up for GitHub

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

Jump to bottom

message view: Do not mark selected message as read on narrow. #16313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

message view: Do not mark selected message as read on narrow. #16313

wants to merge 1 commit into from

Conversation

ryanreh99
Copy link
Collaborator

@ryanreh99 ryanreh99 commented Sep 7, 2020 •
edited
Loading

I have another branch where I'm working on the last read message selection approach.

Now, the selected message isn't read when the message list initially renders,
for the narrows in the left sidebar.
The entire message list gets marked as read if the bottom message is visible
when the narrow activates, this behavior remains the same .
Also works for the All Messages view.

WIP as tests are remaining and I'll do some more manual testing.
Fixes #10886.

@zulipbot zulipbot added size: XS area: message feed (uncategorized) difficult Issues which we expect to be quite difficult priority: high UX UX improvements to an existing workflow. labels Sep 7, 2020
@zulipbot
Copy link
Member

zulipbot commented Sep 7, 2020

Hello @zulip/server-message-view members, this pull request was labeled with the "area: message view" label, so you may want to check it out!

showell
showell reviewed Sep 9, 2020
View reviewed changes
static/js/message_scroll.js Outdated
@@ -166,7 +166,7 @@ exports.initialize = function () {

// Scroll handler that marks messages as read when you scroll past them.
$(document).on("message_selected.zulip", (event) => {
if (event.id === -1) {
if (event.id === -1 || event.id === event.previously_selected) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a good follow up fix here would be to rename event.id and event.previously_selected. It's not obvious that either is a message id, and when you look at this code out of context, it seems funny to compare those two things.

@zulipbot zulipbot added size: L and removed size: XS labels Sep 13, 2020
@timabbott
Copy link
Member

I merged the first commit, which is a nice refactor; we might also want to rename event.id to event.now_selected_id for better readability.

@timabbott timabbott mentioned this pull request Oct 1, 2020
Closed
@ryanreh99
message view: Don't mark selected message as read when narrowing.
c61886b 
On activating a narrow we always mark the first message it selects
as read, which is usually the first unread message. This behaviour
is unwanted.

The flow is as follows:
* On activating the narrow we call the `MessageList.select_id`
  method along with a `force_rerender` param.
  Here event = {
                 id: X,
                 previously_selected_id: -1,
		 ...
	       }
  where X = the first unread message id to be selected.

* Due to this `force_rerender` field, we first call the
  `rerender` method and then trigger our custom event handler
  (`message_selected.zulip`) which is responsible for marking
  message as read.

* However, due to this call path: `this.select_id` >
  `this.rerender` > `this.rerender_view` > `this.redo_selection`
  > `this.select_id`, the `message_selected.zulip` event gets
  triggered with event as = {
	                      id: X,
                              previously_selected_id: X,
		              ...
	                    }

* Thus the second selection event gets triggered before the
  first one having the current and previous id equal to "X".

This above `id = previously_selected_id` consition only occurs
for narrow activation (due to the `force_rerender` flag) or
when a message is selected again by clicking on it (message
is already read in this case).

For the All Messages view we have similar logic where, we pass
`mark_read` as false (which was previously unused) which skips
the "message_selected.zulip" event in `message_scroll.js`.

Work towards zulip#10886.
@zulipbot
Copy link
Member

Hello @ryanreh99, it seems like you have referenced #10886 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #10886..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to Zulip!

@ryanreh99
Copy link
Collaborator Author

I rebased this and reworded the commit message and removed Fixes #10886 from it.

The commit message explains why the event.id === event.previously_selected_id logic works.

Now to begin marking messages as read, the user has to interact with the middle message feed.
Except for the case where all the messages fit within the viewport.

Also, one point to note is that due to the above condition above it seems that the following flow interaction occurs:

  • You narrow to a page
  • The blue pointer is on the first unread message
  • You click the selected message, but the message doesn't get marked as read due to the above condition

However the reselected message does gets marked as read due to this event handler.

@timabbott
Copy link
Member

So, thinking about this PR, I had been envisioning a different approach for fixing #10886, where we'd:

  • When narrowing, select the message just before the first unread message, rather than the first unread message.
  • Select the area at the top of the page with the Zulip logo, if all messages are unread.

I think that's a more intuitive model, since then we preserve the property that selecting a message marks it as read, rather than it depending how you ended up selecting the message. Thoughts?

@zulipbot
Copy link
Member

Heads up @ryanreh99, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@timabbott timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:52
@bew
Copy link

bew commented Jul 15, 2024

Is this still relevant now that #10886 has been closed ?

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

@showell showell showell left review comments

Assignees
No one assigned
Labels
area: message feed (uncategorized) difficult Issues which we expect to be quite difficult has conflicts priority: high size: M UX UX improvements to an existing workflow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Difficult to avoid messages getting marked as read
5 participants
@ryanreh99 @zulipbot @timabbott @bew @showell