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#24086oneeyeman1 wants to merge 3 commits intowxWidgets:masterfrom
Conversation
oneeyeman1
changed the title
|
CI failure is in the grid test that is intermittently fails... |
|
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. |
|
@vadz ,
I just tested it - osx 10.13 + Xcode 9 and it works as expected. |
vadz
left a comment
vadz
left a comment
There was a problem hiding this comment.
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.
|
|
||
| - (void)paste:(id)sender | ||
| { | ||
| NSPasteboard *pb = [NSPasteboard generalPasteboard]; |
There was a problem hiding this comment.
Are you sure this code is needed, i.e. shouldChangeTextInRange:replacementString: is really not invoked when pasting? I'd expect it to be...
| { | ||
| auto range = maxlen - textCtrl->GetValue().length(); | ||
| pbItem = [pbItem substringToIndex:range]; | ||
| [self insertText:pbItem]; |
There was a problem hiding this comment.
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).
|
|
||
| SetCursor( wxCursor( wxCURSOR_IBEAM ) ) ; | ||
|
|
||
| m_maxlen = UINT_MAX; |
There was a problem hiding this comment.
I'd initialize it to 0 to indicate that it's not set and do it in the declaration.
|
|
||
| void wxTextCtrl::SetMaxLength(unsigned long length) | ||
| { | ||
| if( HasFlag( wxTE_MULTILINE ) ) |
There was a problem hiding this comment.
| if( HasFlag( wxTE_MULTILINE ) ) | |
| if ( HasFlag( wxTE_MULTILINE ) ) |
| } | ||
| } | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
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.
| { | ||
| auto maxlen = textCtrl->GetMaxLen(); | ||
| ret = [super shouldChangeTextInRange:affectedCharRange replacementString:replacementString] && | ||
| (self.string.length + (replacementString.length - affectedCharRange.length) <= maxlen); |
There was a problem hiding this comment.
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.
| if( textCtrl->HasFlag( wxTE_MULTILINE ) ) | ||
| { | ||
| auto maxlen = textCtrl->GetMaxLen(); | ||
| if( textCtrl->GetValue().length() + st.length() > maxlen ) |
There was a problem hiding this comment.
Overflow also seems to be possible here. Comparing st.length() with maxlen first would make it safe.
| void wxTextCtrl::SetMaxLength(unsigned long length) | ||
| { | ||
| if( HasFlag( wxTE_MULTILINE ) ) | ||
| m_maxlen = length; |
There was a problem hiding this comment.
What happens if the current length is greater? Should we truncate it or not? What does wxMSW do here?
| void OSXEnableAutomaticDashSubstitution(bool enable); | ||
| void OSXDisableAllSmartSubstitutions(); | ||
|
|
||
| unsigned long GetMaxLen() const { return m_maxlen; } |
There was a problem hiding this comment.
If this is wxOSX-specific, it should have OSX prefix.
| // ---------- | ||
|
|
||
|
|
||
| virtual void SetMaxLength(unsigned long length) override; |
There was a problem hiding this comment.
Minor, but this doesn't belong to the "operations" section.
vadz
added
the
work needed
2 participants
Please review and apply.