-
-
Notifications
You must be signed in to change notification settings - Fork 20
Move logic from UI in AddSubscriptionScreen to AddSubscriptionModel
#727
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
Conversation
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
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.
Pull Request Overview
This PR is the first iteration of cleaning up the AddSubscriptionScreen by removing overloads and moving UI logic to the view model. The main goal is to reduce complexity in the UI layer and better separate concerns.
- Removed one overload of
AddSubscriptionScreenand consolidated the logic - Moved file picking logic and initialization logic from UI to the view model
- Simplified UI state management by removing success handling from the composable
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| AddSubscriptionScreen.kt | Removed overload, simplified LaunchedEffect calls, moved UI logic to view model |
| AddSubscriptionModel.kt | Added initialize() and onFilePicked() methods, moved Toast handling from UI to view model |
Comments suppressed due to low confidence (1)
app/src/main/java/at/bitfire/icsdroid/model/AddSubscriptionModel.kt:1
- The
createSubscription()method returnsUnit, not aJoborDeferred, soinvokeOnCompletioncannot be called on it. This will cause a compilation error.
package at.bitfire.icsdroid.model
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/src/main/java/at/bitfire/icsdroid/model/AddSubscriptionModel.kt
Outdated
Show resolved
Hide resolved
|
Okay, I will take a look at this... |
I mean, in any case, even though it's now being displayed from the ViewModel, before, the toast was still being triggered from the VM, so it doesn't make any sense... |
…el.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I will ignore copilot. |
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
…nto clean-add-subscription-screem
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
sunkup
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.
Adding local file does not work. After picking file the subscribe button does not show up.
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
sunkup
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.
See comments. Looks good otherwise.
app/src/main/java/at/bitfire/icsdroid/model/SubscriptionSettingsUseCase.kt
Show resolved
Hide resolved
# Conflicts: # app/src/main/java/at/bitfire/icsdroid/ui/screen/AddSubscriptionScreen.kt
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
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.
Nice! The init block looks kind of scary, we could improve it for clarity. And I also think hashing and summing the objects might not be as performant as simply comparing the affected properties. And would there not also be risk for unnecessary recompositions?
I am happy to review again if you want to add the suggestions, but feel free to merge if you don't think the changes are necessary. 👍
Edit: I also think we should use a more descriptive title along the lines of: "Move logic from add subscription screen to model".
app/src/main/java/at/bitfire/icsdroid/model/AddSubscriptionModel.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
AddSubscriptionScreen cleanupAddSubscriptionScreen to AddSubscriptionModel
sunkup
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.
Nice! Feel free to merge.
Purpose
Right now
AddSubscriptionScreenhas a lot of overloads, and complicatedLaunchedEffectcalls.Simply too much logic is on the UI. We should move it to the view model, or simplify/remove as needed.
Related to #726
Short description
AddSubscriptionScreenChecklist