Revert ability for move and show/hide toolbars #8719

RussKie · 2021-01-03T13:22:42Z

Due to discovered layout and placement instability of toolbars
reverting most of the functionality added in #8572.

Closes #8680

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

gerhardol

works for me

codecov · 2021-01-03T19:14:34Z

Codecov Report

Merging #8719 (43cf6b5) into master (bd7c527) will increase coverage by 0.31%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #8719      +/-   ##
==========================================
+ Coverage   55.78%   56.09%   +0.31%     
==========================================
  Files         900      900              
  Lines       65072    65099      +27     
  Branches    11745    11813      +68     
==========================================
+ Hits        36300    36518     +218     
+ Misses      25923    25666     -257     
- Partials     2849     2915      +66

Flag	Coverage Δ
production	`43.22% <88.88%> (+0.42%)`	⬆️
tests	`94.81% <ø> (-0.13%)`	⬇️

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

pmiossec · 2021-01-03T20:33:53Z

In my case, that doesn't solve the main problem I introduced with #8572, that is that the "Branch/filter" toolbar is displayed before the "Main" toolbar:

So, with this PR, this is somewhat worse because this layout is now no more changeable...

@RussKie Do you know where the toolbar positions are saved? I would like to see if the values are well saved...

RussKie · 2021-01-03T22:51:21Z

Do you know where the toolbar positions are saved? I would like to see if the values are well saved...

I don't think this has anything to do with the saved data, as it is no longer used - I've removed all associated code.
This may be an artifact of changes in the designer though

RussKie · 2021-01-07T09:20:39Z

@pmiossec I've run this on two different PCs and haven't observed any issues.
Are you having issues in a dev version launched from VS? Can you try run git clean -xdf and then .\build.cmd -launch and see if you still get issues?
The 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 but I don't believe it has any relevance now, as we no longer persist/load the data from it.

pmiossec · 2021-01-07T10:24:12Z

Are you having issues in a dev version launched from VS? Can you try run git clean -xdf and then .\build.cmd -launch and see if you still get issues?

Created a new worktree (so clean workspace) and I have this behavior (still the same):

The settings are stored in a local user config located somewhere like this: C:\Users<user>\AppData\Local\Git_Extensions\GitExtensions.exe_Url_\33.33.33.0\user.config but I don't believe it has any relevance now, as we no longer persist/load the data from it.

I will still have a look...

pmiossec · 2021-01-07T10:35:43Z

The settings are stored in a local user config located somewhere like this: C:\Users\AppData\Local\Git_Extensions\GitExtensions.exe_Url_\33.33.33.0\user.config but I don't believe it has any relevance now, as we no longer persist/load the data from it.

What didn't work:

removing this user.config file
removing the WindowPositions.xml file

But changing the DPI scale to 100% instead of 200% fix the toolbar positioning.
Another DPI scale problem 😭

RussKie · 2021-01-07T22:37:01Z

So any latest build renders correctly on <200%, and incorrect on 200%?

pmiossec · 2021-01-07T23:18:15Z

So any latest build renders correctly on <200%, and incorrect on 200%?

No, it took me some time to find the reason, so I just had some time to test 100% and 200% (that I use every days) with the code corresponding to this PR.

I can just assert that every build since the introduction of the 2 toolbars (with this revert or not) display badly 200%.

So before merging this revert, we have to test different dpi scaling to better understand.

And if doesn't work, perhaps we will just have to revert the cut in 2 toolbars (until we find a fix)

pmiossec · 2021-01-08T11:35:44Z

Quick test: only 100% display toolbars well. 125%, 150%, 175%, 200% and 225% failed (and I stopped here)

RussKie · 2021-01-09T01:36:00Z

I can repro the issue, thank you. Investigating

RussKie · 2021-01-09T02:21:25Z

Pushed a possible workaround, please try it.

RussKie · 2021-01-09T02:32:09Z

I can't seem to repro the issue in a standalone repro, so quite likely the origin of the bug is same as for #8572 and #8698.

RussKie · 2021-01-09T10:22:28Z

It took me awhile, but I got a clean repro: https://github.com/RussKie/Test-ToolStripOrder

RussKie · 2021-01-12T01:09:48Z

@pmiossec have had a chance to test?

pmiossec · 2021-01-12T10:23:31Z

It took me awhile, but I got a clean repro: https://github.com/RussKie/Test-ToolStripOrder
@pmiossec have had a chance to test?

Yes, I reproduce with this repository

RussKie · 2021-01-12T11:19:31Z

Were you able to test the latest fix?

pmiossec

After a new test, work as expected.

pmiossec · 2021-01-17T00:08:19Z

2 things about the cutting of the toolbars.

I find it strange to have the user scripts, that are added to the "Standard" toolbar, to be displayed before the "Filters" toolbar, like that:

I think we should create a new toolbar and display it after the 2 other toolbars to get something like that (so that we kept the toolbar layout in the current release):

I don't really understand why this PR revert also the hiding of the toolbar that works well (now that we discovered what was the cause of the toolbar bad placement and that you provided a workaround).
As you kept the cutting of the toolbar in 2 (i.e. that you didn't revert the whole development), perhaps we could at least kept one feature allowed by this cutting, no!?!

Please have a look at my branch that did this 2 things:

https://github.com/pmiossec/gitextensions/tree/new_toolbar_for_scripts

RussKie · 2021-01-17T11:49:02Z

Please have a look at my branch that did this 2 things:

https://github.com/pmiossec/gitextensions/tree/new_toolbar_for_scripts

Thank you, works for me :)
Though the new toolbar must look the same as the other two:

gerhardol

+1

gerhardol · 2021-01-17T19:00:59Z

GitUI/CommandsDialogs/FormBrowse.cs

+                // if the 1st one becomes longer than the 2nd toolbar's Location.X
+                // the layout engine will be place the 2nd toolbar first
+                toolPanel.TopToolStripPanel.Controls.Clear();
+                toolPanel.TopToolStripPanel.Controls.Add(ToolStripScripts);


If the order is left-to-right display order, should the order consistently be Filters, Sscripts, Main?

and I would prefer to remove/add button by button instead, splitting the toolbar is not adding much...

splitting the toolbar is not adding much

It is adding if a) toolbar can be moved, and b) toolbars can be shown/hidden. "a" obviously doesn't work at this stage, but "b" is working just fine.

should the order consistently be Filters, Sscripts, Main?

The order is: Main, Filters, Scripts.

RussKie · 2021-01-18T11:22:28Z

I don't really understand why this PR revert also the hiding of the toolbar that works well (now that we discovered what was the cause of the toolbar bad placement and that you provided a workaround).
As you kept the cutting of the toolbar in 2 (i.e. that you didn't revert the whole development), perhaps we could at least kept one feature allowed by this cutting, no!?!

We also need to restore persistence of shown/hidden state as well.

Due to discovered layout and placement instability of toolbars reverting most of the functionality added in gitextensions#8572. Closes gitextensions#8680

RussKie · 2021-01-18T12:56:56Z

Merging, we can deal with saving/restoring visibility of the toolbars separately.

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 requested a review from pmiossec January 3, 2021 13:22

ghost assigned RussKie Jan 3, 2021

gerhardol approved these changes Jan 3, 2021

View reviewed changes

RussKie force-pushed the disable_toolbar_movement branch from bb77960 to de4ec45 Compare January 7, 2021 09:22

RussKie marked this pull request as draft January 9, 2021 01:35

This comment has been minimized.

Sign in to view

RussKie marked this pull request as ready for review January 16, 2021 12:16

pmiossec approved these changes Jan 16, 2021

View reviewed changes

gerhardol reviewed Jan 17, 2021

View reviewed changes

Revert ability for move and show/hide toolbars

f5b4e7c

Due to discovered layout and placement instability of toolbars reverting most of the functionality added in gitextensions#8572. Closes gitextensions#8680

Put user scripts in their own toolbar

43cf6b5

RussKie force-pushed the disable_toolbar_movement branch from e10a3d8 to 43cf6b5 Compare January 18, 2021 11:32

RussKie merged commit 9685011 into gitextensions:master Jan 18, 2021

RussKie deleted the disable_toolbar_movement branch January 18, 2021 12:57

RussKie added this to the 3.5 milestone Jan 26, 2021

RussKie mentioned this pull request Jan 30, 2021

Fix toolbar order (revisited) #8807

Merged

RussKie mentioned this pull request Jan 16, 2022

Allow move toolstrips (take 2) #9820

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert ability for move and show/hide toolbars #8719

Revert ability for move and show/hide toolbars #8719

RussKie commented Jan 3, 2021

gerhardol left a comment

codecov bot commented Jan 3, 2021 •

edited

pmiossec commented Jan 3, 2021

RussKie commented Jan 3, 2021

RussKie commented Jan 7, 2021 •

edited

pmiossec commented Jan 7, 2021

pmiossec commented Jan 7, 2021

RussKie commented Jan 7, 2021

pmiossec commented Jan 7, 2021

pmiossec commented Jan 8, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 12, 2021

pmiossec commented Jan 12, 2021

RussKie commented Jan 12, 2021 via email •

edited by ghost

This comment has been minimized.

This comment has been minimized.

pmiossec left a comment

pmiossec commented Jan 17, 2021

RussKie commented Jan 17, 2021

gerhardol left a comment

gerhardol Jan 17, 2021

RussKie Jan 18, 2021

RussKie Jan 18, 2021

RussKie commented Jan 18, 2021

RussKie commented Jan 18, 2021

Revert ability for move and show/hide toolbars #8719

Revert ability for move and show/hide toolbars #8719

Conversation

RussKie commented Jan 3, 2021

gerhardol left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2021 • edited

Codecov Report

pmiossec commented Jan 3, 2021

RussKie commented Jan 3, 2021

RussKie commented Jan 7, 2021 • edited

pmiossec commented Jan 7, 2021

pmiossec commented Jan 7, 2021

RussKie commented Jan 7, 2021

pmiossec commented Jan 7, 2021

pmiossec commented Jan 8, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 9, 2021

RussKie commented Jan 12, 2021

pmiossec commented Jan 12, 2021

RussKie commented Jan 12, 2021 via email • edited by ghost

This comment has been minimized.

This comment has been minimized.

pmiossec left a comment

Choose a reason for hiding this comment

pmiossec commented Jan 17, 2021

RussKie commented Jan 17, 2021

gerhardol left a comment

Choose a reason for hiding this comment

gerhardol Jan 17, 2021

Choose a reason for hiding this comment

RussKie Jan 18, 2021

Choose a reason for hiding this comment

RussKie Jan 18, 2021

Choose a reason for hiding this comment

RussKie commented Jan 18, 2021

RussKie commented Jan 18, 2021

codecov bot commented Jan 3, 2021 •

edited

RussKie commented Jan 7, 2021 •

edited

RussKie commented Jan 12, 2021 via email •

edited by ghost