improve local status updates #3480

connyduck · 2023-03-22T19:40:45Z

The idea here is: Everytime we get hold of a new version of a post, we update everything about that post everywhere.
This makes the distincion between different event types unnecessary, as everythng is just a StatusChangedEvent.
The main benefit is that posts should be up-to-date more often, which is important considering there is now editing and #3413

connyduck · 2023-03-22T20:32:40Z

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt

-                if (viewData != null) { detailedStatus = viewData }
+                val result = api.status(id).getOrNull()
+                if (result != null) {
+                    eventHub.dispatch(StatusChangedEvent(result))


This is not ideal since ViewThreadViewModel is both sending and observing StatusChangedEvent, but I don't have a better idea :/

I wondered a bit why this case here stands out.

And maybe it doesn't: What happens if I refresh a timeline and one of the status there did change?
(A status that might as well be shown on the bookmark timeline for example?)

Yes maybe we should send the event for all statuses 🤔
Still there is a problem that this ViewModel is both sending and receiving

Just a few ideas from thinking about this (and maybe questions). The concept(s) are still blurry in my mind:

If this "UpdateEvent" informs about updates (only) then there should be an actual check if there was an update.
That in turn needs a sort of central instance which stores every status that every activity in the running instance has seen. Only this instance/thing/repository would compare and if needed send a changed event.

Otherwise one would need quite a few locations which would need to send every status they see as "change", because they might have been changed somewhere else?

(Is that actually a wide-spread problem: activities remaining active even when another one is in the foreground and then their ui must be refreshed for every change/data received everywhere else?)

And specifically for the thread view: It receives the event to update its ui (on these status)?
Maybe it should then receive all the data to display as event - esp. the events it sends itself?
(It sounds too complex when it should simply "load things and then display them" but maybe that is because of the discussion above).

Sorry for writing so long but maybe it helps a bit.

This is not ideal since ViewThreadViewModel is both sending and observing StatusChangedEvent, but I don't have a better idea :/

https://developer.android.com/topic/architecture/data-layer

Tell the data layer to update the data, and the data layer content should be exposed as a Flow, so that consumers of the content observe changes as they happen.

See e.g., https://developer.android.com/codelabs/android-room-with-a-view-kotlin#6, https://developer.android.com/topic/architecture/data-layer/offline-first#reads, https://developer.android.com/topic/architecture/data-layer/offline-first#writes

That'll be a lot easier when #3436 is complete.

I kinda agree that observing the DB is kind of the best thing but also what's the problem with both sending and receiving?

It's a layering violation, and a source of bugs. With this approach every view model has to emit events, and if that's missed, or one of them has the wrong logic, then you have a bug.

By putting it in the data layer you guarantee that it doesn't matter what is updating the data, every update will generate the correct event.

I agree with this, model and persistence should be the source of truth. I was referring to @connyduck 's comment what's the problem with both sending and receiving the event

Yes basically what Nik says.
Problem is, we don't have a data layer for non-cached timelines so I'll fix it for cached ones only for now, still better than before.

That's coming in #3436. I'm in the "Cleanup up the TODOs and update the tests" phase in that PR.

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt

Lakoja · 2023-03-23T13:14:21Z

app/src/main/java/com/keylesspalace/tusky/appstore/CacheUpdater.kt

-                        timelineDao.setReblogged(accountId, event.statusId, event.reblog)
-                    is BookmarkEvent ->
-                        timelineDao.setBookmarked(accountId, event.statusId, event.bookmark)
+                    is StatusChangedEvent -> {


Just an observation: "status changed" has much less (specific) information than for example "fav clicked". I would immediately ask "what changed?".

I see that these events are normally not used in their specific sense. But maybe they could?

Is it possible to have an event hierarchy? BookmarkEvent extends StatusChangedEvent? Or is that bad style?

That is the idea that there is less info. We need to update everything anyway. If the post has been bookmarked, the fav count could have changed as well. Or it was favourited on another device.

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt

Lakoja · 2023-03-23T13:17:56Z

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt

-                if (viewData != null) { detailedStatus = viewData }
+                val result = api.status(id).getOrNull()
+                if (result != null) {
+                    eventHub.dispatch(StatusChangedEvent(result))


Maybe one could compare the two if there was actually a change?

Lakoja · 2023-03-23T13:22:54Z

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt

-                if (viewData != null) { detailedStatus = viewData }
+                val result = api.status(id).getOrNull()
+                if (result != null) {
+                    eventHub.dispatch(StatusChangedEvent(result))


I wondered a bit why this case here stands out.

And maybe it doesn't: What happens if I refresh a timeline and one of the status there did change?
(A status that might as well be shown on the bookmark timeline for example?)

connyduck · 2023-03-30T17:37:57Z

How about instead of sending events in ViewThreadFragments, we update the database directly? This way at least cached timelines get updated (which is better than before) and we avoid redundant updates in the ViewThreadFragment itself. Still not ideal but my best idea right now.

charlag · 2023-04-15T08:18:03Z

I think once the request is successful it's fairly safe to update the db and listen to db everywhere, like @nikclayton said

nikclayton · 2023-04-20T10:37:31Z

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt

@@ -144,8 +136,15 @@ class ViewThreadViewModel @Inject constructor(
            // for the status. Ignore errors, the user still has a functioning UI if the fetch
            // failed.
            if (timelineStatus != null) {
-                val viewData = api.status(id).getOrNull()?.toViewData(isDetailed = true)
-                if (viewData != null) { detailedStatus = viewData }
+                val result = api.status(id).getOrNull()


Suggested change

val result = api.status(id).getOrNull()

api.status(id).getOrNull?.let { ... }

?

nikclayton · 2023-04-20T10:45:04Z

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt

+                isExpanded = viewData.isExpanded,
+                isCollapsed = viewData.isCollapsed,
+                isDetailed = viewData.isDetailed
+            )
        }
    }



Do you need to update handleStatusComposedEvent() as well?

Specifically, if the status that was composed is a reply, the status it was replying to needs to be updated so that the reply count is correct.

yes that should probably be done, but I don't see it in the scope of this PR

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/service/SendStatusService.kt

This reverts commit 54e92b2.

Ugh, I didn't notice that #3480 was affected by the notification fragment rollback

connyduck added 4 commits March 22, 2023 20:39

improve local status updates

70ebe9b

fix tests

90427b9

fix ktlint

4e240d2

send StatusChangedEvent from ViewThreadViewModel

21d2c78

connyduck commented Mar 22, 2023

View reviewed changes

connyduck requested review from charlag and Tak March 22, 2023 20:32

connyduck marked this pull request as ready for review March 22, 2023 21:01

Tak approved these changes Mar 23, 2023

View reviewed changes

app/src/main/java/com/keylesspalace/tusky/components/viewthread/ViewThreadViewModel.kt Show resolved Hide resolved

Lakoja reviewed Mar 23, 2023

View reviewed changes

replace StatusEditedEvent with StatusChangedEvent

0d1c993

charlag approved these changes Apr 10, 2023

View reviewed changes

connyduck added 2 commits April 19, 2023 19:51

Merge branch 'develop' into improve_status_updates

de44280

limit updates from ViewThreadViewModel to cached timelines

1eb68d7

connyduck requested review from nikclayton, charlag, Tak and Lakoja April 19, 2023 18:20

nikclayton reviewed Apr 20, 2023

View reviewed changes

code cleanup

b814c2c

Tak approved these changes Apr 21, 2023

View reviewed changes

Merge branch 'develop' into improve_status_updates

7dd4d84

# Conflicts: # app/src/main/java/com/keylesspalace/tusky/service/SendStatusService.kt

Tak merged commit 54e92b2 into develop Sep 26, 2023

Tak deleted the improve_status_updates branch September 26, 2023 07:08

Tak mentioned this pull request Sep 26, 2023

Fix build after logical conflict #4036

Merged

rimar1337 pushed a commit to rimar1337/tooskie that referenced this pull request Sep 26, 2023

Revert "improve local status updates (tuskyapp#3480)"

ff9d055

This reverts commit 54e92b2.

Tak added a commit that referenced this pull request Sep 26, 2023

Fix build after logical conflict (#4036)

2dceecb

Ugh, I didn't notice that #3480 was affected by the notification fragment rollback

charlag added this to the Tusky 24 milestone Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve local status updates #3480

improve local status updates #3480

connyduck commented Mar 22, 2023 •

edited

Loading

connyduck Mar 22, 2023 •

edited

Loading

Lakoja Mar 23, 2023

connyduck Mar 24, 2023

Lakoja Mar 25, 2023

nikclayton Apr 9, 2023

charlag Apr 10, 2023

nikclayton Apr 12, 2023

charlag Apr 15, 2023

connyduck Apr 18, 2023

nikclayton Apr 19, 2023

Lakoja Mar 23, 2023

connyduck Mar 24, 2023

Lakoja Mar 23, 2023

Lakoja Mar 23, 2023

connyduck commented Mar 30, 2023

charlag commented Apr 15, 2023

nikclayton Apr 20, 2023

nikclayton Apr 20, 2023

connyduck Apr 20, 2023

	val result = api.status(id).getOrNull()
	api.status(id).getOrNull?.let { ... }

improve local status updates #3480

improve local status updates #3480

Conversation

connyduck commented Mar 22, 2023 • edited Loading

connyduck Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connyduck commented Mar 30, 2023

charlag commented Apr 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connyduck commented Mar 22, 2023 •

edited

Loading

connyduck Mar 22, 2023 •

edited

Loading