FIX: Resolve keypresslock race condition between multiple TextBox widgets with toolmanager#31268
Closed
herley-shaori wants to merge 2 commits intomatplotlib:mainfrom
Closed
Conversation
…gets with toolmanager (matplotlib#29687) Problem ------- When multiple TextBox widgets are used with toolmanager active (QtAgg, TkAgg), rapidly clicking from one TextBox to another intermittently raises: ValueError: already locked Root cause: button_press_event handlers are dispatched in widget-creation order, not in click order. When TextBox TB2 currently holds the keypresslock (it is the active/typing widget) and the user clicks on TB1, TB1's _click handler fires *before* TB2's _click handler. TB1 calls begin_typing() and tries to acquire the keypresslock while TB2 still holds it, causing the crash. TB2's _click handler — which would have called stop_typing() and released the lock — fires too late. Fix --- Two changes to lib/matplotlib/widgets.py: 1. LockDraw.owner property (new) Added a public `owner` property to `LockDraw` that exposes the current lock holder. This gives callers a safe, API-level way to identify who holds the lock without reaching into private state. 2. TextBox.begin_typing() — graceful lock transfer Before acquiring the keypresslock, begin_typing() now checks whether the lock is already held by a *different* widget. If so, it explicitly calls stop_typing() on that widget, which releases the lock and resets the previous owner's typing state in a clean, idempotent way. When the previous owner's _click handler later fires stop_typing(), it is a no-op because capturekeystrokes is already False. Test ---- Added test_TextBox_keypresslock_race_condition in test_widgets.py that directly reproduces the race scenario: TB2 acquires the lock via begin_typing(), then TB1 calls begin_typing() without TB2 having called stop_typing() first. Asserts that the lock is transferred cleanly to TB1 and that TB2's subsequent stop_typing() call is a no-op.
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. You can also join us on gitter for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
The `owner` property added to `LockDraw` in the previous commit was missing
from the type stub `lib/matplotlib/widgets.pyi`, causing mypy-stubtest to
report:
error: matplotlib.widgets.LockDraw.owner is not present in stub
Added the corresponding property declaration to the stub so the runtime
implementation and the stub are in sync.
Contributor
|
LLM output will not be reviewed. Repeated offenses will lead to banning. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #29687.
When multiple
TextBoxwidgets are used withtoolmanageractive (reproduced onQtAggandTkAgg), rapidly clicking from one TextBox to another intermittently raises:Root Cause Analysis
button_press_eventhandlers are dispatched to all connected widgets in widget-creation order, not in click order. Consider two TextBox widgets,TB1(created first) andTB2(created second, currently active/typing):TB1whileTB2holds thekeypresslock.TB1._clickfires first (creation order) → callsbegin_typing()→ attempts to acquirekeypresslock→ crash, becauseTB2still holds it.TB2._clickfires second → would have calledstop_typing()and released the lock, but it is already too late.The instrumented trace from the issue reporter confirms this ordering:
Fix
Two targeted changes to
lib/matplotlib/widgets.py:1.
LockDraw.ownerproperty (new)Exposes the current lock holder through a clean public API, avoiding the need for callers to reach into private
_ownerstate.2.
TextBox.begin_typing()— graceful lock transferBefore acquiring the
keypresslock,begin_typing()now checks whether another widget currently holds the lock. If so, it callsstop_typing()on that widget directly — releasing the lock and resetting the previous owner's typing state cleanly. When the previous owner's_clickhandler eventually firesstop_typing(), it becomes a no-op becausecapturekeystrokesis alreadyFalse.This implements the "force-grab focus" mechanism suggested by @anntzer in the issue discussion.
Test
Added
test_TextBox_keypresslock_race_conditionintest_widgets.pythat directly reproduces the race scenario:TB2acquires the lock viabegin_typing().TB1callsbegin_typing()withoutTB2having calledstop_typing()first (simulating the event-order race).TB1.TB2's subsequentstop_typing()is a no-op (does not raise, does not affectTB1's lock).TB1.stop_typing().Checklist
TextBoxtests pass (test_TextBox[none],test_TextBox[toolbar2],test_TextBox[toolmanager])LockDraw.ownerproperty