refactor: Migrate manage snaps providers #1737
Conversation
spydon
force-pushed
the
refactor/migrate-manage-snaps-providers
branch
from
a65cd07 to
e03b704
Compare
spydon
force-pushed
the
refactor/migrate-manage-snaps-providers
branch
from
e03b704 to
2c0f752
Compare
spydon
force-pushed
the
refactor/migrate-manage-snaps-providers
branch
from
7804a99 to
e742b06
Compare
|
Looks really good at first glance! I'll add some more detailed comments later. One thing I noticed so far is that the manage page won't work without a network connection (and even the tile in the sidebar breaks)
|
|
Another nitpick: changing the filters for the list of installed snaps now shows a loading spinner for a very short time, causing a bit of a flicker |
Good catch! Fixed in 5c4e9a9 |
| final refreshableSnapNames = | ||
| (await ref.watch(updatesModelProvider.future)).snapNames; |
There was a problem hiding this comment.
I'm wondering if it'd be better to disentangle the updates from the manage model entirely, or at least use select more often to avoid unnecessary rebuilds and loading states. For example:
Before:
Screencast.from.2024-07-12.11-54-33.webm
After:
Screencast.from.2024-07-12.11-54-55.webm
There was a problem hiding this comment.
Yeah, I forgot about this, I had them separately first, but there was some problem with the slivers.
I'll try that again!
There was a problem hiding this comment.
With the latest changes the loading state behavior is still a bit different
Screencast.from.2024-07-15.17-01-24.webm
I'm wondering what's the best option here - do we need to hide the list of installed snaps during the loading state?
@anasereijo do you have an opinion on that?
| try { | ||
| return _snapd.find(filter: SnapFindFilter.refresh); | ||
| } on SnapdException catch (_) { | ||
| // TODO: Should we ignore all SnapdExceptions here? |
There was a problem hiding this comment.
No, with the new error handling setup, we probably shouldn't catch any errors at all here, and let them be handled by the custom error view (like here) or the top-level error dialog
There was a problem hiding this comment.
Then it won't work when we don't have internet though, will it?
There was a problem hiding this comment.
Ah very good point - I'm obviously contradicting myself here :D
But we need to somehow inform the user that we couldn't fetch updates due to network issues.
| // When kind is null it is most likely a problem with the internet | ||
| // connection. |
There was a problem hiding this comment.
That's not generally true, see #1684, which is a case that we'd need to handle differently I think.
There was a problem hiding this comment.
Aaah, how annoying, if we expose statusCode and maybe status in the SnapdClient maybe we can solve this in a better way.
There was a problem hiding this comment.
Overall I think d1e3b04 is a big improvement!
But I do have more loading state nitpicks:
Related to a previous comment, it seems weird to me that the list of snaps enters a loading state, but the list of updateable snaps doesn't:
Screencast.from.2024-07-17.10-03-47.webm
Starting app center without network connection, waiting for the connection timeout - there are many loading spinners:
Eventually a message that indicates the network state is displayed 👍 - but when checking for updates again, only the list of installed snaps enters a loading state:
This migrates the providers for the manage page to the modern style (#1676) it also fixes #1551.
If there is no internet connection, we currently show this:
And if you click check for updates and the connection is back, it will be going back to its normal state.