From 41f4b1716d93e1c91f1c9f6f2b155daad6960c6e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 8 Jun 2020 15:29:42 +0200 Subject: [PATCH 1/4] Set initial wxTextCtrl text earlier in wxOSX This reverts the changes of 63bcc669d8 (fixing setting initial value under osx_cocoa for single line text controls, 2009-10-01) and fixes the problem which this commit probably tried to fix in a different way, using the same approach as in 98f5315405 (Don't set initial label in wxNativeWindow on OS X, 2016-04-29) as the real root of the problem was that the text set in CreateTextControl() was overwritten later when the label was set from SetPeer(). This change means that the text is now set correctly before SetPeer() calls SetInitialSize() call, which makes it possible to set the correct initial size based on the initial text, as will be done in later commits. It also makes Cocoa port more consistent with iOS one, as a nice side effect. --- include/wx/osx/cocoa/private/textimpl.h | 6 ++++++ src/osx/cocoa/textctrl.mm | 12 +++++++++--- src/osx/textctrl_osx.cpp | 6 ------ 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/include/wx/osx/cocoa/private/textimpl.h b/include/wx/osx/cocoa/private/textimpl.h index fc44aeb15d..b0e19367b0 100644 --- a/include/wx/osx/cocoa/private/textimpl.h +++ b/include/wx/osx/cocoa/private/textimpl.h @@ -32,6 +32,12 @@ public : virtual ~wxNSTextBase() { } virtual bool ShouldHandleKeyNavigation(const wxKeyEvent &event) const wxOVERRIDE; + + virtual void SetInitialLabel(const wxString& WXUNUSED(title), wxFontEncoding WXUNUSED(encoding)) wxOVERRIDE + { + // Don't do anything here, text controls don't have any label and + // setting it would overwrite the string value set when creating it. + } }; // implementation exposed, so that search control can pull it diff --git a/src/osx/cocoa/textctrl.mm b/src/osx/cocoa/textctrl.mm index d4d18a5200..da72115bea 100644 --- a/src/osx/cocoa/textctrl.mm +++ b/src/osx/cocoa/textctrl.mm @@ -1556,7 +1556,7 @@ void wxNSTextFieldControl::SetJustification() wxWidgetImplType* wxWidgetImpl::CreateTextControl( wxTextCtrl* wxpeer, wxWindowMac* WXUNUSED(parent), wxWindowID WXUNUSED(id), - const wxString& WXUNUSED(str), + const wxString& str, const wxPoint& pos, const wxSize& size, long style, @@ -1569,8 +1569,11 @@ wxWidgetImplType* wxWidgetImpl::CreateTextControl( wxTextCtrl* wxpeer, { wxNSTextScrollView* v = nil; v = [[wxNSTextScrollView alloc] initWithFrame:r]; - c = new wxNSTextViewControl( wxpeer, v, style ); + wxNSTextViewControl* t = new wxNSTextViewControl( wxpeer, v, style ); + c = t; c->SetNeedsFocusRect( true ); + + t->SetStringValue(str); } else { @@ -1593,7 +1596,8 @@ wxWidgetImplType* wxWidgetImpl::CreateTextControl( wxTextCtrl* wxpeer, [cell setWraps:NO]; [cell setScrollable:YES]; - c = new wxNSTextFieldControl( wxpeer, wxpeer, v ); + wxNSTextFieldControl* t = new wxNSTextFieldControl( wxpeer, wxpeer, v ); + c = t; if ( (style & wxNO_BORDER) || (style & wxSIMPLE_BORDER) ) { @@ -1608,6 +1612,8 @@ wxWidgetImplType* wxWidgetImpl::CreateTextControl( wxTextCtrl* wxpeer, // use native border c->SetNeedsFrame(false); } + + t->SetStringValue(str); } return c; diff --git a/src/osx/textctrl_osx.cpp b/src/osx/textctrl_osx.cpp index 4e64b8c530..71c7bb62ba 100644 --- a/src/osx/textctrl_osx.cpp +++ b/src/osx/textctrl_osx.cpp @@ -105,12 +105,6 @@ bool wxTextCtrl::Create( wxWindow *parent, MacPostControlCreate(pos, size) ; -#if wxOSX_USE_COCOA - // under carbon everything can already be set before the MacPostControlCreate embedding takes place - // but under cocoa for single line textfields this only works after everything has been set up - GetTextPeer()->SetStringValue(str); -#endif - // only now the embedding is correct and we can do a positioning update MacSuperChangedPosition() ; From c351f80481e99d94a1ec94848ace1ecdf2665264 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 7 Jun 2020 20:08:57 +0200 Subject: [PATCH 2/4] Don't call GetBestSize() from DoGetSizeFromTextSize() in wxOSX This could easily result in infinite recursion, as it is very natural to implement DoGetBestSize() in terms of GetSizeFromTextSize() and, in fact, our own implementation of the generic wxSpinCtrl did exactly this. Avoid such problems by only calling GetSizeFromTextSize() from DoGetBestSize(), but not in the other direction. This shouldn't have any effect on the returned size values. --- src/osx/textctrl_osx.cpp | 76 +++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 44 deletions(-) diff --git a/src/osx/textctrl_osx.cpp b/src/osx/textctrl_osx.cpp index 71c7bb62ba..cdf998c5c6 100644 --- a/src/osx/textctrl_osx.cpp +++ b/src/osx/textctrl_osx.cpp @@ -43,8 +43,6 @@ #include "wx/osx/private.h" -static const int TEXTCTRL_BORDER_SIZE = 5; - wxBEGIN_EVENT_TABLE(wxTextCtrl, wxTextCtrlBase) EVT_DROP_FILES(wxTextCtrl::OnDropFiles) EVT_CHAR(wxTextCtrl::OnChar) @@ -180,23 +178,25 @@ bool wxTextCtrl::IsModified() const wxSize wxTextCtrl::DoGetBestSize() const { - int wText = -1; - int hText = -1; - + wxSize size; if (GetTextPeer()) - { - wxSize size = GetTextPeer()->GetBestSize(); - if (size.x > 0 && size.y > 0) - { - hText = size.y; - wText = size.x; - } - } + size = GetTextPeer()->GetBestSize(); - if ( hText == - 1) - { - wText = 100 ; + // Normally the width passed to GetSizeFromTextSize() is supposed to be + // valid, i.e. positive, but we use our knowledge of its implementation + // just below to rely on it returning the default size if we don't have + // any valid best size in the peer and size remained default-initialized. + return GetSizeFromTextSize(size); +} +wxSize wxTextCtrl::DoGetSizeFromTextSize(int xlen, int ylen) const +{ + static const int TEXTCTRL_BORDER_SIZE = 5; + + // Compute the default height if not specified. + int hText = ylen; + if ( hText <= 0 ) + { // these are the numbers from the HIG: switch ( m_windowVariant ) { @@ -220,38 +220,26 @@ wxSize wxTextCtrl::DoGetBestSize() const // the numbers above include the border size, so subtract it before // possibly adding it back below hText -= TEXTCTRL_BORDER_SIZE; + + // make the control 5 lines tall by default for consistently with how + // the old code behaved + if ( m_windowStyle & wxTE_MULTILINE ) + hText *= 5 ; } - // as the above numbers have some free space around the text - // we get 5 lines like this anyway - if ( m_windowStyle & wxTE_MULTILINE ) - hText *= 5 ; + // Keep using the same default 100px width as was used previously in the + // special case of having invalid width. + wxSize size(xlen > 0 ? xlen : 100, hText); + + // Use extra margin size which works under macOS 10.15: note that we don't + // need the vertical margin when using the automatically determined hText. + if ( xlen > 0 ) + size.x += 4; + if ( ylen > 0 ) + size.y += 2; if ( !HasFlag(wxNO_BORDER) ) - hText += TEXTCTRL_BORDER_SIZE ; - - return wxSize(wText, hText); -} - -wxSize wxTextCtrl::DoGetSizeFromTextSize(int xlen, int ylen) const -{ - wxSize size; - - // Initialize to defaults unless both components are specified (at least - // one of them should be, but the other one could be -1). - if ( xlen <= 0 || ylen <= 0 ) - size = DoGetBestSize(); - - // Use extra margin size which works under macOS 10.15 and also add the - // border for consistency with DoGetBestSize() -- we'll remove it below if - // it's not needed. - if ( xlen > 0 ) - size.x = xlen + 4 + TEXTCTRL_BORDER_SIZE; - if ( ylen > 0 ) - size.y = ylen + 2 + TEXTCTRL_BORDER_SIZE; - - if ( HasFlag(wxNO_BORDER) ) - size.DecBy(TEXTCTRL_BORDER_SIZE); + size += wxSize(TEXTCTRL_BORDER_SIZE, TEXTCTRL_BORDER_SIZE) ; return size; } From 0458ad6c4ad5ee0c61acb67baf7d56737adefea5 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 9 Jun 2020 01:11:37 +0200 Subject: [PATCH 3/4] Use temporary NSLayoutManager variable No real changes, just make the code slightly shorter. --- src/osx/cocoa/textctrl.mm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/osx/cocoa/textctrl.mm b/src/osx/cocoa/textctrl.mm index da72115bea..da1a9e0cda 100644 --- a/src/osx/cocoa/textctrl.mm +++ b/src/osx/cocoa/textctrl.mm @@ -1222,9 +1222,9 @@ void wxNSTextViewControl::EnableAutomaticDashSubstitution(bool enable) wxSize wxNSTextViewControl::GetBestSize() const { - if (m_textView && [m_textView layoutManager]) + if ( NSLayoutManager* const layoutManager = [m_textView layoutManager] ) { - NSRect rect = [[m_textView layoutManager] usedRectForTextContainer: [m_textView textContainer]]; + NSRect rect = [layoutManager usedRectForTextContainer: [m_textView textContainer]]; return wxSize((int)(rect.size.width + [m_textView textContainerInset].width), (int)(rect.size.height + [m_textView textContainerInset].height)); } From e5ce18f0cbfe60ba8ff6419ca092da418a8647ca Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 9 Jun 2020 11:13:55 +0200 Subject: [PATCH 4/4] Make multiline controls 5 lines high by default under Mac This is consistent with the current behaviour of GetSizeFromTextSize() and the behaviour of GetBestSize() before the changes in this branch. This is still not consistent with the behaviour of the other ports, but this will be addressed later, by replacing the currently hardcoded 5. Note that calling usedRectForTextContainer: here was apparently wrong in any case because we were not sure to have already performed a layout and we should have had a call to ensureLayoutForTextContainer: before it to make it actually work. However, this made it work "too well" because it then correctly returned potentially very big sizes for the text controls containing a lot of text, which is not what we need here, as explained in the comment added by this commit. --- src/osx/cocoa/textctrl.mm | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/osx/cocoa/textctrl.mm b/src/osx/cocoa/textctrl.mm index da1a9e0cda..138b49e40f 100644 --- a/src/osx/cocoa/textctrl.mm +++ b/src/osx/cocoa/textctrl.mm @@ -1222,13 +1222,25 @@ void wxNSTextViewControl::EnableAutomaticDashSubstitution(bool enable) wxSize wxNSTextViewControl::GetBestSize() const { + wxSize size; if ( NSLayoutManager* const layoutManager = [m_textView layoutManager] ) { - NSRect rect = [layoutManager usedRectForTextContainer: [m_textView textContainer]]; - return wxSize((int)(rect.size.width + [m_textView textContainerInset].width), - (int)(rect.size.height + [m_textView textContainerInset].height)); + // We could have used usedRectForTextContainer: here to compute the + // total rectangle needed for the current text, but this is actually + // not too helpful for determining the best size of the control, as we + // don't always want to fit it to its text: e.g. if it contains 1000 + // lines, we definitely don't want to make it higher than display. + // + // So instead we just get the height of a single line (which works for + // any non-empty control) and then multiply it by the number of lines + // we want to have by default, which is currently just hardcoded as 5 + // for compatibility with the behaviour of the previous versions. + NSRect rect = [layoutManager lineFragmentRectForGlyphAtIndex: 0 + effectiveRange: nil]; + size.y = (int)(5*rect.size.height + [m_textView textContainerInset].height); } - return wxSize(0,0); + + return size; } void wxNSTextViewControl::SetJustification()