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

installer: skip Experimental Options page, if empty #578

Merged

Conversation

dscho
Copy link
Member

@dscho dscho commented Oct 27, 2024

As of a75602a (installer: hide the "experimental" FSMonitor setting by default, 2024-07-17), the FSMonitor option is hidden from the options page unless the user had enabled it previously.

However, this then left the page blank (or: all options hidden), not a good user experience. So let's skip it if it would appear blank.

This fixes git-for-windows/git#5231.

@dscho dscho self-assigned this Oct 27, 2024
Copy link
Member

@rimrul rimrul left a comment

Choose a reason for hiding this comment

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

The code looks great. We should at some point rework the GP_ prefix recycling though to avoid confusion.

@@ -423,6 +423,7 @@ const
GP_BuiltinAddI = 4;
GP_EnablePCon = 5;
GP_EnableFSMonitor = 6;
GP_Max = 6;
Copy link
Member

Choose a reason for hiding this comment

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

This is a great idea, but the name is slightly confusing since we use the same GP_ prefix for other things.

Copy link
Member Author

Choose a reason for hiding this comment

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

I revamped this rather completely. Could you have another look, @rimrul?

dscho added 4 commits October 30, 2024 17:53
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
For drop-down and radio button options, we used to manually pick the
largest value when defining the size of the corresponding array. Let's
instead introduce consistent `..._Max...` constants for this purpose.

This will make the next commit cleaner, where we need to iterate over
all experimental options.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As of a75602a (installer: hide the "experimental" FSMonitor setting by
default, 2024-07-17), the FSMonitor option is hidden from the options
page unless the user had enabled it previously.

However, this then left the page blank (or: all options hidden), not a
good user experience. So let's skip it if it would appear blank.

This fixes git-for-windows/git#5231.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…xperimental option

If all of the experimental options are hidden, we skip that page.
However, this is the last page of the wizard (at least if there are
no active processes that would block the installation), and that is
therefore the page where we re-label the "Next" button so that it says
"Install".

Therefore, if the experimental options page is hidden, we need to move
that button re-labeling to the preceding page.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho force-pushed the avoid-empty-experimental-options-page branch from 70911c3 to 18e1a8d Compare October 30, 2024 17:17
Copy link
Member

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

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

Good fix and nice QOL improvements too :-)

@dscho dscho merged commit ba0f813 into git-for-windows:main Nov 5, 2024
6 checks passed
@dscho dscho deleted the avoid-empty-experimental-options-page branch November 5, 2024 13:58
@dscho
Copy link
Member Author

dscho commented Nov 5, 2024

/add relnote bug Due to a bug introduced in the v2.47 cycle, the installer showed an empty "experimental options" page, which was fixed.

The workflow run was started

github-actions bot pushed a commit that referenced this pull request Nov 5, 2024
Due to a bug introduced in the v2.47 cycle, [the installer showed an
empty "experimental options"
page](git-for-windows/git#5231), which was
[fixed](#578).

Signed-off-by: gitforwindowshelper[bot] <gitforwindowshelper-bot@users.noreply.github.com>
@jerryfleurival
Copy link

jerryfleurival commented Nov 25, 2024

@dscho

As of a75602a (installer: hide the "experimental" FSMonitor setting by default, 2024-07-17), the FSMonitor option is hidden from the options page unless the user had enabled it previously.

However, this then left the page blank (or: all options hidden), not a good user experience. So let's skip it if it would appear blank.

This fixes git-for-windows/git#5231.

Yes, this indeed need to improve user experience of the last page. As for all, This does not effect the installation process or nothing wrong with the installation as you can go ahead and just hit the "Install" button and installation will begin. Though, at some point in the future, this (user experience) needs to be address.

As for enabling experimental options, I would assume the user set core.fsmonitor to true either by system, global or repository(local) levels in the command line themselves. Experimental option such as fsmonitor.allowRemote.

@dscho
Copy link
Member Author

dscho commented Nov 25, 2024

This fixes git-for-windows/git#5231.

Yes, this indeed need to improve user experience of the last page.

@jerryfleurival I don't understand... Do you mean to say that this is not fixed for you? For me, the last page is:

image

After that, I only get the following page if I have a Git Bash open, otherwise it immediately starts to install Git for Windows:

image

At no point do I see an empty "Experimental Options" page. Is this different for you?

@jerryfleurival
Copy link

@dscho I'm on version 2.47.0.2. I left those two unchecked, and then came a blank Experimental Options page.
Ref: git-for-windows/git#5273 (comment)

@dscho
Copy link
Member Author

dscho commented Nov 26, 2024

I'm on version 2.47.0.2.

Well, the fix is in v2.47.1, that's why you don't have it. You need to upgrade.

@kosai-ksdfounder

This comment was marked as off-topic.

@kosai-ksdfounder

This comment was marked as off-topic.

@kosai-ksdfounder

This comment was marked as off-topic.

kosai-ksdfounder

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup: Configuring experimental options is empty
5 participants