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.

Sign up for GitHub

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

Jump to bottom

Display packages from restore device #74

Merged
merged 14 commits into from
Mar 23, 2023
Merged

Display packages from restore device #74

merged 14 commits into from
Mar 23, 2023

Conversation

AmelBawa-msft
Copy link
Contributor

@AmelBawa-msft AmelBawa-msft commented Mar 22, 2023 •
edited
Loading

🚀 What was done?

  • Added the restore packages section in the app management page

🧪 Test

  • Added unit test
    image

@AmelBawa-msft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AmelBawa-msft AmelBawa-msft marked this pull request as ready for review March 22, 2023 22:09
@AmelBawa-msft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

florelis
florelis approved these changes Mar 22, 2023
View reviewed changes
tools/SetupFlow/DevHome.SetupFlow.AppManagement/ViewModels/PackageViewModel.cs Outdated Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/ViewModels/PackageViewModel.cs Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/ViewModels/PackageViewModel.cs Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/Services/WinGetPackageDataSource.cs Outdated Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/Services/WinGetPackageDataSource.cs Outdated Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/ViewModels/PackageCatalogListViewModel.cs Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/ViewModels/PackageViewModel.cs Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/Views/AppManagementReviewView.xaml Outdated Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow.AppManagement/Views/AppManagementView.xaml Outdated Show resolved Hide resolved
tools/SetupFlow/DevHome.SetupFlow/Strings/en-us/Resources.resw Outdated Show resolved Hide resolved
@AmelBawa-msft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

asklar
asklar reviewed Mar 23, 2023
View reviewed changes
src/Services/ThemeSelectorService.cs
// If theme is Default, use the Application.RequestedTheme value
// https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.elementtheme?view=windows-app-sdk-1.2#fields
return Theme == ElementTheme.Dark ||
(Theme == ElementTheme.Default && Application.Current.RequestedTheme == ApplicationTheme.Dark);
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 you want ActualTheme? what the app requested at launch might not be relevant (e.g. if the user changed the theme once the app was already running)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried locally to change the theme while the application is still running. The value of get RequestedTheme reflects the current system theme set by the user:

Example of switching the system them while the app is still running: Dark > Light > Dark
image

I used the suggested behavior from the ElementTheme, but let me know if I should try something else. ActualTheme is not available at the app level, but I can possibly extract it from the MainWindow root element, however ActualTheme is of type ElementTheme with values Default/Dark/Light, hence I would still need a method to resolve Default.

asklar
asklar reviewed Mar 23, 2023
View reviewed changes
tools/SetupFlow/DevHome.SetupFlow.AppManagement/Services/WinGetPackageDataSource.cs Outdated
Comment on lines 54 to 55
Func<T, string> packageIdFunc,
Func<IWinGetPackage, T, Task> packageProcessorFunc = null)
Copy link
Member

Choose a reason for hiding this comment

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

these are starting to be a little unwieldly... can these become delegates?

Copy link
Member

Choose a reason for hiding this comment

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

also, is this an internal api or is this something extensions might interact with?

Copy link
Contributor Author

@AmelBawa-msft AmelBawa-msft Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Updated to use delegates 👍
This is internal and specific for loading packages from JSON and restore API. I don't expect other extensions or projects to depend on it

asklar
asklar reviewed Mar 23, 2023
View reviewed changes
tools/SetupFlow/DevHome.SetupFlow.AppManagement/ViewModels/AppManagementViewModel.cs

// Connect to composite catalog used for searching on a separate
// (non-UI) thread to prevent lagging the UI.
await Task.Run(async () => await _wpm.AllCatalogs.ConnectAsync());
Copy link
Member

@asklar asklar Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

does the page navigation need to wait for these to complete, or can it be more "fire-and-forget" / show a shimmer animation or progress ring while this is happens (how long does it normally take?)

Copy link
Contributor Author

@AmelBawa-msft AmelBawa-msft Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Displaying shimmers happens in the line above. The events sequence on page navigation is:

  • Display shimmers
  • Connect to WinGet catalog
  • Load packages to display and remove shimmers
  • Connect to AllCatalogs composite catalog (this is for enabling the search experience)

how long does it normally take?

Connecting to a catalog takes a couple seconds, but that specific operation should not block the UI.

@AmelBawa-msft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AmelBawa-msft AmelBawa-msft merged commit 244f82b into main Mar 23, 2023
@AmelBawa-msft AmelBawa-msft deleted the user/amelbawa-msft/restore branch March 23, 2023 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huzaifa-d huzaifa-d huzaifa-d left review comments

@florelis florelis florelis approved these changes

@asklar asklar asklar approved these changes

@dhoehna dhoehna Awaiting requested review from dhoehna

@bbonaby bbonaby Awaiting requested review from bbonaby

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AmelBawa-msft @florelis @huzaifa-d @asklar