From 171fefd2095132f49d84a4e9426fec0b293fa7ba Mon Sep 17 00:00:00 2001 From: Robin Jarry Date: Thu, 14 Jul 2022 18:29:56 +0200 Subject: tabs: make fields private The Tabs object exposes an array of Tab objects and the current selected index in that array. The these two fields are sometimes modified in goroutines, which can lead to data races causing fatal out of bounds accesses on the tab array. Hide these fields as private API. Expose only what needs to be seen from the outside. This will prepare for protecting concurrent access with a lock in the next commit. Signed-off-by: Robin Jarry Acked-by: Koni Marti --- commands/compose/postpone.go | 8 +- commands/compose/send.go | 8 +- commands/move-tab.go | 15 +--- lib/ui/tab.go | 180 +++++++++++++++++++++++++------------------ widgets/account.go | 2 +- widgets/aerc.go | 58 ++++++-------- 6 files changed, 145 insertions(+), 126 deletions(-) diff --git a/commands/compose/postpone.go b/commands/compose/postpone.go index e1c0568b..4b1e4419 100644 --- a/commands/compose/postpone.go +++ b/commands/compose/postpone.go @@ -35,9 +35,13 @@ func (Postpone) Execute(aerc *widgets.Aerc, args []string) error { if acct == nil { return errors.New("No account selected") } - composer, _ := aerc.SelectedTabContent().(*widgets.Composer) + tab := aerc.SelectedTab() + if tab == nil { + return errors.New("No tab selected") + } + composer, _ := tab.Content.(*widgets.Composer) config := composer.Config() - tabName := aerc.TabNames()[aerc.SelectedTabIndex()] + tabName := tab.Name if config.Postpone == "" { return errors.New("No Postpone location configured") diff --git a/commands/compose/send.go b/commands/compose/send.go index 776779e6..cca1540f 100644 --- a/commands/compose/send.go +++ b/commands/compose/send.go @@ -43,8 +43,12 @@ func (Send) Execute(aerc *widgets.Aerc, args []string) error { if len(args) > 1 { return errors.New("Usage: send") } - composer, _ := aerc.SelectedTabContent().(*widgets.Composer) - tabName := aerc.TabNames()[aerc.SelectedTabIndex()] + tab := aerc.SelectedTab() + if tab == nil { + return errors.New("No selected tab") + } + composer, _ := tab.Content.(*widgets.Composer) + tabName := tab.Name config := composer.Config() if config.Outgoing == "" { diff --git a/commands/move-tab.go b/commands/move-tab.go index 4151bd74..8648ed3d 100644 --- a/commands/move-tab.go +++ b/commands/move-tab.go @@ -34,18 +34,11 @@ func (MoveTab) Execute(aerc *widgets.Aerc, args []string) error { return fmt.Errorf("failed to parse index argument: %v", err) } - i := aerc.SelectedTabIndex() - l := aerc.NumTabs() - - if strings.HasPrefix(joinedArgs, "+") { - i = (i + n) % l - } else if strings.HasPrefix(joinedArgs, "-") { - i = (((i + n) % l) + l) % l - } else { - i = n + var relative bool + if strings.HasPrefix(joinedArgs, "+") || strings.HasPrefix(joinedArgs, "-") { + relative = true } - - aerc.MoveTab(i) + aerc.MoveTab(n, relative) return nil } diff --git a/lib/ui/tab.go b/lib/ui/tab.go index a67bdabc..153accaf 100644 --- a/lib/ui/tab.go +++ b/lib/ui/tab.go @@ -10,10 +10,10 @@ import ( ) type Tabs struct { - Tabs []*Tab + tabs []*Tab TabStrip *TabStrip TabContent *TabContent - Selected int + curIndex int history []int uiConfig *config.UIConfig @@ -54,18 +54,26 @@ func (tabs *Tabs) Add(content Drawable, name string, uiConf *config.UIConfig) *T Name: name, uiConf: uiConf, } - tabs.Tabs = append(tabs.Tabs, tab) - tabs.TabStrip.Invalidate() + tabs.tabs = append(tabs.tabs, tab) + tabs.Select(len(tabs.tabs) - 1) content.OnInvalidate(tabs.invalidateChild) return tab } +func (tabs *Tabs) Names() []string { + var names []string + for _, tab := range tabs.tabs { + names = append(names, tab.Name) + } + return names +} + func (tabs *Tabs) invalidateChild(d Drawable) { - if tabs.Selected >= len(tabs.Tabs) { + if tabs.curIndex >= len(tabs.tabs) { return } - if tabs.Tabs[tabs.Selected].Content == d { + if tabs.tabs[tabs.curIndex].Content == d { if tabs.onInvalidateContent != nil { tabs.onInvalidateContent(tabs.TabContent) } @@ -74,9 +82,9 @@ func (tabs *Tabs) invalidateChild(d Drawable) { func (tabs *Tabs) Remove(content Drawable) { indexToRemove := -1 - for i, tab := range tabs.Tabs { + for i, tab := range tabs.tabs { if tab.Content == content { - tabs.Tabs = append(tabs.Tabs[:i], tabs.Tabs[i+1:]...) + tabs.tabs = append(tabs.tabs[:i], tabs.tabs[i+1:]...) tabs.removeHistory(i) indexToRemove = i break @@ -86,20 +94,19 @@ func (tabs *Tabs) Remove(content Drawable) { return } // only pop the tab history if the closing tab is selected - if indexToRemove == tabs.Selected { + if indexToRemove == tabs.curIndex { index, ok := tabs.popHistory() if ok { - tabs.Select(index) - interactive, ok := tabs.Tabs[tabs.Selected].Content.(Interactive) - if ok { - interactive.Focus(true) - } + tabs.selectPriv(index) } - } else if indexToRemove < tabs.Selected { + } else if indexToRemove < tabs.curIndex { // selected tab is now one to the left of where it was - tabs.Selected-- + tabs.Select(tabs.curIndex - 1) + } + interactive, ok := tabs.tabs[tabs.curIndex].Content.(Interactive) + if ok { + interactive.Focus(true) } - tabs.TabStrip.Invalidate() } func (tabs *Tabs) Replace(contentSrc Drawable, contentTarget Drawable, name string) { @@ -107,9 +114,9 @@ func (tabs *Tabs) Replace(contentSrc Drawable, contentTarget Drawable, name stri Content: contentTarget, Name: name, } - for i, tab := range tabs.Tabs { + for i, tab := range tabs.tabs { if tab.Content == contentSrc { - tabs.Tabs[i] = replaceTab + tabs.tabs[i] = replaceTab tabs.Select(i) if c, ok := contentSrc.(io.Closer); ok { c.Close() @@ -121,20 +128,44 @@ func (tabs *Tabs) Replace(contentSrc Drawable, contentTarget Drawable, name stri contentTarget.OnInvalidate(tabs.invalidateChild) } -func (tabs *Tabs) Select(index int) { - if index >= len(tabs.Tabs) { - index = len(tabs.Tabs) - 1 +func (tabs *Tabs) Get(index int) *Tab { + if index < 0 || index >= len(tabs.tabs) { + return nil + } + return tabs.tabs[index] +} + +func (tabs *Tabs) Selected() *Tab { + if tabs.curIndex < 0 || tabs.curIndex >= len(tabs.tabs) { + return nil } + return tabs.tabs[tabs.curIndex] +} - if tabs.Selected != index { +func (tabs *Tabs) Select(index int) bool { + if index < 0 || index >= len(tabs.tabs) { + return false + } + + if tabs.curIndex != index { // only push valid tabs onto the history - if tabs.Selected < len(tabs.Tabs) { - tabs.pushHistory(tabs.Selected) + if tabs.curIndex < len(tabs.tabs) { + tabs.pushHistory(tabs.curIndex) } - tabs.Selected = index + tabs.curIndex = index tabs.TabStrip.Invalidate() tabs.TabContent.Invalidate() } + return true +} + +func (tabs *Tabs) SelectName(name string) bool { + for i, tab := range tabs.tabs { + if tab.Name == name { + return tabs.Select(i) + } + } + return false } func (tabs *Tabs) SelectPrevious() bool { @@ -142,24 +173,25 @@ func (tabs *Tabs) SelectPrevious() bool { if !ok { return false } - tabs.Select(index) - return true + return tabs.Select(index) } -func (tabs *Tabs) MoveTab(to int) { - from := tabs.Selected +func (tabs *Tabs) MoveTab(to int, relative bool) { + from := tabs.curIndex + if relative { + to = from + to + } if to < 0 { to = 0 } - - if to >= len(tabs.Tabs) { - to = len(tabs.Tabs) - 1 + if to >= len(tabs.tabs) { + to = len(tabs.tabs) - 1 } - tab := tabs.Tabs[from] + tab := tabs.tabs[from] if to > from { - copy(tabs.Tabs[from:to], tabs.Tabs[from+1:to+1]) + copy(tabs.tabs[from:to], tabs.tabs[from+1:to+1]) for i, h := range tabs.history { if h == from { tabs.history[i] = to @@ -169,7 +201,7 @@ func (tabs *Tabs) MoveTab(to int) { } } } else if from > to { - copy(tabs.Tabs[to+1:from+1], tabs.Tabs[to:from]) + copy(tabs.tabs[to+1:from+1], tabs.tabs[to:from]) for i, h := range tabs.history { if h == from { tabs.history[i] = to @@ -182,44 +214,44 @@ func (tabs *Tabs) MoveTab(to int) { return } - tabs.Tabs[to] = tab - tabs.Selected = to + tabs.tabs[to] = tab + tabs.curIndex = to tabs.TabStrip.Invalidate() } func (tabs *Tabs) PinTab() { - if tabs.Tabs[tabs.Selected].pinned { + if tabs.tabs[tabs.curIndex].pinned { return } - pinEnd := len(tabs.Tabs) - for i, t := range tabs.Tabs { + pinEnd := len(tabs.tabs) + for i, t := range tabs.tabs { if !t.pinned { pinEnd = i break } } - for _, t := range tabs.Tabs { - if t.pinned && t.indexBeforePin > tabs.Selected-pinEnd { + for _, t := range tabs.tabs { + if t.pinned && t.indexBeforePin > tabs.curIndex-pinEnd { t.indexBeforePin -= 1 } } - tabs.Tabs[tabs.Selected].pinned = true - tabs.Tabs[tabs.Selected].indexBeforePin = tabs.Selected - pinEnd + tabs.tabs[tabs.curIndex].pinned = true + tabs.tabs[tabs.curIndex].indexBeforePin = tabs.curIndex - pinEnd - tabs.MoveTab(pinEnd) + tabs.MoveTab(pinEnd, false) } func (tabs *Tabs) UnpinTab() { - if !tabs.Tabs[tabs.Selected].pinned { + if !tabs.tabs[tabs.curIndex].pinned { return } - pinEnd := len(tabs.Tabs) - for i, t := range tabs.Tabs { - if i != tabs.Selected && t.pinned && t.indexBeforePin > tabs.Tabs[tabs.Selected].indexBeforePin { + pinEnd := len(tabs.tabs) + for i, t := range tabs.tabs { + if i != tabs.curIndex && t.pinned && t.indexBeforePin > tabs.tabs[tabs.curIndex].indexBeforePin { t.indexBeforePin += 1 } if !t.pinned { @@ -228,23 +260,23 @@ func (tabs *Tabs) UnpinTab() { } } - tabs.Tabs[tabs.Selected].pinned = false + tabs.tabs[tabs.curIndex].pinned = false - tabs.MoveTab(tabs.Tabs[tabs.Selected].indexBeforePin + pinEnd - 1) + tabs.MoveTab(tabs.tabs[tabs.curIndex].indexBeforePin+pinEnd-1, false) } func (tabs *Tabs) NextTab() { - next := tabs.Selected + 1 - if next >= len(tabs.Tabs) { + next := tabs.curIndex + 1 + if next >= len(tabs.tabs) { next = 0 } tabs.Select(next) } func (tabs *Tabs) PrevTab() { - next := tabs.Selected - 1 + next := tabs.curIndex - 1 if next < 0 { - next = len(tabs.Tabs) - 1 + next = len(tabs.tabs) - 1 } tabs.Select(next) } @@ -284,13 +316,13 @@ func (tabs *Tabs) removeHistory(index int) { // TODO: Color repository func (strip *TabStrip) Draw(ctx *Context) { x := 0 - for i, tab := range strip.Tabs { + for i, tab := range strip.tabs { uiConfig := strip.uiConfig if tab.uiConf != nil { uiConfig = tab.uiConf } style := uiConfig.GetStyle(config.STYLE_TAB) - if strip.Selected == i { + if strip.curIndex == i { style = uiConfig.GetStyleSelected(config.STYLE_TAB) } tabWidth := 32 @@ -319,7 +351,7 @@ func (strip *TabStrip) Invalidate() { func (strip *TabStrip) MouseEvent(localX int, localY int, event tcell.Event) { changeFocus := func(focus bool) { - interactive, ok := strip.parent.Tabs[strip.parent.Selected].Content.(Interactive) + interactive, ok := strip.parent.tabs[strip.parent.curIndex].Content.(Interactive) if ok { interactive.Focus(focus) } @@ -330,8 +362,8 @@ func (strip *TabStrip) MouseEvent(localX int, localY int, event tcell.Event) { case *tcell.EventMouse: switch event.Buttons() { case tcell.Button1: - selectedTab, ok := strip.Clicked(localX, localY) - if !ok || selectedTab == strip.parent.Selected { + selectedTab, ok := strip.clicked(localX, localY) + if !ok || selectedTab == strip.parent.curIndex { return } unfocus() @@ -346,18 +378,12 @@ func (strip *TabStrip) MouseEvent(localX int, localY int, event tcell.Event) { strip.parent.PrevTab() refocus() case tcell.Button3: - selectedTab, ok := strip.Clicked(localX, localY) + selectedTab, ok := strip.clicked(localX, localY) if !ok { return } unfocus() - if selectedTab == strip.parent.Selected { - strip.parent.CloseTab(selectedTab) - } else { - current := strip.parent.Selected - strip.parent.CloseTab(selectedTab) - strip.parent.Select(current) - } + strip.parent.CloseTab(selectedTab) refocus() } } @@ -367,9 +393,9 @@ func (strip *TabStrip) OnInvalidate(onInvalidate func(d Drawable)) { strip.onInvalidateStrip = onInvalidate } -func (strip *TabStrip) Clicked(mouseX int, mouseY int) (int, bool) { +func (strip *TabStrip) clicked(mouseX int, mouseY int) (int, bool) { x := 0 - for i, tab := range strip.Tabs { + for i, tab := range strip.tabs { trunc := runewidth.Truncate(tab.Name, 32, "…") length := len(trunc) + 2 if x <= mouseX && mouseX < x+length { @@ -381,27 +407,27 @@ func (strip *TabStrip) Clicked(mouseX int, mouseY int) (int, bool) { } func (content *TabContent) Children() []Drawable { - children := make([]Drawable, len(content.Tabs)) - for i, tab := range content.Tabs { + children := make([]Drawable, len(content.tabs)) + for i, tab := range content.tabs { children[i] = tab.Content } return children } func (content *TabContent) Draw(ctx *Context) { - if content.Selected >= len(content.Tabs) { + if content.curIndex >= len(content.tabs) { width := ctx.Width() height := ctx.Height() ctx.Fill(0, 0, width, height, ' ', content.uiConfig.GetStyle(config.STYLE_TAB)) } - tab := content.Tabs[content.Selected] + tab := content.tabs[content.curIndex] tab.Content.Draw(ctx) } func (content *TabContent) MouseEvent(localX int, localY int, event tcell.Event) { - tab := content.Tabs[content.Selected] + tab := content.tabs[content.curIndex] switch tabContent := tab.Content.(type) { case Mouseable: tabContent.MouseEvent(localX, localY, event) @@ -412,7 +438,7 @@ func (content *TabContent) Invalidate() { if content.onInvalidateContent != nil { content.onInvalidateContent(content) } - tab := content.Tabs[content.Selected] + tab := content.tabs[content.curIndex] tab.Content.Invalidate() } diff --git a/widgets/account.go b/widgets/account.go index 6e03160a..c76b6fc8 100644 --- a/widgets/account.go +++ b/widgets/account.go @@ -234,7 +234,7 @@ func (acct *AccountView) SelectedMessagePart() *PartInfo { } func (acct *AccountView) isSelected() bool { - return acct.aerc.NumTabs() > 0 && acct == acct.aerc.SelectedAccount() + return acct == acct.aerc.SelectedAccount() } func (acct *AccountView) onMessage(msg types.WorkerMessage) { diff --git a/widgets/aerc.go b/widgets/aerc.go index f480b34c..c56c4df4 100644 --- a/widgets/aerc.go +++ b/widgets/aerc.go @@ -105,8 +105,14 @@ func NewAerc(conf *config.AercConfig, logger *log.Logger, aerc.NewTab(wizard, "New account") } + tabs.Select(0) + tabs.CloseTab = func(index int) { - switch content := aerc.tabs.Tabs[index].Content.(type) { + tab := aerc.tabs.Get(index) + if tab == nil { + return + } + switch content := tab.Content.(type) { case *AccountView: return case *AccountWizard: @@ -284,7 +290,7 @@ func (aerc *Aerc) Event(event tcell.Event) bool { aerc.BeginExCommand("") return true } - interactive, ok := aerc.tabs.Tabs[aerc.tabs.Selected].Content.(ui.Interactive) + interactive, ok := aerc.SelectedTabContent().(ui.Interactive) if ok { return interactive.Event(event) } @@ -334,18 +340,15 @@ func (aerc *Aerc) SelectedAccountUiConfig() *config.UIConfig { } func (aerc *Aerc) SelectedTabContent() ui.Drawable { - if aerc.NumTabs() == 0 || aerc.SelectedTabIndex() >= aerc.NumTabs() { + tab := aerc.tabs.Selected() + if tab == nil { return nil } - return aerc.tabs.Tabs[aerc.SelectedTabIndex()].Content + return tab.Content } -func (aerc *Aerc) SelectedTabIndex() int { - return aerc.tabs.Selected -} - -func (aerc *Aerc) NumTabs() int { - return len(aerc.tabs.Tabs) +func (aerc *Aerc) SelectedTab() *ui.Tab { + return aerc.tabs.Selected() } func (aerc *Aerc) NewTab(clickable ui.Drawable, name string) *ui.Tab { @@ -355,7 +358,6 @@ func (aerc *Aerc) NewTab(clickable ui.Drawable, name string) *ui.Tab { uiConf = conf } tab := aerc.tabs.Add(clickable, name, uiConf) - aerc.tabs.Select(len(aerc.tabs.Tabs) - 1) aerc.UpdateStatus() return tab } @@ -369,8 +371,8 @@ func (aerc *Aerc) ReplaceTab(tabSrc ui.Drawable, tabTarget ui.Drawable, name str aerc.tabs.Replace(tabSrc, tabTarget, name) } -func (aerc *Aerc) MoveTab(i int) { - aerc.tabs.MoveTab(i) +func (aerc *Aerc) MoveTab(i int, relative bool) { + aerc.tabs.MoveTab(i, relative) } func (aerc *Aerc) PinTab() { @@ -390,33 +392,23 @@ func (aerc *Aerc) PrevTab() { } func (aerc *Aerc) SelectTab(name string) bool { - for i, tab := range aerc.tabs.Tabs { - if tab.Name == name { - aerc.tabs.Select(i) - aerc.UpdateStatus() - return true - } + ok := aerc.tabs.SelectName(name) + if ok { + aerc.UpdateStatus() } - return false + return ok } func (aerc *Aerc) SelectTabIndex(index int) bool { - for i := range aerc.tabs.Tabs { - if i == index { - aerc.tabs.Select(i) - aerc.UpdateStatus() - return true - } + ok := aerc.tabs.Select(index) + if ok { + aerc.UpdateStatus() } - return false + return ok } func (aerc *Aerc) TabNames() []string { - var names []string - for _, tab := range aerc.tabs.Tabs { - names = append(names, tab.Name) - } - return names + return aerc.tabs.Names() } func (aerc *Aerc) SelectPreviousTab() bool { @@ -463,7 +455,7 @@ func (aerc *Aerc) focus(item ui.Interactive) { aerc.focused.Focus(false) } aerc.focused = item - interactive, ok := aerc.tabs.Tabs[aerc.tabs.Selected].Content.(ui.Interactive) + interactive, ok := aerc.SelectedTabContent().(ui.Interactive) if item != nil { item.Focus(true) if ok { -- cgit