Move all database queries off the ui thread & add a ViewModel for MainActivity #4786
Conversation
connyduck
commented
Dec 3, 2024
•
connyduck
commented
- Move all database queries off the ui thread - this is a massive performance improvement
- ViewModel for MainActivity - this makes MainActivity smaller and network requests won't be retried when rotating the screen
- removes the Push Notification Migration feature. We had it long enough, all users who want push notifications should be migrated by now
- AccountEntity is now immutable
- converted BaseActivity to Kotlin
- The header image of Accounts is now cached as well
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/MainActivity.kt # app/src/main/java/com/keylesspalace/tusky/components/login/LoginActivity.kt
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/components/preference/AccountPreferencesFragment.kt
| @@ -46,7 +46,6 @@ object StorageModule { | |||
| fun providesDatabase(@ApplicationContext appContext: Context, converters: Converters): AppDatabase { | |||
| return Room.databaseBuilder(appContext, AppDatabase::class.java, "tuskyDB") | |||
| .addTypeConverter(converters) | |||
| .allowMainThreadQueries() | |||
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/MainActivity.kt
| @@ -0,0 +1,259 @@ | |||
| /* Copyright 2024 Tusky Contributors | |||
There was a problem hiding this comment.
We still have the year numbers?
There was a problem hiding this comment.
Yes, why not. I think it is interesting to see when a file was created.
There was a problem hiding this comment.
Well, it was sort of created in 2018, I guess. The original that is. :-)
Meaning: I think it doesn't really help.
(Just an observation.)
app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt
Outdated
Show resolved
Hide resolved
| @@ -75,7 +77,7 @@ class NotificationPreferencesFragment : PreferenceFragmentCompat() { | |||
| isIconSpaceReserved = false | |||
| isChecked = activeAccount.notificationsFollowRequested | |||
| setOnPreferenceChangeListener { _, newValue -> | |||
| updateAccount { it.notificationsFollowRequested = newValue as Boolean } | |||
| updateAccount { copy(notificationsFollowRequested = newValue as Boolean) } | |||
There was a problem hiding this comment.
Create a new instance with all the same attributes except those specified. It is a feature of every data class in Kotlin.
There was a problem hiding this comment.
Ok, never seen it (which doesn't mean much).
But I am looking at it for half an hour now and have a sort of comprehension question. :-)
I see that AccountEntity now is read-only. And this "copy + changer" is a way to deal with that fact. Meaning it can be changed.
Question: What is the advantage now?
There was a problem hiding this comment.
Before: You get an AccountEntity, and then you need to check if it changed. Everyone needs the same instance, and on every update it must be written to the database or it gets inconsistent.
Now: You subscribe to a flow that will notify you if an AccountEntity changed. Changed in the database, that is. If someone just "changed" it by making a copy, nobody gets notified unless the copy is written to the database.
Less room for error, more stuff happening automatically if you will.
app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt
Outdated
Show resolved
Hide resolved
|
Maybe the commits can all be squashed? (Most of the messages are not really helpful.) |
# Conflicts: # app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsViewModel.kt # app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt # app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt
| @@ -109,8 +109,6 @@ class NotificationFetcher @Inject constructor( | |||
|
|
|||
| // NOTE having multiple summary notifications this here should still collapse them in only one occurrence | |||
| notificationManagerCompat.notify(newNotifications) | |||
|
|
|||
| accountManager.saveAccount(account) | |||
There was a problem hiding this comment.
Todo: Find what this line did
The whole branch will be squashed-and-merged. I'll try to improve my commit messages 😅 |
| @@ -587,6 +517,10 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, MenuProvider { | |||
| startActivity(composeIntent) | |||
| } | |||
|
|
|||
| override fun finish() { | |||
| super.finish() | |||
| } | |||
There was a problem hiding this comment.
Needed this to easily set a breakpoint for debugging, will remove, thx
| val accountsFlow: StateFlow<List<AccountEntity>> = runBlocking { | ||
| accountDao.allAccounts() | ||
| .stateIn(CoroutineScope(applicationScope.coroutineContext + Dispatchers.IO)) | ||
| } |
There was a problem hiding this comment.
We do a lot of searching the list by id, would it be nontrivial to make this a map of id to account?
There was a problem hiding this comment.
Yes that should be easy with associate. But actually the "a lot of searching the list by id" is a mistake, this should only happen once in AccountManager and everything else should use that code, let me change this.
