GUI: Search box lose focus in grid launcher when typing text

[criezy]

In the Grid Launcher when typing text in the search box, if this causes games to be removed from the launcher the search box loses focus. We need to click again on the box to set the focus back so that we can continue typing.

The issues does not exist with the list launcher.

The issue exists both in the 2.7.0 release and in the current master code (2ec1487141d053d82e5c3fa4218de3fc265007da from July 19).

[criezy]

I don't know if this is related, but I have also hit the assert in the following code while testing this further. However I am unable to reproduce that.

bool EditableWidget::setCaretPos(int newPos) {
	assert(newPos >= 0 && newPos <= (int)_editString.size());
	_caretPos = newPos;
	return adjustOffset();
}

[criezy]

The issue with the loss of focus might be related to the scrollbar. If the scrollbar is at the top, then the focus is never lost, but if I scroll a bit down and then type in the search box, it sometimes loses focus. This seems to happen systematically if the scrollbar is at the end.

[criezy]

And here is the call stack for the assert:

3   libsystem_c.dylib             	       0x181d41754 __assert_rtn + 272
4   scummvm                       	       0x104dbdd54 GUI::EditableWidget::setCaretPos(int) + 152 (editable.cpp:565)
5   scummvm                       	       0x104dc57bc GUI::EditableWidget::defaultKeyDownHandler(Common::KeyState&, bool&, bool&, bool&) + 1892 (editable.cpp:457)
6   scummvm                       	       0x104dc4e04 GUI::EditableWidget::handleKeyDown(Common::KeyState) + 12376 (editable.cpp:434)
7   scummvm                       	       0x104c66688 GUI::Dialog::handleKeyDown(Common::KeyState) + 916 (dialog.cpp:265)
8   scummvm                       	       0x104c9f224 GUI::LauncherGrid::handleKeyDown(Common::KeyState) + 968 (launcher.cpp:1430)
9   scummvm                       	       0x104c80ed4 GUI::GuiManager::processEvent(Common::Event const&, GUI::Dialog*) + 1732 (gui-manager.cpp:753)
10  scummvm                       	       0x104c7ebc8 GUI::GuiManager::runLoop() + 2288 (gui-manager.cpp:498)
11  scummvm                       	       0x104c8a32c GUI::LauncherDialog::run() + 148 (launcher.cpp:322)
12  scummvm                       	       0x104c94118 GUI::LauncherChooser::runModal() + 156 (launcher.cpp:1013)

The strange part is that it is in part of the code that replace selected text with what I typed. But I don't remember selecting any text. In any case I suspect this is entirely unrelated to the focus issue.

[criezy]

The assert may be fixed by commit 530a972d4c630db89cb1f24e0ac2f9e4fe3532cd.
The reproduction case I had for that commit was hitting a different assert, but I think the same issue could potentially also have caused the callstack above.

[antoniou79]

I don't know if this is helpful at all but my guess, back when I noticed this, was that this line was responsible for the loss of focus:
​https://github.com/scummvm/scummvm/blob/be0e263d2410ba812014671e765fc40e718a6c7e/gui/widgets/grid.cpp#L774

[PushmePullyu]

The assert should be fixed with commit 4867e14d604a2556f77a036d9e5eff0477bd26b8. Since you mentioned that you made no selection, the mouse was probably moving when the LMB was released after clicking the search box. This is interpreted as dragging the mouse to make a selection and sets _selCaretPos (see bug #14584).

The loss of focus seems to be caused by these events:

Typing in the search box leads to a call to GridWidget::setFilter(), which does the following:

_scrollPos = 0;
...
sortGroups();

In sortGroups():

_scrollBar->checkBounds(_scrollBar->_currentPos);

When the filter removes grid entries, _scrollBar->_currentPos can now be larger than the position of the last entry, and so checkBounds() resets it and sends a kSetPositionCmd. This is more likely to happen if _currentPos is large, i.e. the view is scrolled further towards the end.

As antoniou79 stated, the command then causes GridWidget::handleCommand() to set focus to the grid widget.

Since setFilter() already sets _scrollPos to 0 this could be fixed by propagating the change to _scrollBar->_currentPos before calling checkBounds().

edit: PR at ​https://github.com/scummvm/scummvm/pull/5307

[bluegr]

The PR has been merged