Skip to content

Commit

Permalink
Fix issue with Focus call crashing
Browse files Browse the repository at this point in the history 
Fixes #1893
  • Loading branch information
@andydotxyz
andydotxyz committed Feb 4, 2021
1 parent 28eb944 commit 3b7e52a
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 1 deletion.
12 changes: 12 additions & 0 deletions internal/app/focus_manager.go
View file
Expand Up @@ -59,6 +59,18 @@ func (f *FocusManager) Focus(obj fyne.Focusable) bool {
return true
}

// FocusBeforeAdded allows an object to be focused before it is added to the object tree.
// This is typically used before a canvas is visible and should be used with care.
func (f *FocusManager) FocusBeforeAdded(obj fyne.Focusable) {
f.RLock()
defer f.RUnlock()
if dis, ok := obj.(fyne.Disableable); ok && dis.Disabled() {
return
}

f.focus(obj)
}

// Focused returns the currently focused object or nil if none.
func (f *FocusManager) Focused() fyne.Focusable {
f.RLock()
Expand Down
13 changes: 12 additions & 1 deletion internal/driver/glfw/canvas.go
View file
Expand Up @@ -84,13 +84,17 @@ func (c *glCanvas) Focus(obj fyne.Focusable) {
c.RUnlock()

for _, mgr := range focusMgrs {
if mgr == nil {
continue
}
if focusMgr != mgr {
if mgr.Focus(obj) {
return
}
}
}
fyne.LogError("Failed to focus object which is not part of the canvas’ content, menu or overlays.", nil)

c.contentFocusMgr.FocusBeforeAdded(obj) // not found yet assume we are preparing the UI
}

func (c *glCanvas) Focused() fyne.Focusable {
Expand Down Expand Up @@ -452,7 +456,14 @@ func (c *glCanvas) paint(size fyne.Size) {
func (c *glCanvas) setContent(content fyne.CanvasObject) {
c.content = content
c.contentTree = &renderCacheTree{root: &renderCacheNode{obj: c.content}}
var focused fyne.Focusable
if c.contentFocusMgr != nil {
focused = c.contentFocusMgr.Focused() // keep old focus if possible
}
c.contentFocusMgr = app.NewFocusManager(c.content)
if focused != nil {
c.contentFocusMgr.Focus(focused)
}
}

func (c *glCanvas) setDirty(dirty bool) {
Expand Down
11 changes: 11 additions & 0 deletions internal/driver/glfw/canvas_test.go
View file
Expand Up @@ -204,6 +204,17 @@ func TestGlCanvas_Focus(t *testing.T) {
assert.True(t, o2e.focused)
}

func TestGlCanvas_Focus_BeforeVisible(t *testing.T) {
w := createWindow("Test")
w.SetPadded(false)
e := widget.NewEntry()
c := w.Canvas().(*glCanvas)
c.Focus(e) // this crashed in the past

w.SetContent(e)
assert.Equal(t, e, c.Focused(), "Item set to focus before SetContent was not focused after")
}

func TestGlCanvas_FocusHandlingWhenAddingAndRemovingOverlays(t *testing.T) {
w := createWindow("Test")
w.SetPadded(false)
Expand Down

6 comments on commit 3b7e52a

@ababo
Copy link

@ababo ababo commented on 3b7e52a Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will it be merged into develop?

@andydotxyz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know. I am currently in a private chat with @toaster who is not convinced that this is a good fix.

@ababo
Copy link

@ababo ababo commented on 3b7e52a Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for my case.

@toaster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My objections are not about if it works or not. The problem is that the ability to focus an object that is not part of the tree opens pitfalls. If you have to add a comment about being careful to some code, it is most probably better to not add this code at all.

@toaster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S.: Especially if the code and its comment are hidden in some private type whereas the public interface does not include this warning.

@andydotxyz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean to imply that @toaster thought it did not work, sorry.

Whether or not this should be supported is an interesting question. However what is more important is:
"This used to work, did we mean to break it in 2.0.0"
to which the answer is clearly, No - this was an accident.

So do we want to proagate the break, and put it in the notes for all breaking changes in 2.0 upgrade notes, or should we fix it beacause it has been supported in the past.

Without this feature a develop that wishes an element to be focused will have to get a callback from the main() of their app to say that it has set the window, so the focusing can happen. This feels like it is bleeding different concerns of the app.

Please sign in to comment.