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

GUI: Allow text selection #2964

Closed
wants to merge 2 commits into from
Closed

GUI: Allow text selection #2964

wants to merge 2 commits into from

Conversation

@srikeerthis
Copy link

@srikeerthis srikeerthis commented Apr 18, 2021

This PR adds the enhancement requested at
https://bugs.scummvm.org/ticket/6948

The key combination Shift+Left Arrow will highlight and select text towards left of the caret.

Two methods have been added:

  • initHighlight() - resets highlight.
  • getHighlightOffset() - used for scrolling text animation as text is selected.

Files changed:

  • editable.h: declared variables and methods.
  • edittext.cpp: Code segment that prevents highlight from being drawn on any position where mouse is clicked.
  • editable.cpp:
    • Select and highlight text towards left.
    • Copy selected text using Ctrl-C.
    • Remove selected characters on Backspace/Delete key.
    • Replace selected text with a character.
    • Disable and re-initialize highlight on press of Left Arrow/Right Arrow/Home/End keys
srikeerthis added 2 commits Apr 18, 2021
Key combination of shift+left arrow will start or resize highlight and select text towards left of the caret.
Copy link
Member

@sev- sev- left a comment

I reviewed the code and put my comments and questions.

A quick test showed the following:

  • The highlight interferes with the tooltip. If I move the mouse over e.g. game id and start highlighting, the highlight will blink when the tooltip is shown
  • On initial launch and starting highlighting right away, it leads to the visual glitch when whole line becomes green, with flickering white spot where cursor is supposed to be, and doubled input line. I'll attach a screenshot
  • When selecting, clicking "shift-right" clears the whole selection and moves cursor right. The logical thing would be to decrease the highlight size by one
  • If the line is wider than the input (scrollPos is non-zero), highlighting on the left edge leads to the whole line being highlighted
  • When in the middle of a line, Shift-Right does not start the highlighting
@@ -20,11 +20,11 @@
*
*/

#include "gui/widgets/editable.h"

This comment has been minimized.

@sev-

sev- Apr 19, 2021
Member

These changes must be reverted as they're just reordering of includes

@@ -129,49 +138,86 @@ bool EditableWidget::handleKeyDown(Common::KeyState state) {
switch (state.keycode) {
case Common::KEYCODE_RETURN:
case Common::KEYCODE_KP_ENTER:
initHighlight();

This comment has been minimized.

@sev-

sev- Apr 19, 2021
Member

the name of the method is misleading. "init" normally means start. So, better name it clearHighLight()

// Move caret to start
setCaretPos(0);
sendCommand(_cmd, 0);

This comment has been minimized.

@sev-

sev- Apr 19, 2021
Member

What is the purpose of this call?

if (_highlightVisible) {
if (state.ascii < 256 && tryInsertChar((byte)state.ascii, _caretPos)) {
for (uint32 i = _highlightPos + _highlightCharacterCount; i > (uint32)_highlightPos; i--) {
_editString.deleteChar(i - 1);

This comment has been minimized.

@sev-

sev- Apr 19, 2021
Member

please use Common::String::erase(uint32 pos, uint32 len) instead of this loop.

@@ -341,8 +441,8 @@ void EditableWidget::drawCaret(bool erase) {
width = MIN(editRect.width() - caretOffset, width);
if (width > 0) {
g_gui.theme()->drawText(Common::Rect(x, y, x + width, y + editRect.height()), character,
_state, _drawAlign, _inversion, 0, false, _font,

This comment has been minimized.

@sev-

sev- Apr 19, 2021
Member

This whitespace change should be reverted, or put into a separate commit

_highlightString.clear();
// insert blank characters
for (int i = 0; i < (int)_caretPos; i++)
_highlightString.insertChar(' ', i);

This comment has been minimized.

@sev-

sev- Apr 19, 2021
Member

What is the logic behind this?

This comment has been minimized.

@srikeerthis

srikeerthis Apr 19, 2021
Author

I have tried using append() method to add characters, but had difficulty in reversing the string and displaying the same. So the logic thought was to have an empty temporary string equal to the size of the caret position and add characters from the end of the temporary string.

@sev-
Copy link
Member

@sev- sev- commented Apr 19, 2021

Screenshot 2021-04-19 at 13 36 29

Screenshot of glitched highlight. The input string is "kyra1"

@sev-
Copy link
Member

@sev- sev- commented Jun 7, 2021

Closing this due to inactivity, lack of handling of the edge cases.

@sev- sev- closed this Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants