Adjust ToolStripComboBox size based on DPI scaling #11865

mdonatas · 2024-08-31T19:32:40Z

Proposed changes

It seems that ToolStripComboBox is immune to AutoScaling things.. thus it's necessary to perform the scaling manually
- Manually scale all instances of ToolStripComboBox
Fix gpg drop down width

Screenshots

Before / After

Test methodology

Manually at 1x and 2x scaling

Test environment(s)

Windows 11 4K 200% 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.

mstv

looks sensible

src/app/GitUI/CommandsDialogs/FormCommit.cs

Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>

mdonatas · 2024-09-01T11:15:36Z

looks sensible

I didn't include 100% scaling screenshots this time, but if by looks you mean visually then it now looks the same in 100% and 200% scaling..

I've been suffering these tiny search fields for years until I snapped 😅 I haven't had a 100% scaled screen in my life for over 5 years now

RussKie

We have AdjustForDpiScaling extensions, is there a reason we can't enhance it to deal with all ToolStripComboBox?

gitextensions/src/app/GitExtUtils/GitUI/ControlDpiExtensions.cs

Lines 6 to 8 in 7acf63a

    
           public static class ControlDpiExtensions 
        
           { 
        
               public static void AdjustForDpiScaling(this Control control)

RussKie · 2024-09-02T03:00:10Z

src/app/GitUI/UserControls/FilterToolBar.cs

+            tscboBranchFilter.Size = new Size(DpiUtil.Scale(tscboBranchFilter.Size.Width), tscboBranchFilter.Size.Height);
+            tstxtRevisionFilter.Size = new Size(DpiUtil.Scale(tstxtRevisionFilter.Size.Width), tstxtRevisionFilter.Size.Height);


Any reason we can't do the following?

Suggested change

tscboBranchFilter.Size = new Size(DpiUtil.Scale(tscboBranchFilter.Size.Width), tscboBranchFilter.Size.Height);

tstxtRevisionFilter.Size = new Size(DpiUtil.Scale(tstxtRevisionFilter.Size.Width), tstxtRevisionFilter.Size.Height);

tscboBranchFilter.Size = DpiUtil.Scale(tscboBranchFilter.Size);

tstxtRevisionFilter.Size = DpiUtil.Scale(tstxtRevisionFilter.Size);

I had it the way you showed at first, but when comparing with unscaled version I noticed that the toolbar was getting higher for no good reason and thus switched to only scaling the width.

When it gets higher it's with DpiUtil.Scale(tscboBranchFilter.Size) type of scaling

combobox-dpi.mp4

Please add a comment explaining this, otherwise it's a hidden knowledge. Thanks

mdonatas · 2024-09-05T20:21:23Z

@RussKie oh wow, turns out ToolStripComboBox does not inherit from Control

Trying to add it to AdjustForDpiScaling

This would require to create a similar "structure" to iterate through ToolStripItem descendands e.g. toolStrip.FindDescendantItems() but not today. Unless you think it's not necessary, then I'd gladly accept such outcome too :)

RussKie · 2024-09-06T01:45:06Z

@RussKie oh wow, turns out ToolStripComboBox does not inherit from Control

Trying to add it to AdjustForDpiScaling

This would require to create a similar "structure" to iterate through ToolStripItem descendands e.g. toolStrip.FindDescendantItems() but not today. Unless you think it's not necessary, then I'd gladly accept such outcome too :)

Yes, we'll need to create a new overload.

mdonatas · 2024-09-08T07:32:22Z

Reimplemented the fix as part of ControlDpiExtensions.AdjustForDpiScaling

Regarding comment explaining why scaling is done only horizontally, here's how it would look with Size scaling

RussKie

Good stuff! 🚀

RussKie · 2024-09-08T13:37:35Z

src/app/GitExtUtils/GitUI/ControlDpiExtensions.cs

+                            {
+                                case ToolStripComboBox toolStripComboBox:
+                                {
+                                    if (toolStripComboBox.Tag as string != "__DPI_SCALED__")


This could lead to issues if Tag is set to something...
I suggest to add DebugHelpers.Fail check to alert developers in event the tag is set explicitly.

Good catch... 🤦 don't really know yet how to solve it. Suggestions?

yes or #11865 (comment) with your message

@mstv I don't see how this solves the problem. There are two separate use cases that want to use a Tag property and neither would work if the other one took over. After some googling I've seen people use a Dictionary<string, object> and some wrapper methods to add support for multiple tags on a control.

This could lead to issues if Tag is set to something...

...

I don't see how this solves the problem. There are two separate use cases that want to use a Tag property and neither would work if the other one took over. After some googling I've seen people use a Dictionary<string, object> and some wrapper methods to add support for multiple tags on a control.

I was not aware that there are two usages of Tag - at least with your addition.

Then rather do not use Tag at all, which always is a good advice because it is not obvious for what Tag is used.

@mdonatas controls should be scaled in the .ctor, which means the scaling should happen once. Am I missing something?

You are probably not missing anything... I've just seen other cases where this tag was used and assumed that was necessary and then fell into the rabbit hole :)
Removed the check. Scaling still works as expected.

mstv · 2024-09-08T14:41:30Z

src/app/GitExtUtils/GitUI/ControlDpiExtensions.cs

+                                    if (toolStripComboBox.Tag as string != "__DPI_SCALED__")
+                                    {
+                                        toolStripComboBox.Tag = "__DPI_SCALED__";


Suggested change

if (toolStripComboBox.Tag as string != "__DPI_SCALED__")

{

toolStripComboBox.Tag = "__DPI_SCALED__";

const string alreadyScaled = "__DPI_SCALED__";

if (toolStripComboBox.Tag as string != alreadyScaled)

{

Debug.Assert(toolStripComboBox.Tag is null);

toolStripComboBox.Tag = alreadyScaled;

src/app/GitExtUtils/GitUI/ToolStripUtil.cs

src/app/GitUI/CommandsDialogs/FormCommit.cs

src/app/GitExtUtils/GitUI/ControlDpiExtensions.cs

mstv

have not run

Adjust ToolStripComboBox size based on DPI scaling

8ac5a91

mstv reviewed Sep 1, 2024

View reviewed changes

src/app/GitUI/CommandsDialogs/FormCommit.cs Outdated Show resolved Hide resolved

Update src/app/GitUI/CommandsDialogs/FormCommit.cs

60b4e41

Co-authored-by: Michael Seibt <36601201+mstv@users.noreply.github.com>

RussKie reviewed Sep 2, 2024

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 Sep 2, 2024

Adjust ToolStrips as part of ControlDpiExtensions.AdjustForDpiScaling

e6b6e2c

mdonatas requested review from mstv and RussKie September 8, 2024 07:31

RussKie approved these changes Sep 8, 2024

View reviewed changes

mstv reviewed Sep 8, 2024

View reviewed changes

mdonatas added 2 commits September 15, 2024 19:32

Stop tracking "already scaled" in the tag

f360aba

Don't scale values as they'll be scaled as part of another PR

117898b

mstv approved these changes Sep 15, 2024

View reviewed changes

RussKie approved these changes Sep 16, 2024

View reviewed changes

RussKie merged commit 8eb5eb1 into gitextensions:master Sep 16, 2024
4 checks passed

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

RussKie added this to the 5.0.1 milestone Sep 16, 2024

	public static class ControlDpiExtensions
	{
	public static void AdjustForDpiScaling(this Control control)

		tscboBranchFilter.Size = new Size(DpiUtil.Scale(tscboBranchFilter.Size.Width), tscboBranchFilter.Size.Height);
		tstxtRevisionFilter.Size = new Size(DpiUtil.Scale(tstxtRevisionFilter.Size.Width), tstxtRevisionFilter.Size.Height);

-                                    if (toolStripComboBox.Tag as string != "__DPI_SCALED__")
-                                    {
-                                        toolStripComboBox.Tag = "__DPI_SCALED__";
+                                    const string alreadyScaled = "__DPI_SCALED__";
+                                    if (toolStripComboBox.Tag as string != alreadyScaled)
+                                    {
+                                        Debug.Assert(toolStripComboBox.Tag is null);
+                                        toolStripComboBox.Tag = alreadyScaled;

Uh oh!

Adjust ToolStripComboBox size based on DPI scaling #11865

Adjust ToolStripComboBox size based on DPI scaling #11865

Uh oh!

Conversation

mdonatas commented Aug 31, 2024

Proposed changes

Screenshots

Before / After

Test methodology

Test environment(s)

Merge strategy

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdonatas commented Sep 1, 2024

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdonatas commented Sep 5, 2024

Uh oh!

RussKie commented Sep 6, 2024

Uh oh!

mdonatas commented Sep 8, 2024

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!