refactor: make feed notifications more robust against subscription changes

[Bnyro]

closes #7366

[FineFindus]

Using lastFeedWatchedTime will cause the all-caught-up marker to be moved.
From a UX perspective that would be unexpected, as the app would notify a user about new videos, but than not actually display them as new in the feed.
Not sure, if that is intended, but I think it would be better to use a separate variable to store the notification time.

[Bnyro]

Using lastFeedWatchedTime will cause the all-caught-up marker to be moved.
From a UX perspective that would be unexpected, as the app would notify a user about new videos, but than not actually display them as new in the feed.

Yes, I thought about this too, but forgot to write something about in in the PR description.

Not sure, if that is intended, but I think it would be better to use a separate variable to store the notification time.
The problem is that we already have 2 very similar variables

• LAST_FEED_REFRESH_TIMESTAMP_MILLIS: only used for local feed extraction
• LAST_WATCHED_FEED_TIME: last time the subscriptions feed was opened

and I hesitated to add yet another one to prevent additional complexity.

But yes, you're right, the way it's now with this PR doesn't make much sense.

[FineFindus]

Maybe we could reuse LAST_FEED_REFRESH_TIMESTAMP_MILLIS here, since if it isn't updated there is no new video and if we update it in the notification, the new video must have been already fetched.

Alternatively I think it is fine to add a new variable for this as

• it's not visible/modifiable by the user
• it doesn't interact with the rest of the system

so the additional complexity is quite low.

[Bnyro]

Maybe we could reuse LAST_FEED_REFRESH_TIMESTAMP_MILLIS here, since if it isn't updated there is no new video and if we update it in the notification, the new video must have been already fetched.

In theory yes, but I want to keep the implementation of the LocalFeedRepository independent of the other logic, because it additionally has the differentiation whether the feed has been fully refreshed or not.

Alternatively I think it is fine to add a new variable for this as

it's not visible/modifiable by the user
it doesn't interact with the rest of the system

so the additional complexity is quite low.

Yes, that's probably the best solution.

[Bnyro]

Not 100% happy with it yet, but at least it should work (although untested).

[FineFindus]

Just to confirm, we're no longer updating the LastFeedWatchedTime every time the feed is shown, but now only when the feed is fetched?

[FineFindus]

I'm not super happy about a comment telling user not to use something and would prefer to encode this in the type-system, but that's unrelated to the PR.

[Bnyro]

Yeah, me neither.

[Bnyro]

Just to confirm, we're no longer updating the LastFeedWatchedTime every time the feed is shown, but now only when the feed is fetched?

That usually should happen at the same time because we had only been updating the feed via the subscriptions fragment (and the home fragment, but I had a boolean for it to skip updating the variable then). I've pushed changes to clarify it, there are 3 places where we update any of these variables now:

• both of them, when the user views the subscriptions fragment/feed
• only the last refreshed time (but not the watched/seen time) in the NotificationWorker
• the refreshed time everytime the feed is loaded in the view model (which is also done by the home fragment)

[Bnyro]

I'll cleanup the commit history in a sec.

[FineFindus]

the refreshed time everytime the feed is loaded in the view model (which is also done by the home fragment)

I think it might be worthwhile discussing if changing that to avoid cases where the all-caught-up marker disappears (e.g. a users searches something, causing the fragment to reload, loosing the marker), but this isn't the right place for it.