GNOME Bugzilla – Bug 765129
chatView: occassionally the view doesn’t follow the newest messages
Last modified: 2021-06-10 11:04:49 UTC
Created attachment 326143 [details] The demonstration The attached video demonstrates the issue. When I come back to polari after being tabbed-out for some time or after change rooms after looking at some other room for a while, the chatView stays in the same position as I’ve left it and many messages get accumulated after “the fold”. This is annoying, because I have to remember to scroll down to see there’s new messages and to scroll all the way to the bottom to see the “current” messages (usually I’m not interested in the messages from a hour or two ago) I hypothesize, that, since I use touchpad to scroll, sometime chatView gets left scrolled away from the bottom-most possible position by a pixel or two before I tab-out and thus not doing that new new-message-animation thing that got implemented this release. This wasn’t an issue previous release.
(In reply to Simonas Kazlauskas from comment #0) > I hypothesize, that, since I use touchpad to scroll, sometime chatView gets > left scrolled away from the bottom-most possible position by a pixel or two > before I tab-out and thus not doing that new new-message-animation thing > that got implemented this release. I don't think that's what's happening. If a message is added, we check whether we were scrolled to the bottom and auto-scroll to the new bottom[0] (no changes there). However as we now animate the auto-scrolling, the check fails when a message arrives while the animation is still going on (because we aren't scrolled to the bottom, we are only scrolling towards it). I'm pretty sure that's the issue, but no immediate idea how to address it. I'll try to take a look this weekend - we still haven't released 3.20.1, and it would be nice to include a fix for this. [0] https://git.gnome.org/browse/polari/tree/src/chatView.js#n563
(In reply to Florian Müllner from comment #1) > I don't think that's what's happening. If a message is added, we check > whether we were scrolled to the bottom and auto-scroll to the new bottom[0] > (no changes there). However as we now animate the auto-scrolling, the check > fails when a message arrives while the animation is still going on (because > we aren't scrolled to the bottom, we are only scrolling towards it). It might also make sense to increase the threshold of what is perceved to be "scrolled to bottom". When you remove your hand from the touchpad after scrolling down it may scroll slightly up again. Perhaps adressing that might help the issue?
*** Bug 768609 has been marked as a duplicate of this bug. ***
Created attachment 331138 [details] [review] chatView: Make auto-scrolling more robust We currently implement auto-scrolling by keeping track of the scroll position that corresponds to the bottom, and checking whether it matches the current scroll position. This used to work (mostly), until we started to animate auto-scrolling in commit 65f9c8a07 - now if more text is added to the log while the animation is still ongoing (another message, or even just a timestamp), the check will fail and stop auto-scrolling until the user manually scrolls to the bottom again. To fix this, simplify the code to keep track of whether we should auto-scroll instead of a particular scroll position: We simply disable auto-scrolling when the user scrolls up, and re-enable it when reaching the bottom edge. This should be much more robust, and probably fixes tons of other edge cases in addition to the issue outlined above.
(In reply to Florian Müllner from comment #4) > Created attachment 331138 [details] [review] [review] > chatView: Make auto-scrolling more robust > > We simply disable auto-scrolling when the user scrolls up, > and re-enable it when reaching the bottom edge. This should > be much more robust, and probably fixes tons of other edge > cases in addition to the issue outlined above. That's exactly how it works on polari 3.18
(In reply to Ricardo Ramos from comment #5) > That's exactly how it works on polari 3.18 No, all versions of polari have used the auto-scrolling implementation outlined at the beginning of the commit message. It's just that it usually worked until animating the scrolling in 3.20 broke it.
Review of attachment 331138 [details] [review]: looks good to me, a few questions: ::: src/chatView.js @@ +266,3 @@ Lang.bind(this, this._updateToplevel)); this.connect('scroll-event', Lang.bind(this, this._onScroll)); + this.connect('edge-reached', (w, pos) => { no Lang.bind() shenanigans necessary? I haven't seen this syntax before, hence asking. :-) @@ +320,3 @@ Lang.bind(this, this._onLogEventsReady)); + this._autoscroll = true; just to be sure, i assume the view in kunaal search branch will continue to be a separate class (resultsView)? so we can assume all chatviews want autoscroll = true on init.
(In reply to Bastian Ilsø from comment #7) > no Lang.bind() shenanigans necessary? I haven't seen this syntax before, > hence asking. :-) Yup, that's one of the niceties of arrow notation :-) We only started to use it recently, but I'm already a fan - eventually I plan to move all anonymous functions to that notation. > @@ +320,3 @@ > + this._autoscroll = true; > > just to be sure, i assume the view in kunaal search branch will continue to > be a separate class (resultsView)? so we can assume all chatviews want > autoscroll = true on init. Yes. The plan is to take a look at what we end up needing the ResultsView and split out an appropriate shared class from ChatView. Auto-scrolling would obviously stay a ChatView-only feature ...
if i have a restored room that you hadn't selected yet in the current session, which receives a notification, then the autoscroll still applies: http://bastianilso.com/wp-content/uploads/2016/07/videocast09072016_22.55.51.mkv
Created attachment 331163 [details] [review] chatView: Re-enable auto-scrolling after reading mention at bottom We stop auto-scrolling when a message is highlighted, to keep it visible on screen. However once the message has been read (e.g. by switching to the room in question), it makes sense to turn auto-scrolling back on if the message is at the end of the buffer, as the view is already scrolled to the bottom in this case.
The last attached patch is just an improvement on the auto-scrolling behavior (both old and new afaict), not a fix for the issue mentioned in comment #9.
(In reply to Bastian Ilsø from comment #9) > if i have a restored room that you hadn't selected yet in the current > session, which receives a notification, then the autoscroll still applies I've been able to reproduce this, but it's not at all obvious what's going on. It seems unrelated to the patches on this bug though, as I can reproduce the exact same issue on master.
Review of attachment 331163 [details] [review]: looks good to me
Attachment 331138 [details] pushed as 5df0c4f - chatView: Make auto-scrolling more robust Attachment 331163 [details] pushed as 475d5a3 - chatView: Re-enable auto-scrolling after reading mention at bottom Keeping the bug open for the weird highlight issue, but as that is apparently unrelated to the patches, there's no point in blocking landing on that issue.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of Polari, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/polari/-/issues/ Thank you for your understanding and your help.