Skip to content

Add prompts when installer aborts terminal or needs install location - #1814

Merged
Flor Chacón (florelis) merged 18 commits into
microsoft:masterfrom
florelis:prompts
Aug 12, 2022
Merged

Add prompts when installer aborts terminal or needs install location#1814
Flor Chacón (florelis) merged 18 commits into
microsoft:masterfrom
florelis:prompts

Conversation

@florelis

@florelis Flor Chacón (florelis) commented Dec 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Resolves: #713
Resolves: #970

Changes:

  • Added user setting for default install root
  • Added prompts when an installer has InstallerAbortsTerminal (to confirm that we want to install) or InstallLocationRequired (to provide the install location).
  • Refactored the existing package agreements prompt to a common setup also used by the two new prompts. Before installing a package, we walk through all the possible prompts and show them if applicable. When installing multiple packages, we check all the packages that need the prompt and then show the prompt once.

Remaining items:

  • Testing
  • Use install root from settings
  • Update settings doc
  • Move installers that abort the terminal to the end on upgrade all/import
  • Skip prompts when running non-interactively
Microsoft Reviewers: Open in CodeFlow

JohnMcPMS
JohnMcPMS requested changes Jan 20, 2022
View reviewed changes

@JohnMcPMS JohnMcPMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My comments on the default install root setting also apply to the install location argument on a multipackage install. I'm just not sure it makes sense to have one location apply to multiple packages.

Comment thread doc/Settings.md Outdated
Comment thread schemas/JSON/settings/settings.schema.0.2.json
Comment thread src/AppInstallerCLICore/ExecutionReporter.cpp
@ghost ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Jan 20, 2022
@github-actions

This comment was marked as outdated.

Sign in to view
@florelis

Flor Chacón (florelis) commented Jan 25, 2022

Copy link
Copy Markdown
Member Author

Note that this is still missing tests, validation of the install root and the arg/setting to disable prompts. (Hence still a draft)

I had thought of moving installers that abort the terminal to the end so as to minimize their impact, but that seems tricky with dependencies.

@florelis
Flor Chacón (florelis) marked this pull request as ready for review July 28, 2022 19:25
@florelis
Flor Chacón (florelis) requested a review from a team as a code owner July 28, 2022 19:25
@florelis

Flor Chacón (florelis) commented Jul 28, 2022

Copy link
Copy Markdown
Member Author

Reviving this old PR...

Main changes from last time:

  • Rebased on recent main branch
  • Added a setting and a CLI arg to prevent all prompts
  • Added unit tests

@ghost ghost added Area-Manifest This may require a change to the manifest Issue-Feature This is a feature request for the Windows Package Manager client. labels Aug 5, 2022
JohnMcPMS
JohnMcPMS approved these changes Aug 9, 2022
View reviewed changes
Comment thread doc/Settings.md
Comment thread schemas/JSON/settings/settings.schema.0.2.json Outdated
Comment thread src/AppInstallerCLICore/Argument.cpp Outdated
Comment thread src/AppInstallerCLICore/Workflows/SourceFlow.cpp
@florelis
PR comments
91ac067
@florelis
Merge branch 'master' into prompts
ccded00 
# Conflicts:
#	src/AppInstallerCLICore/ExecutionArgs.h
#	src/AppInstallerCLIE2ETests/Constants.cs
#	src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
#	src/AppInstallerCommonCore/Public/AppInstallerErrors.h
#	src/AppInstallerCommonCore/Public/winget/UserSettings.h
#	src/AppInstallerCommonCore/UserSettings.cpp
@florelis
Fix bad merge
90bc3fe
@florelis
Merge branch 'master' into prompts
0ebeccb
@florelis
Fix bad merge
3870fc4
@florelis
Remove new use of ResourceLoader in COM
eac5677
@florelis
Merge branch 'master' into prompts
e91ab6c 
# Conflicts:
#	src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
@florelis

Flor Chacón (florelis) commented Aug 11, 2022

Copy link
Copy Markdown
Member Author

This PR is having issues with the COM InProc E2E tests because I added something that causes us to load the string resources in a place we weren't doing it before, and the tests fail there because they cannot find the resources. I still don't know how to fix it (and it would make this change bigger), so I'm sidestepping it for now by preventing this new use of the ResourceLoader. I'll open an issue for actually fixing it because I imagine it will hit us again later.

@florelis
Flor Chacón (florelis) merged commit d9f2688 into microsoft:master Aug 12, 2022
@florelis
Flor Chacón (florelis) deleted the prompts branch August 12, 2022 00:17
@denelon Demitrius Nelon (denelon) mentioned this pull request Feb 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohnMcPMS JohnMcPMS JohnMcPMS approved these changes

Assignees

No one assigned

Labels

Area-Manifest This may require a change to the manifest Issue-Feature This is a feature request for the Windows Package Manager client.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Default install root unexpected behaviour - Installation abandoned

2 participants

@florelis @JohnMcPMS