From 4ff05196c39a865beb5c11542ba5cbc0dc30cfa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilo=20Pr=C3=BCtz?= Date: Mon, 14 Sep 2020 10:39:05 +0200 Subject: [PATCH 1/7] name glCanvas test methods consistent (and sort them) --- internal/driver/glfw/canvas_test.go | 416 ++++++++++++++-------------- 1 file changed, 208 insertions(+), 208 deletions(-) diff --git a/internal/driver/glfw/canvas_test.go b/internal/driver/glfw/canvas_test.go index cdc7b7a0c6..7e74dc7a19 100644 --- a/internal/driver/glfw/canvas_test.go +++ b/internal/driver/glfw/canvas_test.go @@ -15,176 +15,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestGlCanvas_Content(t *testing.T) { - content := &canvas.Circle{} - w := createWindow("Test") - w.SetContent(content) - - assert.Equal(t, content, w.Content()) -} - -func TestGlCanvas_NilContent(t *testing.T) { - w := createWindow("Test") - - assert.NotNil(t, w.Content()) // never a nil canvas so we have a sensible fallback -} - -func TestGlCanvas_Resize(t *testing.T) { - w := createWindow("Test") - w.SetPadded(false) - - content := widget.NewLabel("Content") - w.SetContent(content) - - size := fyne.NewSize(200, 100) - assert.NotEqual(t, size, content.Size()) - - w.Resize(size) - assert.Equal(t, size, content.Size()) -} - -// TODO: this can be removed when #707 is addressed -func TestGlCanvas_ResizeWithPopUpOverlay(t *testing.T) { - w := createWindow("Test") - w.SetPadded(false) - - content := widget.NewLabel("Content") - over := widget.NewPopUp(widget.NewLabel("Over"), w.Canvas()) - w.SetContent(content) - w.Canvas().Overlays().Add(over) - - size := fyne.NewSize(200, 100) - overContentSize := over.Content.Size() - assert.NotEqual(t, size, content.Size()) - assert.NotEqual(t, size, over.Size()) - assert.NotEqual(t, size, overContentSize) - - w.Resize(size) - assert.Equal(t, size, content.Size(), "canvas content is resized") - assert.Equal(t, size, over.Size(), "canvas overlay is resized") - assert.Equal(t, overContentSize, over.Content.Size(), "canvas overlay content is _not_ resized") -} - -// TODO: this can be removed when #707 is addressed -func TestGlCanvas_ResizeWithOtherOverlay(t *testing.T) { - w := createWindow("Test") - w.SetPadded(false) - - content := widget.NewLabel("Content") - over := widget.NewLabel("Over") - w.SetContent(content) - w.Canvas().SetOverlay(over) - // TODO: address #707; overlays should always be canvas size - over.Resize(w.Canvas().Size()) - - size := fyne.NewSize(200, 100) - assert.NotEqual(t, size, content.Size()) - assert.NotEqual(t, size, over.Size()) - - w.Resize(size) - assert.Equal(t, size, content.Size(), "canvas content is resized") - assert.Equal(t, size, over.Size(), "canvas overlay is resized") -} - -func TestGlCanvas_ResizeWithOverlays(t *testing.T) { - w := createWindow("Test") - w.SetPadded(false) - - content := widget.NewLabel("Content") - o1 := widget.NewLabel("o1") - o2 := widget.NewLabel("o2") - o3 := widget.NewLabel("o3") - w.SetContent(content) - w.Canvas().Overlays().Add(o1) - // TODO: address #707; overlays should always be canvas size - o1.Resize(w.Canvas().Size()) - w.Canvas().Overlays().Add(o2) - // TODO: address #707; overlays should always be canvas size - o2.Resize(w.Canvas().Size()) - w.Canvas().Overlays().Add(o3) - // TODO: address #707; overlays should always be canvas size - o3.Resize(w.Canvas().Size()) - - size := fyne.NewSize(200, 100) - assert.NotEqual(t, size, content.Size()) - assert.NotEqual(t, size, o1.Size()) - assert.NotEqual(t, size, o2.Size()) - assert.NotEqual(t, size, o3.Size()) - - w.Resize(size) - assert.Equal(t, size, content.Size(), "canvas content is resized") - assert.Equal(t, size, o1.Size(), "canvas overlay 1 is resized") - assert.Equal(t, size, o2.Size(), "canvas overlay 2 is resized") - assert.Equal(t, size, o3.Size(), "canvas overlay 3 is resized") -} - -func TestGlCanvas_Scale(t *testing.T) { - w := createWindow("Test").(*window) - c := w.Canvas().(*glCanvas) - - c.scale = 2.5 - assert.Equal(t, 5, int(2*c.Scale())) -} - -func TestGlCanvas_PixelCoordinateAtPosition(t *testing.T) { - w := createWindow("Test").(*window) - c := w.Canvas().(*glCanvas) - - pos := fyne.NewPos(4, 4) - c.scale = 2.5 - x, y := c.PixelCoordinateForPosition(pos) - assert.Equal(t, int(10*c.texScale), x) - assert.Equal(t, int(10*c.texScale), y) - - c.texScale = 2.0 - x, y = c.PixelCoordinateForPosition(pos) - assert.Equal(t, 20, x) - assert.Equal(t, 20, y) -} - -func Test_glCanvas_SetContent(t *testing.T) { - fyne.CurrentApp().Settings().SetTheme(theme.DarkTheme()) - var menuHeight int - if hasNativeMenu() { - menuHeight = 0 - } else { - menuHeight = NewMenuBar(fyne.NewMainMenu(fyne.NewMenu("Test", fyne.NewMenuItem("Empty", func() {}))), nil).MinSize().Height - } - tests := []struct { - name string - padding bool - menu bool - expectedPad int - expectedMenuHeight int - }{ - {"window without padding", false, false, 0, 0}, - {"window with padding", true, false, theme.Padding(), 0}, - {"window with menu without padding", false, true, 0, menuHeight}, - {"window with menu and padding", true, true, theme.Padding(), menuHeight}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - w := createWindow("Test").(*window) - w.SetPadded(tt.padding) - if tt.menu { - w.SetMainMenu(fyne.NewMainMenu(fyne.NewMenu("Test", fyne.NewMenuItem("Test", func() {})))) - } - content := canvas.NewCircle(color.Black) - canvasSize := 200 - w.SetContent(content) - w.Resize(fyne.NewSize(canvasSize, canvasSize)) - - newContent := canvas.NewCircle(color.White) - assert.Equal(t, fyne.NewPos(0, 0), newContent.Position()) - assert.Equal(t, fyne.NewSize(0, 0), newContent.Size()) - w.SetContent(newContent) - assert.Equal(t, fyne.NewPos(tt.expectedPad, tt.expectedPad+tt.expectedMenuHeight), newContent.Position()) - assert.Equal(t, fyne.NewSize(canvasSize-2*tt.expectedPad, canvasSize-2*tt.expectedPad-tt.expectedMenuHeight), newContent.Size()) - }) - } -} - -func Test_glCanvas_ChildMinSizeChangeAffectsAncestorsUpToRoot(t *testing.T) { +func TestGlCanvas_ChildMinSizeChangeAffectsAncestorsUpToRoot(t *testing.T) { w := createWindow("Test").(*window) c := w.Canvas().(*glCanvas) leftObj1 := canvas.NewRectangle(color.Black) @@ -212,7 +43,7 @@ func Test_glCanvas_ChildMinSizeChangeAffectsAncestorsUpToRoot(t *testing.T) { assert.Equal(t, expectedCanvasSize, c.Size()) } -func Test_glCanvas_ChildMinSizeChangeAffectsAncestorsUpToScroll(t *testing.T) { +func TestGlCanvas_ChildMinSizeChangeAffectsAncestorsUpToScroll(t *testing.T) { w := createWindow("Test").(*window) c := w.Canvas().(*glCanvas) leftObj1 := canvas.NewRectangle(color.Black) @@ -247,7 +78,7 @@ func Test_glCanvas_ChildMinSizeChangeAffectsAncestorsUpToScroll(t *testing.T) { assert.Equal(t, expectedRightColSize, rightCol.Size()) } -func Test_glCanvas_ChildMinSizeChangesInDifferentScrollAffectAncestorsUpToScroll(t *testing.T) { +func TestGlCanvas_ChildMinSizeChangesInDifferentScrollAffectAncestorsUpToScroll(t *testing.T) { w := createWindow("Test").(*window) c := w.Canvas().(*glCanvas) leftObj1 := canvas.NewRectangle(color.Black) @@ -291,44 +122,15 @@ func Test_glCanvas_ChildMinSizeChangesInDifferentScrollAffectAncestorsUpToScroll assert.Equal(t, expectedRightColSize, rightCol.Size()) } -func Test_glCanvas_MinSizeShrinkTriggersLayout(t *testing.T) { - w := createWindow("Test").(*window) - c := w.Canvas().(*glCanvas) - leftObj1 := canvas.NewRectangle(color.Black) - leftObj1.SetMinSize(fyne.NewSize(100, 50)) - leftObj2 := canvas.NewRectangle(color.Black) - leftObj2.SetMinSize(fyne.NewSize(100, 50)) - leftCol := widget.NewVBox(leftObj1, leftObj2) - rightObj1 := canvas.NewRectangle(color.Black) - rightObj1.SetMinSize(fyne.NewSize(100, 50)) - rightObj2 := canvas.NewRectangle(color.Black) - rightObj2.SetMinSize(fyne.NewSize(100, 50)) - rightCol := widget.NewVBox(rightObj1, rightObj2) - content := widget.NewHBox(leftCol, rightCol) +func TestGlCanvas_Content(t *testing.T) { + content := &canvas.Circle{} + w := createWindow("Test") w.SetContent(content) - oldCanvasSize := fyne.NewSize(200+3*theme.Padding(), 100+3*theme.Padding()) - assert.Equal(t, oldCanvasSize, c.Size()) - repaintWindow(w) - - oldRightColSize := rightCol.Size() - leftObj1.SetMinSize(fyne.NewSize(90, 40)) - rightObj1.SetMinSize(fyne.NewSize(80, 30)) - rightObj2.SetMinSize(fyne.NewSize(80, 20)) - c.Refresh(leftObj1) - c.Refresh(rightObj1) - c.Refresh(rightObj2) - repaintWindow(w) - - assert.Equal(t, oldCanvasSize, c.Size()) - expectedRightColSize := oldRightColSize.Subtract(fyne.NewSize(20, 0)) - assert.Equal(t, expectedRightColSize, rightCol.Size()) - assert.Equal(t, fyne.NewSize(100, 40), leftObj1.Size()) - assert.Equal(t, fyne.NewSize(80, 30), rightObj1.Size()) - assert.Equal(t, fyne.NewSize(80, 20), rightObj2.Size()) + assert.Equal(t, content, w.Content()) } -func Test_glCanvas_ContentChangeWithoutMinSizeChangeDoesNotLayout(t *testing.T) { +func TestGlCanvas_ContentChangeWithoutMinSizeChangeDoesNotLayout(t *testing.T) { w := createWindow("Test").(*window) c := w.Canvas().(*glCanvas) leftObj1 := canvas.NewRectangle(color.Black) @@ -362,7 +164,7 @@ func Test_glCanvas_ContentChangeWithoutMinSizeChangeDoesNotLayout(t *testing.T) assert.Nil(t, layout.popLayoutEvent()) } -func Test_glCanvas_InsufficientSizeDoesntTriggerResizeIfSizeIsAlreadyMaxedOut(t *testing.T) { +func TestGlCanvas_InsufficientSizeDoesntTriggerResizeIfSizeIsAlreadyMaxedOut(t *testing.T) { w := createWindow("Test").(*window) c := w.Canvas().(*glCanvas) w.Resize(fyne.NewSize(200, 100)) @@ -384,7 +186,205 @@ func Test_glCanvas_InsufficientSizeDoesntTriggerResizeIfSizeIsAlreadyMaxedOut(t assert.Equal(t, fyne.NewSize(200, 100), popUp.Size()) } -func Test_glCanvas_walkTree(t *testing.T) { +func TestGlCanvas_MinSizeShrinkTriggersLayout(t *testing.T) { + w := createWindow("Test").(*window) + c := w.Canvas().(*glCanvas) + leftObj1 := canvas.NewRectangle(color.Black) + leftObj1.SetMinSize(fyne.NewSize(100, 50)) + leftObj2 := canvas.NewRectangle(color.Black) + leftObj2.SetMinSize(fyne.NewSize(100, 50)) + leftCol := widget.NewVBox(leftObj1, leftObj2) + rightObj1 := canvas.NewRectangle(color.Black) + rightObj1.SetMinSize(fyne.NewSize(100, 50)) + rightObj2 := canvas.NewRectangle(color.Black) + rightObj2.SetMinSize(fyne.NewSize(100, 50)) + rightCol := widget.NewVBox(rightObj1, rightObj2) + content := widget.NewHBox(leftCol, rightCol) + w.SetContent(content) + + oldCanvasSize := fyne.NewSize(200+3*theme.Padding(), 100+3*theme.Padding()) + assert.Equal(t, oldCanvasSize, c.Size()) + repaintWindow(w) + + oldRightColSize := rightCol.Size() + leftObj1.SetMinSize(fyne.NewSize(90, 40)) + rightObj1.SetMinSize(fyne.NewSize(80, 30)) + rightObj2.SetMinSize(fyne.NewSize(80, 20)) + c.Refresh(leftObj1) + c.Refresh(rightObj1) + c.Refresh(rightObj2) + repaintWindow(w) + + assert.Equal(t, oldCanvasSize, c.Size()) + expectedRightColSize := oldRightColSize.Subtract(fyne.NewSize(20, 0)) + assert.Equal(t, expectedRightColSize, rightCol.Size()) + assert.Equal(t, fyne.NewSize(100, 40), leftObj1.Size()) + assert.Equal(t, fyne.NewSize(80, 30), rightObj1.Size()) + assert.Equal(t, fyne.NewSize(80, 20), rightObj2.Size()) +} + +func TestGlCanvas_NilContent(t *testing.T) { + w := createWindow("Test") + + assert.NotNil(t, w.Content()) // never a nil canvas so we have a sensible fallback +} + +func TestGlCanvas_PixelCoordinateAtPosition(t *testing.T) { + w := createWindow("Test").(*window) + c := w.Canvas().(*glCanvas) + + pos := fyne.NewPos(4, 4) + c.scale = 2.5 + x, y := c.PixelCoordinateForPosition(pos) + assert.Equal(t, int(10*c.texScale), x) + assert.Equal(t, int(10*c.texScale), y) + + c.texScale = 2.0 + x, y = c.PixelCoordinateForPosition(pos) + assert.Equal(t, 20, x) + assert.Equal(t, 20, y) +} + +func TestGlCanvas_Resize(t *testing.T) { + w := createWindow("Test") + w.SetPadded(false) + + content := widget.NewLabel("Content") + w.SetContent(content) + + size := fyne.NewSize(200, 100) + assert.NotEqual(t, size, content.Size()) + + w.Resize(size) + assert.Equal(t, size, content.Size()) +} + +// TODO: this can be removed when #707 is addressed +func TestGlCanvas_ResizeWithOtherOverlay(t *testing.T) { + w := createWindow("Test") + w.SetPadded(false) + + content := widget.NewLabel("Content") + over := widget.NewLabel("Over") + w.SetContent(content) + w.Canvas().SetOverlay(over) + // TODO: address #707; overlays should always be canvas size + over.Resize(w.Canvas().Size()) + + size := fyne.NewSize(200, 100) + assert.NotEqual(t, size, content.Size()) + assert.NotEqual(t, size, over.Size()) + + w.Resize(size) + assert.Equal(t, size, content.Size(), "canvas content is resized") + assert.Equal(t, size, over.Size(), "canvas overlay is resized") +} + +func TestGlCanvas_ResizeWithOverlays(t *testing.T) { + w := createWindow("Test") + w.SetPadded(false) + + content := widget.NewLabel("Content") + o1 := widget.NewLabel("o1") + o2 := widget.NewLabel("o2") + o3 := widget.NewLabel("o3") + w.SetContent(content) + w.Canvas().Overlays().Add(o1) + // TODO: address #707; overlays should always be canvas size + o1.Resize(w.Canvas().Size()) + w.Canvas().Overlays().Add(o2) + // TODO: address #707; overlays should always be canvas size + o2.Resize(w.Canvas().Size()) + w.Canvas().Overlays().Add(o3) + // TODO: address #707; overlays should always be canvas size + o3.Resize(w.Canvas().Size()) + + size := fyne.NewSize(200, 100) + assert.NotEqual(t, size, content.Size()) + assert.NotEqual(t, size, o1.Size()) + assert.NotEqual(t, size, o2.Size()) + assert.NotEqual(t, size, o3.Size()) + + w.Resize(size) + assert.Equal(t, size, content.Size(), "canvas content is resized") + assert.Equal(t, size, o1.Size(), "canvas overlay 1 is resized") + assert.Equal(t, size, o2.Size(), "canvas overlay 2 is resized") + assert.Equal(t, size, o3.Size(), "canvas overlay 3 is resized") +} + +// TODO: this can be removed when #707 is addressed +func TestGlCanvas_ResizeWithPopUpOverlay(t *testing.T) { + w := createWindow("Test") + w.SetPadded(false) + + content := widget.NewLabel("Content") + over := widget.NewPopUp(widget.NewLabel("Over"), w.Canvas()) + w.SetContent(content) + w.Canvas().Overlays().Add(over) + + size := fyne.NewSize(200, 100) + overContentSize := over.Content.Size() + assert.NotEqual(t, size, content.Size()) + assert.NotEqual(t, size, over.Size()) + assert.NotEqual(t, size, overContentSize) + + w.Resize(size) + assert.Equal(t, size, content.Size(), "canvas content is resized") + assert.Equal(t, size, over.Size(), "canvas overlay is resized") + assert.Equal(t, overContentSize, over.Content.Size(), "canvas overlay content is _not_ resized") +} + +func TestGlCanvas_Scale(t *testing.T) { + w := createWindow("Test").(*window) + c := w.Canvas().(*glCanvas) + + c.scale = 2.5 + assert.Equal(t, 5, int(2*c.Scale())) +} + +func TestGlCanvas_SetContent(t *testing.T) { + fyne.CurrentApp().Settings().SetTheme(theme.DarkTheme()) + var menuHeight int + if hasNativeMenu() { + menuHeight = 0 + } else { + menuHeight = NewMenuBar(fyne.NewMainMenu(fyne.NewMenu("Test", fyne.NewMenuItem("Empty", func() {}))), nil).MinSize().Height + } + tests := []struct { + name string + padding bool + menu bool + expectedPad int + expectedMenuHeight int + }{ + {"window without padding", false, false, 0, 0}, + {"window with padding", true, false, theme.Padding(), 0}, + {"window with menu without padding", false, true, 0, menuHeight}, + {"window with menu and padding", true, true, theme.Padding(), menuHeight}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := createWindow("Test").(*window) + w.SetPadded(tt.padding) + if tt.menu { + w.SetMainMenu(fyne.NewMainMenu(fyne.NewMenu("Test", fyne.NewMenuItem("Test", func() {})))) + } + content := canvas.NewCircle(color.Black) + canvasSize := 200 + w.SetContent(content) + w.Resize(fyne.NewSize(canvasSize, canvasSize)) + + newContent := canvas.NewCircle(color.White) + assert.Equal(t, fyne.NewPos(0, 0), newContent.Position()) + assert.Equal(t, fyne.NewSize(0, 0), newContent.Size()) + w.SetContent(newContent) + assert.Equal(t, fyne.NewPos(tt.expectedPad, tt.expectedPad+tt.expectedMenuHeight), newContent.Position()) + assert.Equal(t, fyne.NewSize(canvasSize-2*tt.expectedPad, canvasSize-2*tt.expectedPad-tt.expectedMenuHeight), newContent.Size()) + }) + } +} + +func TestGlCanvas_walkTree(t *testing.T) { leftObj1 := canvas.NewRectangle(color.Gray16{Y: 1}) leftObj2 := canvas.NewRectangle(color.Gray16{Y: 2}) leftCol := &modifiableBox{Box: widget.NewVBox(leftObj1, leftObj2)} From a082427cb65885c56a5b5524277522729cd5f1c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilo=20Pr=C3=BCtz?= Date: Mon, 14 Sep 2020 07:51:11 +0200 Subject: [PATCH 2/7] manage FocusManagers per overlay --- internal/driver/glfw/canvas.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/driver/glfw/canvas.go b/internal/driver/glfw/canvas.go index 62a8f5df6d..2ee31232b3 100644 --- a/internal/driver/glfw/canvas.go +++ b/internal/driver/glfw/canvas.go @@ -520,8 +520,9 @@ func (c *glCanvas) walkTrees( type overlayStack struct { internal.OverlayStack - onChange func() - renderCaches []*renderCacheTree + focusManagers []*app.FocusManager + onChange func() + renderCaches []*renderCacheTree } func (o *overlayStack) Add(overlay fyne.CanvasObject) { @@ -530,12 +531,15 @@ func (o *overlayStack) Add(overlay fyne.CanvasObject) { } o.OverlayStack.Add(overlay) o.renderCaches = append(o.renderCaches, &renderCacheTree{root: &renderCacheNode{obj: overlay}}) + o.focusManagers = append(o.focusManagers, app.NewFocusManager(overlay)) o.onChange() } func (o *overlayStack) Remove(overlay fyne.CanvasObject) { o.OverlayStack.Remove(overlay) - o.renderCaches = o.renderCaches[:len(o.List())] + overlayCount := len(o.List()) + o.renderCaches = o.renderCaches[:overlayCount] + o.focusManagers = o.focusManagers[:overlayCount] o.onChange() } From 70255c3849a3ad5c4f032b622c4d92a598344f4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilo=20Pr=C3=BCtz?= Date: Mon, 14 Sep 2020 15:12:20 +0200 Subject: [PATCH 3/7] fix #948: delegate focus handling for overlays to correct manager --- internal/driver/glfw/canvas.go | 36 +++++++++++++-- internal/driver/glfw/canvas_test.go | 72 +++++++++++++++++++++++++++++ internal/driver/glfw/window_test.go | 11 ++++- 3 files changed, 113 insertions(+), 6 deletions(-) diff --git a/internal/driver/glfw/canvas.go b/internal/driver/glfw/canvas.go index 2ee31232b3..02d84352f1 100644 --- a/internal/driver/glfw/canvas.go +++ b/internal/driver/glfw/canvas.go @@ -352,6 +352,9 @@ func (c *glCanvas) ensureMinSize() bool { func (c *glCanvas) focusManager() *app.FocusManager { c.RLock() defer c.RUnlock() + if focusMgr := c.overlays.TopFocusManager(); focusMgr != nil { + return focusMgr + } return c.focusMgr } @@ -386,6 +389,21 @@ func (c *glCanvas) objectTrees() []fyne.CanvasObject { return trees } +func (c *glCanvas) overlayChanged(prevFocusMgr *app.FocusManager) { + c.setDirty(true) + c.RLock() + if prevFocusMgr == nil { + prevFocusMgr = c.focusMgr + } + focusMgr := c.overlays.TopFocusManager() + if focusMgr == nil { + focusMgr = c.focusMgr + } + c.RUnlock() + prevFocusMgr.FocusLost() + focusMgr.FocusGained() +} + func (c *glCanvas) paint(size fyne.Size) { if c.Content() == nil { return @@ -521,7 +539,7 @@ type overlayStack struct { internal.OverlayStack focusManagers []*app.FocusManager - onChange func() + onChange func(prevFocusMgr *app.FocusManager) renderCaches []*renderCacheTree } @@ -529,18 +547,28 @@ func (o *overlayStack) Add(overlay fyne.CanvasObject) { if overlay == nil { return } + pfm := o.TopFocusManager() o.OverlayStack.Add(overlay) o.renderCaches = append(o.renderCaches, &renderCacheTree{root: &renderCacheNode{obj: overlay}}) o.focusManagers = append(o.focusManagers, app.NewFocusManager(overlay)) - o.onChange() + o.onChange(pfm) } func (o *overlayStack) Remove(overlay fyne.CanvasObject) { + top := o.TopFocusManager() o.OverlayStack.Remove(overlay) overlayCount := len(o.List()) o.renderCaches = o.renderCaches[:overlayCount] o.focusManagers = o.focusManagers[:overlayCount] - o.onChange() + o.onChange(top) +} + +func (o *overlayStack) TopFocusManager() *app.FocusManager { + var pfm *app.FocusManager + if len(o.focusManagers) > 0 { + pfm = o.focusManagers[len(o.focusManagers)-1] + } + return pfm } type renderCacheNode struct { @@ -568,7 +596,7 @@ func newCanvas() *glCanvas { c.setContent(&canvas.Rectangle{FillColor: theme.BackgroundColor()}) c.padded = true - c.overlays = &overlayStack{onChange: func() { c.setDirty(true) }} + c.overlays = &overlayStack{onChange: c.overlayChanged} c.refreshQueue = make(chan fyne.CanvasObject, 4096) c.dirtyMutex = &sync.Mutex{} diff --git a/internal/driver/glfw/canvas_test.go b/internal/driver/glfw/canvas_test.go index 7e74dc7a19..1bc61d6c50 100644 --- a/internal/driver/glfw/canvas_test.go +++ b/internal/driver/glfw/canvas_test.go @@ -9,6 +9,7 @@ import ( "fyne.io/fyne" "fyne.io/fyne/canvas" + "fyne.io/fyne/layout" "fyne.io/fyne/theme" "fyne.io/fyne/widget" @@ -164,6 +165,77 @@ func TestGlCanvas_ContentChangeWithoutMinSizeChangeDoesNotLayout(t *testing.T) { assert.Nil(t, layout.popLayoutEvent()) } +func TestGlCanvas_FocusHandlingWhenAddingAndRemovingOverlays(t *testing.T) { + w := createWindow("Test") + w.SetPadded(false) + c := w.Canvas().(*glCanvas) + + ce1 := &focusable{id: "ce1"} + ce2 := &focusable{id: "ce2"} + content := layout.NewVBoxContainer(ce1, ce2) + o1e1 := &focusable{id: "o1e1"} + o1e2 := &focusable{id: "o1e2"} + overlay1 := layout.NewVBoxContainer(o1e1, o1e2) + o2e1 := &focusable{id: "o2e1"} + o2e2 := &focusable{id: "o2e2"} + overlay2 := layout.NewVBoxContainer(o2e1, o2e2) + w.SetContent(content) + + assert.Nil(t, c.Focused()) + + c.FocusPrevious() + assert.Equal(t, ce2, c.Focused()) + assert.True(t, ce2.focused) + + c.Overlays().Add(overlay1) + ctxt := "adding overlay removes focus from content" + assert.Nil(t, c.Focused(), ctxt) + assert.False(t, ce2.focused, ctxt) + + c.FocusNext() + ctxt = "changing focus affects overlay instead of content" + assert.Equal(t, o1e1, c.Focused(), ctxt) + assert.False(t, ce1.focused, ctxt) + assert.False(t, ce2.focused, ctxt) + assert.True(t, o1e1.focused, ctxt) + + c.Overlays().Add(overlay2) + ctxt = "adding overlay removes focus from previous overlay" + assert.Nil(t, c.Focused(), ctxt) + assert.False(t, o1e1.focused, ctxt) + + c.FocusPrevious() + ctxt = "changing focus affects top overlay only" + assert.Equal(t, o2e2, c.Focused(), ctxt) + assert.False(t, o1e1.focused, ctxt) + assert.False(t, o1e2.focused, ctxt) + assert.True(t, o2e2.focused, ctxt) + + c.FocusNext() + assert.Equal(t, o2e1, c.Focused()) + assert.False(t, o2e2.focused) + assert.True(t, o2e1.focused) + + c.Overlays().Remove(overlay2) + ctxt = "removing overlay removes focus from removed overlay and restores focus on new top overlay" + assert.Equal(t, o1e1, c.Focused(), ctxt) + assert.False(t, o2e1.focused, ctxt) + assert.False(t, o2e2.focused, ctxt) + assert.True(t, o1e1.focused, ctxt) + + c.FocusPrevious() + assert.Equal(t, o1e2, c.Focused()) + assert.False(t, o1e1.focused) + assert.True(t, o1e2.focused) + + c.Overlays().Remove(overlay1) + ctxt = "removing last overlay removes focus from removed overlay and restores focus on content" + assert.Equal(t, ce2, c.Focused(), ctxt) + assert.False(t, o1e1.focused, ctxt) + assert.False(t, o1e2.focused, ctxt) + assert.True(t, ce2.focused, ctxt) +} + func TestGlCanvas_InsufficientSizeDoesntTriggerResizeIfSizeIsAlreadyMaxedOut(t *testing.T) { w := createWindow("Test").(*window) c := w.Canvas().(*glCanvas) diff --git a/internal/driver/glfw/window_test.go b/internal/driver/glfw/window_test.go index 9dc43cc25d..1d352588cb 100644 --- a/internal/driver/glfw/window_test.go +++ b/internal/driver/glfw/window_test.go @@ -778,7 +778,7 @@ func TestWindow_Focus(t *testing.T) { func TestWindow_ManualFocus(t *testing.T) { w := createWindow("Test").(*window) - content := &focusable{Rectangle: canvas.NewRectangle(color.Black)} + content := &focusable{} content.SetMinSize(fyne.NewSize(10, 10)) w.SetContent(content) repaintWindow(w) @@ -1035,7 +1035,9 @@ var _ fyne.Focusable = (*focusable)(nil) var _ fyne.Disableable = (*focusable)(nil) type focusable struct { - *canvas.Rectangle + canvas.Rectangle + id string // helps identifying instances in comparisons + focused bool focusedTimes int unfocusedTimes int disabled bool @@ -1053,10 +1055,15 @@ func (f *focusable) TypedKey(*fyne.KeyEvent) { func (f *focusable) FocusGained() { f.focusedTimes++ + if f.Disabled() { + return + } + f.focused = true } func (f *focusable) FocusLost() { f.unfocusedTimes++ + f.focused = false } func (f *focusable) Enable() { From 2ec5a3348ddf7983257cbfe1803792dffe2f75bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilo=20Pr=C3=BCtz?= Date: Fri, 18 Sep 2020 07:39:50 +0200 Subject: [PATCH 4/7] add locking to glCanvas/OverlayStack --- internal/driver/glfw/canvas.go | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/internal/driver/glfw/canvas.go b/internal/driver/glfw/canvas.go index 02d84352f1..6a88480206 100644 --- a/internal/driver/glfw/canvas.go +++ b/internal/driver/glfw/canvas.go @@ -540,6 +540,7 @@ type overlayStack struct { focusManagers []*app.FocusManager onChange func(prevFocusMgr *app.FocusManager) + propertyLock sync.RWMutex renderCaches []*renderCacheTree } @@ -547,7 +548,10 @@ func (o *overlayStack) Add(overlay fyne.CanvasObject) { if overlay == nil { return } - pfm := o.TopFocusManager() + o.propertyLock.Lock() + defer o.propertyLock.Unlock() + + pfm := o.topFocusManager() o.OverlayStack.Add(overlay) o.renderCaches = append(o.renderCaches, &renderCacheTree{root: &renderCacheNode{obj: overlay}}) o.focusManagers = append(o.focusManagers, app.NewFocusManager(overlay)) @@ -555,7 +559,13 @@ func (o *overlayStack) Add(overlay fyne.CanvasObject) { } func (o *overlayStack) Remove(overlay fyne.CanvasObject) { - top := o.TopFocusManager() + if overlay == nil { + return + } + o.propertyLock.Lock() + defer o.propertyLock.Unlock() + + top := o.topFocusManager() o.OverlayStack.Remove(overlay) overlayCount := len(o.List()) o.renderCaches = o.renderCaches[:overlayCount] @@ -564,11 +574,17 @@ func (o *overlayStack) Remove(overlay fyne.CanvasObject) { } func (o *overlayStack) TopFocusManager() *app.FocusManager { - var pfm *app.FocusManager + o.propertyLock.RLock() + defer o.propertyLock.RUnlock() + return o.topFocusManager() +} + +func (o *overlayStack) topFocusManager() *app.FocusManager { + var fm *app.FocusManager if len(o.focusManagers) > 0 { - pfm = o.focusManagers[len(o.focusManagers)-1] + fm = o.focusManagers[len(o.focusManagers)-1] } - return pfm + return fm } type renderCacheNode struct { From fb92aec1e1eb104b5ca096df703a6bae0d88d700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilo=20Pr=C3=BCtz?= Date: Fri, 18 Sep 2020 07:49:08 +0200 Subject: [PATCH 5/7] inject current overlay focus manager into glCanvas.overlayChanged, too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/driver/glfw/canvas.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/driver/glfw/canvas.go b/internal/driver/glfw/canvas.go index 6a88480206..49a719a849 100644 --- a/internal/driver/glfw/canvas.go +++ b/internal/driver/glfw/canvas.go @@ -389,13 +389,12 @@ func (c *glCanvas) objectTrees() []fyne.CanvasObject { return trees } -func (c *glCanvas) overlayChanged(prevFocusMgr *app.FocusManager) { +func (c *glCanvas) overlayChanged(focusMgr, prevFocusMgr *app.FocusManager) { c.setDirty(true) c.RLock() if prevFocusMgr == nil { prevFocusMgr = c.focusMgr } - focusMgr := c.overlays.TopFocusManager() if focusMgr == nil { focusMgr = c.focusMgr } @@ -539,7 +538,7 @@ type overlayStack struct { internal.OverlayStack focusManagers []*app.FocusManager - onChange func(prevFocusMgr *app.FocusManager) + onChange func(focusMgr, prevFocusMgr *app.FocusManager) propertyLock sync.RWMutex renderCaches []*renderCacheTree } @@ -554,23 +553,24 @@ func (o *overlayStack) Add(overlay fyne.CanvasObject) { pfm := o.topFocusManager() o.OverlayStack.Add(overlay) o.renderCaches = append(o.renderCaches, &renderCacheTree{root: &renderCacheNode{obj: overlay}}) - o.focusManagers = append(o.focusManagers, app.NewFocusManager(overlay)) - o.onChange(pfm) + fm := app.NewFocusManager(overlay) + o.focusManagers = append(o.focusManagers, fm) + o.onChange(fm, pfm) } func (o *overlayStack) Remove(overlay fyne.CanvasObject) { - if overlay == nil { + if overlay == nil || len(o.List()) == 0 { return } o.propertyLock.Lock() defer o.propertyLock.Unlock() - top := o.topFocusManager() + pfm := o.topFocusManager() o.OverlayStack.Remove(overlay) overlayCount := len(o.List()) o.renderCaches = o.renderCaches[:overlayCount] o.focusManagers = o.focusManagers[:overlayCount] - o.onChange(top) + o.onChange(o.topFocusManager(), pfm) } func (o *overlayStack) TopFocusManager() *app.FocusManager { From b69f249bfc48190049b181126a9effbe74c3a3e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilo=20Pr=C3=BCtz?= Date: Fri, 18 Sep 2020 17:07:11 +0200 Subject: [PATCH 6/7] fix deprecated overlay methods regarding locking --- internal/driver/glfw/canvas.go | 63 +++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/internal/driver/glfw/canvas.go b/internal/driver/glfw/canvas.go index 49a719a849..6eae2274d9 100644 --- a/internal/driver/glfw/canvas.go +++ b/internal/driver/glfw/canvas.go @@ -112,9 +112,7 @@ func (c *glCanvas) OnTypedRune() func(rune) { // Deprecated: Use Overlays() instead. func (c *glCanvas) Overlay() fyne.CanvasObject { - c.RLock() - defer c.RUnlock() - return c.overlays.Top() + return c.Overlays().Top() } func (c *glCanvas) Overlays() fyne.OverlayStack { @@ -210,13 +208,10 @@ func (c *glCanvas) SetOnTypedRune(typed func(rune)) { // Deprecated: Use Overlays() instead. func (c *glCanvas) SetOverlay(overlay fyne.CanvasObject) { - c.Lock() - defer c.Unlock() - if len(c.overlays.List()) > 0 { - c.overlays.Remove(c.overlays.List()[0]) - } - c.overlays.Add(overlay) - c.setDirty(true) + c.RLock() + o := c.overlays + c.RUnlock() + o.setOverlay(overlay) } func (c *glCanvas) SetPadded(padded bool) { @@ -390,15 +385,15 @@ func (c *glCanvas) objectTrees() []fyne.CanvasObject { } func (c *glCanvas) overlayChanged(focusMgr, prevFocusMgr *app.FocusManager) { - c.setDirty(true) - c.RLock() + c.Lock() + defer c.Unlock() + c.dirty = true if prevFocusMgr == nil { prevFocusMgr = c.focusMgr } if focusMgr == nil { focusMgr = c.focusMgr } - c.RUnlock() prevFocusMgr.FocusLost() focusMgr.FocusGained() } @@ -549,13 +544,9 @@ func (o *overlayStack) Add(overlay fyne.CanvasObject) { } o.propertyLock.Lock() defer o.propertyLock.Unlock() - pfm := o.topFocusManager() - o.OverlayStack.Add(overlay) - o.renderCaches = append(o.renderCaches, &renderCacheTree{root: &renderCacheNode{obj: overlay}}) - fm := app.NewFocusManager(overlay) - o.focusManagers = append(o.focusManagers, fm) - o.onChange(fm, pfm) + o.add(overlay) + o.onChange(o.topFocusManager(), pfm) } func (o *overlayStack) Remove(overlay fyne.CanvasObject) { @@ -564,12 +555,8 @@ func (o *overlayStack) Remove(overlay fyne.CanvasObject) { } o.propertyLock.Lock() defer o.propertyLock.Unlock() - pfm := o.topFocusManager() - o.OverlayStack.Remove(overlay) - overlayCount := len(o.List()) - o.renderCaches = o.renderCaches[:overlayCount] - o.focusManagers = o.focusManagers[:overlayCount] + o.remove(overlay) o.onChange(o.topFocusManager(), pfm) } @@ -579,6 +566,34 @@ func (o *overlayStack) TopFocusManager() *app.FocusManager { return o.topFocusManager() } +func (o *overlayStack) add(overlay fyne.CanvasObject) { + o.OverlayStack.Add(overlay) + o.renderCaches = append(o.renderCaches, &renderCacheTree{root: &renderCacheNode{obj: overlay}}) + o.focusManagers = append(o.focusManagers, app.NewFocusManager(overlay)) +} + +func (o *overlayStack) remove(overlay fyne.CanvasObject) { + o.OverlayStack.Remove(overlay) + overlayCount := len(o.List()) + o.renderCaches = o.renderCaches[:overlayCount] + o.focusManagers = o.focusManagers[:overlayCount] +} + +// concurrency safe implementation of deprecated c.SetOverlay +func (o *overlayStack) setOverlay(overlay fyne.CanvasObject) { + o.propertyLock.Lock() + defer o.propertyLock.Unlock() + + pfm := o.topFocusManager() + if len(o.List()) > 0 { + o.remove(o.List()[0]) + } + if overlay != nil { + o.add(overlay) + } + o.onChange(o.topFocusManager(), pfm) +} + func (o *overlayStack) topFocusManager() *app.FocusManager { var fm *app.FocusManager if len(o.focusManagers) > 0 { From 0b06e767532d6461176264b22e353b690d22805f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilo=20Pr=C3=BCtz?= Date: Tue, 22 Sep 2020 07:38:40 +0200 Subject: [PATCH 7/7] add clarifying comment on `FocusGained`/`FocusLost` regarding locking --- canvasobject.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/canvasobject.go b/canvasobject.go index f7b9a101d7..bdb350a333 100644 --- a/canvasobject.go +++ b/canvasobject.go @@ -66,6 +66,9 @@ type Draggable interface { // It will receive the FocusGained and FocusLost events appropriately. // When focused it will also have TypedRune called as text is input and // TypedKey called when other keys are pressed. +// +// Note: You must not change canvas state (including overlays or focus) in FocusGained or FocusLost +// or you would end up with a dead-lock. type Focusable interface { FocusGained() FocusLost()