Skip to content
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

Scrollable text input #9393

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

felix642
Copy link
Contributor

@felix642 felix642 commented Dec 30, 2024

Improved the editable text field in the save menu. When overflowing, the textbox would previously display the beginning of the text and complete with '...' for the overflowing text. The user could continue typing but would not get any feedback. With those changes the input box now scrolls when the text overflows.

save.webm

The changes were also applied to the virtual keyboard, but since this keyboard does not have any arrow key the user is not able to scroll left or right. This is still an improvement from the current implementation since the user it at least able to see what he is typing, but ideally we should also add some arrow keys or similar feature to be able to move the cursor left and right.

The changes are related to : #7275 and #1318 both issues are already fixed but with the current feedback it's hard to know the actual character limit.

@oleg-derevenetz oleg-derevenetz added improvement New feature, request or improvement ui UI/GUI related stuff labels Dec 30, 2024
@oleg-derevenetz oleg-derevenetz added this to the 1.1.6 milestone Dec 30, 2024
@ihhub ihhub requested review from ihhub and Districh-ru December 31, 2024 04:11
Copy link
Owner

@ihhub ihhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @felix642 , I put few comments here. Would you mind to check them?

src/fheroes2/gui/ui_text.h Outdated Show resolved Hide resolved
src/fheroes2/gui/ui_text.cpp Outdated Show resolved Hide resolved
src/fheroes2/gui/ui_text.cpp Outdated Show resolved Hide resolved
src/fheroes2/gui/ui_text.cpp Outdated Show resolved Hide resolved
Comment on lines 406 to 411
while ( _textOffset + maxCharacterCount <= _cursorPosition ) {
// If the cursor is to the right of the textBox
++_textOffset;
maxCharacterCount = getMaxCharacterCount( reinterpret_cast<const uint8_t *>( _text.data() + _textOffset ), static_cast<int32_t>( _text.size() - _textOffset ),
charHandler, maxWidth );
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct to say that the complexity of this loop is O(N * (N / 2)) where N is the number of characters present in the string? Is there a way to avoid recalculating all the character lengths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could always call getMaxCharacterCount() right before the while loop and then compute the new maximum character count using the characters' width at the beginning / end of the loop. This should give us better performances.

I don't know if the added complexity is really needed though. This while loop is only used to make sure that the cursor is always visible if it overflows to the right. This means that the while loop should, on average, be executed only once or twice (moving the view by one character should be enough for most scenarios).
The only cases where the while loop might be executed more than once is when a large character is added to the right of the string and small characters are removed to the left.

@felix642 felix642 force-pushed the scrollable-text-input branch from 4c9760a to 6209461 Compare January 2, 2025 21:07
@felix642 felix642 force-pushed the scrollable-text-input branch from c619c4e to f829da9 Compare January 3, 2025 18:35
@ihhub ihhub self-requested a review January 4, 2025 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement New feature, request or improvement ui UI/GUI related stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants