Repository navigation
fix: keep Lua autocomplete popup from stealing editor focus - #10780
Conversation
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
|
Too many files changed for review (1487 files, 100 file limit). |
The header lives at edbee/views/components/texteditorcomponent.h, not edbee/texteditorcomponent.h, which broke the compile on every platform.
824f4e9 to
bfbdbdf
Compare
|
Small update: rebased onto current Root cause in one line: edbee's autocomplete list calls Happy to tweak anything after review — |
|
Thanks for working on this. Of the three open attempts at #5310, this is the one whose approach holds up. It works much like A quick note on expectations: #5310 has no bounty attached, so there is nothing to claim here. We'd still be glad to merge the fix. Blocking: stack overflow on arrow keys at the ends of the list Pressing Up on the first suggestion crashes Mudlet. edbee selects row 0 every time it fills the list, so typing
The cause:
Reproduced with the harness (exit code 139). gdb shows about 59k frames of A re-entrancy guard in the KeyPress branch fixes it. In the same harness, all four edge cases stopped crashing and normal navigation was unaffected: // a key the list ignored comes back up to its parent, this menu - drop it
if (mRoutingAutoCompleteKey) {
return true;
}
QScopedValueRollback<bool> guard(mRoutingAutoCompleteKey, true);
routeAutoCompleteKeyPress(static_cast<QKeyEvent*>(pEvent));Forwarding every key straight to the list still needs this guard, because the loop comes from the list's parent being the filtered menu. Smaller items
The original symptom doesn't reproduce on Linux offscreen, so a manual check on Windows would still be good before merge. Generated by Claude Code |
Pressing Up on the first suggestion, Down on the last, or any arrow key when there is a single suggestion, crashed Mudlet with a stack overflow. dlgSourceEditorArea routes a key press received by the autocomplete popup either to the suggestion list or to the editor. edbee's own filter lets Up/Down/PageUp/PageDown through to the list so QAbstractItemView can move the selection, and a key that cannot move it - which is every one of those four at the end of the list - is left unhandled. QApplication::notify() then passes an ignored key on to the next widget up the parent chain, and edbee parents the list to the popup QMenu, which is the very widget the routing filter is installed on. The key therefore came back up to the filter already routed once, and routing it to the list again repeated until the stack ran out. Drop such a key instead, guarded by a flag that is only set while a key is being routed. Keys the list can act on are unaffected, so navigation and acceptance still work. Adds EditorAutoCompleteFocusTest, which delivers keys through the popup's own QWindow so Qt's popup routing is what is exercised - a test built on QApplication::sendEvent() to a widget would skip that path and pass either way. It covers the four end-of-list cases, checks that the keyboard focus stays on the editor, and keeps a case that navigation still moves the selection so a filter that dropped every key could not pass. Without this commit the new test ends the process with SIGSEGV. Assisted-by: opencode:opencode/space-bunny-free
Addresses the rest of the review on #10780. The autocomplete key routing moves out of dlgSourceEditorArea into EditorAutoCompleteFocusHandler, because two editors need it and the preferences dialog's preview was left stealing the focus as it stood. The script editor and the preview now each get the same treatment from the same class, so the contract cannot drift between them. - The FocusOut filter consumed every FocusOut while the popup was up, not just the synthetic one Qt sends with Qt::PopupFocusReason when a popup opens. Any other reason is a real focus change, and hiding it from the editor left it believing it still had the focus. Only PopupFocusReason is swallowed now. - Qt::WA_ShowWithoutActivating is only consulted for a top-level window - QWidget checks it in show() under isWindow() - and edbee parents the suggestion list to the menu, so setting it on the list had never done anything. It stays on the menu, where Qt does look at it, and is off the list so it no longer reads as if it were load-bearing there. The list keeps the focus proxy and Qt::NoFocus, which are what actually hold the keyboard in the editor. - edbeePreviewWidget gets a handler too, installed in the constructor so it holds even when loadEditorTab() returns early for want of a profile. - EditorAutoCompleteFocusTest gains cases for each of the three, and keeps the four end-of-list navigation cases driving keys through the popup's own QWindow. The FocusOut case works from a probe event filter installed before the handler, so it only sees the FocusOuts the handler let through: the popup's own is swallowed, OtherFocusReason and MouseFocusReason are not. - The test also stops including MudletPaths.h, which it never used. That header no longer exists on development (removed in 2ea5d46, #10472), and it was the only file in the merge tree still reaching for it, so it was breaking the build on every platform. Assisted-by: opencode:opencode/space-bunny-free
|
Hi! The CI workflows for this PR are waiting for approval (action_required). Could you please approve the workflow runs so the checks can execute? The PR is ready for review. Thanks! |
|
Hi @vadi2 — following up: the blocking stack-overflow issue and the smaller items you listed were addressed in |
…again (#11449) #### Brief overview of PR changes/additions Clicking into the Lua code area of the trigger, alias, script, timer, key or button editor gives it the keyboard again, and Tab reaches it. Before this fix, what you typed went to the Name field (silently renaming the item) or to the item tree. The autocomplete focus handler from #10780 made the editor the completion list's focus proxy and then set the list to `Qt::NoFocus`. `QWidget::setFocusPolicy()` also applies the policy to a widget's focus proxy, so the editor itself became `Qt::NoFocus`. The two calls are now swapped, so the list is non-focusable and the editor keeps `Qt::WheelFocus`. The same handler sits on the editor preview in Settings > Editor, which is fixed too. `EditorAutoCompleteFocusTest` gains a case that clicks into an editor next to a line edit holding the focus, and checks that the editor takes it. It also checks the focus policy on both the script editor and the preview. The existing cases used `setFocus()`, which works whatever the policy is, so they could not catch this. The new case fails without the fix and passes with it. #### Motivation for adding to Mudlet Users could not type Lua into an item at all, which blocks the core scripting workflow. This is a regression since 5.0.1 and is not in any release yet. #### Other info (issues closed, discussion etc) Closes #11445 Assisted-by: Claude Code 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01VgAGrxnNUmXfYH2ZsDdbCq --- _Generated by [Claude Code](https://claude.ai/code/session_01VgAGrxnNUmXfYH2ZsDdbCq)_ --------- Co-authored-by: Claude <[email protected]>
Brief overview of PR changes/additions
Keeps the keyboard focus with the source editor while the Lua autocomplete popup is open, instead of letting the popup take it.
dlgSourceEditorArea(the widget that owns and configures the edbee editor) now applies the same approach Qt's ownQCompleteruses for completion popups:QMenu) and itsQListWidgetare made non-focusable (Qt::NoFocus) and non-activating (WA_ShowWithoutActivating);listWidget()->setFocus()call into a no-op;Motivation for adding to Mudlet
Fixes #5310. Users intermittently lose the ability to type in the Lua editor while the autocomplete popup is showing; pressing Esc removes the popup, but it reappears on the next letter and typing stays blocked.
Root cause analysis (Qt event level)
In the bundled edbee-lib,
TextEditorAutoCompleteComponent::updateList()runs after every character typed/backspaced and does:menuRef_is aQMenu, i.e. aQt::Popupwindow. Two Qt behaviours then conspire against the editor:QWidget::isActiveWindow()unconditionally reports a visibleQt::Popupwindow as active, solistWidgetRef_->setFocus()really does moveQApplication::focusWidget()from the editor's text component to the list (QWidget::setFocus()does not consult the focus policy, so settingQt::NoFocusalone does not prevent this).QWidgetWindow::handleKeyEvent()delivers all keyboard events topopup->focusWidget()(or the popup itself) — never to the editor. The editor only receives text because edbee's event filter forwards letters back to it one by one, and every forwarded character re-entersupdateList()→popup()+setFocus()again. This focus ping-pong (plus the popup window activation churn thatWA_ShowWithoutActivatingis missing) loses keys and IME input on Windows, leaving the editor unusable until the popup is dismissed — matching the video in Autocomplete steals window focus, prevents further typing #5310.Since Qt delivers popup-mode keys to the popup widget, the fix routes them from there — the same place
QCompleterroutes them from (QCompleter::setPopup()setsQt::Popup+Qt::NoFocus+setFocusProxy(widget)on its popup, andQCompleter::eventFilter()forwards unhandled keys to the source widget):list->setFocusProxy(editorComponent)makes edbee'ssetFocus()provably do nothing:QWidget::setFocus()resolves it to the deepest focus proxy (the editor component), seesQApplication::focusWidget()already equals it, and returns early — no FocusOut/FocusIn, no state change.WA_ShowWithoutActivatingon the popup window stops the window-activation steal (notably on Windows), so the main window stays active on every popup refresh.QCompleter::eventFilter()'s routing, preserving edbee's existing key semantics exactly (letters/digits/Backspace keep the popup open, Up/Down/PageUp/PageDown navigate the list through edbee's own list filter, Enter/Tab accept, Escape setscanceled_so the popup stays dismissed until a new word is typed, other keys close the popup and reach the editor).FocusOutthatQApplicationPrivate::openPopup()sends to the current focus widget when the popup first shows — edbee'sTextEditorComponent::focusOutEvent()resets undo coalescing, which would otherwise fragment the undo stack on every popup appearance (this is the same reasonQCompleterhas itseatFocusOutflag).How to reproduce/test
Reproduce (on
development, Windows):ce(createButton, createGauge, ...).developmentfocus can end up in the popup and typing stops working (keys are swallowed or lost); the user must press Esc and the popup reappears on the next letter.With this PR:
ce: the popup appears and the editor keeps the caret/focus.createButton: text appears normally while the popup tracks the word; typing is never blocked.No local build was possible for this PR, so it is verified by code inspection against the Qt event flow described above; CI build + reviewer manual test are much appreciated.
Other info (issues closed, discussion etc)
Fixes #5310.
/claim #5310