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.

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

Improve chat channel unread marker handling #9006

Merged
merged 9 commits into from
Jul 12, 2022

Conversation

notbakaneko
Copy link
Collaborator

@notbakaneko notbakaneko commented Jun 9, 2022

Decouples the previous markAsRead from setting the unread marker position and uses the marker position when marking as read instead of assuming the last message.

There was an issue where when scrolling to the bottom of a channel and then scrolling back up and down again, if the scroll was not quite at the bottom of the channel when switching, it would not mark the channel as read.

(As an aside, this also means the client can just set the marker to some midway point instead of the end)

should fix #9003

@nanaya
Copy link
Collaborator

nanaya commented Jul 6, 2022

what's the exact repro steps for this?

@notbakaneko
Copy link
Collaborator Author

  1. switch to channel with unread message
  2. scroll to bottom
  3. scroll up 1px
  4. switch away -> channel still unread (should be considered read because got scrolled to bottom while window had focus)

cases where it shouldn't be considered read:

  • window does not have focus
  • did not scroll to bottom before switching away

this.markAsReadTimeout = null;
}

this.markAsReadLastSent = lastReadId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this check if LastSent >= lastReadId as well? updateWithJson may update the value outside this function (and the same check when setting the value in there?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure the delay is even needed, thinking about it again...

we were sending a captured value anyway, so there was really no reason to delay it...
...not even sure the throttle is needed 🤔
@nanaya nanaya enabled auto-merge July 8, 2022 07:42
@nanaya nanaya merged commit 526aee0 into ppy:master Jul 12, 2022
@notbakaneko notbakaneko deleted the feature/channel-mark-as-read-tweak branch July 22, 2022 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Announcement shows there are unread messages despite having read them
2 participants