-
Notifications
You must be signed in to change notification settings - Fork 310
anchors 9/n: Add anchor, fetchNewer, haveNewest to msglist view-model #1515
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
base: main
Are you sure you want to change the base?
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! One commit-message nit and one substantive comment on an asymmetry between the fetch-older and fetch-newer logics that we should handle.
@@ -716,16 +716,21 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat | |||
} | |||
|
|||
Widget _buildStartCap() { | |||
// These assertions are invariants of [MessageListView]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msglist [nfc]: Say we'll show "loading" even when fetch is at other end
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 (#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.
commit-message nit: In the last paragraph, I don't think I understand "but there won't be one yet".
Maybe: "there's no fetch right now, but one is queued behind a backoff wait, so effectively the loading is busy."?
@@ -94,7 +94,7 @@ mixin _MessageSequence { | |||
/// | |||
/// This may or may not represent all the message history that | |||
/// conceptually belongs in this message list. | |||
/// That information is expressed in [fetched] and [haveOldest]. | |||
/// That information is expressed in [fetched], [haveOldest], [haveNewest]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msglist [nfc]: Introduce haveNewest in model, always true for now
haveNewest
should also control how we respond to new-message events, right? When a message event comes in, we shouldn't add to the end of the list unless haveNewest
is true. If we do add it to the list, then we'll skip messages after the last that had been fetched, and they won't be filled in by subsequent fetches because fetchNewer
will use the event's message as the anchor.
/// The model has an active `fetchOlder` request. | ||
fetchingMore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say "an active fetchOlder
or fetchNewer
request", I think.
This is the next round after #1507 (plus #1512), toward #82 and #80.
At the end of this branch, the view-model MessageListView has everything it needs for #82 and #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.
This doesn't yet actually use the new functionality outside of tests. As a result this branch is NFC as to the live app, given a well-behaved server (there are a couple of commits that are not NFC in how they act with a server that gives incoherent responses). I have a draft branch (as I mentioned at #mobile-team > beta release management @ 💬) that completes both #82 and #80; that'll be for an upcoming PR or two.
Selected commit messages
83b89a9 msglist [nfc]: Say we'll show "loading" even when fetch is at other end
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 (#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.
e3588e9 msglist [nfc]: Use
fetched
getter when readingGenerally 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.
0d5e84b msglist [nfc]: Use an enum for fetched/fetching/backoff state
This makes the relationships between these flags clearer.
It will also simplify some upcoming refactors that change their
semantics.
5e1f267 msglist [nfc]: Rename backoff state to share between older/newer
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".
9fcd550 msglist [nfc]: Introduce haveNewest in model, always true for now
e12d706 msglist: Set haveNewest from response, like haveOldest
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
towork with other anchor values; we'll use the
found_newest
valuegiven 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 verynewest 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 truewhen
anchor=newest
; it has specific logic to do so.)f9efd71 msglist [nfc]: Document narrow field; make setter private
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 withouthaving them intermingled with all the sites that just read it.
7b0e405 msglist: Send positive numAfter for fetchInitial
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.8ea4c8d msglist: Make initial fetch from any anchor, in model
This is NFC as to the live app, because we continue to always set
the anchor to AnchorCode.newest there.
8051630 msglist [nfc]: Factor out _fetchMore from fetchOlder
4e7a188 msglist: Add fetchNewer method to model
This completes the model layer of #82 and #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.