Improve wxToolbar ToolPacking at high DPI#26115
Conversation
|
Looks good to me, thanks! @0tkl can you please test this to confirm that it works for you? |
|
I didn't have dual monitors yesterday so I didn't test I feel that calling If no additional 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; |
|
@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
|
I see both problems. When the toolbar is recreated with known packing, it should also scale the other axis. |
fc15ffa to
b72a48b
Compare
|
Sorry, I have second thoughts about this one: we also have 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().
|
I'm not sure how |
Maybe remove this line, it doesn't make much sense. At least on Windows. For me the default padding at 100%/96DPI is 7px. |
|
This looks good to me, thanks a lot! It's a bit unusual that @0tkl Does the latest version work for you? |
|
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 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%.
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. |
|
@MaartenBent Should I merge this (and leave any potential enhancements for later) or do you plan more changes here? Thanks again! |
|
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. |




Treat
m_toolPackingas DPI independent, and scale it to the active DPI. Also scale other hard-coded sizes to the active DPI.Fixes #26038