anchors n/n: Open at first unread #1517
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
I've just pushed a small revision with some updates to comments and commit messages, as well as rebasing atop the now-merged #1515. With this revision, the only remaining TODOs are
So @chrisbobbe hopefully that helps with reading the branch. |
gnprice
changed the title
|
A little earlier today, I split out the first portion of this, up through handling /near/ links, as #1566. Just now I finished implementing the setting discussed previously, and pushed that as a revision here. This PR is now fully implemented, except for tests. |
chrisbobbe
reviewed
Jun 12, 2025
Comment on lines
+144
to
+148
| // TODO(#1545) stop using the deprecated members | ||
| // ignore: deprecated_member_use | ||
| groupValue: globalSettings.visitFirstUnread, | ||
| // ignore: deprecated_member_use | ||
| onChanged: (newValue) => _handleChange(context, newValue)), |
There was a problem hiding this comment.
Yeah. So I guess whichever of these lands first, there'll be a bit of a nonlocal merge conflict to resolve by applying that migration to this new bit of code.
This is NFC as to the live app, because nothing yet passes this argument.
When the message list is truly far back in history -- for example, at first unread in the combined feed or a busy channel, for a user who has some old unreads going back months and years -- trying to scroll smoothly to the bottom is hopeless. The only way to get to the newest messages in any reasonable amount of time is to jump there. So, do that.
This will give us room to start returning additional information from parsing the URL, in particular the /near/ operand.
Fixes zulip#82. One TODO comment for this issue referred to an aspect I'm leaving out of scope for now, namely when opening a notification rather than an internal Zulip link. So I've filed a separate issue zulip#1565 for that, and this updates that comment to point there.
Fixes zulip#80. This differs from the behavior of the legacy mobile app, and the original plan for zulip#80: instead of always using the first unread, by default we use the first unread only for conversation narrows, and stick with the newest message for interleaved narrows. The reason is that once I implemented this behavior (for all narrows) and started trying it out, I immediately found that it was a lot slower than fetching the newest message any time I went to the combined feed, or to some channel feeds -- anywhere that I have a large number of unreads, it seems. The request to the server can take several seconds to complete. In retrospect it's unsurprising that this might be a naturally slower request for the database to handle. In any case, in a view where I have lots of accumulated old unreads, I don't really want to start from the first unread anyway: I want to look at the newest history, and perhaps scroll back a bit from there, to see the messages that are relevant now. OTOH for someone who makes a practice of keeping their Zulip unreads clear, the first unread will be relevant now, and they'll likely want to start from there even in interleaved views. So make it a setting. See also chat thread: https://chat.zulip.org/#narrow/channel/48-mobile/topic/opening.20thread.20at.20end/near/2192303
|
OK, updated to make this ready to merge. I did the same tactic as in #1566: I filed a follow-up issue for writing tests: |
|
And merged. Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #80.
This is stacked atop #1566.
This is marked as a draft only because of a lack of tests. The functionality is fully implemented, and otherwise ready for review.
This implementation has one key difference from the legacy app and from the original plan for #80:
When fetching at first unread, it turns out the resulting /api/get-messages request can be quite slow, if the user's first unread is far back in history (e.g. in combined feed or a busy channel). This seems to be on the server side — probably it's just a naturally slower query for the database to handle.
In retrospect this is probably a major contributor (though far from the only one) to why the legacy RN app has always felt slow to me.
In this PR, fetching at first unread therefore (a) is controlled by a setting, and (b) by default is done only for conversation views, not interleaved views.
If I turn the setting to "always go to first unread", opening my combined feed on chat.zulip.org takes multiple seconds. But then if I tap the "go to bottom" button, it loads the newest messages like usual, and that is zippy like zulip-flutter has always been. (Also if I scroll around in history, even way back in the vicinity of the first unread, it remains as zippy as always.)
Without that setting (or if the default were "always go to first unread"), this would be a substantial regression relative to zulip-flutter main for users who have old unreads, and would give up a large part of this app's ability to impress users with its zippy handling of messages.
Previous notes about the portion that now belongs to #1566:
The main remaining work this branch needs is tests. There are also some directly user-visible aspects still to do:
When opening a /near/ link, this PR ensures the feed opens with the target message in view. But it'd be good to visibly highlight the specific message, e.g. by having its background blink once or twice.
That will probably be a post-launch follow-up — the lack of it isn't a regression relative to either zulip-flutter main, or the legacy app.
(→ Filed as Highlight target message on following a /near/ link #1564.)
When opening a /near/ link, this doesn't implement following where the message has been moved to.
That's Follow /near/ links through message moves #683. Currently M6 but arguably should be M5b; either way, won't block this, and most likely won't happen before launch (which is coming soon).