Skip to content

Improve wxToolbar ToolPacking at high DPI#26115

Merged
vadz merged 2 commits into
wxWidgets:masterfrom
MaartenBent:msw-toolbar-packing-dpi
Feb 11, 2026
Merged

Improve wxToolbar ToolPacking at high DPI#26115
vadz merged 2 commits into
wxWidgets:masterfrom
MaartenBent:msw-toolbar-packing-dpi

Conversation

@MaartenBent

Copy link
Copy Markdown
Contributor

Treat m_toolPacking as DPI independent, and scale it to the active DPI. Also scale other hard-coded sizes to the active DPI.

Fixes #26038

@vadz

vadz commented Jan 25, 2026

Copy link
Copy Markdown
Contributor

Looks good to me, thanks!

@0tkl can you please test this to confirm that it works for you?

@0tkl

0tkl commented Jan 26, 2026

Copy link
Copy Markdown
Contributor

This works on Aegisub, but there are some minor issues in the toolbar sample.

image

To Reproduce:

  1. Build toolbar sample
  2. Run on 250% scaling
  3. Capture the left image without any operation
  4. Click "Toolbar > Show icon" and then click "Toolbar > Show both"
  5. Capture the right image

As can be seen, the horizontal padding of items in the horizontal toolbar appear to be fine, but the vertical padding reverts to its value before DPI scaling after redrawing. Take a closer look:

@0tkl

0tkl commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

I didn't have dual monitors yesterday so I didn't test OnDPIChanged. Now I just connected dual-screen and realized there's another issue in OnDPIchanged.

I feel that calling MSWSetPadding here is not quite right, because MSWSetPadding only modifies the padding on the primary axis, while OnDPIChanged should scale both the horizontal and vertical padding.

If no additional m_toolPackingOrtho member is introduced, I suggest applying the following refactoring to OnDPIChanged instead of the current. However, I'm unsure if it will accumulate rounding errors if the DPI changes multiple times:

 void wxToolBar::OnDPIChanged(wxDPIChangedEvent& event)
 {
+    // Adjust the tool packing to the new DPI
+    DWORD curPadding = ::SendMessage(GetHwnd(), TB_GETPADDING, 0, 0);
+    DWORD newPadding = MAKELPARAM( event.ScaleX(LOWORD(curPadding)),
+                                   event.ScaleY(HIWORD(curPadding)) );
+    ::SendMessage(GetHwnd(), TB_SETPADDING, 0, newPadding);

     // Manually scale the size of the controls. Even though the font has been
     // updated, the internal size of the controls does not.
     wxToolBarToolsList::compatibility_iterator node;

@vadz

vadz commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

@MaartenBent I'm leaving this to you as you've started working on it, but please let me know if I should do anything here. TIA!

Treat m_toolPacking as DPI independent, and scale it to the active DPI.
Also scale other hard-coded sizes to the active DPI.

Fixes wxWidgets#26038
@MaartenBent

Copy link
Copy Markdown
Contributor Author

I see both problems. When the toolbar is recreated with known packing, it should also scale the other axis.
I'll push the fixes.

@MaartenBent MaartenBent force-pushed the msw-toolbar-packing-dpi branch from fc15ffa to b72a48b Compare January 27, 2026 19:12
@0tkl

0tkl commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

LGTM.

Final question unrelated to the code itself: should we keep #26038 open, since another aspect of the High-DPI improvement to the toolbar on MSW is to scale up the separator's width? Oh never mind, we can just keep #22150 open, and close #26038 .

@vadz

vadz commented Jan 28, 2026

Copy link
Copy Markdown
Contributor

Sorry, I have second thoughts about this one: we also have SetToolPacking() in wxAuiToolBar and that one still takes logical pixels and not DIPs. These functions should be consistent, so either wxAuiToolBar needs to be changed too or, maybe, we could keep the public function semantics and just apply ToDIP() to store its value as DIP internally? This wouldn't be lossless for fractional scaling, but I don't think we care that much about the precision here. And I couldn't actually find any actual use of SetToolPacking() in wx applications, so maybe it doesn't matter at all...

But, all else being equal, I think it's better to keep taking/returning LPs and not DIPs in/from all public functions.

What do you think?

For the record, here is the diff I was going to apply before pushing this:

diff --git a/docs/changes.txt b/docs/changes.txt
index a50bc6141a..8f68eb4c9f 100644
--- a/docs/changes.txt
+++ b/docs/changes.txt
@@ -136,6 +136,10 @@ Changes in behaviour not resulting in compilation errors
   changed when called with "false" argument, please review the documentation
   and update your code if you called them with "false" (which is rarely done).
 
+- wxToolBar::SetToolPacking() now takes DIPs instead of logical pixels, please
+  remove any FromDIP() calls from arguments to this function.
+
+
 Changes in behaviour which may result in build errors
 -----------------------------------------------------
 
diff --git a/interface/wx/toolbar.h b/interface/wx/toolbar.h
index b180e55985..7497215543 100644
--- a/interface/wx/toolbar.h
+++ b/interface/wx/toolbar.h
@@ -652,6 +652,8 @@ public:
     /**
         Returns the value used for packing tools.
 
+        Note that this value is in DPI-independent pixels.
+
         @see SetToolPacking()
     */
     virtual int GetToolPacking() const;
@@ -962,7 +964,14 @@ public:
     virtual void SetToolNormalBitmap(int id, const wxBitmapBundle& bitmap);
 
     /**
-        Sets the value used for spacing tools. The default value is 1.
+        Sets the value used for spacing tools.
+
+        Note that this value is in DPI-independent pixels, i.e. it will be
+        scaled appropriately by the toolbar according to the current DPI
+        setting if necessary. See @ref high_dpi_pixel_types "explanation of
+        different pixel types" for more information.
+
+        The default value is 1.
 
         @param packing
             The value for packing.

Keep the public API consistent and the same as wxAuiToolBar::SetToolPacking().
@MaartenBent

Copy link
Copy Markdown
Contributor Author

I'm not sure how ToDIP can be used here. I changed it so m_toolPacking is in logical pixels, and will be initialized to the active DPI on creation, and updated on DPI changes.

@MaartenBent

Copy link
Copy Markdown
Contributor Author

The default value is 1

Maybe remove this line, it doesn't make much sense. At least on Windows. For me the default padding at 100%/96DPI is 7px.

@vadz

vadz commented Jan 29, 2026

Copy link
Copy Markdown
Contributor

This looks good to me, thanks a lot!

It's a bit unusual that GetToolPacking() may return a value different from the one last passed to SetToolPacking() but I'd be actually in favour of deprecating these functions as they don't make sense for the portable code anyhow, so I don't really worry about this (but will probably still add a note to the docs).

@0tkl Does the latest version work for you?

@0tkl

0tkl commented Jan 30, 2026

Copy link
Copy Markdown
Contributor

I created two toolbar.exe files, both based on master and applied b72a48b, but one applied 9f0235f additionally while the other did not. I launched both toolbar.exe files at 100%-250% scaling. Both had the same size.

Then I tested DPI changes using the exe file with 9f0235f applied.

Unfortunately, I didn't have a dual-screen setup for testing today, so I split my screen for screenshots: toolbar.exe open on the left, and the Windows settings interface open on the right to adjust scaling. My discussion below assumes this triggers the OnDPIchanged() signal, but please let me know if my assumption is wrong.

Both images have two rows. The first row of both images shows the results of launching the program directly at the corresponding scaling (Scaling in Image 1 is 100%-150%, and in Image 2 it's 175%-250%). The second row of both images shows what happens when the scaling is changed from 250% to 100%-225%.

1x-3x 2x-3x

The padding obtained by changing the DPI is MORE THAN one or two pixels larger than the DPI obtained by directly scaling the padding using the data from the initial 100% scaling. I feel this cannot be fully explained by rounding error. I'm not sure if this was introduced in b72a48b or 9f0235f.


Handling DPI changes during program execution is a niche requirement for me; I don't need to move Aegisub between two screens with different scaling ratios. If you're willing to delve deeper into the bug mentioned above, I appreciate it; but if you think the cause of the bug is difficult to pinpoint, I also encourage you to simply ignore it and merge this PR, because the padding appears to be correct before any DPI changes.

@vadz

vadz commented Feb 9, 2026

Copy link
Copy Markdown
Contributor

@MaartenBent Should I merge this (and leave any potential enhancements for later) or do you plan more changes here? Thanks again!

@MaartenBent

Copy link
Copy Markdown
Contributor Author

I don't know what could cause the pixel(s) difference. The PR already improves padding a lot, so you can merge it like this.
And we can try to enhance it more if/when it ever becomes a problem.

@vadz vadz merged commit 62b4432 into wxWidgets:master Feb 11, 2026
15 of 16 checks passed
@MaartenBent MaartenBent deleted the msw-toolbar-packing-dpi branch March 6, 2026 21:54
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.

Toolbar high DPI on MSW

3 participants