8000 EditBox: Add onCaretPositionChange Signal and Fix Bugs in EditBox::selectText() by CasualYT31 · Pull Request #206 · texus/TGUI · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
/ TGUI Public

EditBox: Add onCaretPositionChange Signal and Fix Bugs in EditBox::selectText() #206

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 31, 2023
Merged

EditBox: Add onCaretPositionChange Signal and Fix Bugs in EditBox::selectText() #206

merged 3 commits into from
Aug 31, 2023

Conversation

CasualYT31
Copy link
Contributor

After completing extensive testing I've concluded that the new onCaretPositionChange signal added to EditBox can be merged, barring any additional comments. Here are most of the tests I carried out, which also help describe how the signal operates:

Test Case Actions Result
moveCaretWordEnd move caret to end of last word emitted
moveCaretWordEnd move caret to end of non-last word emitted
moveCaretWordBegin move caret to beginning of first word emitted
moveCaretWordBegin move caret to beginning of non-first word emitted
moveCaretRight highlighted text, caret on right side does not emit as caret does not change position
moveCaretRight highlighted text, caret on left side emitted
moveCaretRight no highlighted text, caret in middle/start of text emitted
moveCaretRight no highlighted text, caret at end of text does not emit
moveCaretLeft highlighted text, caret on right side emitted
moveCaretLeft highlighted text, caret on left side does not emit
moveCaretLeft no highlighted text, caret in middle/end of text emitted
moveCaretLeft no highlighted text, caret at beginning of text does not emit
keyPressed(DocumentBegin OR PageUp OR Up) caret at beginning of text does not emit
keyPressed(DocumentBegin OR PageUp OR Up) caret at middle/end of text emitted
keyPressed(DocumentEnd OR PageDown OR Down) caret at end of text does not emit
keyPressed(DocumentEnd OR PageDown OR Down) caret at middle/start of text emitted
mouseMoved no fixed width emitted once for every new character selected or deselected
mouseMoved fixed width ditto
leftMousePressed double click on caret at end of text does not emit
leftMousePressed other double click cases emits twice, one for the single click and one for the double click
leftMousePressed single clicks emitted, unless you click on the caret
setCaretPosition calls to this method from within EditBox all work as expected; setMaximumCharacters() if it causes to move caret; Selecting and pasting, pasting, cutting, etc. all work, and the text is always updated before the signal is emitted; Fixed width works as expected
setCaretPosition different position emitted
setCaretPosition same position does not emit
selectText select different range emitted
selectText select same range does not emit
selectText select range that causes caret to be at the same end position does not emit

I've also identified two cases where giving out-of-bounds values to EditBox::selectText() would cause crashes and/or unexpected behaviour:

  1. If you set start to be greater than the length of the text, an out-of-bounds string exception would be thrown. Capping start to the size of the text prevents this from occurring, and it will mean that no text is selected.
  2. Previously, if you set start to a non-0 value, but left length to its default npos value, it would cause an overflow in the start + length calculation which would mean that selectText(1) would select the first character of the text, and would not start at the second character and select the rest of the text (since npos + 1 would overflow to 0). As far as I can tell this does not seem like the intended behaviour, so I've added some code that always explicitly sets the end of the selection to the end of the text when npos is given to length. You could still cause overflows now but you'd have to go out of your way to do it.

@CasualYT31
Add onCaretPositionChange signal to EditBox
4ec3dba
@CasualYT31
Add additional guards to EditBox::selectText()
9574f12 
 * You could cause a crash/exception by issuing a start value that was greater than the text size. start is now capped at the text length.
 * Issuing just the start parameter would cause an overflow when added to length, meaning the default length value (npos) would not always represent "end of the text." Now, if length is npos, it will always mean "end of the text."
@CasualYT31
Add extra blank line to match style guides
2282280
@texus
Copy link
Owner
texus commented Aug 31, 2023

Thanks again for the extensive testing and for fixing the issue with selectText. I'll merge this PR now and will have a look at the TextArea PR later, but on first glance that one looks great as well.

While writing tests for selectText, I discovered that getSelectedText() also has a bug where it doesn't return the correct string if the selection doesn't start at the beginning. I also wrote tests for the getSignal() function in case there were other widgets that didn't override the function, and this also resulted in two typos being found and fixed. While I'm pretty happy with more than 80% of my code being covered by tests, it is starting to look like the remaining part just contains one bug after the other.

@texus texus merged commit a7670f5 into texus:1.x Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CasualYT31 @texus
0