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

feat: Pre-cache all images from the PageConfigProvider #285

Merged
merged 8 commits into from
Jan 2, 2024

Conversation

spydon
Copy link
Contributor

@spydon spydon commented Dec 19, 2023

This PR adds a image cache that loads all images specific by the white labeling config, the loading page is shown meanwhile.
It also speeds up some loading by starting to run multiple futures at once instead of running the sequentially.

@spydon spydon requested a review from d-loose as a code owner December 19, 2023 14:56
@@ -78,6 +78,8 @@ Future<void> runInstallerApp(
);
tryRegisterService<DesktopService>(GnomeService.new);
tryRegisterServiceFactory<GSettings, String>(GSettings.new);
tryRegisterService(
() => PageConfigService(config: tryGetService<ConfigService>()));
Copy link
Member

Choose a reason for hiding this comment

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

Could we keep those services in alphabetic order?

import 'package:ubuntu_wizard/ubuntu_wizard.dart';
import 'package:yaru/yaru.dart';

import 'confirm/test_confirm.dart';
Copy link
Member

Choose a reason for hiding this comment

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

We should probably import '../../ubuntu_provision/test/test_utils.mocks.dart' here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, in the long run we should probably create an internal package in the monorepo with test helpers, so that we don't have to import files like this.

@@ -138,7 +138,7 @@ class _$PageConfigEntryImpl implements _PageConfigEntry {
}

@override
bool operator ==(dynamic other) {
bool operator ==(Object other) {
Copy link
Member

Choose a reason for hiding this comment

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

Are those unintended changes caused by the linter? Freezed seems to generate those lines with a dynamic type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new version generates Object instead of dynamic, it's strange that it isn't Object? though...

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I was certain I ran pub upgrade.. I'll check again :)

Copy link
Contributor

github-actions bot commented Jan 2, 2024

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

void setupMockPageConfig({Map<String, PageConfigEntry> pages = const {}}) {
final pageConfigService = MockPageConfigService();
registerMockService<PageConfigService>(pageConfigService);
when(pageConfigService.pages).thenReturn(pages);
Copy link
Member

Choose a reason for hiding this comment

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

Misses a stub for excludedPages (error is thrown in installer_test/initializes subiquity, which somehow doesn't cause a test failure 😕)

@d-loose
Copy link
Member

d-loose commented Jan 2, 2024

Looks very nice so far, great job!
Did you have a look at the test failures? I can't tell if they're related to recent yaru changes, since I can't reproduce them locally again..

Oh, and you seem to be mixing up github accounts causing the CLA check to fail.

@spydon spydon force-pushed the feat/images-cache branch from 148a2a8 to 2ecf76e Compare January 2, 2024 10:35
@d-loose
Copy link
Member

d-loose commented Jan 2, 2024

Did you have a look at the test failures? I can't tell if they're related to recent yaru changes, since I can't reproduce them locally again..

Sorted out flutter versions and dependency issues - tests fail locally for me now. But it seems like they even fail on the main branch, so they might be related to other dependency changes. I'll investigate after lunch.

@d-loose
Copy link
Member

d-loose commented Jan 2, 2024

I suspect this is what broke the tests.

@spydon
Copy link
Contributor Author

spydon commented Jan 2, 2024

Sorted out flutter versions and dependency issues - tests fail locally for me now. But it seems like they even fail on the main branch, so they might be related to other dependency changes. I'll investigate after lunch.

Ah, I just rebased on main, that must have been why I didn't see it previously.

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.

LGTM 👍

@spydon spydon force-pushed the feat/images-cache branch from 4fe41d8 to 248b5a8 Compare January 2, 2024 12:38
@spydon spydon merged commit 19e7f9c into main Jan 2, 2024
12 of 14 checks passed
@spydon spydon deleted the feat/images-cache branch January 2, 2024 12:45
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.

2 participants