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

Break the toolstrip #8572

Merged
merged 8 commits into from Oct 26, 2020
Merged

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Oct 24, 2020

Relates to #5525

Proposed changes

  • Break the main toolbar into two - general operations and filters. This is arbitrary, filters were the easiest to break out. The main point of the PR was to attempt the split and see how well it works. This then maybe used as a foundation for further refactoring of the toolstrip area, such as moving user scripts to own toolstrip, view-related, repo-related... Possibilities are endless.
  • Save/restore positions of the toolbars

Screenshots

Before

image

After

image

image

image


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

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #8572 into master will increase coverage by 0.04%.
The diff coverage is 87.23%.

@@            Coverage Diff             @@
##           master    #8572      +/-   ##
==========================================
+ Coverage   55.06%   55.10%   +0.04%     
==========================================
  Files         901      902       +1     
  Lines       65156    65220      +64     
  Branches    11755    11753       -2     
==========================================
+ Hits        35875    35939      +64     
+ Misses      26504    26501       -3     
- Partials     2777     2780       +3     
Flag Coverage Δ
#production 42.09% <87.23%> (+0.07%) ⬆️
#tests 94.82% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@gerhardol
Copy link
Member

Relates to #5525

Is this the correct reference?

I like the change, but I rather like to remove/add specific buttons:
#4952 (comment) (or some other issue)

I hardly use the filters, but most buttons in the (new) main toolstrip (except toggle panel and hardly branches (but submodule "up").
However, the filters do not occupy any used space for me, they are just hidden in windowed mode.

The code looks good and works as expected, but I believe that was not what you asked for.

Will continue discussion in #4952

@ghost
Copy link

ghost commented Oct 24, 2020

👍
How can I hide / show the panel?

@RussKie
Copy link
Member Author

RussKie commented Oct 25, 2020

Relates to #5525

Is this the correct reference?

Yes, there was a discussion pertaining to breaking the main toolbar into different ones (e.g. #5525 (comment)).

I like the change, but I rather like to remove/add specific buttons:
#4952 (comment) (or some other issue)

This one is a good one too, 👍

The code looks good and works as expected, but I believe that was not what you asked for.

This is the first step to allow users customise the toolbar - either via a coarser solution of showing/hiding toolbars, or via a fine-grainer one of showing/hiding individual buttons.
This is something has been on my mind, for at least 3 years now judging by commits in https://github.com/RussKie/gitextensions/tree/rework_toolbars

@gerhardol
Copy link
Member

This is the first step to allow users customise the toolbar - either via a coarser solution of showing/hiding toolbars, or via a fine-grainer one of showing/hiding individual buttons.

I am not opposing the change in any way. It also requires a way to hide/show the toolbars to be really useful, as well as splitting up the toolbars further.
However, if someone puts time on this, I rather see that the effort is on the individual buttons rather than the toolbars.

@ghost
Copy link

ghost commented Oct 25, 2020

I see several strategies here.
One panel and customizable buttons.
Multiple panels and the ability to disable a panel.
Both options.

@ghost
Copy link

ghost commented Oct 25, 2020

Plugins could also register their buttons.

@RussKie
Copy link
Member Author

RussKie commented Oct 25, 2020

Since it bothered me, and you asked for it - there you go:
image
image

NB: FormFileHistory is unaffected, though it uses the same FormBrowseMenus API.

@RussKie
Copy link
Member Author

RussKie commented Oct 25, 2020

Btw, the toolbars settings are stored in a local user config located somewhere like this: C:\Users\<user>\AppData\Local\Git_Extensions\GitExtensions.exe_Url_<some crazy string>\33.33.33.0\user.config

@drewnoakes
Copy link
Member

If you arrange the toolbars on the same row, and the leftmost one reduces its width, does the right one stay still or move to the left?

Or in other words, if you don't want to give up any vertical screen height and you want both toolbars, will this make the experience different/worse?

@RussKie
Copy link
Member Author

RussKie commented Oct 25, 2020

Thank you @ivangrek, can you please explain the nature of your changes? [EDIT] What did I miss? Found it - checkboxes get out of sync.

Also your changes introduce a number of perf regressions - LINQ is significantly more expensive and requires more allocations (for example).

@RussKie
Copy link
Member Author

RussKie commented Oct 26, 2020

If you arrange the toolbars on the same row, and the leftmost one reduces its width, does the right one stay still or move to the left?

This would be the default behaviour of Windows Forms. If toolbars are sequentially positioned (i.e. no gaps) they remain together when the left one contracts or expands - this is easy to verify by switching repositories (the repositories toolbar dropdown changes width).

. . .

I'm currently unhappy with the visual tearing when the toolbars are refreshed, something is missing somewhere... #Resolved

@RussKie RussKie marked this pull request as draft October 26, 2020 00:02
@RussKie RussKie marked this pull request as ready for review October 26, 2020 00:29
@RussKie
Copy link
Member Author

RussKie commented Oct 26, 2020

What I see as immediate low hanging fruits from here

  • breaking user scripts into own toolbar, and
  • adding the following items to the 'Filters' toobar:
    • Show all / current branch only / filtered - either as 3 different buttons or a dropdown list (requires POC to see what works best for UI/UX)
    • Show reflogs
    • ❔ Highlight the current branch - whilst not filtering, I find myself using it often, going to the menu feels distracting.

@ghost
Copy link

ghost commented Oct 26, 2020

Also your changes introduce a number of perf regressions - LINQ is significantly more expensive and requires more allocations (for example).

needs to choice

@ghost
Copy link

ghost commented Oct 26, 2020

If you arrange the toolbars on the same row, and the leftmost one reduces its width, does the right one stay still or move to the left?

Would be nice to "float left"

@ghost
Copy link

ghost commented Oct 26, 2020

image

@RussKie
Copy link
Member Author

RussKie commented Oct 26, 2020

image

This is how a user positioned the toolbars, the runtime obeys the user's choice.
We could maybe add "Reset" option to reset back to the default, but this is outside the scope of this change.

@ghost
Copy link

ghost commented Oct 26, 2020

Just VS UX.
We could create new issue when ok.

@RussKie RussKie merged commit fb26d8b into gitextensions:master Oct 26, 2020
@RussKie RussKie deleted the Break_the_toolstrip branch October 26, 2020 05:41
@ghost ghost added this to the 4.0 milestone Oct 26, 2020
@pmiossec
Copy link
Member

@RussKie

Save/restore positions of the toolbars

I see in the description of the PR that the positions of the toolbars are saved and restored. Should it be the case or is it in fact not yet implemented?

Because I always end up with this configuration (that I hate):
image

Only hidding of toolbar is saved for me.
Tested with custom version, version build from HEAD and the portable version from AppVeyor.

Tell me and I will open an issue if that's already implemented...

@gerhardol
Copy link
Member

Only hidding of toolbar is saved for me.

Positions are normally stored for me, but not always if they are viewed or not...
But probably the same cause

Mentioned in #8636

RussKie added a commit to RussKie/gitextensions that referenced this pull request Jan 3, 2021
Due to discovered layout and placement instability of toolbars
reverting most of the functionality added in gitextensions#8572.

Closes gitextensions#8680
RussKie added a commit to RussKie/gitextensions that referenced this pull request Jan 7, 2021
Due to discovered layout and placement instability of toolbars
reverting most of the functionality added in gitextensions#8572.

Closes gitextensions#8680
RussKie added a commit to RussKie/gitextensions that referenced this pull request Jan 18, 2021
Due to discovered layout and placement instability of toolbars
reverting most of the functionality added in gitextensions#8572.

Closes gitextensions#8680
RussKie added a commit to RussKie/gitextensions that referenced this pull request Jan 16, 2022
Reintroduce ability to move toolstrips (as proposed in gitextensions#8572).

This implementation is heavily influenced by the original implementation
ToolStripSettingsManager in Windows Forms, however we only track visibility
and location of each individual toolstrip. We're also only saving specified
toolstrips as opposed to saving all known toolstrips (which in various
situations results in terminal failures to restore toolstrips information).

There are two key changes:

1. Restore toolstrip locations after the form's geometry has been restored.
   This ensures the toolstrips can be placed at the same coords as they
   were saved.

2. Hosting ToolStripPanel's padding must be set to 0 before toolstrips'
   locations can be restored/saved. This mitigates dotnet/winforms#4449

Fixes gitextensions#8680
Reverts gitextensions#8719
RussKie added a commit to RussKie/gitextensions that referenced this pull request Jan 16, 2022
Reintroduce ability to move toolstrips (as proposed in gitextensions#8572).

This implementation is heavily influenced by the original implementation
ToolStripSettingsManager in Windows Forms, however we only track visibility
and location of each individual toolstrip. We're also only saving specified
toolstrips as opposed to saving all known toolstrips (which in various
situations results in terminal failures to restore toolstrips information).

There are two key changes:

1. Restore toolstrip locations after the form's geometry has been restored.
   This ensures the toolstrips can be placed at the same coords as they
   were saved.

2. Hosting ToolStripPanel's padding must be set to 0 before toolstrips'
   locations can be restored/saved. This mitigates dotnet/winforms#4449

Fixes gitextensions#8680
Reverts gitextensions#8719
RussKie added a commit to RussKie/gitextensions that referenced this pull request Jan 16, 2022
Reintroduce ability to move toolstrips (as proposed in gitextensions#8572).

This implementation is heavily influenced by the original implementation
ToolStripSettingsManager in Windows Forms, however we only track visibility
and location of each individual toolstrip. We're also only saving specified
toolstrips as opposed to saving all known toolstrips (which in various
situations results in terminal failures to restore toolstrips information).

There are two key changes:

1. Restore toolstrip locations after the form's geometry has been restored.
   This ensures the toolstrips can be placed at the same coords as they
   were saved.

2. Hosting ToolStripPanel's padding must be set to 0 before toolstrips'
   locations can be restored/saved. This mitigates dotnet/winforms#4449

Fixes gitextensions#8680
Reverts gitextensions#8719
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.

None yet

6 participants