anchors n/n: Open at first unread; open /near/ links at message #1517
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
This will give us a natural home for logic that makes these depend on whether we have the newest messages, once that becomes something that varies.
This is NFC ultimately because we currently only ever fetch, or show loading indicators, at one end of the message list, namely the start. When we do start supporting a message list in the middle of history, though (zulip#82), and consequently loading newer as well as older messages, my conclusion after thinking it through is that we'll want a "busy fetching" state at one end to mean we show a loading indicator at the other end too, if it still has more to be fetched. This would look weird if the user actually saw both at the same time -- but that shouldn't happen, because if both ends (or even either end) is still open then the original fetch should have found plenty of messages to separate them, many screenfuls' worth. And conversely, if the user does kick off a fetch at one end and then scroll swiftly to the other end and witness how that appears, we want to show them a "loading" sign. The situation is exactly like if they'd had a fetch attempt on that same end and we were backing off from failure: there's no fetch right now, but there won't be one yet, so effectively the loading is busy.
Generally this is helpful because it means that viewing references to the field will highlight specifically the places that set it. Here it's also helpful because we're about to replace the field with an enum shared across several getters.
This makes the relationships between these flags clearer. It will also simplify some upcoming refactors that change their semantics.
Now the distinction between these two states exists only for asserts.
If a fetch in one direction has recently failed, we'll want the backoff to apply to any attempt to fetch in the other direction too; after all, it's the same server. We can also drop the term "cooldown" here, which is effectively redundant with "backoff".
This matches the symmetry expressed in the description of busyFetchingMore and at the latter's call site in widgets code: whichever direction (older or newer) we might have a fetch request active in, the consequences we draw are the same in both directions.
This tightens up a bit the logic for maintaining the fetching status, and hopefully makes it a bit easier to read.
This is NFC with a correctly-behaved server: we set `anchor=newest`, so the server always sets `found_newest` to true. Conversely, this will be helpful as we generalize `fetchInitial` to work with other anchor values; we'll use the `found_newest` value given by the server, without trying to predict it from the anchor. The server behavior that makes this effectively NFC isn't quite explicit in the API docs. Those say: found_newest: boolean Whether the server promises that the messages list includes the very newest messages matching the narrow (used by clients that paginate their requests to decide whether there may be more messages to fetch). https://zulip.com/api/get-messages#response But with `anchor=newest`, the response does need to include the very newest messages in the narrow -- that's the meaning of that `anchor` value. So the server is in fact promising the list includes those, and `found_newest` is therefore required to be true. (And indeed in practice the server does set `found_newest` to true when `anchor=newest`; it has specific logic to do so.)
Also expand a bit of docs to reflect what happens on a request using AnchorCode.firstUnread.
In particular this causes the handful of places where each field of MessageListView needs to appear to all be next to each other.
Even if the reader is already sure that the field doesn't get mutated from outside this file, giving it a different name from the getter is useful for seeing exactly where it does get mutated: now one can look at the references to `_narrow`, and see the mutation sites without having them intermingled with all the sites that just read it.
This is effectively NFC given normal server behavior. In particular, the Zulip server is smart enough to skip doing any actual work to fetch later messages when the anchor is already `newest`. When we start passing anchors other than `newest`, we'll need this.
This is NFC as to the live app, because we continue to always set the anchor to AnchorCode.newest there.
There's no value that's a natural default for this at a model level: different UI scenarios will use different values. So require callers to be explicit.
This completes the model layer of zulip#82 and zulip#80: the message list can start at an arbitrary anchor, including a numeric message-ID anchor or AnchorCode.firstUnread, and can fetch more history from there in both directions. Still to do is to work that into the widgets layer. This change is therefore NFC as to the live app: nothing calls this method yet.
TODO test: call even if also near top (because if haveOldest but not haveNewest, then want that fetch)
… typing-status and mark-as-read; TODO revisit realism of data
TODO test: on jump and then new store, preserve jumped anchor
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.
…, e.g. blink highlight
…eal setting TODO: for me, in combined feed, this is a lot slower than fetching newest -- seems like the server request is a lot slower, which makes sense. So I want to just always start at newest in, I guess, interleaved views. Fixes 80.
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 #82.
Fixes #80 (given projected revisions; see below).
This is stacked atop #1515. It will probably become two separate PRs before it's ready to merge.
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.
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).
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 branch, fetching at first unread is therefore kept behind an experimental flag. With that flag enabled, 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.)
This draft won't actually fix Open message list at first unread, rather than latest message #80 until it fetches at first unread without that experimental flag. Given the stark difference in performance, though, I think I'll want a setting for it — akin to the "automatically mark as read on scroll" setting. Without that setting, this would be a substantial regression relative to zulip-flutter main for users who have old unreads. So I plan to add such a setting.