-
Notifications
You must be signed in to change notification settings - Fork 8
feat: appstore #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: appstore #45
Conversation
…ctionButtonMenu, change installation flow slightly to use said FAB
…ay Chalk screenshots
Lint reportResults
Suppressed ResultsNothing here. Lint reportResults
Suppressed ResultsNothing here. |
Lint reportResults
Suppressed ResultsNothing here. Lint reportResults
Suppressed ResultsNothing here. |
matejdro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
It already looks pretty good. I have made several minor comments, mostly related to the architecture. Unfortunately, because amount of added code is large, there are also quite a few comments.
| @Composable | ||
| private fun WatchappListScreenContent( | ||
| state: WatchappListState, | ||
| appStatuses: Outcome<Map<Uuid, AppStatus>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of both of these properties being a map, I think just adding this to the WatchappListState would be much cleaner. Instead of using a LockerWrapper, we could expose our own data class, with these two properties + a reference to the LockerWrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b3cbd17 should fix this, instead of doing this I moved it all into a single flow that combines private flows instead of exposing them directly to the screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, there is still appInstallSources. Ideally, all of this should be exposed directly in the state, without any maps.
apps/ui/src/main/kotlin/com/matejdro/micropebble/apps/ui/list/WatchappListScreen.kt
Outdated
Show resolved
Hide resolved
apps/ui/src/main/kotlin/com/matejdro/micropebble/apps/ui/list/WatchappListScreen.kt
Show resolved
Hide resolved
| import androidx.compose.runtime.setValue | ||
| import androidx.compose.ui.Modifier | ||
|
|
||
| @Suppress("ModifierNotUsedAtRoot") // ExposedDropdownMenuBox is weird |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting suppressions 👍
| import com.matejdro.micropebble.ui.R | ||
|
|
||
| @Composable | ||
| fun NoSourcesDisplay( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is only needed in the appstore module. Is there a reason it's in the common module?
appstore/ui/src/main/kotlin/com/matejdro/micropebble/appstore/ui/AppstoreScreen.kt
Outdated
Show resolved
Hide resolved
appstore/ui/src/main/kotlin/com/matejdro/micropebble/appstore/ui/AppstoreDetailsViewModel.kt
Show resolved
Hide resolved
| actionLogger.logAction { "AppstoreDetailsViewModel.onServiceRegistered()" } | ||
|
|
||
| resources.launchResourceControlTask(_appState) { | ||
| emit(Outcome.Progress()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this is unnecessary, launchResourceControlTask will automatically emit progress at the start. Same for the functions below
| private val actionLogger: ActionLogger, | ||
| private val api: ApiClient, | ||
| ) : SingleScreenViewModel<AppstoreCollectionScreenKey>(resources.scope) { | ||
| val platform by lazy { key.platformFilter?.let { WatchType.fromCodename(it) } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not return any data from non-flow values in the ViewModel.
All ViewModel -> UI data should move through Flows. Can this be added to the appState?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value never changes though, there is no state to it since the platform filter just comes from the key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but the idea is that everything is returned from the ViewModel via a single flow variable. That way it's easier to reason on what data exact is retrieved.
appstore/ui/src/main/kotlin/com/matejdro/micropebble/appstore/ui/AppstoreCollectionViewModel.kt
Outdated
Show resolved
Hide resolved
|
I have pushed CI improvement for the Datastore directly, as it is a pretty specific thing and it was easier than explaining (but you can check it out if you want, it ends up with pretty neat pattern, where Datastore is injected directly into constructor). |
| import kotlin.time.toJavaInstant | ||
| import com.matejdro.micropebble.sharedresources.R as sharedR | ||
|
|
||
| private val LocalDateTimeFormatter = compositionLocalOf { DateTimeFormatter.ISO_INSTANT } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually duplicates existing si.inova.kotlinova.compose.time.LocalDateFormatter: https://github.com/inovait/kotlinova/blob/master/compose/src/main/kotlin/si/inova/kotlinova/compose/time/ComposeAndroidDateTimeFormatter.kt#L184
Adds a new appstore UI, allowing users to search for, download, and install apps from either the Rebble Web Services appstore API or Core Devices appstore API. Installed apps are then checked for updates with a background service and a notification is generated when an app is updatable.
This also upgrades material3 to an alpha version for floating action button menus, which may be able to be replaced if this is undesirable.