Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wxGrid resize issue when using DPI aware manifest #24445

Open
gsxruk opened this issue Mar 30, 2024 · 19 comments
Open

wxGrid resize issue when using DPI aware manifest #24445

gsxruk opened this issue Mar 30, 2024 · 19 comments

Comments

@gsxruk
Copy link
Contributor

gsxruk commented Mar 30, 2024

Description

This issue relates to a wxGrid used on a dual monitor setup. One monitor is 4K and the other is 1080p.

I am unable to confirm if this issue affects a setup where the monitors are of the same type (i.e. both 4K or both 1080p). Perhaps someone with a suitable setup could confirm this?

The issue occurs when the application window is dragged from one monitor to the other. The direction is not important as it occurs in both directions.

Bug Description

When the application is run using the setup described above, the window resizes when it is transferred from one monitor to the other in the event the wxGrid requires a scrollbar. For large grids, this results in the window being resized to a size larger than the monitor and disappears from view completely. It appears the resize attempts to resize the window to fit the entire grid.

There is a simple sample attached to demonstrate the issue. The sample consists of a simple wxGrid. There is also a resource file that declares a DPI aware level 2.

If the sample is run and the steps to reproduce the behaviour are followed the issue will be observed. If the DPI aware declaration within the resource file is commented out, the sample is run and the steps to reproduce the behaviour are followed the issue will not be observed.

To Reproduce

  1. Run the sample.
  2. Reduce the width of the window to ensure the horizontal scrollbar is present.
  3. Drag the window from one monitor to the other.
  4. The window will have resized to show the entire grid and eliminate the need for the scrollbars.

Expected vs Observed Behaviour

It is expected that the window would not resize during the drag operation from one monitor to the other and this is the observed behaviour when not using a DPI aware manifest.

Attachment

sample.zip

Platform and Version Information

  • wxWidgets version you use: v3.2.4
  • wxWidgets port you use: wxMSW
  • OS and its version: Windows 10 v22H2 (Build: 19045.4170)
@PBfordev
Copy link
Contributor

I think this is basically the same as #23091 or #23404.

@PBfordev
Copy link
Contributor

PBfordev commented Mar 31, 2024

Just in case it was lost here as it was in the penultimate comment in #23091.

The wxGrid behaviour is not just annoying, it is basically a show stopper: After moving a frame containing a large grid allowed to expand, the frame just disappears and cannot be really brought back, i.e., no further interaction with it is possible.

For example, my display setup is
mydisplays

When I run this code

#include <wx/wx.h>
#include <wx/grid.h>

class MyApp : public wxApp
{
    bool OnInit() override
    {
        wxFrame* frame = new wxFrame(nullptr, wxID_ANY, "Test");
        wxBoxSizer* sizer = new wxBoxSizer(wxVERTICAL);

        wxGrid* grid = new wxGrid(frame, wxID_ANY);
        grid->CreateGrid(100, 100);
        sizer->Add(grid, wxSizerFlags(1).Expand());

        frame->SetSizer(sizer);
        frame->Show();
        return true;
    }
}; wxIMPLEMENT_APP(MyApp);

after launching it on my primary (HiDPI) monitor, the frame display coordinates are
frame-grid-before
but after moving it to the secondary screen with standard DPI, the frame disappears since its coordinates are now nonsensical
frame-grid-after

@gsxruk
Copy link
Contributor Author

gsxruk commented Mar 31, 2024

Yes, that is basically what I see as well.

I intentionally made my sample a small grid (1x4) so that the resize can be seen as in this case it does not cause the window to become too large to fit in the screen. However, as you describe, for large grids the window often disappears from view due to being so large.

This issue is a show stopper if you want to save the window position and size on exit which is what I wanted to do. But I think I'll have to leave this for now.

@PBfordev
Copy link
Contributor

However, as you describe, for large grids the window often disappears from view due to being so large.

In my case, it always disappears since it gets moved way outside the visible display area.

This issue is a show stopper if you want to save the window position and size on exit

It is not just about saving the window position: It actually leaves the user baffled and helpless when their window disappears forever after they tried to move it to another screen.

I do understand that this issue may not be easy to fix but still...

@gsxruk
Copy link
Contributor Author

gsxruk commented Mar 31, 2024

It would leave the user confused. There is no doubt of that. But in that scenario, it's possible a restart would bring the application back to a normal state. In the scenario I mentioned, this would not be the case. The application would remain unusable even after a restart.

But to be honest, I think we are seeing the same thing and probably describing it slightly differently. Not sure if this helps move the issue forward.

There is a minimal sample to illustrate the issue, so that's a start at least.

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

I have had a quick look into this issue and it looks like this is the method causing the issue:

bool wxNonOwnedWindow::HandleDPIChange(const wxSize& newDPI, const wxRect& newRect)
{
    if ( !m_perMonitorDPIaware )
    {
        return false;
    }
    // Update the window decoration size to the new DPI: this seems to be the
    // call with the least amount of side effects that is sufficient to do it
    // and we need to do this in order for the size calculations, either in the
    // user-defined wxEVT_DPI_CHANGED handler or in our own GetBestSize() call
    // below, to work correctly.
    ::SetWindowPos(GetHwnd(),
                   0, 0, 0, 0, 0,
                   SWP_NOSIZE | SWP_NOMOVE | SWP_NOZORDER | SWP_NOREDRAW |
                   SWP_NOACTIVATE | SWP_NOOWNERZORDER | SWP_NOSENDCHANGING |
                   SWP_FRAMECHANGED);
    const bool processed = MSWUpdateOnDPIChange(m_activeDPI, newDPI);
    m_activeDPI = newDPI;
    // We consider that if the event was processed, the application resized the
    // window on its own already, but otherwise do it ourselves.
    if ( !processed )
    {
        // The best size doesn't scale exactly with the DPI, so while the new
        // size is usually a decent guess, it's typically not exactly correct.
        // We can't always do much better, but at least ensure that the window
        // is still big enough to show its contents if it uses a sizer.
        //
        // Note that if it doesn't use a sizer, we can't do anything here as
        // using the best size wouldn't be the right thing to do: some controls
        // (e.g. multiline wxTextCtrl) can have best size much bigger than
        // their current, or minimal, size and we don't want to expand them
        // significantly just because the DPI has changed, see #23091.
        wxRect actualNewRect = newRect;
        if ( wxSizer* sizer = GetSizer() )
        {
            const wxSize minSize = ClientToWindowSize(sizer->GetMinSize());
            wxSize diff = minSize - newRect.GetSize();
            diff.IncTo(wxSize(0, 0));
            // Use wxRect::Inflate() to ensure that the center of the bigger
            // rectangle is at the same position as the center of the proposed
            // one, to prevent moving the window back to the old display from
            // which it might have been just moved to this one, as doing this
            // would result in an infinite stream of WM_DPICHANGED messages.
            actualNewRect.Inflate(diff);
        }
        SetSize(actualNewRect);
    }
    Refresh();
    return true;
}

The problem appears to occur when processed = false and the additional code is executed. In this section if the size is set as the rectangle provided to the method (i.e. SetSize(newRect)) everything appears to work perfectly. In this case, the window transfers from one screen to the other and keeps the same size. So for example, if the grid was showing columns A and B on the previous DPI screen, it shows the same on the new DPI screen. I think as a user this is what I would expect.

I'm not entirely sure what that section of code is trying to do. Is it trying to change the size of the window to fit everything it contains within it? If so, I'm not sure I would expect that. As a user, if the window size could not fit everything on after a change in DPI, I would not be surprised to have to resize it manually.

Does anyone have any further information on this?

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

It also appears that there is a bug in that section of code as well.

The line actualNewRect.Inflate(diff); actually causes the rectangle to increase by double the values of diff and I don't think that is what is intended here. Therefore I think diff should be halved prior to the actualNewRect.Inflate(diff); command so that it is increased in size by diff.

However, I'm still leaning towards the size being set to newRect because in the examples I try it on, this section of code ends up with the same values (unless of cause there are scrollbars present).

I'd be very interested if anybody has a small sample application that puts this section of code to use in another way and demonstrates its purpose.

@MaartenBent
Copy link
Contributor

The commit history of nonownedwnd.cpp has a lot of info of why stuff was added, and links to relevant PRs and issues. https://github.com/wxWidgets/wxWidgets/commits/master/src/msw/nonownedwnd.cpp

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

Having a quick look through I can see 4 issues raised that have minimal test examples including this one (feel free to let me know if I missed any).

When SetSize(newRect) is used:

#18649 = OK
#23404 = OK (Fails in current v3.2.4)
#23091 = OK (Fails in current v3.2.4)
#24445 (this issue) = OK

That's what testing on my system shows. Happy to try any others I have missed.

@MaartenBent
Copy link
Contributor

#18649 is not OK with newRect. When starting at 100% and moving to 150%:
Screenshot 2024-04-01 195520
This was fixed by that patch.

@MaartenBent
Copy link
Contributor

But maybe that behaviour is less problematic than the infinite grow of text/list/grid controls.

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

It works fine both ways on mine. Are you just changing the SetSize statement in the original code?

@MaartenBent
Copy link
Contributor

Yes. And the app has to start on the 100% monitor.

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

Ok. I can make it do that by adjusting the 4K monitor from 100% to 150%. But no switching of one monitor to the other is necessary. But this is not something I was checking. I don't remember seeing that in #18649. The original text was talking about changing from one DPI to the other.

@MaartenBent
Copy link
Contributor

Well, for me it does happen when starting on a 100% (secondary) monitor and moving to the (main, system DPI) 150% monitor.

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

I don't see it across monitors so I guess there may be a slight difference between different graphics cards, monitors etc.

I agree this is a lot less problematic than what we are seeing with the text/list/grid controls. Assuming that the window can be resized, this would involve the user performing a slight drag to increase the size. With the text/list/grid controls the application becomes unusable.

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

One thing I notice from this is a difference between the dpi change from moving across monitors and the dpi change on a single monitor changing from 100% to 150%.

On my system:

  1. Moving from the 1080p monitor to the 4K monitor (at 150%) results in the method reporting a newDPI of 144 and a m_activeDPI of 96.

  2. Changing the scale of the 4K monitor from 100% to 150% results in the method reporting a newDPI of 144 and a m_activeDPI of 96.

So exactly the same in each scenario. However, for these scenarios newRect differs as follows:

  1. 407x254
  2. 408x245

It seems odd to me that the 2nd scenario has a slightly lower value for the height (which is the issue in this example). It is also odd that this is the figure the current solution calculates.

Any idea why the width on these would be near identical and the height be different?

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 1, 2024

I think I have found out why but not sure why it is happening. Perhaps you will have a better idea?

It appears that when when my 4k monitor is set to 150% and the application is started to appear on the 1080p screen it causes the DPI changed event (144dpi to 96dpi) to occur so the size of the window is immediately changed as it starts. However, if the 4k monitor is set to 100% this does not happen. This event is false as the window has not transitioned from 144dpi to 96dpi.

This does not happen when it starts on the 4k monitor whatever scale it is set to.

I think this event might be clouding the potential answer to this issue.

I'm signing off for now as I've spent far too long on this already! Will try to investigate further over the next few days.

@gsxruk
Copy link
Contributor Author

gsxruk commented Apr 6, 2024

I've looked into this issue a bit further.

It appears there are already a couple of hacks related to the size of wxStaticText. This causes some of the extra size being seen here. There are also some comments in the source that suggest these hacks are related to positioning the text in relation to wxTextCtrl as well. Therefore, messing with the size of wxStaticText may well be opening a can of worms.

However, the issue being seen with wxGrid and wxListCtrl is quite a problem as it can result in an application becoming unusable to the user.

So the only possible solution I can think of is to change the size used for DPI changes for wxGrid and wxTextCtrl and any others that experience this issue.

Currently, the HandleDPIChange method uses the minimum size returned by the sizer. The sizer uses the minimum or best size as applicable to calculate this.

Would it be possible for there to be new methods that return the same as the current method by default, but for problematic classes like wxGrid, it can return the current size? This would mean these problematic items are simply resized according to their current size which appears to work very well.

Anybody have any thoughts on this? Or ideas for other possible solutions?

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

No branches or pull requests

3 participants