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.

Sign up for GitHub

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

Jump to bottom

FormBrowse: transform Settings button in split button for easy access… #10292

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

FormBrowse: transform Settings button in split button for easy access… #10292

wants to merge 1 commit into from

Conversation

pmiossec
Copy link
Member

… of other settings

Proposed changes

  • The gear open the previously opened setting page or the general page the first time (like before)
  • "Git Extensions settings" open the "General" page
  • "Git settings" open the Git "config" page
  • "Plugins settings" open the "Plugins page"

Screenshots

Before

(just the gear)

After

image

Test methodology

  • Manual

Test environment(s)

  • Git Extensions 33.33.33
  • Build b78e768d55af39e5d79310d8f1b7e772cefcf30a (Dirty)
  • Git 2.38.0.windows.1
  • Microsoft Windows NT 10.0.19042.0
  • .NET 6.0.10
  • DPI 96dpi (no scaling)

Merge strategy

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.

@ghost ghost assigned pmiossec Oct 24, 2022
@gerhardol
Copy link
Member

The button uses more space with this, not sure I want that...
No comment on the implementation.

@RussKie RussKie added this to the vNext milestone Oct 25, 2022
mstv
mstv reviewed Oct 28, 2022
View reviewed changes
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

+0

GitUI/CommandsDialogs/FormBrowse.Designer.cs Outdated
Comment on lines 580 to 541
this.EditSettings.Click += new System.EventHandler(this.OnShowSettingsClick);
this.EditSettings.ButtonClick += new System.EventHandler(this.OnShowSettingsClick);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is the cause why the Ctrl+, hotkey does not work any longer.
s/EditSettings.PerformClick/OpenSettings/

GitUI/CommandsDialogs/FormBrowse.Designer.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormBrowse.Designer.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormBrowse.Designer.cs Outdated Show resolved Hide resolved
@pmiossec pmiossec force-pushed the settings_button_split branch 2 times, most recently from 9d35b44 to 7fa1426 Compare October 28, 2022 21:19
@pmiossec
Copy link
Member Author

done.

@mstv
Copy link
Member

mstv commented Oct 29, 2022

Tests need to be adapted

@pmiossec
Copy link
Member Author

pmiossec commented Oct 29, 2022 •
edited

Tests need to be adapted

This PR is not high priority because it was clear that it won't end up in v4.0 (due to new translation strings).
So I have decided to investigate later because at first thought, I really don't have an idea if the new result is expected or not.

@RussKie RussKie marked this pull request as draft November 16, 2022 06:42
mstv
mstv reviewed Jan 2, 2023
View reviewed changes
GitUI/CommandsDialogs/FormBrowse.Designer.cs Outdated
this.pluginsSettingsToolStripMenuItem.Image = global::GitUI.Properties.Images.Plugin;
this.pluginsSettingsToolStripMenuItem.Name = "pluginsSettingsToolStripMenuItem";
this.pluginsSettingsToolStripMenuItem.Size = new System.Drawing.Size(180, 22);
this.pluginsSettingsToolStripMenuItem.Text = "&Plugins settings";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.pluginsSettingsToolStripMenuItem.Text = "&Plugins settings";
this.pluginsSettingsToolStripMenuItem.Text = "&Plugin settings";

The existing main menu item should be Plugin &settings (not capitalized, too).

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about your suggestion, I think we should keep "Plugins settings" and even update the "Plugins" menu entry to add an 's' because it's to manage the "plugins" and not a specific one. You even end up focus on the "Plugins" item in the treeview:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. It remains this integration test failing that I don't absolutely know what to do about 😢

@pmiossec
Copy link
Member Author

pmiossec commented Jan 5, 2023

Tests need to be adapted

@mstv I had a look in the past and I have absolutely no idea why the test that is failing need to be updated 😢. What change has an impact on this test...
I could change the value in the test to fix it but I don't think it's the good solution and I need to understand before doing the changes...

@mstv
Copy link
Member

mstv commented Jan 8, 2023

Tests need to be adapted

I had a look in the past and I have absolutely no idea why the test that is failing need to be updated 😢. What change has an impact on this test... I could change the value in the test to fix it but I don't think it's the good solution and I need to understand before doing the changes...

I agree, the Splitter tests should not be affected by this PR. I wonder why do they only fail for this PR (on AppVeyor)? Locally, they have not worked for me for longer time. Though with "CommitInfo left of the RevisionGrid", the splitter is really restored incorrectly.

You could rebase and fixup the capitalization of the menu item labels.

@pmiossec pmiossec force-pushed the settings_button_split branch 2 times, most recently from 4c8df88 to 799808c Compare January 20, 2023 09:24
RussKie
RussKie requested changes Jan 21, 2023
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

In #10647 I started encapsulating menu-related functionality trying to reduce complexities of FormBrowse class. This change in its current form is moving in the opposite direction.
Could you please rework it to encapsulate the functionality of the button (e.g., as EditSettingsSplitButton type), so that we add minimal changes to the FormBrowse.

@ghost ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 21, 2023
@ghost ghost added the 💤 status: no recent activity The issue is stale label Feb 20, 2023
@ghost
Copy link

ghost commented Feb 20, 2023

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs.

@ghost ghost removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity 💤 status: no recent activity The issue is stale labels Feb 20, 2023
@RussKie RussKie modified the milestones: 4.1, vNext Apr 10, 2023
@RussKie RussKie removed this from the 4.2 milestone Oct 7, 2023
@RussKie RussKie added this to the vNext milestone Oct 7, 2023
@pmiossec @mstv
FormBrowse: transform Settings button in split button for easy access…
356893f 
… of other settings

Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstv mstv mstv left review comments

@RussKie RussKie RussKie requested changes

Assignees

@pmiossec pmiossec

Labels
None yet
Projects
None yet
Milestone
vNext
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pmiossec @gerhardol @mstv @RussKie