-
Notifications
You must be signed in to change notification settings - Fork 115
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
feat: multisnap progress indicator #1637
base: main
Are you sure you want to change the base?
Conversation
little design feedback: |
5fec6c0
to
6fcc6f3
Compare
await _getLocalSnaps(change.snapNames); | ||
notifyListeners(); |
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.
It probably only makes sense to update the model state once all of the changes are ready
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.
I'm a little bit lost here. I have a list of listeners and I remove them as the changes they're tracking are ready. Should I call notifyListeners when this list is empty then? And getLocalSnaps in this case doesn't do anything within the model, just asks snapd if the snaps are there but doesn't store them
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.
I'm a little bit lost here. I have a list of listeners and I remove them as the changes they're tracking are ready. Should I call notifyListeners when this list is empty then?
Yes, I'd treat the installation of the entire bundle as a 'busy' state and would only call notifyListeners()
again when all changes are finished.
And getLocalSnaps in this case doesn't do anything within the model, just asks snapd if the snaps are there but doesn't store them
Hm, so that raises a couple of questions:
- How do you know whether a bundle has already been installed or not? (In the single snap model that's determined by whether or not a local snap is returned from snapd).
- What happens in the case of partially installed bundles? E.g. a user could already have a snap from the bundle installed on their system, but wants to install the remaining ones.
I'd recommend adding some tests for the MultiSnapModel
that tests the installAll
method for various possibilities with clear assertions on what should happen in each case. (probably beyond the scope of this PR)
49b2623
to
fae6ab1
Compare
89d49e5
to
ea37fc8
Compare
@d-loose implemented your feedback regarding the columns/rows and the circular progress indicator. I think it's ready, the only thing I don't like is that if you switch to a different tab, for instance explore and then go back to the bundle, the UI resets. Buuuut to have a persistent indicator, it would require to query snapd for active changes regarding all the apps in the bundle and for some of the bundles it won't scale very nice, wdyt? |
More design feedback :) |
The snapd API isn't great in this regard - you can obtain a list of all active changes from snapd, but unfortunately the individual changes don't tell you which snap they belong to, so you'd have to explicitly request the active changes for each snap in your bundle. I guess that's what you meant, but unless you have dozens of snaps in a bundle that shouldn't really be an issue. |
@@ -12,7 +12,7 @@ part of 'chart.dart'; | |||
T _$identity<T>(T value) => value; | |||
|
|||
final _privateConstructorUsedError = UnsupportedError( | |||
'It seems like you constructed your class using `MyClass._()`. This constructor is only meant to be used by freezed and you are not supposed to need it nor use it.\nPlease check the documentation here for more information: https://github.com/rrousselGit/freezed#adding-getters-and-methods-to-our-models'); |
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.
There are still some outdated mocks here, probably caused by running the build runner with an outdated local version of freezed
. If you run melos pub upgrade
followed by melos generate
you should be in sync again :)
Otherwise you can simply git restore
the entire app_center_ratings_client
.
Right now:
Feedback and ideas are welcomed for this behaviour