-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Implement maximum number of characters in Multi line text control under OSX #24086
base: master
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -562,6 +583,29 @@ - (void)cut:(id)sender | |||
|
|||
- (void)paste:(id)sender | |||
{ | |||
NSPasteboard *pb = [NSPasteboard generalPasteboard]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -107,7 +107,7 @@ bool wxTextCtrl::Create( wxWindow *parent, | |||
SetEditable( false ) ; | |||
|
|||
SetCursor( wxCursor( wxCURSOR_IBEAM ) ) ; | |||
|
|||
m_maxlen = UINT_MAX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if( HasFlag( wxTE_MULTILINE ) ) | |
if ( HasFlag( wxTE_MULTILINE ) ) |
} | ||
} | ||
} | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the current length is greater? Should we truncate it or not? What does wxMSW do here?
@@ -147,6 +147,7 @@ class WXDLLIMPEXP_CORE wxTextCtrl: public wxTextCtrlBase | |||
void OSXEnableAutomaticDashSubstitution(bool enable); | |||
void OSXDisableAllSmartSubstitutions(); | |||
|
|||
unsigned long GetMaxLen() const { return m_maxlen; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is wxOSX-specific, it should have OSX
prefix.
@@ -68,7 +68,7 @@ class WXDLLIMPEXP_CORE wxTextCtrl: public wxTextCtrlBase | |||
// operations | |||
// ---------- | |||
|
|||
|
|||
virtual void SetMaxLength(unsigned long length) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but this doesn't belong to the "operations" section.
Please review and apply.