Skip to content
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

refactor: Migrate away snap model from old provider style #1660

Merged
merged 31 commits into from
Jun 14, 2024

Conversation

spydon
Copy link
Collaborator

@spydon spydon commented Apr 29, 2024

This migrates away from the custom ChangeNotifierProvider approach.

It also makes sure that futures aren't completed until the polling of the changeId has returned which makes the the structure slightly cleaner.

Things that are left to consider:

  • Whether the snap operations should be happening within a model instead of in separate providers like they do now.
  • How to handle actions from buttons.

With a refactor this big I'm sure that some bugs have been introduced, so make sure to test a lot if you continue working on this!

@spydon spydon force-pushed the refactor/migrate-away-from-old-providers branch 2 times, most recently from 2489a36 to 6f591ab Compare May 7, 2024 16:09
@spydon spydon force-pushed the refactor/migrate-away-from-old-providers branch 3 times, most recently from b2de44b to 5946480 Compare June 4, 2024 13:55
@spydon spydon marked this pull request as ready for review June 6, 2024 15:54
@spydon spydon requested a review from d-loose June 6, 2024 15:54
@d-loose
Copy link
Member

d-loose commented Jun 7, 2024

Looks like there are some yaru related style changes (background colors of the containers) we need to address:

Screenshot from 2024-06-07 14-11-05

@spydon spydon force-pushed the refactor/migrate-away-from-old-providers branch from 89dccdb to 8fed937 Compare June 12, 2024 16:31
Comment on lines 27 to 28
// TODO: Do we really need a try-catch here, don't we want to go into
// the AsyncError in all cases?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we're handling the case where the snap isn't locally installed, but it's available in the store (SnapdClient throws an exception when it receives a 4xx status from snapd).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a very smooth way of dealing with a 4xx, maybe we should fix that in the snapdclient at some point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all our dart client libraries for HTTP APIs do it that way at the moment.

In the subiquity client we've got this:

    if (response.statusCode != 200) {
      throw SubiquityException(method, response.statusCode, str);
    }

But if you think it's worth improving this, I'm up for it :)

@d-loose
Copy link
Member

d-loose commented Jun 13, 2024

Looks like there are some yaru related style changes (background colors of the containers) we need to address:

Maybe until we sort out the yaru.dart issues we can simply force AppCard to use the theme's cardColor

diff --git a/packages/app_center/lib/src/widgets/app_card.dart b/packages/app_center/lib/src/widgets/app_card.dart
index 365bf709..5948faec 100644
--- a/packages/app_center/lib/src/widgets/app_card.dart
+++ b/packages/app_center/lib/src/widgets/app_card.dart
@@ -75,6 +75,7 @@ class AppCard extends StatelessWidget {
   @override
   Widget build(BuildContext context) {
     return YaruBanner(
+      color: Theme.of(context).cardColor,
       padding: const EdgeInsets.all(kCardSpacing),
       onTap: onTap,
       child: Flex(

P.S. We'd need a corresponding change for the SnapImageCard, which is a separate widget

@spydon
Copy link
Collaborator Author

spydon commented Jun 13, 2024

Looks like there are some yaru related style changes (background colors of the containers) we need to address:

Maybe until we sort out the yaru.dart issues we can simply force AppCard to use the theme's cardColor

diff --git a/packages/app_center/lib/src/widgets/app_card.dart b/packages/app_center/lib/src/widgets/app_card.dart
index 365bf709..5948faec 100644
--- a/packages/app_center/lib/src/widgets/app_card.dart
+++ b/packages/app_center/lib/src/widgets/app_card.dart
@@ -75,6 +75,7 @@ class AppCard extends StatelessWidget {
   @override
   Widget build(BuildContext context) {
     return YaruBanner(
+      color: Theme.of(context).cardColor,
       padding: const EdgeInsets.all(kCardSpacing),
       onTap: onTap,
       child: Flex(

P.S. We'd need a corresponding change for the SnapImageCard, which is a separate widget

Fixed in 06fe6cf and 79f7dc4

@spydon spydon force-pushed the refactor/migrate-away-from-old-providers branch from 5abe153 to 26b715d Compare June 13, 2024 12:49
Comment on lines -516 to -549
enum SnapAction {
cancel,
install,
open,
remove,
switchChannel,
update;

String label(AppLocalizations l10n) => switch (this) {
cancel => l10n.snapActionCancelLabel,
install => l10n.snapActionInstallLabel,
open => l10n.snapActionOpenLabel,
remove => l10n.snapActionRemoveLabel,
switchChannel => l10n.snapActionSwitchChannelLabel,
update => l10n.snapActionUpdateLabel,
};

IconData? get icon => switch (this) {
update => YaruIcons.refresh,
remove => YaruIcons.trash,
_ => null,
};

VoidCallback? callback(SnapModel model, [SnapLauncher? launcher]) =>
switch (this) {
cancel => model.cancel,
install => model.storeSnap != null ? model.install : null,
open => launcher?.isLaunchable ?? false ? launcher!.open : null,
remove => model.remove,
switchChannel => model.storeSnap != null ? model.refresh : null,
update => model.storeSnap != null ? model.refresh : null,
};
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a final nitpick/question, since it came up in the meeting with Innes as well: to me this feels like UI specific code (maybe icons/l10n more than the actions and corresponding callbacks). Should we keep (some of) this in snap_page.dart?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't remember why I moved it now, it should maybe even get its own file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, I think this is the first big step towards a massive improvement of the overall state management and code structure!

@spydon spydon force-pushed the refactor/migrate-away-from-old-providers branch from 381e159 to a87a41f Compare June 14, 2024 08:28
@spydon spydon merged commit 3e81048 into main Jun 14, 2024
8 checks passed
@spydon spydon deleted the refactor/migrate-away-from-old-providers branch June 14, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants