-
Notifications
You must be signed in to change notification settings - Fork 312
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
Expanding on App Install URI Protocol Support - Winget Package URIs #2733
Expanding on App Install URI Protocol Support - Winget Package URIs #2733
Conversation
.FirstOrDefault(); | ||
_log.Information("Starting add-apps-to-cart activation"); | ||
_navigationService.NavigateTo(typeof(SetupFlowViewModel).FullName!); | ||
_setupFlowViewModel.StartAppManagementFlow(queryType == ActivationQueryType.Search ? identifiers[0] : null); |
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.
why null if ActivationQueryType
isn't search?
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.
That's because we only want to do the query prefilling into the search box piece in the search scenario.
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.
similar to the other comment, lmk if you think using a bool prefillSearch
might help with clarity here.
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.
Do you know if null is passed anywhere else into StartAppManagementFlow
? MainPageViewModel.StartAppManagementFlow throws if the string is null when trying to log the query.
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.
this is the only place we call StartAppManagementFlow -- it was added with the previous PR: https://github.com/microsoft/devhome/pull/2642/files
With the change to not log the query string, StartAppManagementFlow should not throw if null (tested locally to confirm)
tools/SetupFlow/DevHome.SetupFlow/TaskGroups/AppManagementTaskGroup.cs
Outdated
Show resolved
Hide resolved
tools/SetupFlow/DevHome.SetupFlow/TaskGroups/AppManagementTaskGroup.cs
Outdated
Show resolved
Hide resolved
tools/SetupFlow/DevHome.SetupFlow/ViewModels/MainPageViewModel.cs
Outdated
Show resolved
Hide resolved
After discussing this feature with @AmelBawa-msft, I will be refactoring this PR slightly to take in and utilize WingetPackageURIs instead of WingetIDs. This way we'll be enabling package sources other than winget with minimal extra effort and this can later benefit from any WingetPackageUri feature additions. Converting to draft until I get this change in. |
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.
Approving pending WindowEx modification.
Has any thought been put into having the flow start at the "Review and finish" page since the URI may have already selected what the user wants to install and they can confirm the selection on that page? |
I could see both ways working. As it is now, this would give the user an opportunity to add additional packages while they're there/explore this Dev Home offering. Whereas starting at the "Review and finish" page would reduce a click for any users looking to install only the packages specified in the URI. @joadoumie @krschau any thoughts on this? |
I am also worried about users missing that we added items to the "shopping cart", hence my preference for going to the confirmation "Review and Finish" page. |
My concern with jumping right into the review page is that users can't really edit any of the selected packages there. They can potentially go to 'Previous' but that feels off. If a URI triggers Dev Home to open, I may still want to edit the list of packages that were included in the URI, so to me the cart feels the most fluid. I'm curious to see what @shakersMSFT @denelon think. |
Folks offline agreed to check in as-is, and we can re-evaluate in the future if we want to add the ability to go straight to the review page. |
Summary of the pull request
With this PR, if you win+R:
![image](https://private-user-images.githubusercontent.com/41223743/327247972-ee683634-d0f5-4cf7-90e9-e30bf1b5da70.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYwNjA4NjYsIm5iZiI6MTcyNjA2MDU2NiwicGF0aCI6Ii80MTIyMzc0My8zMjcyNDc5NzItZWU2ODM2MzQtZDBmNS00Y2Y3LTkwZTktZTMwYmYxYjVkYTcwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTElMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTExVDEzMTYwNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTYyMjMzNGZkMjc0Yjk0N2U3ODZmMDdlZTMwMWQ2MWVmZjNhMTRiN2RkZjMwYjAxY2RiMjA5MDIzZTc4YTU4YzImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.PapQtsV2hH2uvqVj7P-UAwSTUF7qStWUbi0M1RFYdkA)
ms-devhome:add-apps-to-cart?WingetURIs="x-ms-winget:https://winget/Git.Git","x-ms-winget:https://winget/Microsoft.VisualStudioCode", "x-ms-winget:https://winget/Microsoft.PowerToys"
[updated 5/22]You will be taken to this view of Dev Home:
This also adds a message dialog for when there is a machine configuration already in progress: [updated 5/22]
![image](https://private-user-images.githubusercontent.com/41223743/332881138-4bcbbbda-ad72-4e63-a211-521f82dbed8f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYwNjA4NjYsIm5iZiI6MTcyNjA2MDU2NiwicGF0aCI6Ii80MTIyMzc0My8zMzI4ODExMzgtNGJjYmJiZGEtYWQ3Mi00ZTYzLWEyMTEtNTIxZjgyZGJlZDhmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTElMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTExVDEzMTYwNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQ5Njg5ZjU0MDljN2ZmZjY5NTY4MmE0ODlhN2ZlZTQ4ODQ1NDA1NDViYzI2M2I1NGI3YjAwMzU3MGU3MjBkMmImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.2OLZ34qc0gsAZev4PXfGWc9unW129lUnX4Z1HEAnuIc)
References and relevant issues
#2428
Detailed description of the pull request / Additional comments
This is mostly a refactoring of the code added in: #2642 to support the winget URIs scenario which allows for specifying multiple packages to select and unlike the "search" option this does not modify the search input box in the AppManagementViewModel.
Validation steps performed
Manually tested through win+R both the search and the new URIs query params.
PR checklist