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

wxFrame::GetSize() returns a too large size in a wxAUI program with GTK3 and Wayland #23041

Closed
taler21 opened this issue Dec 16, 2022 · 10 comments
Labels
GTK Wayland wxGTK Wayland-specific issues
Milestone

Comments

@taler21
Copy link
Contributor

taler21 commented Dec 16, 2022

Description

In a wxAUI program, wxWindow::GetSize() returns an incorrect (too large) size of the program window.

It's a Wayland specific problem as it works correctly with X11.

To Reproduce

It can be reproduced in the aui sample using the following changes:

diff --git "a/samples/aui/auidemo.cpp" "b/samples/aui/auidemo.cpp"
index 1e415b311b..1996bf2c46 100644
--- "a/samples/aui/auidemo.cpp"
+++ "b/samples/aui/auidemo.cpp"
@@ -129,6 +129,7 @@ private:
 
 private:
 
+    void OnClose(wxCloseEvent& evt);
     void OnEraseBackground(wxEraseEvent& evt);
     void OnSize(wxSizeEvent& evt);
 
@@ -169,6 +170,8 @@ private:
     long m_notebook_style;
     long m_notebook_theme;
 
+    wxSize m_initSize;
+
     wxDECLARE_EVENT_TABLE();
 };
 
@@ -572,6 +575,7 @@ bool MyApp::OnInit()
 }
 
 wxBEGIN_EVENT_TABLE(MyFrame, wxFrame)
+    EVT_CLOSE(MyFrame::OnClose)
     EVT_ERASE_BACKGROUND(MyFrame::OnEraseBackground)
     EVT_SIZE(MyFrame::OnSize)
     EVT_MENU(MyFrame::ID_CreateTree, MyFrame::OnCreateTree)
@@ -662,6 +666,8 @@ MyFrame::MyFrame(wxWindow* parent,
                  long style)
         : wxFrame(parent, id, title, pos, size, style)
 {
+    m_initSize = size;
+
     // tell wxAuiManager to manage this frame
     m_mgr.SetManagedWindow(this);
 
@@ -1016,6 +1022,15 @@ void MyFrame::DoUpdate()
     m_mgr.Update();
 }
 
+void MyFrame::OnClose(wxCloseEvent& event)
+{
+    wxLogDebug("init : %d, %d", m_initSize.x, m_initSize.y);
+    wxLogDebug("close: %d, %d", GetSize().x, GetSize().y);
+    wxASSERT( m_initSize == GetSize() );
+
+    event.Skip();
+}
+
 void MyFrame::OnEraseBackground(wxEraseEvent& event)
 {
     event.Skip();
  1. Start the aui sample.
  2. Close it.
    The size returned by wxWindow::GetSize() does not correspond to the actual window size.

Platform and version information

  • wxWidgets version you use: 3.2.0
  • wxWidgets port you use: wxGTK
  • OS and its version: Ubuntu 22.04
  • GTK version: 3.24.33
  • Which GDK backend is used: Wayland
  • Current theme:
@paulcor
Copy link
Contributor

paulcor commented Dec 16, 2022

It's not specific to AUI, here's a simpler diff against the minimal sample:

diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
index 501caf9096..1ac8d0c54b 100644
--- a/samples/minimal/minimal.cpp
+++ b/samples/minimal/minimal.cpp
@@ -139,7 +139,7 @@ bool MyApp::OnInit()
 
 // frame constructor
 MyFrame::MyFrame(const wxString& title)
-       : wxFrame(NULL, wxID_ANY, title)
+       : wxFrame(NULL, wxID_ANY, title, wxDefaultPosition, wxSize(600, 400))
 {
     // set the frame icon
     SetIcon(wxICON(sample));
@@ -188,6 +188,9 @@ void MyFrame::OnQuit(wxCommandEvent& WXUNUSED(event))
 
 void MyFrame::OnAbout(wxCommandEvent& WXUNUSED(event))
 {
+const wxSize s(GetSize());
+fprintf(stderr, "%d %d\n", s.x, s.y);
+return;
     wxMessageBox(wxString::Format
                  (
                     "Welcome to %s!\n"

The size of decorations (which seems to include a large area for a composited shadow) is getting added to the requested window size. I think we try to compensate for that, but before the first TLW is showing, we don't know how big the decorations are.

@paulcor paulcor added GTK Wayland wxGTK Wayland-specific issues labels Dec 16, 2022
@vadz
Copy link
Contributor

vadz commented Dec 17, 2022

Could we always create the (first) TLW with some minimal size and then call SetSize() on it once we know the decorations size?

If not, can we make some backwards-incompatible change in 3.3 that would finally allow us to solve (all cases of) this problem? E.g. would deprecating the size parameter of wxFrame ctor and telling people to call SetClientSize() instead help?

@paulcor
Copy link
Contributor

paulcor commented Dec 17, 2022

Could we always create the (first) TLW with some minimal size and then call SetSize() on it once we know the decorations size?

We do something like this for X11 with the "deferred show" mechanism. It's quite horrible. I don't know if there is an equivalent to _NET_REQUEST_FRAME_EXTENTS for Wayland, but I somehow doubt it.

would deprecating the size parameter of wxFrame ctor and telling people to call SetClientSize() instead help?

It's very common for code to set the frame size using SetSize() rather than the ctor initial size, so I don't think deprecating the parameter would really help much. The usual case where this issue is a problem is saving and restoring the frame size, and the workaround is to save and restore the client size, so documenting that somewhere might be helpful.

@vadz
Copy link
Contributor

vadz commented Dec 18, 2022

I know that it's horrible, but it was a great improvement when you implemented (or improved, I don't remember any longer) it. So if it were possible to do something similar for Wayland, it would be worth doing it, IMO.

The usual case where this issue is a problem is saving and restoring the frame size, and the workaround is to save and restore the client size, so documenting that somewhere might be helpful.

We also need to change wxTLWGeometry::ApplyTo() to use it then (at least for GTK).

@vadz
Copy link
Contributor

vadz commented Jan 15, 2023

It's very common for code to set the frame size using SetSize() rather than the ctor initial size, so I don't think deprecating the parameter would really help much.

I'd like to return to this because calling SetSize() actually does work for me, it's only specifying the size in the ctor which doesn't, i.e. this

diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
index 501caf9096..9d6600fd7e 100644
--- a/samples/minimal/minimal.cpp
+++ b/samples/minimal/minimal.cpp
@@ -175,6 +175,18 @@ MyFrame::MyFrame(const wxString& title)
     CreateStatusBar(2);
     SetStatusText("Welcome to wxWidgets!");
 #endif // wxUSE_STATUSBAR
+
+    SetSize(654, 432);
+
+    auto showSize = [this](const char* when)
+    {
+        const wxSize s = GetSize();
+        const wxSize cs = GetClientSize();
+        wxFprintf(stderr, "%s:\tclient = (%d,%d), total = (%d,%d)\n", when, cs.x, cs.y, s.x, s.y);
+    };
+
+    showSize("Ctor");
+    CallAfter([=]() { showSize("Later"); });
 }

works as expected, i.e. outputs

Ctor:   client = (654,381), total = (654,432)
Later:  client = (654,381), total = (654,432)

while this:

diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
index 501caf9096..cc60200e05 100644
--- a/samples/minimal/minimal.cpp
+++ b/samples/minimal/minimal.cpp
@@ -139,7 +139,7 @@ bool MyApp::OnInit()

 // frame constructor
 MyFrame::MyFrame(const wxString& title)
-       : wxFrame(NULL, wxID_ANY, title)
+       : wxFrame(NULL, wxID_ANY, title, wxDefaultPosition, wxSize(654, 432))
 {
     // set the frame icon
     SetIcon(wxICON(sample));
@@ -175,6 +175,16 @@ MyFrame::MyFrame(const wxString& title)
     CreateStatusBar(2);
     SetStatusText("Welcome to wxWidgets!");
 #endif // wxUSE_STATUSBAR
+
+    auto showSize = [this](const char* when)
+    {
+        const wxSize s = GetSize();
+        const wxSize cs = GetClientSize();
+        wxFprintf(stderr, "%s:\tclient = (%d,%d), total = (%d,%d)\n", when, cs.x, cs.y, s.x, s.y);
+    };
+
+    showSize("Ctor");
+    CallAfter([=]() { showSize("Later"); });
 }

doesn't work correctly and outputs

Ctor:   client = (654,381), total = (654,432)
Later:  client = (654,381), total = (706,521)

So it looks like we should be able to make the size specified in the ctor work too.

@vadz vadz added this to the 3.3.0 milestone Jan 15, 2023
@paulcor
Copy link
Contributor

paulcor commented Jan 16, 2023

calling SetSize() actually does work for me

I can reproduce your results, but I find that in either case, the actual size of the window is the same (I measured it with a ruler). So the size is just being reported incorrectly when SetSize() is used. To make it even more fun, SetSize() does not have the same effect if called before CreateStatusBar(). ATM I don't know why any of this is happening.

@vadz
Copy link
Contributor

vadz commented Jan 16, 2023

SetSize() indirectly uses the client size, so I can understand how it's affected by CreateStatusBar(), but this is just another proof that it's implemented incorrectly now, of course...

I also can confirm that the actual size of the window doesn't correspond to the reported size by using something called "lg" (looking glass) in the GNOME Shell (I don't know of any better/less inconvenient way to check the window size under Wayland, if anybody does, please let me know).

Anyhow, it would be really nice to do something about it, it's rather embarrassing that something as simple as setting the window size fails in several different creative ways.

paulcor added a commit that referenced this issue Jan 17, 2023
In particular this fixes reported size in some situations where
SetSize() has been called before decoration size is known.
See #23041
GillesDuvert added a commit to GillesDuvert/gdl that referenced this issue Jan 17, 2023
paulcor added a commit that referenced this issue Jan 24, 2023
…s known

Fixes initial TLW size being larger than requested
See #23041
@vadz
Copy link
Contributor

vadz commented Jan 30, 2023

@paulcor Should both commits above be backported to 3.2? Things seem to work for me now if I call GetSize() late enough and, according to lg Gnome tool, the window really does have the size passed to SetSize().

Here is the patch I used for testing:

diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
index 501caf9096..eea0970a15 100644
--- a/samples/minimal/minimal.cpp
+++ b/samples/minimal/minimal.cpp
@@ -137,9 +137,16 @@ bool MyApp::OnInit()
 // main frame
 // ----------------------------------------------------------------------------

+//#define SIZE_CTOR
+
 // frame constructor
 MyFrame::MyFrame(const wxString& title)
+#ifdef SIZE_CTOR
+       : wxFrame(NULL, wxID_ANY, title, wxDefaultPosition, wxSize(600, 400))
+#else
        : wxFrame(NULL, wxID_ANY, title)
+#endif
+
 {
     // set the frame icon
     SetIcon(wxICON(sample));
@@ -175,6 +182,24 @@ MyFrame::MyFrame(const wxString& title)
     CreateStatusBar(2);
     SetStatusText("Welcome to wxWidgets!");
 #endif // wxUSE_STATUSBAR
+
+#ifndef SIZE_CTOR
+    SetSize(600, 400);
+#endif
+
+    auto showSize = [this](const char* when)
+    {
+        const wxSize s = GetSize();
+        const wxSize cs = GetClientSize();
+        wxFprintf(stderr, "%s:\tclient = (%d,%d), total = (%d,%d)\n", when, cs.x, cs.y, s.x, s.y);
+    };
+
+    showSize("Ctor");
+    CallAfter([=]() { showSize("Later"); });
+
+    new wxStaticText(this, wxID_ANY, "");
+    (new wxButton(this, wxID_ANY, "Show size", wxPoint(100, 100)))->
+        Bind(wxEVT_BUTTON, [=](wxCommandEvent&) { showSize("Now"); });
 }

and here is the output:

Ctor:   client = (600,349), total = (600,400)
Later:  client = (548,260), total = (600,400)
Now:    client = (548,260), total = (600,400)

i.e. the size is always correct and GetClientSize() is wrong initially but might be correct later (although I find the difference in horizontal direction very big: the borders are definitely not 26px, so I'm not sure what's going on here, it looks like Wayland might consider a band around the window part of its total size?).

With SIZE_CTOR defined it's a bit worse:

Ctor:   client = (600,349), total = (600,400)
Later:  client = (600,349), total = (652,489)
Now:    client = (548,260), total = (600,400)

i.e. you can't get the right size value until later (I click the button manually once the window is shown, but I guess a timer would work too), but at least it is correct eventually.

@paulcor
Copy link
Contributor

paulcor commented Jan 31, 2023

i.e. the size is always correct and GetClientSize() is wrong initially

Depends on what you mean by "wrong". That is the actual client size at that moment. It will become smaller later to keep the overall size constant. The same thing happens with X11.

the borders are definitely not 26px

They are. Most of it is space for a composited drop shadow. All done client-side by GTK, Wayland is not involved.

i.e. you can't get the right size value until later

Again, those are the actual sizes at that moment. And again, same thing happens with X11, except the timing is a little different in this case.

paulcor added a commit that referenced this issue Jan 31, 2023
In particular this fixes reported size in some situations where
SetSize() has been called before decoration size is known.
See #23041

(cherry picked from commit 08301b1)
paulcor added a commit that referenced this issue Jan 31, 2023
…s known

Fixes initial TLW size being larger than requested
See #23041

(cherry picked from commit 997d20e)
@paulcor paulcor closed this as completed Jan 31, 2023
@vadz
Copy link
Contributor

vadz commented Jan 31, 2023

OK, thanks for the explanation, I didn't realize the "drop shadow" thing. And thanks for cherry-picking this. Could you please update docs/changes.txt in 3.2 with a concise description of this change? I'm not sure what exactly to put there, but I'm pretty sure it's worth mentioning.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GTK Wayland wxGTK Wayland-specific issues
Projects
None yet
Development

No branches or pull requests

3 participants