Improve action menu escape handling

Make the F11 action menu close through a shared close action so Escape and the explicit Close actions menu item follow the same path. Treat Escape-prefixed Up and Down events as close inputs while the action menu is active, which handles termbox InputAlt behavior after pressing Escape. Preserve and restore the user/channel tree selection across action menu open and close, and preserve selection across live tree rebuilds.

Tested with: GOCACHE=/tmp/barnard-go-cache go test ./...

admin.go

@@ -50,19 +50,21 @@ func (b *Barnard) OnAdminEscape(ui *uiterm.Ui, key uiterm.Key) {
 		return
 	}
 	if b.Ui.Active() == uiViewAdmin {
-		b.Ui.SetActive(uiViewTree)
-		width, height := termboxSize()
-		b.OnUiResize(ui, width, height)
+		b.CloseAdminMenu(ui)
 		b.AddOutputLine("Admin: closed")
-		ui.Refresh()
 	}
 }
 
+func (b *Barnard) closeAdminAction() {
+	b.OnAdminEscape(b.Ui, uiterm.KeyEsc)
+}
+
 func (b *Barnard) OpenAdminMenu() {
 	if b.Client == nil || b.Client.Self == nil {
 		b.AddOutputLine("Admin: not connected")
 		return
 	}
+	b.adminReturnItem = b.UiTree.ActiveItem()
 	b.adminTargetUser = b.selectedUser
 	b.adminTargetChan = b.Client.Self.Channel
 	if b.Ui.Active() == uiViewTree {
@@ -98,6 +100,15 @@ func (b *Barnard) OpenAdminMenu() {
 	b.Ui.Refresh()
 }
 
+func (b *Barnard) CloseAdminMenu(ui *uiterm.Ui) {
+	b.Ui.SetActive(uiViewTree)
+	b.UiTree.SetActiveItem(b.adminReturnItem, sameUserChannelTreeItem)
+	b.adminReturnItem = nil
+	width, height := termboxSize()
+	b.OnUiResize(ui, width, height)
+	ui.Refresh()
+}
+
 func termboxSize() (int, int) {
 	width, height := termbox.Size()
 	return width, height
@@ -140,11 +151,15 @@ func (b *Barnard) AdminItemBuild(item uiterm.TreeItem) []uiterm.TreeItem {
 	if children := b.adminContextActionItems(); len(children) > 0 {
 		items = append(items, adminItem{label: "Context actions", children: children})
 	}
-	items = append(items, adminItem{label: "Close actions menu", action: func() { b.OnAdminEscape(b.Ui, uiterm.KeyEsc) }})
+	items = append(items, adminItem{label: "Close actions menu", action: b.closeAdminAction})
 	return items
 }
 
 func (b *Barnard) AdminItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm.TreeItem, key uiterm.Key) {
+	if isAdminEscapeKey(key) {
+		b.OnAdminEscape(ui, key)
+		return
+	}
 	if key != uiterm.KeyEnter {
 		return
 	}
@@ -158,6 +173,22 @@ func (b *Barnard) AdminItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiter
 }
 
 func (b *Barnard) AdminItemCharacter(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm.TreeItem, ch rune) {
+	if isAdminEscapeCharacter(ch) {
+		b.OnAdminEscape(ui, uiterm.KeyEsc)
+	}
+}
+
+func isAdminEscapeKey(key uiterm.Key) bool {
+	switch key {
+	case uiterm.KeyEsc, uiterm.KeyAltEsc, uiterm.KeyAltArrowUp, uiterm.KeyAltArrowDown:
+		return true
+	default:
+		return false
+	}
+}
+
+func isAdminEscapeCharacter(ch rune) bool {
+	return ch == rune(uiterm.KeyEsc)
 }
 
 func (b *Barnard) informationItems() []uiterm.TreeItem {

admin_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"git.stormux.org/storm/barnard/gumble/gumble"
+	"git.stormux.org/storm/barnard/uiterm"
 )
 
 func TestParseToggleState(t *testing.T) {
@@ -63,3 +64,27 @@ func TestPermissionList(t *testing.T) {
 		t.Fatalf("expected write,ban, got %q", got)
 	}
 }
+
+func TestAdminEscapeInputs(t *testing.T) {
+	if !isAdminEscapeKey(uiterm.KeyEsc) {
+		t.Fatal("expected escape key to close admin menu")
+	}
+	if !isAdminEscapeKey(uiterm.KeyAltEsc) {
+		t.Fatal("expected alt escape key to close admin menu")
+	}
+	if !isAdminEscapeKey(uiterm.KeyAltArrowUp) {
+		t.Fatal("expected escape-prefixed up arrow to close admin menu")
+	}
+	if !isAdminEscapeKey(uiterm.KeyAltArrowDown) {
+		t.Fatal("expected escape-prefixed down arrow to close admin menu")
+	}
+	if isAdminEscapeKey(uiterm.KeyEnter) {
+		t.Fatal("expected enter key not to close admin menu")
+	}
+	if !isAdminEscapeCharacter(rune(uiterm.KeyEsc)) {
+		t.Fatal("expected escape character to close admin menu")
+	}
+	if isAdminEscapeCharacter('x') {
+		t.Fatal("expected non-escape character not to close admin menu")
+	}
+}

barnard.go

@@ -43,6 +43,7 @@ type Barnard struct {
 	selectedUser    *gumble.User
 	adminTargetUser *gumble.User
 	adminTargetChan *gumble.Channel
+	adminReturnItem uiterm.TreeItem
 	statusText      string
 	statusNotice    bool
 
@@ -144,7 +145,7 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm
 				delete(b.MutedChannels, treeItem.Channel.ID)
 			}
 
-			b.UiTree.Rebuild()
+			b.RebuildUserChannelTreePreservingSelection()
 			b.Ui.Refresh()
 		}
 		if key == *b.Hotkeys.VolumeDown {
@@ -169,7 +170,7 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm
 					treeItem.User.AudioSource.SetGain(treeItem.User.Volume)
 				}
 			}
-			b.UiTree.Rebuild()
+			b.RebuildUserChannelTreePreservingSelection()
 			b.Ui.Refresh()
 		}
 		if key == *b.Hotkeys.VolumeDown {

client.go

@@ -216,7 +216,7 @@ func (b *Barnard) OnUserChange(e *gumble.UserChangeEvent) {
 		b.AddOutputLine(formatUserStats(e.User))
 	}
 	b.updateUserChannel(e)
-	b.UiTree.Rebuild()
+	b.RebuildUserChannelTreePreservingSelection()
 	b.Ui.Refresh()
 }
 
@@ -317,7 +317,7 @@ func (b *Barnard) OnChannelChange(e *gumble.ChannelChangeEvent) {
 			b.AddOutputLine(fmt.Sprintf("Channel permissions for %s: %s", e.Channel.Name, permissionList(*permission)))
 		}
 	}
-	b.UiTree.Rebuild()
+	b.RebuildUserChannelTreePreservingSelection()
 	b.Ui.Refresh()
 }
 

ui.go

@@ -66,7 +66,7 @@ func (b *Barnard) UpdateInputStatus(status string) {
 		status = status[:17] + "..." + "]"
 	}
 	b.UiInputStatus.Text = status
-	b.UiTree.Rebuild()
+	b.RebuildUserChannelTreePreservingSelection()
 	b.Ui.Refresh()
 }
 

ui_tree.go

@@ -153,3 +153,25 @@ func (b *Barnard) TreeItemBuild(item uiterm.TreeItem) []uiterm.TreeItem {
 
 	return append(users, channels...)
 }
+
+func (b *Barnard) RebuildUserChannelTreePreservingSelection() {
+	b.UiTree.RebuildPreservingActiveItem(sameUserChannelTreeItem)
+}
+
+func sameUserChannelTreeItem(previous, current uiterm.TreeItem) bool {
+	prev, ok := previous.(TreeItem)
+	if !ok {
+		return false
+	}
+	cur, ok := current.(TreeItem)
+	if !ok {
+		return false
+	}
+	if prev.User != nil && cur.User != nil {
+		return prev.User.Session == cur.User.Session
+	}
+	if prev.Channel != nil && cur.Channel != nil {
+		return prev.Channel.ID == cur.Channel.ID
+	}
+	return false
+}

uiterm/tree.go

@@ -59,11 +59,34 @@ func (t *Tree) uiSetBounds(x0, y0, x1, y1 int) {
 }
 
 func (t *Tree) Rebuild() {
+	t.rebuild(false, nil)
+}
+
+func (t *Tree) RebuildPreservingActiveItem(sameItem func(previous, current TreeItem) bool) {
+	t.rebuild(true, sameItem)
+}
+
+func (t *Tree) SetActiveItem(target TreeItem, sameItem func(previous, current TreeItem) bool) bool {
+	if target == nil || sameItem == nil {
+		return false
+	}
+	for line, item := range t.lines {
+		if sameItem(target, item.Item) {
+			t.SetActiveLine(line, false)
+			return true
+		}
+	}
+	return false
+}
+
+func (t *Tree) rebuild(preserveActive bool, sameItem func(previous, current TreeItem) bool) {
 	if t.Generator == nil {
 		t.lines = []renderedTreeItem{}
 		return
 	}
 
+	previousItem := t.ActiveItem()
+	previousLine := t.activeLine
 	lines := []renderedTreeItem{}
 	for _, item := range t.Generator(nil) {
 		children := t.rebuild_rec(item, 0)
@@ -72,9 +95,23 @@ func (t *Tree) Rebuild() {
 		}
 	}
 	t.lines = lines
+	if preserveActive {
+		t.SetActiveLine(previousLine, false)
+		if previousItem != nil && sameItem != nil {
+			for line, item := range t.lines {
+				if sameItem(previousItem, item.Item) {
+					t.SetActiveLine(line, false)
+					break
+				}
+			}
+		}
+	} else {
 		t.SetActiveLine(0, false)
+	}
+	if t.ui != nil {
 		t.uiDraw()
 	}
+}
 
 func (t *Tree) rebuild_rec(parent TreeItem, level int) []renderedTreeItem {
 	if parent == nil {
@@ -138,11 +175,10 @@ func (t *Tree) uiDraw() {
 			fg := t.Fg
 			bg := t.Bg
 			dx := x - t.x0
-			dy := y - t.y0
 			if reader != nil && level*2 <= dx {
 				if ch, _, err := reader.ReadRune(); err == nil {
 					chr = ch
-					fg, bg = item.TreeItemStyle(fg, bg, t.active && t.activeLine == dy)
+					fg, bg = item.TreeItemStyle(fg, bg, t.active && t.activeLine == line)
 				}
 			}
 			termbox.SetCell(x, y, chr, termbox.Attribute(fg), termbox.Attribute(bg))

uiterm/tree_test.go

@@ -0,0 +1,86 @@
+package uiterm
+
+import "testing"
+
+type testTreeItem string
+
+func (i testTreeItem) String() string {
+	return string(i)
+}
+
+func (i testTreeItem) TreeItemStyle(fg, bg Attribute, active bool) (Attribute, Attribute) {
+	return fg, bg
+}
+
+func TestTreeRebuildResetsActiveLine(t *testing.T) {
+	t.Parallel()
+
+	tree := Tree{
+		Generator: func(item TreeItem) []TreeItem {
+			if item != nil {
+				return nil
+			}
+			return []TreeItem{testTreeItem("one"), testTreeItem("two"), testTreeItem("three")}
+		},
+	}
+
+	tree.Rebuild()
+	tree.SetActiveLine(2, false)
+	tree.Rebuild()
+
+	if tree.activeLine != 0 {
+		t.Fatalf("expected active line to reset to 0, got %d", tree.activeLine)
+	}
+}
+
+func TestTreeRebuildPreservingActiveItemKeepsMatchingItem(t *testing.T) {
+	t.Parallel()
+
+	items := []TreeItem{testTreeItem("one"), testTreeItem("two"), testTreeItem("three")}
+	tree := Tree{
+		Generator: func(item TreeItem) []TreeItem {
+			if item != nil {
+				return nil
+			}
+			return items
+		},
+	}
+
+	tree.Rebuild()
+	tree.SetActiveLine(1, false)
+	items = []TreeItem{testTreeItem("three"), testTreeItem("one"), testTreeItem("two")}
+	tree.RebuildPreservingActiveItem(func(previous, current TreeItem) bool {
+		return previous.String() == current.String()
+	})
+
+	if tree.activeLine != 2 {
+		t.Fatalf("expected active line to follow matching item to line 2, got %d", tree.activeLine)
+	}
+	if got := tree.ActiveItem().String(); got != "two" {
+		t.Fatalf("expected active item two, got %q", got)
+	}
+}
+
+func TestTreeSetActiveItemSelectsMatchingItem(t *testing.T) {
+	t.Parallel()
+
+	tree := Tree{
+		Generator: func(item TreeItem) []TreeItem {
+			if item != nil {
+				return nil
+			}
+			return []TreeItem{testTreeItem("one"), testTreeItem("two"), testTreeItem("three")}
+		},
+	}
+
+	tree.Rebuild()
+	if ok := tree.SetActiveItem(testTreeItem("three"), func(previous, current TreeItem) bool {
+		return previous.String() == current.String()
+	}); !ok {
+		t.Fatal("expected matching item to be selected")
+	}
+
+	if tree.activeLine != 2 {
+		t.Fatalf("expected active line 2, got %d", tree.activeLine)
+	}
+}