Opened 7 months ago

Last modified 4 weeks ago

#18649 confirmed defect

Sizer borders and spaces not (re)scaled on DPI change

Reported by: vadz Owned by:
Priority: normal Milestone: 3.2.0
Component: GUI-all Version: 3.1.3
Keywords: HiDPI sizer Cc:
Blocked By: Blocking: #18474
Patch: yes

Description

Quoting Dion Whittaker's wx-users post from 2020-01-14:

I have encountered an issue using wxWidgets 3.1.3 when moving windows
from a high resolution display (144 dpi) to a standard display (96 dpi)
and vice-versa when using Per-Monitor (v2) DPI awareness.

The code is compiled using Mingw-w64.

You can see the issue in the widgets sample, specifically the button
page, when it is compiled with the appropriate manifest. When you move
the window to the low resolution monitor, the Reset button gets pushed
off the bottom of the window.

After some investigation it looks like border size of sizers, and the
size of spacers are not getting scaled for the DPI change when the
window is moved from one display to another. If you set the size of the
sizer borders and spacers to 0, the windows look pretty similar on both
displays.

Similarly if you add a wxEVT_DPI_CHANGED handler to the window, and
manually scale all the sizer border sizes and spacer sizes the window
looks pretty similar on both displays.

Dion also made a test case for the problem:

#include "wx/wx.h"
class MyApp: public wxApp
{
public:
     bool OnInit() wxOVERRIDE;
};

class MyFrame: public wxFrame
{
public:
     MyFrame(const wxString& title);
};

wxIMPLEMENT_APP(MyApp);

bool MyApp::OnInit()
{
     if ( !wxApp::OnInit() )
         return false;

     // Create the main frame window
     MyFrame *frame = new MyFrame("Sizer Border Test");

     frame->Show(true);

     // report success
     return true;
}

MyFrame::MyFrame(const wxString& title)
        : wxFrame(NULL, wxID_ANY, title, wxDefaultPosition,
        wxDefaultSize, wxCAPTION|wxCLOSE_BOX|wxSYSTEM_MENU)
{
     const int borderSize = 5;
     const int spacerSize = 10;

     wxSizer* mainSizer = new wxBoxSizer(wxVERTICAL);

     wxSizerFlags sizerFlags(0);
     sizerFlags.Border(wxALL, FromDIP(borderSize));


     mainSizer->Add(new wxStaticText(this, wxID_ANY, "This is text line 1. It should be completely visible."), sizerFlags);
     mainSizer->AddSpacer(FromDIP(spacerSize));
     mainSizer->Add(new wxStaticText(this, wxID_ANY, "This is text line 2. It should be completely visible."), sizerFlags);
     mainSizer->AddSpacer(FromDIP(spacerSize));
     mainSizer->Add(new wxStaticText(this, wxID_ANY, "This is text line 3. It should be completely visible."), sizerFlags);
     mainSizer->AddSpacer(FromDIP(spacerSize));
     mainSizer->Add(new wxStaticText(this, wxID_ANY, "This is text line 4. It should be completely visible."), sizerFlags);

     SetSizerAndFit(mainSizer);
}

which shows the problem:

When you move the window from the high res display to the low res
display the bottom line of text is not visible on the window, neither is
the last word of the message. With the borders and spacers set to zero
all the lines of text are visible, however the right most word is still
missing - that might be another issue.

If the app starts on the low res display and is moved to the high res
display extra space is displayed at the bottom of the window.

Attachments (7)

dw_dpifix.patch download (2.9 KB) - added by dwhittaker 6 months ago.
Recursively update child sizers when DPI changes in MSW
dpichangetest.cpp download (3.8 KB) - added by dwhittaker 6 months ago.
Quick program for testing DPI change handling. Child sizer issue shows when moving from 144dpi to 96dpi. Also displays possible text scaling issue.
lowtohigh.png download (19.2 KB) - added by MaartenB 6 months ago.
hightolow.png download (12.0 KB) - added by MaartenB 6 months ago.
wm_getdpiscaledsize.patch download (16.7 KB) - added by dwhittaker 6 months ago.
Window resizing using WM_GETDPISCALEDSIZE
dpipatch3.diff download (6.0 KB) - added by dwhittaker 4 weeks ago.
Window resizing using best client size
dpipatch4.diff download (6.8 KB) - added by dwhittaker 4 weeks ago.
Updated DPI patch using best client size

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 months ago by MaartenB

  • Status changed from new to confirmed

Sizer and spacer DPI scaling was added in
918e102533f2a5e437beabec41da851b2d0f3cce (after 3.1.3), see #18551.

The described problem is gone, but at some resolutions the bottom text is now partially hidden because the new window height (given by Windows via WM_DPICHANGED) is too small.
This happens when moving from lower to higher DPI, and only for some DPI. For example, it occurs when moving the window from a monitor at 100% to a monitor at 150%, but not when moving from 100% to 200%.

I suspect it is related to ScaleCoordIfSet() in msw/window.cpp. If I always use floor() the problem is gone. I'll investigate this further.

comment:2 Changed 7 months ago by vadz

Oops, I've completely forgotten that this was already done, thanks for correcting me, Maarten! And thanks for looking at this, of course.

Changed 6 months ago by dwhittaker

Recursively update child sizers when DPI changes in MSW

comment:3 Changed 6 months ago by dwhittaker

  • Cc dwhittaker@… added

After some more testing it seems like the head DPI adjustment code was only scaling the items in the main sizer of a window, any child sizer items were not being updated. I attached a patch above that should scale all the sizers and spacers in a window.

This improves the layout, however there is still an issue with text disappearing off the bottom, or to the right if the Window when the DPI changes. I am pretty sure at least part of this is due to non-linear scaling of text when the DPI changes.

For example I have a wxStaticText control using 9pt Segoe UI where GetBestSize() returns a width of 311 and a height of 16 at 96dpi. The same wxStaticText returns a GetBestSize() width of 466 and a height of 26 at 144dpi.

I think the extra couple of pixels in the height are causing items to display off the bottom of the window, and the more wxStaticText controls you have stacked on the Window, the further off the bottom of the window the components appear.

Changed 6 months ago by dwhittaker

Quick program for testing DPI change handling. Child sizer issue shows when moving from 144dpi to 96dpi. Also displays possible text scaling issue.

comment:4 Changed 6 months ago by vadz

  • Patch set

I think the idea was that the other sizers will be updated when their windows get WM_DPICHANGED, but this doesn't take into account the sizers that are children of other sizers and not directly associated with a window, so this patch looks correct to me, thanks! Maarten, do you have any objections to applying it?

As for the remaining issue, it looks like we need to recalculate the best size of the TLW when its DPI changes and ensure that it's at least as big, right?

comment:5 Changed 6 months ago by MaartenB

The patch Looks good.
But no need for if ( GetSizer() ) if the new function also has if ( !sizer ).


As for the remaining issue, it looks like we need to recalculate the best size of the TLW when its DPI changes and ensure that it's at least as big, right?

Currently we rely on the size that Windows provides, which is indeed linearly scaled. It is incorrect for both width and height in the test program (with no spacers and borders).

96->144

144->96

Recalculating the best size, and increasing the window size if it does not fit, should be possible, but could have side effects.
First, the center of the bigger window should still be on the same monitor so it does not trigger another dpi change.
Second, if the size has been increased and the window is moved to the original monitor, the window size will be bigger than it originally was. Or we have to keep track of all the adjusted sizes.

Another option could be implementing WM_GETDPISCALEDSIZE. But then we somehow need to determine the scaled size of the window at a specific dpi, and use the text extend of the font at that dpi.

Changed 6 months ago by MaartenB

Changed 6 months ago by MaartenB

comment:6 Changed 6 months ago by vadz

Would it be a problem to actually change the DPI of the window to the one provided by WM_GETDPISCALEDSIZE? I.e. is it possible that the DPI change will be cancelled/rolled back or can we rely on getting WM_DPICHANGED after this one?

comment:7 Changed 6 months ago by MaartenB

I tried changing the position inside WM_GETDPISCALEDSIZE and if I move the window fast enough from one monitor to another, I do not always get a WM_DPICHANGED.
Changing the DPI of the window inside WM_GETDPISCALEDSIZE also changes the size / center position of the window, so something similar could (probably?) happen.

case WM_GETDPISCALEDSIZE:
    SetPosition(wxPoint(100, 100));
    processed = true;
    break;

comment:8 Changed 6 months ago by dwhittaker

  • Cc dwhittaker@… removed

I also tested WM_GETDPISCALEDSIZE yesterday by overriding the value returned by GetDPI() and recalculating the size of the Window, mainly just to test the feasibility. It seemed to work pretty well and the test window looked correct, however if you can't count on getting WM_DPICHANGED, this might not be the way to go.

I also found you had to adjust for the Window borders and captions manually in the WM_GETDPISCALEDSIZE handler. The borders sizes seem to be the same at 96 and 144dpi (3 pixels), however the caption area is scaled (23 and 34 pixels), so you can't just simply scale the difference between the content size and the window size.

I was thinking of trying to implement a GetBestSizeForDPI() function or similar today for use in the WM_GETDPISCALED handler.

Changed 6 months ago by dwhittaker

Window resizing using WM_GETDPISCALEDSIZE

comment:9 Changed 6 months ago by dwhittaker

  • Cc dwhittaker@… added

I have attached a new patch wm_getdpiscaledsize.patch that implements the WM_GETDPISCALEDSIZE message to determine the size of the window at the new DPI. It displays the test program and several other dialogs I have tested correctly.

It probably isn't viable as a final patch but I wanted to add it in case someone who is more familiar with the code base would find it useful. Alternately if you think this might be a path worth more investigation and can provide improvement suggestions I can keep working on it.

The new window size is calculated by setting a flag in GetDPI() to return the new DPI value instead of the current one. The window size is then recalculated using the new DPI.

A couple of new functions were added. wxAdjustWindowRectExForDpi() and MSWAdjustWindowRectForDPI() to calculate the size of the window at the new DPI given the client rectangle size.

GetMinClientSize() was added to calculate the minimum client size at the new DPI. Any user size adjustments to the window are determined by comparing the current client size with the minimum client size. GetBestSize() couldn't be used as it assumes the difference between the client size and the window size is constant. GetClientSize() also didn't seem to return the correct value.

It also looks like simply scaling the window borders may result in minor errors. I found the border sizes on the window with resizeable borders at 96 dpi were 16 wide and 39 high, while at 144 dpi they were 22 wide and 56 high. GetSystemMetricsForDpi() seems to support this as mentioned in the code.

I haven't encountered a situation where a WM_DPICHANGED message wasn't received after a WM_GETSCALEDSIZE. Not sure of the preferred method of accounting for this possibility - could always restore the original values in WM_GETSCALEDSIZE, then update again in WM_DPICHANGED.

I did notice the is a little extra space at the bottom of the window when moving from 144 to 96dpi for the first time. However nothing is truncated and the window size is maintained after the first change. Going from 96 to 144 looks fine. I suspect this is because I am using AdjustWindowRectExForDpi to calculate the window size.

Hope someone finds this useful.

Last edited 6 months ago by dwhittaker (previous) (diff)

comment:10 Changed 4 months ago by Vadim Zeitlin <vadim@…>

In 4072c0634/git-wxWidgets:

Scale all sizers and spacers on a DPI change

Do not only scale the first sizer of a window, but scale all its child
sizers and spacers recursively.

Closes https://github.com/wxWidgets/wxWidgets/pull/1804

See #18649.

comment:11 Changed 4 months ago by dwhittaker

  • Cc dwhittaker@… removed

I've had an opportunity to look at this again the last couple of days.
I found that during the WM_GETDPISCALED message processing the windows device context associated with the window calculates text extents according to the new DPI, however the Window still reports it's DPI as the old value. So this makes it possible to calculate the new best Window size using the new text sizes, however it would make it difficult I think to revert the Window size during this function - so simply overriding the DPI value reported by the Window does work. I can tidy up the WM_GETDPISCALED patch above to implement a fix using this method.

I also have a patch that simply calls SetSize again in the current WM_DPICHANGED handler using the new Window best size, keeping the Window centre in the same location. This patch only adds a few lines of code, I think I could get it down to a single SetSize call by adding a couple of extra functions.

Another possible solution would be to add something like a GetBestSizeAtDPI function to the Window interface, and something similar to the Sizer interface. However it would only be valid to call it during the WM_GETDPISCALED handler when the window device context is set to the new DPI. The default behaviour of the function could be to return the current size scaled for the new DPI, and it could be overridden in Windows for any controls that rely on text size to calculate their best size.

Something I noted for all the solutions, if the Window minimum size has been set to the best size, you can't just scale the minimum size, you need to change it to the new best size.

The patches use the current size of the Window in comparison to the best size to keep track of user Window sizing. It seems to work well.

I don't mind spending some time working on any of these changes, or any other if someone has a better idea. What would be your preferred solution?

comment:12 Changed 4 weeks ago by vadz

Sorry for the delay with replying, I've postponed doing it initially because it wasn't obvious what was the right thing to do here to me, so I decided to take more time to think about it. Unfortunately returning to it now, I still don't know what should be done.

One thing I'm more or less sure about is that we do not want to add GetBestSizeAtDPI(): it makes sense from implementation point of view, but it's going to be too bothersome to implement it for wx users and how would they do it, anyhow? We really need to find some of making the normal GetBestSize() work using the new proposed DPI.

I'm less sure about the idea of calling SetSize(): the danger is that it will result in some visible flicker, but OTOH DPI changes don't happen that often, so maybe it won't be noticeable. The main problem with this is that we don't know if the window size is the same as its best size -- it can easily be greater.

Anyhow, if the patch above works, i.e. fixes the issues shown in the comment:5, then we probably should apply it. Maarten, what do you think?

comment:13 Changed 4 weeks ago by MaartenB

It works for the provided example code, but in the samples and controls there are issues.

In the widgets sample the fonts are not correctly updated. I could fix this by re-adding MSWUpdateFontOnDPIChange() to MSWUpdateOnDPIChange().

The dialogs sample is worse. Just moving the main window between displays breaks the entire window. I have not looked into the cause of this.

Last edited 4 weeks ago by MaartenB (previous) (diff)

comment:14 Changed 4 weeks ago by vadz

  • Milestone changed from 3.1.4 to 3.2.0

Do you really mean the "dialogs" sample? There is nothing in it... And the only problem I see there is that wxInfoBar icons are not scaled with DPI (see #18822).

Anyhow, let's postpone this to 3.2.0 as it doesn't seem realistic to expect to fix this during the next couple of days.

Changed 4 weeks ago by dwhittaker

Window resizing using best client size

comment:15 Changed 4 weeks ago by dwhittaker

  • Cc dwhittaker@… added

Hi. In the end I went with the much simpler and more self contained patch (dpipatch3.diff) that uses the SetSize() option. I have been using it in my application for the last couple of weeks, and it seems to work pretty well.

It determines the best client size before the DPI change, along with the actual client size. Then after the DPI change it once again determines the best client size and scales the new client size so that proportionally it is the same size as previously.

It does resize the window in the dpi changed handler, which is not ideal but attempts to keep the center of the window at the same position. There is a bit of flicker, but I don't think it's too bad.

I've checked it against the layout, dialogs and widgets samples as well as the test code, and it doesn't seem to break anything, but does fix the test code. The widgets sample has issues resizing the book control, however the issues are there whether or not this patch is applied.

Sorry I didn't get this uploaded earlier, wanted to check it thoroughly in my app before uploading.

comment:16 follow-up: Changed 4 weeks ago by MaartenB

This seems to work quite good in my first tests, and it's a lot simpler than the other patch.

I do notice problems with the minimal and dialogs samples, they do not scale correctly. For the dialog sample, moving it from 150% to 100%, the original size is 400x250, the suggested newRect size is 167x267, and the newly calculated size is 394x213. For the minimal sample the original and new sizes are 400x250 and 16x82.

I think this happens because they do not contain any controls. As a result, DoGetBestSize(); returns almost the same size as the calculated border size, and then orgBestClientSize and newBestClientSize are wxSize(1,1) (dialogs sample) or wxSize(0,0) (minimal sample). I could fix it with the following change (same for newSize):

wxSize orgClientSize = GetClientSize();
...
if ( orgBestClientSize.x <=1 || orgBestClientSize.y <=1 )
    orgBestClientSize = orgClientSize;

Also, the area around the client area is called nonclient area, maybe you can use this instead of border, to prevent any confusion.

I see you only use ceil in ScaleSizeToKeepProportion(). In ScaleCoordIfSet() (msw/window.cpp) we also needed to use floor for the scaling to be commutative. E.g. when switching between DPI 96 (100%) and 120 (125%): ceil for 29*1.25=36.25->37px, and floor for 37*0.8=29.6->29.

What book-control problem do you mean? The size of the tab icons?

Changed 4 weeks ago by dwhittaker

Updated DPI patch using best client size

comment:17 in reply to: ↑ 16 Changed 4 weeks ago by dwhittaker

  • Cc dwhittaker@… removed

Replying to MaartenB:

I do notice problems with the minimal and dialogs samples, they do not scale correctly. For the dialog sample, moving it from 150% to 100%, the original size is 400x250, the suggested newRect size is 167x267, and the newly calculated size is 394x213. For the minimal sample the original and new sizes are 400x250 and 16x82.

Yep my logic for non-usable old or new client areas was broken. I adjusted ScaleSizeToKeepProportion() to account for non-usable (<=1) best reference areas in either or both directions.

Also, the area around the client area is called nonclient area, maybe you can use this instead of border, to prevent any confusion.

Done

I see you only use ceil in ScaleSizeToKeepProportion(). In ScaleCoordIfSet() (msw/window.cpp) we also needed to use floor for the scaling to be commutative. E.g. when switching between DPI 96 (100%) and 120 (125%): ceil for 29*1.25=36.25->37px, and floor for 37*0.8=29.6->29.

Changed to use floor for scaling < 1.0
The adjustment to the top left window position was also backwards so I fixed that.

What book-control problem do you mean? The size of the tab icons?

If I start the widgets sample on the 100% monitor with the text page showing at it's minimum size and move it to the 150% monitor the 'Reset' button disappears.

I did find another issue with this approach. A multi-line wxTextCtrl returns a best size dependent on the number of lines of text it is showing. This can cause a significant change in the window size when it is moved to another monitor. For example if you move the 'Message Box Test Dialog' from the dialogs sample to another monitor the 'Main Message' text box shrinks down to just one line, and the dialog also shrinks accordingly.

What's your opinion on whether this is a feasible approach? I suspect it works for a subset of widgets but that there will be some widgets like the multi-line wxTextCtrl where the best size can change significantly where it wont.

Note: See TracTickets for help on using tickets.