Notebook sample gets bigger every DPI change

[MaartenBent]

To Reproduce:

1. Open notebook sample
2. Switch some tabs so the log fills up
3. Switch between displays with different DPI
4. See error

Description

This is caused because the minimum size of the multiline wxTextCtrl used for the log is based on the number of lines it contains.
And the log vs the notebook uses a proportion of 1-to-5, so if the wxTextCtrl gets a bit bigger, the notebook grows a a lot more.

It has the following call stack: wxNonOwnedWindow::HandleDPIChange() -> wxSizer::GetMinSize() -> wxSizerItem::CalcMin() -> wxWindowBase::GetEffectiveMinSize() -> wxTextCtrl::DoGetBestSize() -> wxTextCtrl::DoGetSizeFromTextSize()

A possible fix I found is overriding GetEffectiveMinSize() in wxTextCtrl, but I have no idea what other consequences this might have:

 include/wx/msw/textctrl.h | 1 +
 src/msw/textctrl.cpp      | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/wx/msw/textctrl.h b/include/wx/msw/textctrl.h
index ffc322a8ca..e6755d8175 100644
--- a/include/wx/msw/textctrl.h
+++ b/include/wx/msw/textctrl.h
@@ -237,6 +237,7 @@ protected:
 
     virtual wxSize DoGetBestSize() const override;
     virtual wxSize DoGetSizeFromTextSize(int xlen, int ylen = -1) const override;
+    virtual wxSize GetEffectiveMinSize() const override;
 
     virtual void DoMoveWindow(int x, int y, int width, int height) override;
 
diff --git a/src/msw/textctrl.cpp b/src/msw/textctrl.cpp
index 6f15bb6284..39cafaac32 100644
--- a/src/msw/textctrl.cpp
+++ b/src/msw/textctrl.cpp
@@ -2506,6 +2506,11 @@ wxSize wxTextCtrl::DoGetBestSize() const
     return DoGetSizeFromTextSize( FromDIP(DEFAULT_ITEM_WIDTH) );
 }
 
+wxSize wxTextCtrl::GetEffectiveMinSize() const
+{
+    return GetMinSize();
+}
+
 wxSize wxTextCtrl::DoGetSizeFromTextSize(int xlen, int ylen) const
 {
     int cy;

Platform and version information

• wxWidgets version you use: master
• wxWidgets port you use: wxMSW
• OS and its version: Windows 10

[vadz]

Thanks for reporting this, but unfortunately I don't think overriding GetEffectiveMinSize() is a good idea. In fact, I believe that this function shouldn't be virtual at all, and disagree with the rationale of a20a357, as it just combines GetMinSize() and GetBestSize() which are both already virtual. It's, of course, too late to change this now for compatibility reasons, so it will have to remain virtual, but I think we should at least tell people not to override it -- and avoid doing it ourselves.

Of course, this leaves the question of what do we need to do to actually fix the problem...

I think that the root reason of it is that we want to be smart about determining the initial size of the control if it's not explicitly specified, and, in particular, make it bigger than the strict minimum size if it has some contents, but we do not want to do this when determining its minimum size later one. But the trouble is that the same [Do]GetBestSize() function is used in both cases, assuming that GetMinSize() returns wxDefaultSize, as it does by default.

So the logical thing to do here seems to return the latter size from GetMinSize() and only use GetBestSize() for the initial size. I.e. overriding GetMinSize() in wxTextCtrl should fix the problem and this doesn't seem to be too bad from compatibility point of view. We also need to change wxWindowBase::SetInitialSize() to use best size even if min size is set then, but this also seems mostly compatible.

But "mostly" doesn't mean "perfectly", of course. E.g. overriding MyTextCtrl::DoGetBestSize() in an application wouldn't have the same effect after these changes as before and as much as I hate to say it, but there are probably some applications that will get broken by this :-(

So, if we want to be on the safe side, and if my initial diagnosis of the root of the problem is correct, we're probably going to have to add yet another virtual size-related function. I think if we add something like GetIdealSize() and use it in SetInitialSize() only and then change wx itself to move any value-dependent code into GetIdealSize() and let GetBestSize() return just the min size (e.g. for 2 lines of text only), then this should be 100% compatible and work.

But this is going to make explaining how sizing works in wx even more complicated... So maybe the best solution is the simplest one and we should just hardcode some number (5?) of lines in wxTextCtrl::GetBestSize() instead of using GetNumberOfLines() there?

[MaartenBent]

I hope I understand all correctly. Is this what you propose before-last?
I don't like to hardcode a number (currently it is 10), because it will still cause the window to increase size without reason.

 include/wx/msw/textctrl.h | 1 +
 include/wx/window.h       | 2 ++
 src/common/wincmn.cpp     | 7 ++++++-
 src/msw/textctrl.cpp      | 7 ++++++-
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/wx/msw/textctrl.h b/include/wx/msw/textctrl.h
index ffc322a8ca..a98d785ffd 100644
--- a/include/wx/msw/textctrl.h
+++ b/include/wx/msw/textctrl.h
@@ -235,6 +235,7 @@ protected:
     // send TEXT_UPDATED event, return true if it was handled, false otherwise
     bool SendUpdateEvent();
 
+    virtual wxSize GetIdealSize() const override;
     virtual wxSize DoGetBestSize() const override;
     virtual wxSize DoGetSizeFromTextSize(int xlen, int ylen = -1) const override;
 
diff --git a/include/wx/window.h b/include/wx/window.h
index 598d547a3f..ef081b0788 100644
--- a/include/wx/window.h
+++ b/include/wx/window.h
@@ -409,6 +409,8 @@ public:
         // returns the results.
     virtual wxSize GetEffectiveMinSize() const;
 
+    virtual wxSize GetIdealSize() const;
+
         // A 'Smart' SetSize that will fill in default size values with 'best'
         // size.  Sets the minsize to what was passed in.
     void SetInitialSize(const wxSize& size=wxDefaultSize);
diff --git a/src/common/wincmn.cpp b/src/common/wincmn.cpp
index 7cf091356a..681f65e420 100644
--- a/src/common/wincmn.cpp
+++ b/src/common/wincmn.cpp
@@ -890,7 +890,7 @@ wxSize wxWindowBase::GetEffectiveMinSize() const
 
     if (min.x == wxDefaultCoord || min.y == wxDefaultCoord)
     {
-        wxSize best = GetBestSize();
+        wxSize best = GetIdealSize();
         if (min.x == wxDefaultCoord) min.x =  best.x;
         if (min.y == wxDefaultCoord) min.y =  best.y;
     }
@@ -898,6 +898,11 @@ wxSize wxWindowBase::GetEffectiveMinSize() const
     return min;
 }
 
+wxSize wxWindowBase::GetIdealSize() const
+{
+    return GetBestSize();
+}
+
 wxSize wxWindowBase::GetBestSize() const
 {
     if ( !m_windowSizer && m_bestSizeCache.IsFullySpecified() )
diff --git a/src/msw/textctrl.cpp b/src/msw/textctrl.cpp
index 6f15bb6284..bdeea06233 100644
--- a/src/msw/textctrl.cpp
+++ b/src/msw/textctrl.cpp
@@ -2501,11 +2501,16 @@ bool wxTextCtrl::AcceptsFocusFromKeyboard() const
     return (IsEditable() || IsMultiLine()) && wxControl::AcceptsFocus();
 }
 
-wxSize wxTextCtrl::DoGetBestSize() const
+wxSize wxTextCtrl::GetIdealSize() const
 {
     return DoGetSizeFromTextSize( FromDIP(DEFAULT_ITEM_WIDTH) );
 }
 
+wxSize wxTextCtrl::DoGetBestSize() const
+{
+    return GetMinSize();
+}
+
 wxSize wxTextCtrl::DoGetSizeFromTextSize(int xlen, int ylen) const
 {
     int cy;

[vadz]

Thanks for looking into this!

I hope I understand all correctly. Is this what you propose before-last?

Sorry, no, I don't think this is right: if GetEffectiveMinSize() calls GetIdealSize(), which does the same thing as GetBestSize() does currently, the behaviour is going to remain the same, isn't it?

With this proposal the idea was for GetEffectiveMinSize() to keep calling GetBestSize(), but to change SetInitialSize() to use the new GetIdealSize(). I.e. "best size" would be used for minimal size not depending on the current contents while "ideal size" would be the size best suited for the current contents. As I said, this is clearly very confusing :-(

I don't like to hardcode a number (currently it is 10), because it will still cause the window to increase size without reason.

With that proposal the idea was to stop using the number of lines in the existing DoGetBestSize(), i.e. make it independent of the current contents. So the idea was to always make the text control 5 lines high instead of trying to be smart. This is going to change the layout of the existing code, but would at least be relatively simple to explain...