Track active changes in the application and show list in manage page. #1517

codinesh · 2023-12-07T06:26:33Z

Fixes #1438, by displaying a section for installing/uninstalling snaps.

To Listen to the changes from Snap details page, tapped on to SnapModel._activeChangeListener to know the installs, uninstalls and change updates. This will call the ManageModel to maintain the list of active 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.

github-actions · 2023-12-07T06:27:16Z

Everyone contributing to this PR have now signed the CLA. Thanks!

codinesh · 2023-12-07T17:37:43Z

packages/app_center/lib/src/snapd/snap_model.dart

@@ -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>(


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.

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?

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.

d-loose · 2023-12-12T11:08:05Z

Hey @codinesh sorry for the slow response time these days (I hope it'll improve in January).
I haven't had time to look at your PR in detail yet, but I'll have a look soon! In the meantime, would you kindly sign the CLA?

codinesh · 2023-12-12T11:16:10Z

Hey @codinesh sorry for the slow response time these days (I hope it'll improve in January).
I haven't had time to look at your PR in detail yet, but I'll have a look soon! In the meantime, would you kindly sign the CLA?

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.

d-loose · 2023-12-14T13:29:41Z

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.

I re-ran it manually, looks good now - thanks!

Can you please share which email I should use for the canonical contact detail field in CLA form.

You can add @tim-hm ([email protected]) as a contact.

d-loose · 2023-12-14T13:32:28Z

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!

d-loose · 2023-12-14T13:45:19Z

packages/app_center/lib/src/manage/manage_model.dart

@@ -25,6 +26,7 @@ class ManageModel extends ChangeNotifier {
  AsyncValue<void> _state;

  List<Snap>? _installedSnaps;
+  Map<String, (Snap snap, SnapdChange snapdChange)> installingSnaps = new Map();


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)

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.

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.

d-loose · 2023-12-14T13:56:40Z

packages/app_center/lib/src/snapd/snap_model.dart

@@ -149,6 +149,7 @@ class SnapModel extends ChangeNotifier {
  }

  Future<void> _activeChangeListener(SnapdChange change) async {
+    ref?.read(manageModelProvider).handleSnapChange(snap, change);


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?

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, thanks a lot!

d-loose · 2023-12-14T13:59:15Z

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).

codinesh · 2023-12-14T14:00:49Z

There are so

Sure, will do that and also see some conflicts with couple of files, will do a rebase and resolve those.

d-loose · 2023-12-14T14:08:03Z

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.

codinesh · 2023-12-15T16:04:14Z

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.

codinesh · 2023-12-15T16:11:47Z

Hello @d-loose, apologies, closed this PR by mistake while rebasing changes from upstream to my fork.

Created a new PR with these same changes, can you please have a look here.

codinesh commented Dec 7, 2023

View reviewed changes

d-loose reviewed Dec 14, 2023

View reviewed changes

codinesh closed this Dec 15, 2023

codinesh force-pushed the main branch from 789b560 to e2a4bdd Compare December 15, 2023 15:20

codinesh mentioned this pull request Dec 15, 2023

Track active changes in the application and show list in manage page. #1535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track active changes in the application and show list in manage page. #1517

Track active changes in the application and show list in manage page. #1517

codinesh commented Dec 7, 2023

github-actions bot commented Dec 7, 2023 •

edited

Loading

codinesh Dec 7, 2023 •

edited

Loading

d-loose Dec 14, 2023

codinesh Dec 14, 2023

d-loose commented Dec 12, 2023

codinesh commented Dec 12, 2023 •

edited

Loading

d-loose commented Dec 14, 2023

d-loose commented Dec 14, 2023

d-loose Dec 14, 2023

codinesh Dec 14, 2023

d-loose Dec 14, 2023

d-loose Dec 14, 2023

codinesh Dec 14, 2023

d-loose Dec 14, 2023

d-loose commented Dec 14, 2023

codinesh commented Dec 14, 2023

d-loose commented Dec 14, 2023

codinesh commented Dec 15, 2023

codinesh commented Dec 15, 2023 •

edited

Loading

Track active changes in the application and show list in manage page. #1517

Track active changes in the application and show list in manage page. #1517

Conversation

codinesh commented Dec 7, 2023

github-actions bot commented Dec 7, 2023 • edited Loading

codinesh Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-loose commented Dec 12, 2023

codinesh commented Dec 12, 2023 • edited Loading

d-loose commented Dec 14, 2023

d-loose commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-loose commented Dec 14, 2023

codinesh commented Dec 14, 2023

d-loose commented Dec 14, 2023

codinesh commented Dec 15, 2023

codinesh commented Dec 15, 2023 • edited Loading

github-actions bot commented Dec 7, 2023 •

edited

Loading

codinesh Dec 7, 2023 •

edited

Loading

codinesh commented Dec 12, 2023 •

edited

Loading

codinesh commented Dec 15, 2023 •

edited

Loading