MAM-4134-feed-refresh-glitches

[derspyy]

started as an effort to document this (so i could understand it better) but i ended up noticing some weird behavior. i think we should test these changes.

[derspyy]

your refactor assumes that we don't want to refresh if "did view recently" is true. this is wrong, because the user could have left the app in a outdated state. my solution was "leave pollingReachedTop() untouched if the user just viewed that feed".

also, i don't see why we'd want to check for it when the user double-taps, my solution was checking for that when the user reopens the app/feed.

i liked the refactor of making that a separate function, but the behavioral change you did in that same commit is probably not what we want.

[derspyy]

BTW, please merge main onto the branch instead of force-rebasing, because that breaks my local work. 🥲

[bnolens]

i don't see why we'd want to check for it when the user double-taps, my solution was checking for that when the user reopens the app/feed.

The pollingReachedTop variable determines what happens when double-tapping. If FALSE, we perform a hard refresh; if TRUE, we scroll up with animation and poll for new content. That's why, instead of reusing the same boolean by attaching additional logic to it, I prefer to add more logic separately in the function called when double-tapping. This makes it more explicit and so easier to understand. This also keeps the value of pollingReachedTop strictly what it is meant to be. I may have missed something in the logic by doing so. Thanks for reverting it.

BTW, please merge main onto the branch instead of force-rebasing, because that breaks my local work. 🥲

Sorry, should have communicated that. Did you lose content, or did you have conflicts when pulling? Ultimately, using rebase is my preferred method over merging, same for having one person per feature branch. Let's do that from now on.

[derspyy]

cool. i end up having to reset, stash and re-commit, which is a bit annoying.

[derspyy]

abt the change in logic, the idea is that not every time didViewRecently is true we want to avoid refreshing. for example, if i leave the app while the feed is outdated (hasn't reached the top) and i come back, i'd still want a refresh, because it's still outdated. is the way i'm putting it clear?

[bnolens]

abt the change in logic, the idea is that not every time didViewRecently is true we want to avoid refreshing. for example, if i leave the app while the feed is outdated (hasn't reached the top) and i come back, i'd still want a refresh, because it's still outdated. is the way i'm putting it clear?

Yes, that's a good catch. I should have written !self.viewModel.pollingReachedTop || !self.viewModel.didViewRecently, instead of &&, I think. But yeah..

[derspyy]

i think we shouldn't tie didViewRecently to the double-tap directly, but tie it to leaving pollingReachedTop untouched when the user gets back to the app/feed.

i think that's more reliable because then we extract the information solely to know if there was enough time for the feed to have changed substantially since the user changed apps/feeds instead of at the moment of double-tapping (because then the time that the user spent looking at the feed itself doesn't count).

[derspyy]

Yes, that's a good catch. I should have written !self.viewModel.pollingReachedTop || !self.viewModel.didViewRecently, instead of &&, I think. But yeah..

this would mean that it would always refresh if the time > 10 secs, no?