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

Added progress indication to InstallMultiplePackages. #1613

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

jedieaston
Copy link
Contributor

@jedieaston jedieaston commented Oct 19, 2021


Resolves #1566.

This PR adds a simple progress indication to the output of installs during InstallMultiplePackages, so that users can see what package is currently being installed and how many they are to go. It's shown to the left of the Found PackageName text (I didn't ask anyone about the design of this before I did it, so if anyone doesn't like it I can fix it).

image

Microsoft Reviewers: Open in CodeFlow

@jedieaston jedieaston requested a review from a team as a code owner October 19, 2021 16:26
@ghost ghost added the Issue-Feature This is a feature request for the Windows Package Manager client. label Oct 19, 2021
@JohnMcPMS
Copy link
Member

@denelon , this is more of an experience review than of code. What do you think?

@JohnMcPMS
Copy link
Member

This will affect upgrade and import right now I believe, but will also affect dependencies when they arrive. We would need to think about how we deal with imports with dependencies, as I believe that in that case the plan is to let each install handle its own dependencies at this time. So the numbering might get thrown off, where the import numbers are interspersed with dependencies numbers.

@jedieaston
Copy link
Contributor Author

I'm curious (I haven't read the spec for dependency management in a hot minute, so apologies if this is answered in there), why won't import create a single dependency tree based on the packages it has to install? If each package in the file had to go through the dependency resolution process, it seems like to me you would have to do a lot more checks than if you started with "I want to install packages X, Y, Z, which require A, B, C...".

Is it just that it's easier to do on a package to package basis?

@JohnMcPMS
Copy link
Member

Given that we are starting with only limited support for dependencies (only minimum version supported, not maximum) there isn't a lot to be gained by doing the extra work at this time. But we might find out that this is wrong and need to go ahead and build the full list before starting. Or I am misremembering it all; would need to go back and look at the PR to see what state it is in. It might also just be an intermediate state of an feature in development.

@vedantmgoyal9
Copy link
Contributor

DISCLAIMER: I am not good at photoshop 😁

Instead of printing the "Found test [test.test] Version xx.xx" for each package, this should be better:

image

(Text representation of the design):

PS C:\Users\easton> wingetdev upgrade --all
The following packages will be upgraded:
-> calibre [calibre.calibre]
-> Node.js [OpenJS.NodeJS]

Number of packages to be upgraded: 2

Starting upgrade... # I forgot this to include it in the above image

(1/2) Found calibre [calibre.calibre] Version 5.29.0
This application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party pack...
Downloading <link>
<Progress Bar>
Successfully verified installer hash
Starting pachage install...
Successfully installed

(2/2) Found Node.js [OpenJS.NodeJS] Version 16.11.1
This application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party pack...
Downloading <link>
<Progress Bar>
Successfully verified installer hash
Starting pachage install...
Successfully installed

PS C:\Users\easton>

@jedieaston would you be able to do it?

@jedieaston
Copy link
Contributor Author

jedieaston commented Oct 24, 2021

That will have to be another PR. That's a side-effect of the Agreements prompting code, where it prints the Found... text regardless of whether or not the package has an agreement to show to the user. If you want to make an issue about it we can talk about it there.

@denelon
Copy link
Contributor

denelon commented Oct 25, 2021

@denelon Demitrius Nelon FTE , this is more of an experience review than of code. What do you think?

I'm fine with this UX as an incremental improvement.

@JohnMcPMS JohnMcPMS merged commit 7ea1caf into microsoft:master Oct 25, 2021
@jedieaston jedieaston deleted the progressing branch October 26, 2021 00:18
@lfshr
Copy link

lfshr commented Oct 26, 2021

@denelon Demitrius Nelon FTE , this is more of an experience review than of code. What do you think?

I'm fine with this UX as an incremental improvement.

Is it worth getting an issue opened to track the next iteration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature This is a feature request for the Windows Package Manager client.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add overall progress for upgrade --all command.
5 participants