Implement maximum number of characters in Multi line text control under OSX #24086

oneeyeman1 · 2023-11-24T16:02:04Z

Please review and apply.

oneeyeman1 · 2023-11-24T21:22:32Z

CI failure is in the grid test that is intermittently fails...

vadz · 2023-12-04T23:45:16Z

I'm not sure but I don't think comparing lengths like this is a good idea because it doesn't take selection into account: if you select some text and paste (or type) something, the selected text is removed. E.g. doing this would probably prevent pasting/typing even a single character in a control filled up to capacity when its entire contents is selected.

oneeyeman1 · 2023-12-08T03:37:16Z

@vadz ,

I'm not sure but I don't think comparing lengths like this is a good idea because it doesn't take selection into account: if you select some text and paste (or type) something, the selected text is removed. E.g. doing this would probably prevent pasting/typing even a single character in a control filled up to capacity when its entire contents is selected.

I just tested it - osx 10.13 + Xcode 9 and it works as expected.
What doesn't work however is the multi-line text control hint. I see a blank control

vadz

Please make a function taking the current length and the length of the text being added and checking if there is enough space left and call it from all places where it's necessary because verifying that all these comparisons work/don't suffer from overflows is difficult.

vadz · 2024-04-25T21:21:08Z

src/osx/cocoa/textctrl.mm

@@ -562,6 +583,29 @@ - (void)cut:(id)sender

 - (void)paste:(id)sender
 {
+    NSPasteboard *pb = [NSPasteboard generalPasteboard];


Are you sure this code is needed, i.e. shouldChangeTextInRange:replacementString: is really not invoked when pasting? I'd expect it to be...

vadz · 2024-04-25T21:22:22Z

src/osx/cocoa/textctrl.mm

+                {
+                    auto range = maxlen - textCtrl->GetValue().length();
+                    pbItem = [pbItem substringToIndex:range];
+                    [self insertText:pbItem];


This looks wrong: if we insert text here and call [super paste:] below too, the text will be inserted twice (assuming replacementString: is not called).

vadz · 2024-04-25T21:23:24Z

src/osx/textctrl_osx.cpp

@@ -107,7 +107,7 @@ bool wxTextCtrl::Create( wxWindow *parent,
        SetEditable( false ) ;

    SetCursor( wxCursor( wxCURSOR_IBEAM ) ) ;
-
+    m_maxlen = UINT_MAX;


I'd initialize it to 0 to indicate that it's not set and do it in the declaration.

vadz · 2024-04-25T21:23:36Z

src/osx/textctrl_osx.cpp

+
+void wxTextCtrl::SetMaxLength(unsigned long length)
+{
+    if( HasFlag( wxTE_MULTILINE ) )


Suggested change

if( HasFlag( wxTE_MULTILINE ) )

if ( HasFlag( wxTE_MULTILINE ) )

vadz · 2024-04-25T21:25:12Z

src/osx/cocoa/textctrl.mm

+            }
+        }
+    }
+    return ret;


This is wrong because you don't call super version for multiline controls any more. You need to do it here and return NO above when the change is not allowed.

vadz · 2024-04-25T21:26:13Z

src/osx/cocoa/textctrl.mm

+            {
+                auto maxlen = textCtrl->GetMaxLen();
+                ret = [super shouldChangeTextInRange:affectedCharRange replacementString:replacementString] &&
+                      (self.string.length + (replacementString.length - affectedCharRange.length) <= maxlen);


Not sure about the types here but it's not obvious that this can't overflow, when replacementString.length < affectedCharRange.length. You need to compare them first.

vadz · 2024-04-25T21:27:07Z

src/osx/cocoa/textctrl.mm

+            if( textCtrl->HasFlag( wxTE_MULTILINE ) )
+            {
+                auto maxlen = textCtrl->GetMaxLen();
+                if( textCtrl->GetValue().length() + st.length() > maxlen )


Overflow also seems to be possible here. Comparing st.length() with maxlen first would make it safe.

vadz · 2024-04-25T21:29:15Z

src/osx/textctrl_osx.cpp

+void wxTextCtrl::SetMaxLength(unsigned long length)
+{
+    if( HasFlag( wxTE_MULTILINE ) )
+        m_maxlen = length;


What happens if the current length is greater? Should we truncate it or not? What does wxMSW do here?

vadz · 2024-04-25T21:29:33Z

include/wx/osx/textctrl.h

@@ -147,6 +147,7 @@ class WXDLLIMPEXP_CORE wxTextCtrl: public wxTextCtrlBase
    void OSXEnableAutomaticDashSubstitution(bool enable);
    void OSXDisableAllSmartSubstitutions();

+    unsigned long GetMaxLen() const { return m_maxlen; }


If this is wxOSX-specific, it should have OSX prefix.

vadz · 2024-04-25T21:29:47Z

include/wx/osx/textctrl.h

@@ -68,7 +68,7 @@ class WXDLLIMPEXP_CORE wxTextCtrl: public wxTextCtrlBase
    // operations
    // ----------

-
+    virtual void SetMaxLength(unsigned long length) override;


Minor, but this doesn't belong to the "operations" section.

oneeyeman1 added 2 commits November 24, 2023 09:55

Implement Maximum text length for multi-line text control in Cocoa

782fbf5

Undo changes to the file

5c640e7

oneeyeman1 changed the title ~~Multi line osx~~ Implement maximum number of characters in Multi line text control under OSX Nov 24, 2023

Remove extra white spaces

df743f2

vadz added the macOS Specific to Cocoa macOS port label Dec 4, 2023

vadz reviewed Apr 25, 2024

View reviewed changes

vadz added the work needed Too useful to close, but can't be applied in current state label Apr 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement maximum number of characters in Multi line text control under OSX #24086

Implement maximum number of characters in Multi line text control under OSX #24086

oneeyeman1 commented Nov 24, 2023

oneeyeman1 commented Nov 24, 2023

vadz commented Dec 4, 2023

oneeyeman1 commented Dec 8, 2023

vadz left a comment

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

vadz Apr 25, 2024

	if( HasFlag( wxTE_MULTILINE ) )
	if ( HasFlag( wxTE_MULTILINE ) )

Implement maximum number of characters in Multi line text control under OSX #24086

Are you sure you want to change the base?

Implement maximum number of characters in Multi line text control under OSX #24086

Conversation

oneeyeman1 commented Nov 24, 2023

oneeyeman1 commented Nov 24, 2023

vadz commented Dec 4, 2023

oneeyeman1 commented Dec 8, 2023

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment