Improve wxToolbar ToolPacking at high DPI (PR #26115)
Maarten
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
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26115
Commit Summary
- fc15ffa Improve wxToolbar ToolPacking at high DPI
File Changes
(1 file)
- M src/msw/toolbar.cpp (25)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26115.patch
- https://github.com/wxWidgets/wxWidgets/pull/26115.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26115)Looks good to me, thanks!
@0tkl can you please test this to confirm that it works for you?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
0tkl
0tkl left a comment (wxWidgets/wxWidgets#26115)This works on Aegisub, but there are some minor issues in the toolbar sample.
image.png (view on web)To Reproduce:
- Build toolbar sample
- Run on 250% scaling
- Capture the left image without any operation
- Click "Toolbar > Show icon" and then click "Toolbar > Show both"
- 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:
3.png (view on web)—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
0tkl
0tkl left a comment (wxWidgets/wxWidgets#26115)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 propose applying the following refactor 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;—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26115)@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!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26115)I see both problems. When the toolbar is recreated with known packing, it should also scale the other axis.
I'll push the fixes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 1 commit.
- b72a48b Improve wxToolbar ToolPacking at high DPI
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
0tkl
0tkl left a comment (wxWidgets/wxWidgets#26115)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?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26115)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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 1 commit.
- 9f0235f Store wxMSW wxToolBar packing in logical pixels
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26115)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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26115)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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26115)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?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
0tkl
0tkl left a comment (wxWidgets/wxWidgets#26115)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% DPI. 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.png (view on web) 2x-3x.png (view on web)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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.