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

skip options pages when upgrading #270

Merged
merged 9 commits into from Dec 16, 2019
Merged

Conversation

@denised
Copy link
Contributor

denised commented Nov 20, 2019

When upgrading Git-for-Windows, the current default behavior is to go through the entire installation dialog, which shows a number of pages for setting options. This change suppresses those pages during upgrade, using the previous values (or values specified on the command installer command line, if they are present). It does so by checking, for each options page, whether all the values on it have previously-set values.

Behavior on two edge cases:
If a new option page is added, it's values won't be previously set, so it will be shown, even on upgrade.
If the user uninstalls and then reinstalls Git for Windows, all the pages/options are shown.

One known flaw: when all pages are shown, the last page before installation has a button labeled 'Install' instead of 'Next'. With this change, the user may not see an 'Install' button. I think that is a reasonable trade-off for the simpler experience for users.

        Signed-off-by: Denise Draper <draperd@acm.org>
@dscho

This comment has been minimized.

Copy link
Member

dscho commented Nov 20, 2019

Great, thank you for working on this!

Signed-off-by: Denise Draper draperd@acm.org

@denised could you add that to the commit message, please? It should be as easy as git commit --amend -s, then force-pushing.

Copy link
Member

dscho left a comment

This is a great initial revision, really good to start the conversation and get the ball rolling.

In addition to the suggestions I left as inline comments, could I ask you to change the commit message so that it starts with installer: ? That will make it easier to discern this commit from commits that touch non-installer parts in build-extra.

Thanks!

installer/install.iss Outdated Show resolved Hide resolved
installer/install.iss Outdated Show resolved Hide resolved
installer/install.iss Outdated Show resolved Hide resolved
installer/install.iss Outdated Show resolved Hide resolved
@dscho dscho mentioned this pull request Nov 20, 2019
@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Nov 21, 2019

Let me do a meta-comment that addresses several of your comments and the rationale for the way I coded it. I'm open to another solution, but I want to be sure this rationale is understood:

There are two kinds of pages put out by the installer: built in pages like the components page, and custom pages that are all created within the InitializeWizard procedure. The code changes I have made affect only the custom pages.

Currently, the standard pages that can be displayed are the Licensing page, the Installation Location page, the Components Page and the Start Menu location page. By experimentation, I see that it is already the case that the installer does not show the Installation Location or Start Menu Location pages on upgrade. It only shows the License and components pages. I did not investigate how or why what is the case, but point it out that there is precedence for upgrade being a simpler process.

The existence of standard pages, not created in code, is also why I chose to make the list an affirmative list of pages to be skipped. If there were instead a list of pages to be shown, then I would have to be sure that list contained the ID of every page, including the standard pages that are created only by configuration. No matter how that list was initialized, it would create a point in the code that anyone modifying the standard pages would have to remember to check and possibly update.

Conversely, the fact that all the custom pages are created within one procedure in install.iss makes it fairly easy for a future developer to see the pattern and follow it if they want to add another page to the installer.

Finally, if someone does by chance omit the code to skip a page, the result is to show the user a page they didn't have to see. This is less problematic than the other way around, which is to not show the user a page that they should see. In summary, I think the direction of the logic I chose for determining if a page should be shown is the safer of the two alternatives.

The usability question is whether to show a 'Fast Install' option or simply to do a fast install in the case of upgrade. That is a matter of taste, I think. Personally, I cannot imagine a situation in which a user would want to revisit the installation options when doing a routine upgrade, so I find this solution preferable. But if you feel strongly that it should be the other way, I don't object, and it should be possible to do.

BTW, I tested various combinations of this patched version and the existing installer on a VM to validate the behavior, particularly distinguishing between upgrade and uninstall/reinstall, to make sure it behaved as expected.

@dscho

This comment has been minimized.

Copy link
Member

dscho commented Nov 21, 2019

Let me do a meta-comment that addresses several of your comments and the rationale for the way I coded it.

Thank you for this thoughtful and detailed write-up.

The existence of standard pages, not created in code, is also why I chose to make the list an affirmative list of pages to be skipped.

Let me add my rationale for doing the reverse: to me, "add to skip" is a contradiction in itself. To me, it would make a lot more sense to add pages to the list to be shown.

As to the conundrum about internal pages: you're right, I overlooked that. But that is also easily solved: In the very same function where I'd like to build the list of pages to be shown (GetPreviousData()), I would now also like to build the list of custom pages. That way, we can test very clearly: is this a custom page (i.e. a candidate for skipping)? If yes, does it have previously unseen choices?

The reason why I harp on this is that following the Don't Repeat Yourself principle has saved us time and time again from maintenance hassle.

inally, if someone does by chance omit the code to skip a page, the result is to show the user a page they didn't have to see. This is less problematic than the other way around, which is to not show the user a page that they should see.

I agree that it is less problematic, but not by much. Users would complain, rightfully, and very very loudly, that this piece of *** is so broken it shows some pages and some not and it is inconsistent. I'd rather not even hear those voices, I've had my share.

The better idea is to make it easier to get future changes right. And that means that we should avoid forcing future changes to repeat the same statements (or very, very similar ones) that we already added in plenty of places.

Not repeating yourself has a further benefit: it makes it easier for reviewers not to miss something obvious, such as a forgotten place to add those repeated statements.

And as I write this, I realize that I repeat myself quite often here. I'd rather not, but it is an important point: Don't Repeat Yourself in your code. It won't end well, ever.

Personally, I cannot imagine a situation in which a user would want to revisit the installation options when doing a routine upgrade, so I find this solution preferable.

Sure, during an upgrade you might want to skip those options, that's a nice thing to have.

But we do suggest, frequently, to reinstall Git for Windows, to change options. For example, if a user wants to use some command-line program that would require winpty in the Git Bash, and that does not actually play well with winpty, then we would suggest for that user to re-install Git for Windows and choosing the default Windows Console instead of MinTTY.

We must not break that use case.

Of course, it is a rarer use case, so I agree that we should probably skip the already-seen options by default.

BTW there is one particular scenario that is still not addressed, not even by ideas. Suppose that Git for Windows adds support for Yet Another Editor, and users are eager to choose that option. In the current state of this PR, they would not get that opportunity, ever. Any ideas how to address that?

@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Nov 21, 2019

@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Nov 21, 2019

@dscho

This comment has been minimized.

Copy link
Member

dscho commented Nov 21, 2019

So yes, switching to a fast-path / complete path installation option is the better way to go.

Right, and if we detect that the same version (or a downgrade), then we don't skip a thing.

the nature of the changes proposed is extensive enough that it is easier to do a new pull request than to amend this one

I agree that the changes are extensive, but the goal remains the same, and it builds on top of the conversation here. So I'd much prefer amending over opening a separate (seemingly unrelated) PR.

@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 1, 2019

I have completely rewritten this feature following the discussion above. There is now an additional page, which shows only during upgrade, allowing the user to select whether to skip options or not.
As before, if there is a new option that the user has not previously selected a value for, that option page will be shown even if the user selects 'skip options'.

I have tested upgrade and re-install scenarios, and also temporarily added a new option just to make sure that logic worked.

Some comments/observations:

  • I have still left the original logic that suppresses two built-in installer pages: setting the installation directory and start menu directory. As it stands, these pages are only shown for an initial installation (no git-for-windows installed). I think this is reasonable, but if you really wanted consistency, you might also want these pages to be shown if the user requests 'review all options.'
  • The built-in components page is always shown. This is by design from the inno install perspective, but given that most of the things on the components page are actually installation options, it may be confusing from a user's perspective. The fix would be to move the components 'options' to custom options pages similar to the other options.
  • It bothers me a little bit that when you skip options, the last 'Next' button jumps straight into installation, whereas when you review options, you get an 'Install' button. Inno Setup supports a 'pre-install' page that gives the user a stopping point before jumping into the actual install. That page can be used to show all the selected options. I think this might be a good idea to add (especially since it will allow a user to quickly review all the options they are implicitly agreeing to.)
  • @dscho previously brought up a corner case: what if a new editor is added to the list? With the current code this will not be caught as a 'new option', however re-installing or selecting 'review all options' will easily permit a user to get select the new editor.
@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 1, 2019

Hmm... I left a big comment just a few minutes ago, and it seems to have disappeared. If this is a duplicate, you can skip reading it now...

Commit 8c84615 is a complete rewrite of the feature following the discussion above. The installer now shows a page (only during upgrade), allowing a user to chose to skip options or review all options. The options pages are shown or hidden accordingly. As before, if there is a new option (one that the user has not previously selected a value for), that options page will be shown even if the user selected skip options.

I have tested upgrade and re-install scenarios, and also (temporarily) added a new option just to make sure that part of the logic worked as well.

@PhilipOakley

This comment has been minimized.

Copy link
Contributor

PhilipOakley commented Dec 1, 2019

I left a big comment just a few minutes ago, and it seems to have disappeared.

It did come through on email, and is currently showing on this issue page #270 with your comments marked as "1 hour ago", so looks OK.

Copy link
Member

dscho left a comment

Would you mind squashing the commits into a single one? I do not think that it would make sense to keep the earlier attempt in the commit history.

If you want, though, I would not mind having the change separated out into its own commit where ReplayChoice() is modified to take the page ID as a function parameter.

Or could we use PrevPageID instead of having to change every ReplayChoice() call?

installer/install.iss Outdated Show resolved Hide resolved
installer/install.iss Outdated Show resolved Hide resolved
@dscho

This comment has been minimized.

Copy link
Member

dscho commented Dec 1, 2019

  • It bothers me a little bit that when you skip options, the last 'Next' button jumps straight into installation, whereas when you review options, you get an 'Install' button

You could imitate the existing code that sets that label to Install on the last page when no processes have to be shown.

Or maybe we can adjust PageIDBeforeInstall to reflect the last page that is not skipped? One way to do that would be to have a loop that tests whether PageIDBeforeInstall would be skipped, and if so, automatically goes back by one. Probably similar to this completely untested code:

while (PageIDBeforeInstall>PathPage.ID) and
         IsInSet(CustomOptionsPages, PageIDBeforeInstall) and
        not IsInSet(CustomOptionsPagesWithNewValues,PageIDBeforeInstall) do
    PageIDBeforeInstall:=PageIDBeforeInstall-1;
@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 2, 2019

  • It bothers me a little bit that when you skip options, the last 'Next' button jumps straight into installation, whereas when you review options, you get an 'Install' button

You could imitate the existing code that sets that label to Install on the last page when no processes have to be shown.

Or maybe we can adjust PageIDBeforeInstall to reflect the last page that is not skipped? One way to do that would be to have a loop that tests whether PageIDBeforeInstall would be skipped, and if so, automatically goes back by one. Probably similar to this completely untested code:

while (PageIDBeforeInstall>PathPage.ID) and
         IsInSet(CustomOptionsPages, PageIDBeforeInstall) and
        not IsInSet(CustomOptionsPagesWithNewValues,PageIDBeforeInstall) do
    PageIDBeforeInstall:=PageIDBeforeInstall-1;

I don't think these would work because the last page may very well be the page asking the user if they skip or full options, and I don't think it would be a good idea to change the button interactively :-) If you are not comfortable with a separate 'pre-install' page, then I think the current design should be left alone.

@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 2, 2019

Would you mind squashing the commits into a single one? I do not think that it would make sense to keep the earlier attempt in the commit history.

If you want, though, I would not mind having the change separated out into its own commit where ReplayChoice() is modified to take the page ID as a function parameter.

Or could we use PrevPageID instead of having to change every ReplayChoice() call?

Whoops! I did the squash before reading your full comment.
PrevPageID is not global currently, which is why I did not use it that way. We could make it global, but then it really should have a different name (since it actually refers to the current page, not the previous one), ... but that would then contradict the naming convention used in the Inno Setup samples that uses the variable PrevPageID in exactly the way it is used in this code. In my view, passing the ID explicitly into ReplayChoice is a reasonable thing to do anyway, considering we are asking it to perform this page-accumulation as a side effect.

Separating the code into two commits is possible if you think it important.

@denised denised requested a review from dscho Dec 2, 2019
@dscho

This comment has been minimized.

Copy link
Member

dscho commented Dec 2, 2019

PrevPageID is not global currently

That's a good point which I had missed. (And yes, that means I would suggest to split out the refactoring as a preparatory step, as it makes the commit topology clearer as well as easier to review.)

Yet another idea occurred to me after sleeping over it: other installers have a Next and an Install button, where the latter just skips the rest of the wizard pages.

And a variation of that idea would be to add a checkbox to the bottom, adjacent to the Next button. This checkbox would toggle whether or not to skip wizard pages with only already-seen options. The only question would be how to label it (it should have a succinct, yet intuitive label).

What do you think?

@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 3, 2019

PrevPageID is not global currently

That's a good point which I had missed. (And yes, that means I would suggest to split out the refactoring as a preparatory step, as it makes the commit topology clearer as well as easier to review.)
Yet another idea occurred to me after sleeping over it: other installers have a Next and an Install button, where the latter just skips the rest of the wizard pages.
And a variation of that idea would be to add a checkbox to the bottom, adjacent to the Next button. This checkbox would toggle whether or not to skip wizard pages with only already-seen options. The only question would be how to label it (it should have a succinct, yet intuitive label).
What do you think?

I thought of this approach, but also didn't know how to label the button properly :-)
I think it is okay to accept this design as is, and if someone later has a better inspiration, they can make that a separate change.

I have separated the commits into two. Unless there is anything more, I think this pull request is complete.

@denised denised closed this Dec 3, 2019
@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 3, 2019

Ack! I hit the close button by accident!

@denised denised reopened this Dec 3, 2019
@dscho

This comment has been minimized.

Copy link
Member

dscho commented Dec 5, 2019

I haven't forgotten about this PR, promise! Life just got in the way, and I still want to sleep a couple nights over the question whether we can introduce a checkbox with a nice label to skip ahead, as well as whether there is possibly a good way to point out new options (such as a new editor) while upgrading.

@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 16, 2019

denised and others added 7 commits Nov 20, 2019
This patch introduces the concept of a "page set": a string consisting
of comma-delimited page IDs.

Then, two such sets are built up: a set containing all the pages with
custom options that we potentially want to show to the user, and a set
with all the pages containing previously-unseen options.

So far, this does not help anything yet, the next commit will introduce
a way to be able to skip pages that contain only options that the user
saw before and for which they already chose the desired option.

Signed-off-by: Denise Draper <draperd@acm.org>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This introduces a new custom page at the very beginning when an upgrade
is detected, allowing the user to skip all pages that contain only
options that were previously seen (and for which choices were made).

Signed-off-by: Denise Draper <draperd@acm.org>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
With the new `--test` option, the `installer/release.sh` script can be
used to debug not only one page, but all of them. This comes in
particularly handy in conjunction with the `--skip-files` option.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This makes for a much nicer user experience: this way, it is possible to
turn on the page-skipping even after a couple of pages, without having
to go all the way back to the beginning.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We already change the label to `Install` when the `Processes` page is
suppressed and we're just before said page.

Now we also change the label if all of the remaining pages would be
skipped.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When determining which page is the last one before the `Processes` page,
we are hard-coding the choice between the experimental options page (if
there are any experimental options) or the extra options page.

However, this hard-coding is totally unnecessary, as we do keep track of
the latest page we created, therefore it is just a matter of assigning
the variable at the exact right time.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
If no previous Git for Windows installation was found, then very
obviously there are no previously unseen options we might want to skip.
In this case, simply suppress the checkbox.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added 2 commits Dec 16, 2019
In `RefreshProcessList()`, we want to make sure that a selected range of
`.exe` and `.dll` files are not currently in use because we are about to
overwrite them.

In this code, we expand the `app` constant over and over.

That turns out to be totally unnecessary, as we already have the global
variable `AppDir` ready for use.

A further advantage of using `AppDir` is that it is available even in
the `wpInfoBefore` page (i.e. the page where we show the GPL before even
starting the installer). This is in contrast to the `app` constant,
which is only available after the `wpSelectDir` page (whether that page
was shown or skipped).

As a consequence, we will be able to call `RefreshProcessList()` even on
that first wizard page, to determine whether we can skip essentially all
the pages because the user has seen all of the options before already.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Except for fresh installs, users arguably have seen all the non-custom
pages, including the Components page. Therefore, it makes sense to allow
skipping those when that box "Only show new options" is checked.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho

This comment has been minimized.

Copy link
Member

dscho commented Dec 16, 2019

And have you come to any conclusions? :-)

Yes, I have.

In light of the download statistics (three quarters of a million, slightly less than a week after it has been released), I do think that it makes sense to put in an extra effort to make this thing easy and pleasant to use.

Personally I cannot think of a way to do either of these things where the code impact does not far outweigh the potential benefit.

Please have a look at the changes I added (either look at the interdiff, or at the individual commits -- including your initial commits which I fixed up a little).

The short version of it is that an upgrade will now ideally look similar to this:

image

Note that:

  1. there is a checkbox at the bottom,
  2. it is checked by default for upgrades, and
  3. the label of the Next > button was changed to Install.

All the user has to do is to hit Enter, and not a dozen times, but only once. If they so desire, they can uncheck that box (even with the convenient keyboard shortcut Ctrl+O) and walk through all the pages at their own leisure.

@dscho
dscho approved these changes Dec 16, 2019
@dscho dscho merged commit 3c00b55 into git-for-windows:master Dec 16, 2019
1 check passed
1 check passed
DCO DCO
Details
dscho added a commit that referenced this pull request Dec 16, 2019
When upgrading Git for Windows, by default the
installer [now only shows pages with previously-unseen
options](#270).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@denised

This comment has been minimized.

Copy link
Contributor Author

denised commented Dec 17, 2019

@linquize

This comment has been minimized.

Copy link

linquize commented Jan 17, 2020

When "Only show new options" is checked, it will automatically show release notes at the end of installation, skipping "Completing the Git Setup Wizard" page.

@dscho

This comment has been minimized.

Copy link
Member

dscho commented Jan 17, 2020

When "Only show new options" is checked, it will automatically show release notes at the end of installation, skipping "Completing the Git Setup Wizard" page.

Yes. This is an option you already saw (but your choice has not been recorded) ;-)

@dscho

This comment has been minimized.

Copy link
Member

dscho commented Jan 20, 2020

@linquize if you think that the last page should be shown after a successful installation even if that checkbox is checked, please open a PR to make it so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.