Track active changes in the application and show list in manage page. #1517
Conversation
|
Everyone contributing to this PR have now signed the CLA. Thanks! |
| @@ -9,16 +10,16 @@ import 'package:ubuntu_service/ubuntu_service.dart'; | |||
| import '/snapd.dart'; | |||
| import 'logger.dart'; | |||
|
|
|||
| final snapModelProvider = | |||
| ChangeNotifierProvider.family.autoDispose<SnapModel, String>( | |||
| final snapModelProvider = ChangeNotifierProvider.family<SnapModel, String>( | |||
There was a problem hiding this comment.
removed autoDispose to avoid the error while reading in _activeChangeListener, ref.read, since navigating away from snap_page to manage_page will dispose the provider, leading to error related to accessing already disposed object.
I couldn't find better way to listen to changes in snap page, for install and uninstall and notfiy in manage_page.
Any suggestions are appreciated.
There was a problem hiding this comment.
I'm wondering if it's necessary to manually dispose of the snap provider once it's no longer needed to prevent high memory use.
@BLKKKBVSIK you did some profiling on the app-center, right? Do you have any thoughts on this?
There was a problem hiding this comment.
I am also curious if there is any better way to handle this, treating this as a temporary workaround.
Better approach would to get this Snapd REST api issue fixed so that we can listen as an independent provider.
Hello @d-loose, no problem, I am expecting a delayed response due to the holiday season . I have signed the CLA already, have to verify if it's updated, I didn't receive any confirmation, looking for a way to trigger the checks again in the PR. Can you please share which email I should use for the canonical contact detail field in CLA form. |
I re-ran it manually, looks good now - thanks!
You can add @tim-hm ([email protected]) as a contact. |
|
There are some small code style issues that should get caught by the new linter rules we merged this week. Could I ask you to rebase your PR onto the main branch and resolve those issues? I can help you if you run into any issues! |
| @@ -25,6 +26,7 @@ class ManageModel extends ChangeNotifier { | |||
| AsyncValue<void> _state; | |||
|
|
|||
| List<Snap>? _installedSnaps; | |||
| Map<String, (Snap snap, SnapdChange snapdChange)> installingSnaps = new Map(); | |||
There was a problem hiding this comment.
handleSnapChange() handles all types of snap changes, not only installing snaps. If we want to display snap updates and removals on the manage page as well, we should use a more general name here.
Also, is there any reason why we can't use the snap itself as the map key? (i.e. use a Map<Snap, SnapdChange> instead)
There was a problem hiding this comment.
Hello @d-loose, thanks for review.
Agreed on name changes, will make this change, thanks. Current implementation shows the snaps that are being installed and uninstalled as well. So it is good idea to reflect that in the variable name too!
Also, is there any reason why we can't use the snap itself as the map key? (i.e. use a
Map<Snap, SnapdChange>instead)
Used snap name as key to reduce the key size/memory footprint, and also to avoid potential issues with the hashCode changing due to modifications in the object's less significant properties, missing the reads of the keys in Map. Please let me know your thoughts on this.
There was a problem hiding this comment.
Agreed on name changes, will make this change, thanks. Current implementation shows the snaps that are being installed and uninstalled as well. So it is good idea to reflect that in the variable name too!
We'd need to add additional l10n strings that reflect the nature of the active change in the UI, too.
Used snap name as key to reduce the key size/memory footprint, and also to avoid potential issues with the hashCode changing due to modifications in the object's less significant properties, missing the reads of the keys in Map. Please let me know your thoughts on this.
Right, this makes a lot of sense, thanks for clarifying!
Could we maybe use named fields in the record then?
return _ManageSnapTile(
snapdChangeId: snapDetails.$2.id,
snap: snapDetails.$1,
...is a bit confusing to read.
| @@ -149,6 +149,7 @@ class SnapModel extends ChangeNotifier { | |||
| } | |||
|
|
|||
| Future<void> _activeChangeListener(SnapdChange change) async { | |||
| ref?.read(manageModelProvider).handleSnapChange(snap, change); | |||
There was a problem hiding this comment.
I don't have any experience with using WidgetRefs in the view models - is there a good way of testing this in the snap model tests, to make sure handleSnapChange() is called when a snap is installed/refreshed/removed?
There was a problem hiding this comment.
it is of type ChangeNotifierProviderRef and not WidgetRef. I remember seeing this approach of passing refs to ChangeNotifies in Riverpod's official documentation. Will try to find that and share here.
Will explore on a test case to verify the behavior. Thank you.
|
Great work so far, @codinesh! I think it's a good idea to display the active snap changes on the manage page as a first step. We could think about generalizing this later and extract this into some sort of an 'active snap changes model' that can be accessed on any page (for example in some sort of an android-style notification bar on the main page). |
Sure, will do that and also see some conflicts with couple of files, will do a rebase and resolve those. |
|
It'd also be great to get some UI/UX feedback from @anasereijo (probably after the holiday break). I think adding a 'cancel' button would be a good improvement. |
|
Hello @d-loose, closed this PR by mistake while rebasing changes from upstream to my fork. I will create a new PR with the same changes, and will link this PR for reference. Apologies for the inconvenience. |
Fixes #1438, by displaying a section for installing/uninstalling snaps.
Scope of this change to track the changes made from this app, as a workaround to an issue in /v2/changes api, which doesn't give enough information to identify corresponding snap from the response.
Workaround is discussed here.