Pull dialog: add mnemonics, fix tab order #9622

Timwi · 2021-10-06T07:47:05Z

Proposed changes

Adds mnemonics to every interactable element in the Pull dialog.
Fixes the tab order in that dialog.

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

Timwi · 2021-10-06T18:19:58Z

Why did this build fail? There is no error message in the build log.

gerhardol · 2021-10-06T18:54:04Z

Why did this build fail? There is no error message in the build log.

The build hanged for some reason.
Maintaining tests requires about the same effort as maintaining code

Edit: I restarted the build, OK now

RussKie · 2021-10-07T05:00:57Z

Please post screenshots before and after

Timwi · 2021-10-08T13:52:53Z

Before:

After:

RussKie · 2021-10-09T00:50:49Z

We are changing existing mnemonics, some users may have come to depend on those.

mstv

We are changing existing mnemonics, some users may have come to depend on those.

I agree, Merge and Rebase are very important options of this dialog. Their mnemonics should not be changed.

mstv · 2021-10-10T15:47:57Z

GitUI/CommandsDialogs/FormPull.Designer.cs

            this.PullFromRemote.TabStop = true;
-            this.PullFromRemote.Text = "Remote";
+            this.PullFromRemote.Text = "&Remote";


Suggested change

this.PullFromRemote.Text = "&Remote";

this.PullFromRemote.Text = "R&emote";

mstv · 2021-10-10T15:49:50Z

GitUI/CommandsDialogs/FormPull.Designer.cs

-            this.AddRemote.TabIndex = 2;
-            this.AddRemote.Text = "Manage remotes";
+            this.AddRemote.TabIndex = 7;
+            this.AddRemote.Text = "&Manage remotes";


Suggested change

this.AddRemote.Text = "&Manage remotes";

this.AddRemote.Text = "Mana&ge remotes";

mstv · 2021-10-10T15:50:27Z

GitUI/CommandsDialogs/FormPull.Designer.cs

            this.Merge.TabStop = true;
-            this.Merge.Text = "&Merge remote branch into current branch";
+            this.Merge.Text = "Mer&ge remote branch into current branch";


Suggested change

this.Merge.Text = "Mer&ge remote branch into current branch";

this.Merge.Text = "&Merge remote branch into current branch";

mstv · 2021-10-10T15:51:05Z

GitUI/CommandsDialogs/FormPull.Designer.cs

-            this.Rebase.Text = "&Rebase current branch on top of remote branch, creates linear history (use with " +
+            this.Rebase.Size = new System.Drawing.Size(504, 21);
+            this.Rebase.TabIndex = 18;
+            this.Rebase.Text = "R&ebase current branch on top of remote branch, creates linear history (use with " +


Suggested change

this.Rebase.Text = "R&ebase current branch on top of remote branch, creates linear history (use with " +

this.Rebase.Text = "&Rebase current branch on top of remote branch, creates linear history (use with " +

Timwi · 2021-10-11T08:18:51Z

Thank you for your feedback. However, I’m not sure I can do much here:

You want to keep existing mnemonics and also keep mnemonics consistent between multiple dialogs. I don’t think this is a realistic goal. I changed “Manage Remotes” to M because it’s M in many other places (most notably, the right-click menu on the Remotes list on the left).
I am trying to be fairly consistent by using R for Remote and E for Rebase in most places. This is a dialog containing both words, and the “Remote” option is BOTH the first one (arguably the most important one), AND one that occurs in other places (most notably, the PUSH dialog which I haven’t gotten to yet, and that one doesn’t have a “Rebase” button so using E for Remote would be silly.) Therefore I think it should be R and Rebase should be E.
Before my changes, this dialog only had few mnemonics. It seems to me that the vast majority of your users were probably using the mouse in these dialogs the whole time anyway. Sure, some people are probably using the mnemonics and a tiny number who’ve gotten used to them may have to relearn them, but when viewed in context with all of my other mnemonics that I added in Rebase and Apply Patch (and others that I’m still planning to add in other places), I think most users will agree that it’s worth it for consistency and ease of use.

For these reasons, I believe that my changes are thought-through, consistent, and desirable. If you still disagree, I’m willing to change them back, but I think simply wanting to keep old mnemonics, even in a dialog that didn’t have many mnemonics to begin with and mnemonics that are inconsistent with other dialogs, would lead to greater inconsistency down the line and an inflexibility of change. Therefore, I humbly ask for you to accept these changes and I’ll keep my future mnemonic changes as consistent with these as I can.

RussKie · 2021-10-13T13:10:06Z

@mstv what do you think?

mstv · 2021-10-13T19:35:17Z

You want to keep existing mnemonics and also keep mnemonics consistent between multiple dialogs. I don’t think this is a realistic goal. I changed “Manage Remotes” to M because it’s M in many other places (most notably, the right-click menu on the Remotes list on the left).

The number of mnemonics it too limited. That's why I prefer a good solution per dialog. But I agree, similar context menus and confirmation popups should have consistent mnemonics.
I repeat / emphasize my argument:
Merge and Rebase are very important options of this dialog. Their mnemonics should not be changed.

IMO, consistency of mnemonics between dialogs does not improve the UX significantly. Though it attaches too many strings.
Just tell Windows to underline the mnemonics all the time, and you will automatically hit the correct keys.

I will not insist in reverting, (especially since I am not affected myself).

…k to G

Timwi · 2021-10-13T21:34:49Z

Would you accept a compromise where I change Merge/Manage Remotes back to M/G but keep Rebase as E? I agree with you that Rebase and Merge are both important options in the dialog, but so is the Remotes box (which I assigned R to). Otherwise, I am happy to also swap Remote/Rebase’s R/E.

mstv

OK, just update the English.xlf yet

Timwi · 2021-10-13T22:19:28Z

My bad! I’ll get used to it in time. Thank you so much.

Timwi · 2021-10-14T13:46:24Z

It seems that the AppVeyor build failure is due to the same issue we had earlier, which wasn’t due to the code in the PR. Please let me know if there’s anything I need to do.

mstv · 2021-10-14T17:31:49Z

It seems that the AppVeyor build failure is due to the same issue we had earlier, which wasn’t due to the code in the PR. Please let me know if there’s anything I need to do.

AV has built. Just waiting for @gerhardol's vote.

gerhardol

+1
I wish the refactoring of the form and the changes was separate to simplify the review.
What existing mnemonics was changed in the end?

Timwi · 2021-10-17T01:41:33Z

Compared to the After screenshot in the original posting, these are the latest changes:

Timwi · 2021-10-17T01:43:26Z

Compared to the Before situation, this mnemonic has changed to accommodate “&Remote” at the top:

RussKie · 2021-10-17T08:51:53Z

@msftbot merge if @gerhardol approves

ghost · 2021-10-17T08:51:56Z

Hello @RussKie!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @gerhardol

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

RussKie · 2021-10-17T08:52:37Z

@msftbot forget everything I just told you

ghost · 2021-10-17T08:52:39Z

Hello @RussKie!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

RussKie · 2021-10-17T08:53:17Z

I forgot that the bot isn't configured to squash merge.

ghost assigned Timwi Oct 6, 2021

Pull dialog: Add mnemonics, fix tab order

adda4cb

Timwi force-pushed the pull-dlg branch from 43b9792 to adda4cb Compare October 6, 2021 07:51

RussKie added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity hacktoberfest-accepted labels Oct 7, 2021

RussKie removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 9, 2021

RussKie requested review from gerhardol and mstv October 9, 2021 00:51

RussKie approved these changes Oct 9, 2021

View reviewed changes

mstv reviewed Oct 10, 2021

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 10, 2021

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 11, 2021

Pull dialog mnemonic changes: “Merge” back to M, “Manage remotes” bac…

aea50a8

…k to G

mstv approved these changes Oct 13, 2021

View reviewed changes

Update English.xlf

d535e90

gerhardol reviewed Oct 16, 2021

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 17, 2021

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 17, 2021

ghost added the status: auto merge label Oct 17, 2021

RussKie removed the status: auto merge label Oct 17, 2021

ghost merged commit fbb4000 into gitextensions:master Oct 17, 2021

ghost added this to the vNext milestone Oct 17, 2021

Timwi deleted the pull-dlg branch October 17, 2021 15:33

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull dialog: add mnemonics, fix tab order #9622

Pull dialog: add mnemonics, fix tab order #9622

Timwi commented Oct 6, 2021

Timwi commented Oct 6, 2021

gerhardol commented Oct 6, 2021 •

edited

RussKie commented Oct 7, 2021

Timwi commented Oct 8, 2021

RussKie commented Oct 9, 2021

mstv left a comment

mstv Oct 10, 2021

mstv Oct 10, 2021

mstv Oct 10, 2021

mstv Oct 10, 2021

Timwi commented Oct 11, 2021 •

edited

RussKie commented Oct 13, 2021

mstv commented Oct 13, 2021

Timwi commented Oct 13, 2021

mstv left a comment

Timwi commented Oct 13, 2021

Timwi commented Oct 14, 2021

mstv commented Oct 14, 2021

gerhardol left a comment

Timwi commented Oct 17, 2021 •

edited

Timwi commented Oct 17, 2021 •

edited

RussKie commented Oct 17, 2021

ghost commented Oct 17, 2021

RussKie commented Oct 17, 2021

ghost commented Oct 17, 2021

RussKie commented Oct 17, 2021

	this.PullFromRemote.Text = "&Remote";
	this.PullFromRemote.Text = "R&emote";

	this.AddRemote.Text = "&Manage remotes";
	this.AddRemote.Text = "Mana&ge remotes";

	this.Merge.Text = "Mer&ge remote branch into current branch";
	this.Merge.Text = "&Merge remote branch into current branch";

	this.Rebase.Text = "R&ebase current branch on top of remote branch, creates linear history (use with " +
	this.Rebase.Text = "&Rebase current branch on top of remote branch, creates linear history (use with " +

Pull dialog: add mnemonics, fix tab order #9622

Pull dialog: add mnemonics, fix tab order #9622

Conversation

Timwi commented Oct 6, 2021

Proposed changes

Timwi commented Oct 6, 2021

gerhardol commented Oct 6, 2021 • edited

RussKie commented Oct 7, 2021

Timwi commented Oct 8, 2021

RussKie commented Oct 9, 2021

mstv left a comment

Choose a reason for hiding this comment

mstv Oct 10, 2021

Choose a reason for hiding this comment

mstv Oct 10, 2021

Choose a reason for hiding this comment

mstv Oct 10, 2021

Choose a reason for hiding this comment

mstv Oct 10, 2021

Choose a reason for hiding this comment

Timwi commented Oct 11, 2021 • edited

RussKie commented Oct 13, 2021

mstv commented Oct 13, 2021

Timwi commented Oct 13, 2021

mstv left a comment

Choose a reason for hiding this comment

Timwi commented Oct 13, 2021

Timwi commented Oct 14, 2021

mstv commented Oct 14, 2021

gerhardol left a comment

Choose a reason for hiding this comment

Timwi commented Oct 17, 2021 • edited

Timwi commented Oct 17, 2021 • edited

RussKie commented Oct 17, 2021

ghost commented Oct 17, 2021

RussKie commented Oct 17, 2021

ghost commented Oct 17, 2021

RussKie commented Oct 17, 2021

gerhardol commented Oct 6, 2021 •

edited

Timwi commented Oct 11, 2021 •

edited

Timwi commented Oct 17, 2021 •

edited

Timwi commented Oct 17, 2021 •

edited