db refactor by yate · Pull Request #973 · LemmyNet/jerboa

yate · 2023-07-05T03:46:25Z

Reorganize db into different packages (entity, dao, repository)
Make all queries async, remove need for allowMainThreadQueries() (Fixes Remove the suppression for DB queries on UI thread check. #949)
Refactor the way the viewModels were getting initialized with repositories based on this codelab https://developer.android.com/codelabs/basic-android-kotlin-compose-persisting-data-room#8

- reorganize db into different packages - make all queries async, remove need for allowMainThreadQueries() - refactor the way the viewModels were getting initialized

dessalines · 2023-07-05T15:52:05Z

I'm fairly busy, but ping me when you're ready for me to take a look.

… default/null account

yate · 2023-07-05T21:14:08Z


+fun CreationExtras.jerboaApplication(): JerboaApplication =
+    (this[ViewModelProvider.AndroidViewModelFactory.APPLICATION_KEY] as JerboaApplication)
+


This is referenced in view model factories here https://github.com/dessalines/jerboa/pull/973/files#diff-41754fa505a06441b96271ba020ae2e014472578466e14a44fd7ce01a6c578f4R37, based on the compose room workshop again https://github.com/google-developer-training/basic-android-kotlin-compose-training-inventory-app/blob/room/app/src/main/java/com/example/inventory/ui/AppViewModelProvider.kt#L65

yate · 2023-07-05T21:21:52Z

+            val account = getCurrentAccount(accountViewModel)
+
+            LaunchedEffect(account) {
+                if (account == null || account.id != -1) {


There was an occasional race condition type issue where getCurrentAccount returns an initial value of null because it hasn't gotten the data from the database yet and there was no initial state set on the observeAsState method here https://github.com/dessalines/jerboa/pull/973/files#diff-697367ddff23d571ded643b597f312aeba4b29669b058e9edb03edf9bb030d92R14. It ended up making two calls to fetchInitialData. Once with the null initial value, then again with the actual data from the database. This caused two site data calls to go out, depending on which one returned first, the user data in the siteViewModel could set incorrectly set to the wrong value.

As a solution to this, I set an initial state value in observeAsState, so I know if the account id is -1, we haven't actually gotten any data yet from Room, so there is no need to call fetchInitialData. If there is no account in Room, it will return null and work like before.

Let me know if there's a better way to handle something like this

Seems fine to me.

yate · 2023-07-05T21:23:49Z

                    ) {
                        LoginActivity(
                            navController = navController,
-                            accountViewModel = accountViewModel,


I saw in the workshop, the viewModel parameters weren't passed in directly, they used the factory in the composable instead, like https://github.com/dessalines/jerboa/pull/973/files#diff-6ca2c69ae054a88358d16c98f47efd02bb16eae48a09295b4db9bd5e99ba8e76R25.

yate · 2023-07-05T22:09:37Z

+    @Query("SELECT * FROM account")
+    fun getAll(): LiveData<List<Account>>
+
+    @Query("SELECT * FROM account where current = 1 limit 1")


Made a query to get the current account instead of getting all and fetching the first here https://github.com/dessalines/jerboa/pull/973/files#diff-697367ddff23d571ded643b597f312aeba4b29669b058e9edb03edf9bb030d92L24

yate · 2023-07-05T22:09:50Z

Thanks @dessalines, it should be ready. I added some initial comments. It's mostly large because of the reorganization of the db package, but I also changed the way the view models/factories were getting initialized which could all be split into a separate PR, or removed, if you prefer.

# Conflicts: # app/src/main/java/com/jerboa/db/AppDB.kt

# Conflicts: # app/src/main/java/com/jerboa/Utils.kt # app/src/main/java/com/jerboa/ui/components/comment/reply/CommentReplyActivity.kt # app/src/main/java/com/jerboa/ui/components/community/list/CommunityListActivity.kt # app/src/main/java/com/jerboa/ui/components/inbox/InboxActivity.kt # app/src/main/java/com/jerboa/ui/components/login/LoginActivity.kt # app/src/main/java/com/jerboa/ui/components/privatemessage/PrivateMessageReplyActivity.kt # app/src/main/java/com/jerboa/ui/components/report/comment/CreateCommentReportActivity.kt # app/src/main/java/com/jerboa/ui/components/report/post/CreatePostReportActivity.kt # app/src/main/java/com/jerboa/ui/components/settings/account/AccountSettingsViewModel.kt

dessalines

Looks good, but please remove the defaults, and make the requirement explicit again.

dessalines · 2023-07-10T20:51:36Z

+            val account = getCurrentAccount(accountViewModel)
+
+            LaunchedEffect(account) {
+                if (account == null || account.id != -1) {


Seems fine to me.

dessalines · 2023-07-10T20:54:29Z

 @Composable
 fun CreatePostActivity(
-    accountViewModel: AccountViewModel,
+    accountViewModel: AccountViewModel = viewModel(factory = AccountSettingsViewModelFactory.Factory),


With regards to all of these, I think I'd much prefer them not having defaults, and filling this from MainActivity. Its much more explicit that way, and I've tried to avoid defaults, especially for viewmodels, wherever possible.

dessalines · 2023-07-10T20:56:49Z

                )

-                ShowChangelog(appSettingsViewModel = appSettingsViewModel)
+                ShowChangelog()


Stuff like this scares me, it seems like it has no actual dependencies now, when in reality the appSettings does need to be injected.

# Conflicts: # app/src/main/java/com/jerboa/db/AppDB.kt # app/src/main/java/com/jerboa/model/AccountSettingsViewModel.kt # app/src/main/java/com/jerboa/ui/components/home/Home.kt # app/src/main/java/com/jerboa/ui/components/home/HomeActivity.kt

- fix drawer not showing switch account/sign out

yate · 2023-07-12T03:42:02Z

@dessalines Thanks, I removed the ViewModel parameter defaults.

MV-GH

Rather have this one merged before the others that modify appsettings

dessalines

K I just tested this locally, and it works great. Thx!

yate added 3 commits July 4, 2023 23:36

db refactor

9299528

- reorganize db into different packages - make all queries async, remove need for allowMainThreadQueries() - refactor the way the viewModels were getting initialized

Merge branch 'main' into db-refactor

52b5cf9

formatting

71fda4f

MV-GH mentioned this pull request Jul 5, 2023

Fix recompositions due to autofill #975

Merged

yate added 2 commits July 5, 2023 17:04

fix race condition where it would sometimes get the site info for the…

2268b6f

… default/null account

Merge branch 'main' into db-refactor

4127be2

yate commented Jul 5, 2023

View reviewed changes

add back the login icon enable condition I accidentally removed

8db1842

yate commented Jul 5, 2023

View reviewed changes

yate marked this pull request as ready for review July 5, 2023 22:09

yate requested review from dessalines and twizmwazin as code owners July 5, 2023 22:09

yate added 4 commits July 7, 2023 21:30

Merge branch 'main' into db-refactor

5c0b689

# Conflicts: # app/src/main/java/com/jerboa/db/AppDB.kt

merge main/fix conflicts

d0c7f3c

import reorder

bf13d05

dessalines reviewed Jul 10, 2023

View reviewed changes

yate added 3 commits July 11, 2023 21:13

Merge branch 'main' into db-refactor

076491d

# Conflicts: # app/src/main/java/com/jerboa/db/AppDB.kt # app/src/main/java/com/jerboa/model/AccountSettingsViewModel.kt # app/src/main/java/com/jerboa/ui/components/home/Home.kt # app/src/main/java/com/jerboa/ui/components/home/HomeActivity.kt

remove viewModel injection for view model parameters

7bac450

- move view models to model package

010eefd

- fix drawer not showing switch account/sign out

yate requested a review from MV-GH as a code owner July 12, 2023 03:14

yate added 2 commits July 11, 2023 23:18

formatting

5f3ab5a

formatting

d5d7c6b

Merge branch 'main' into db-refactor

5970f59

MV-GH approved these changes Jul 12, 2023

View reviewed changes

Merge branch 'main' into db-refactor

df94188

dessalines approved these changes Jul 12, 2023

View reviewed changes

dessalines merged commit d545c2c into LemmyNet:main Jul 12, 2023

yate deleted the db-refactor branch July 12, 2023 17:13


		fun CreationExtras.jerboaApplication(): JerboaApplication =
		(this[ViewModelProvider.AndroidViewModelFactory.APPLICATION_KEY] as JerboaApplication)

Conversation

yate commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dessalines commented Jul 5, 2023

Uh oh!

yate Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

yate Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dessalines Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

yate Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

yate Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

yate commented Jul 5, 2023

Uh oh!

dessalines left a comment

Choose a reason for hiding this comment

Uh oh!

dessalines Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

dessalines Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

dessalines Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

yate commented Jul 12, 2023

Uh oh!

MV-GH left a comment

Choose a reason for hiding this comment

Uh oh!

dessalines left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yate commented Jul 5, 2023 •

edited

Loading

yate Jul 5, 2023 •

edited

Loading