Keep scroll position when loading missing statuses #3000
Conversation
Previous behaviour loaded missing statuses by using "since_id" and "max_id". This loads the most recent N statuses, looking backwards from "max_id". Change to load the oldest statuses first, assuming the user is scrolling up through the timeline and will want to load statuses in reverse chronological order.
- Remember the position of the "Load more" placeholder - Check the position of inserted entries - If they match, scroll to the bottom
Previous behaviour loaded missing statuses by using "since_id" and "max_id". This loads the most recent N statuses, looking backwards from "max_id". Change to load the oldest statuses first, assuming the user is scrolling up through the timeline and will want to load statuses in reverse chronological order.
- Remember the position of the "Load more" placeholder - Check the position of inserted entries - If they match, scroll to the bottom
…into load-more-jump
Having two simultanous coroutines would break the calculation used to figure out which item in the list to scroll to after a "Load more" in the timeline. Do this by: - Creating a TimelineUiState and associated flow that tracks the "Load more" state - Updating this in the (Cached|Network)TimelineViewModel - Listening for changes to it in TimelineFragment, and notifying the adapter - The adapter will disable any placeholder views while "Load more" is active
|
I've been keeping notes and questions in #2938 |
|
Are you planning to add a preference for the reading direction? (I definitely would not be in favor of an unconditional direction change…) |
|
Reading direction doesn't change in this PR. I'll make a separate PR for that. I'll break this one in to two as well. One to keep the position in the list consistent (no new preference change needed), and one to control whether newest or oldest statuses are loaded (that needs a pref, there are more details about that pref in #2788 (comment)) |
|
That's what I mean, changing which statuses get loaded effectively changes the reading direction |
|
I don't think it does. Here's a slightly simplified example to keep the numbers and diagrams very easy. Consider a timeline that shows 40 statuses, with IDs 1..40. ID 1 is the oldest (so appears at the bottom of the timeline), 40 is the newest, so appears at the top of the timeline). Now suppose that some of those statuses are missing, because the app hasn't loaded them. Statuses 1-10 and 30-40 are present, but 11-29 are missing. And the user's device has enough vertical height to show about 4 statuses at the same time (again, simplified, assume all statuses take the same vertical height). In the app that looks like this: The user scrolls so status 31, "Load more", and status 10 are visible (marked on the diagram above) and clicks "Load more". This is actually the status with ID 30, and it's visible on the screen.
I.e., the state immediately after "Load more" finishes is: So the user has clicked "Load more" because they want to see more statuses, the statuses have loaded, but the user has to carry out another action (scroll) before they can achieve their goal. This PR changes that behaviour[1]. After clicking "Load more" in this example the user's view looks like this: So the newly loaded statuses have scrolled in to view, and the user can continue reading their timeline by scrolling up (which is the same direction they normally scroll to read their timeline). So the reading direction hasn't changed, this is just a bug fix. The reason it's a bug fix and not new behaviour that needs to be behind a preference is because if the user didn't want to read statuses 21-30 they could have scrolled past the "Load more" entry and not clicked it. By clicking it they've clearly signaled they do want to read those statuses, and it's a bug to require them to first scroll down. [1] It will -- at the time I'm writing this it also changes the anchor position so that in this example it's statuses 11..20 that will be loaded, not statuses 21-30. That's new behaviour, which I'll revert from this PR and put behind a preference in a new PR. |
Agree, changing reading direction will cause an uproar among users. Can we maybe have two buttons inside the gap, like other apps do? |
|
As I say, that's not necessary because this PR doesn't change the reading direction. And in the master issue, #2788 (comment), I've clearly set out the two new preferences that will be necessary. But this PR doesn't make those changes yet (or rather, it won't in an hour or so when I send an update to revert part of it). |
|
I don't particularly want to argue about it, but this explanation makes even less sense. |
Weirdness with the PagingData library meant that positionStart could still be wrong after "Load more" was clicked. Instead, remember the position of the "Load more" item and the ID of the status immediately after it. When new items are added, search for the remembered status at the position of the "Load more" item. This is quick, testing at most LOAD_AT_ONCE items in the adapter. If the remembered status is not visible on screen then scroll to it.
That ("In no circumstance do I want to load a block of posts and jump to the far end of the loaded block) is the current behaviour (i.e., without this PR) if you're reading oldest to newest. That's why people are complaining about it in:
This PR fixes that.
Huh. I think you might be the first person in all of these issues that's reported that they do that. In that case, this could be put behind a preference (which I can do in this PR), but I imagine the default should be this behaviour, since the current behaviour was a regression and has lead to these complaints. |
|
That's likely because people who are suited by the current behavior don't have as much motivation to speak up |
That is a possibility. But I think that would be more likely to be true if, pre Tusky 17, there were issues filed by people who wanted the newest-oldest behaviour. But I can't find any. So I think it's reasonable to conclude that the majority want the oldest-newest behaviour. |
Default behaviour (oldest first) is for "load more" to load statuses and stay at the oldest of the new statuses. Alternative behaviour (if the user is reading from top to bottom) is to stay at the newest of the new statuses.
nikclayton
changed the title
|
Preference added, PR title and description updated to reflect reality. |
|
Just occurred to me that the reading order preference would affect pull-to-refresh behaviour as well. I'll add another commit so if the user's reading order is set to "newest first" then pull-to-refresh puts them at the start of their timeline. |
app/src/main/java/com/keylesspalace/tusky/usecase/DeveloperToolsUseCase.kt
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/adapter/PlaceholderViewHolder.kt
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/adapter/PlaceholderViewHolder.kt
Outdated
Show resolved
Hide resolved
| @@ -150,7 +153,8 @@ fun Status.toEntity( | |||
| } | |||
|
|
|||
| fun TimelineStatusWithAccount.toViewData(gson: Gson): StatusViewData { | |||
| if (this.status.authorServerId == null) { | |||
| if (this.status.isPlaceholder) { | |||
| Log.d(TAG, "Constructing Placeholder(${this.status.serverId}, ${this.status.expanded})") | |||
| @@ -51,6 +51,26 @@ class PreferencesFragment : PreferenceFragmentCompat(), Injectable { | |||
| private val iconSize by lazy { resources.getDimensionPixelSize(R.dimen.preference_icon_size) } | |||
| private var httpProxyPref: Preference? = null | |||
|
|
|||
| enum class ReadingOrder { | |||
| /** User scrolls up, reading statuses oldest to newest. Default behaviour. */ | |||
| OLDEST_FIRST, | |||
There was a problem hiding this comment.
NEWEST_FIRST is the current behavior, right? Can we have it as the default please?
There was a problem hiding this comment.
I think NEWEST_FIRST is a bug, that this fixes.
From what I can tell, up to and including v16 beta 1 Tusky OLDEST_FIRST behaviour. I verified this by checking out 31da851, adding the DeveloperTools functionality, and experimenting with creating "Load more" gaps of different sizes and seeing how the app behaved when I clicked the button. It kept the position in the list.
I think the NEWEST_FIRST behaviour was introduced in 6d4f5ad with the migration to Paging3.
I can't see anything that suggests that this was a deliberate change, it's just how Paging3 works by default.
This change rapidly started annoying users, and they submitted issues about it. The earliest issue I can see is #2423 from Apr 7th.
I can't find any issues from before the change where users were asking for "Load more" to have its current behaviour. Only issues from after the change with users asking for the behaviour to be reverted.
This is why I think the best thing to do for the users is for the default to be the previous behaviour, which fixes the bug introduced in 6d4f5ad.
There was a problem hiding this comment.
But OLDEST_FIRST is loading the oldest statuses in the gap, we never did that?
There was a problem hiding this comment.
If you don't do that (i.e,. use since_id), with this you can get the experience where you click "Load more", the statuses load, but they don't reach the bottom of the gap (i.e., the gap is more than LOAD_AT_ONCE statuses in size).
So then there's a new "Load more" button, and you have to click again. And potentially again.
This code, using min_id (and with OLDEST_FIRST) means the UX is to click "Load more", then scroll up and read the loaded statuses, eventually encounter another "Load more", click that, and repeat, which seems like the right way to do it (and is requested from the filed issues).
There was a problem hiding this comment.
Using min_id is also consistent with Mastodon's paging in the link headers in HTTP responses.
There was a problem hiding this comment.
The default usage behavior is to read from newest to oldest - that's why the timeline is in reverse chronological order.
In that case, we want to load the newest statuses in the gap by default, so that the user who reads from newest to oldest gets an uninterrupted stream of posts. It's possible that there was a bug or otherwise incorrect behavior here in the past, but I believe that today's behavior is correct for this use case
I get that we want to have better support for the vocal minority of users who want to read every post on their timelines from oldest to newest, but we should absolutely not change such a fundamental default behavior on a whim or due to selection bias.
There was a problem hiding this comment.
As a side note, I have felt like I have maybe been coming across as adversarial in some of these discussions, and I've been trying to limit my participation for that reason. However, I also feel like there's an abnormally high amount of talking past each other going on, and I don't understand whether it's because one or more of us are fundamentally misunderstanding what the others are saying, or there's some hidden assumption, or something else.
There was a problem hiding this comment.
If it helps, I haven't been interpreting your feedback as adversarial.
To address what I believe are your key points:
"The default usage behavior is to read from newest to oldest"
That's not always true, the app is inconsistent about this behaviour.
For example:
- Check out the current tip of
develop(eb02d0b at the time I'm writing this) - Launch on a device or emulator
- Open the Federated timeline (just because it changes very quickly and you don't have to wait long)
- Scroll to the very top, and swipe-to-refresh to make sure you have the most recent statuses
- Wait maybe 10 seconds, so that new statuses are available
- Swipe-to-refresh to load new statuses
You should observe that your position in the timeline is retained, and the new statuses are loaded above the one you are currently looking at. This is oldest-first behaviour.
This behaviour is the same on at least the Federated, Local, and Home timelines.
If "the default usage behaviour is to read from newest to oldest" was correct, refreshing any of these timelines should take you to the newest of the freshly loaded statuses.
I suspect this is the source of some of the demand for this behaviour from users. They observe that, in some situations the app behaves the way that they expect, but in others it doesn't.
This also gets weird if there were more than LOAD_AT_ONCE statuses loaded and there's also a "Load more" placeholder. Because if the user clicks that then the statuses are loaded below their current position.
So swipe = load above and keep my position, "Load more" = load below and lose my position
"vocal minority"
I agree about "vocal". There are at least 14 issues that reference this, many of which either have comments from other users saying they also want the oldest-first behaviour, or heart / thumbs-up emoji from people who haven't commented but also indicating that they want the oldest-first behaviour.
I don't think the assertion that it's a minority is knowable.
Creating a Github issue or commenting on it has a fairly high barrier to entry -- you have to know where to file the issue, you have to create a Github account if you don't already have one, and you have to take the time to actually write the issue.
Participation inequality is a thing (https://www.nngroup.com/articles/participation-inequality/), and I think it's reasonable to assume that for every 1 user who has taken the time to report this issue, there are N other users who either have the same problem, or have abandoned using the app in favour of a different one.
Unfortunately, we don't know what N is. Could be 10, could be 100, could be 1,000, or more, or less.
This also extends to Play reviews. It doesn't appear to be possible to link to individual reviews, so, from the last month or so:
"Tusky experience (if you try to read from oldest to newest): Scrolling up. Tap "Load more". Scroll down to find where you stopped before it loaded newest posts and lost scrolling position. Tap "Load more" again, rushing to scroll down so that you don't lose scrolling progress. "Load more". "Load more". Repeat 15 times [...]" 2 stars, "3 people found this review helpful"
"Looks pretty good. Nice and clean view. However the load more button defaults to going to the top of your feed losing position. Can this be switched or an option included?", 3 stars, "4 people found this review helpful"
"This App is okay for the most cases. But the main feature is complety insane. The timeline is always at the latest twee...äh..toot(?). You can't continue reading the toots from the place were you stopped reading. You always have to start at the newest toot and must search were you left", 2 stars, "25 people found this review helpful"
My intuition from all this is that N is a large number, and that changing the default approach to oldest-first is a net-win for the majority of the userbase.
You, reasonably, disagree.
We can't both be right.
Proposal
The Tusky Mastodon account has about 15K followers.
Create a poll, run it for 7 days, and boost it once per day for those 7 days. Suggested text:
We are thinking of changing the default "Load more" behaviour (https://github.com/tuskyapp/Tusky/pull/3000)
Which do you prefer when you press the "Load more" button?
We'll support both, this is about what the *default* should be.
1. Jump to the newest post (current behaviour)
2. Stay at the oldest post
Use the result of that to determine the default.
There was a problem hiding this comment.
"There are behavioral inconsistencies" is not the same as "the default is secretly the opposite of what you think".
I agree that when the user actively refreshes, the timeline should move to the top (in the default newest-first order).
There's no point in having a poll about the load more behavior, because it should be determined by the reading order, where it doesn't "jump" in either case, but remains at the user's last-read (from the default top or optional bottom) post.
There was a problem hiding this comment.
"There are behavioral inconsistencies" is not the same as "the default is secretly the opposite of what you think".
That is true.
But that's not what I'm saying.
I'm saying that the default behaviour is inconsistent and is a source (probably a significant one) of user confusion and dissatisfaction with the app.
By pointing out the inconsistency, I intend to suggest that when you say "I believe that today's behavior is correct for this use case" I think you may not have considered all the ways that the app behaves.
In response, you suggested "I agree that when the user actively refreshes, the timeline should move to the top (in the default newest-first order)."
That is something that we could do (not in this PR, and I'm not suggesting we actually do it, I'm using "could" to mean only "This is a technically possible change").
But you've also said (about default reading order) "we should absolutely not change such a fundamental default behavior on a whim or due to selection bias."
I agree with that too -- that's why I suggested the poll, so that it's not a whim, or selection bias.
But I feel this is also inconsistent. On one hand you are suggesting that we should not change fundamental default behaviour like this, on the other hand you suggest we should change the user's reading position when they swipe-to-refresh, which I think would definitely be a change to fundamental behaviour.
I think that may be why some of this discussion is challenging; it feels like you are arguing two different and opposite positions, depending on the behaviour that you want to see, and it's difficult to address your concerns because of that.
Aside: As I've already said, I agree with you when you say "absolutely not change such a fundamental default behaviour on a whim or due to selection bias".
And I absolutely do not want to discuss the specific decision here and derail the conversation, but I'll note that has absolutely happened in the past, probably most recently with #2552.
Finally:
There's no point in having a poll about the load more behavior, because it should be determined by the reading order, where it doesn't "jump" in either case, but remains at the user's last-read (from the default top or optional bottom) post.
That's exactly what this PR does (once I revert 1480c6a, see my other reply #3000 (comment) about that).
[Edit: I have now reverted that change]
The only thing the poll would be for is to decide the default value for the preference, OLDEST_FIRST or NEWEST_FIRST.
app/src/main/java/com/keylesspalace/tusky/adapter/PlaceholderViewHolder.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineFragment.kt
Outdated
Show resolved
Hide resolved
|
I'm trying this now. |
That's because of #3000 (comment) (which I didn't get any feedback on) and 1480c6a which is trivial to revert if that's not the desired behaviour. Edit to add: I've reverted that commit. |
It does, at least in my testing. To reproduce I:
Have you seen different behaviour? |
Oh, and also whenever I post / reply |
Thanks, I'll check that. Which commit hash did you build from? |
|
I'm still using 5928159 |
I can't reproduce this on 3f7db5f For creating new posts:
For replies:
|
|
It would be nice to get this merged, I think it is very close. As far as I can tell without reading all 44 comments, open todos are:
|
Up to you. I've provided all the evidence I have that either it should be the default, or a poll is the best way to determine the default.
It mostly does already (tested with the local and federated timelines). The difference is whether you (reading oldest first):
|
|
Ok, let's please do the following:
|
|
Does this mean when reading timeline from old to new and tapping load more, users would end up at the newest post in display? |
No. When reading oldest to newest, and clicking "Load more", you remain at the oldest post you've read. |
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineTypeMappers.kt # app/src/main/res/values/strings.xml
1 and 2, done. |
|
When I'm reading newest-first and I pull to refresh, I think the timeline should scroll to the top |
Add a preference for user reading behaviour (oldest first or newest first) and honour it when loading more statuses in the middle of a timeline.
"Oldest first" means the user scrolls up, expecting to load new statuses as they go, and when new statuses load the user's reading position is retained at the oldest status they've read.
"Newest first" means the user is scrolling down, expecting to load new statuses as they go, and the reading position is retained at the newest status they've read.
Default is "oldest first", as this was the behaviour before Tusky 17, and was unintentionally changed.
Ensure that a circular progress spinner appears while loading.
Add a "Developer tools" feature that is enabled in debug builds. Use this to provide an option to remove content from the local cached timeline, to make it easy for developers to examine the behaviour.