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
Bugfix #948: support focus traversal in dialogs #1318
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
prevFocusMgr.FocusLost() | ||
focusMgr.FocusGained() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for prevFocusMgr == focusMgr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! It might be possible with concurrent changes to the overlay stack.
I’ll add missing locking to the stack and also change the callback to be safe by design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of locking, what if you just return early if they are equal?
This makes `overlayChanged` robust against concurrent changes to the overlay stack. The locking, that was introduced into the stack earlier, already ensured `focusMgr != prevFocusMgr` but using explicit values from the caller is a more elegant approach which does not rely on caller’s lock.
@@ -386,6 +384,20 @@ func (c *glCanvas) objectTrees() []fyne.CanvasObject { | |||
return trees | |||
} | |||
|
|||
func (c *glCanvas) overlayChanged(focusMgr, prevFocusMgr *app.FocusManager) { | |||
c.Lock() | |||
defer c.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It might be better to not use a defer here, and instead move c.Unlock()
down to after line 396. In case dev is doing something weird in Focus{Lost|Gained} which could cause deadlock
Description:
This PR introduces
FocusManager
s per overlay and the according handling in theglCanvas
. Thus it is possible to traverseFocusable
s in dialogs (and other overlays).Fixes #948
Checklist: