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 <robin@jarry.cc>
Acked-by: Koni Marti <koni.marti@gmail.com>

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")

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)
+	tab := aerc.SelectedTab()
-	tabName := aerc.TabNames()[aerc.SelectedTabIndex()]
+	if tab == nil {
+		return errors.New("No selected tab")
+	}
+	composer, _ := tab.Content.(*widgets.Composer)
+	tabName := tab.Name
 	config := composer.Config()
 
 	if config.Outgoing == "" {

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()
+	var relative bool
-	l := aerc.NumTabs()
+	if strings.HasPrefix(joinedArgs, "+") || strings.HasPrefix(joinedArgs, "-") {
-
+		relative = true
-	if strings.HasPrefix(joinedArgs, "+") {
-		i = (i + n) % l
-	} else if strings.HasPrefix(joinedArgs, "-") {
-		i = (((i + n) % l) + l) % l
-	} else {
-		i = n
 	}
-
+	aerc.MoveTab(n, relative)
-	aerc.MoveTab(i)
 
 	return nil
 }

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.tabs = append(tabs.tabs, tab)
-	tabs.TabStrip.Invalidate()
+	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)
+			tabs.selectPriv(index)
-			interactive, ok := tabs.Tabs[tabs.Selected].Content.(Interactive)
+		}
+	} else if indexToRemove < tabs.curIndex {
+		// selected tab is now one to the left of where it was
+		tabs.Select(tabs.curIndex - 1)
+	}
+	interactive, ok := tabs.tabs[tabs.curIndex].Content.(Interactive)
 	if ok {
 		interactive.Focus(true)
 	}
-		}
-	} else if indexToRemove < tabs.Selected {
-		// selected tab is now one to the left of where it was
-		tabs.Selected--
-	}
-	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) {
+func (tabs *Tabs) Get(index int) *Tab {
-	if index >= len(tabs.Tabs) {
+	if index < 0 || index >= len(tabs.tabs) {
-		index = len(tabs.Tabs) - 1
+		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]
+}
+
+func (tabs *Tabs) Select(index int) bool {
+	if index < 0 || index >= len(tabs.tabs) {
+		return false
 	}
 
-	if tabs.Selected != index {
+	if tabs.curIndex != index {
 		// only push valid tabs onto the history
-		if tabs.Selected < len(tabs.Tabs) {
+		if tabs.curIndex < len(tabs.tabs) {
-			tabs.pushHistory(tabs.Selected)
+			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 tabs.Select(index)
-	return true
 }
 
-func (tabs *Tabs) MoveTab(to int) {
+func (tabs *Tabs) MoveTab(to int, relative bool) {
-	from := tabs.Selected
+	from := tabs.curIndex
 
+	if relative {
+		to = from + to
+	}
 	if to < 0 {
 		to = 0
 	}
-
+	if to >= len(tabs.tabs) {
-	if to >= len(tabs.Tabs) {
+		to = len(tabs.tabs) - 1
-		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.tabs[to] = tab
-	tabs.Selected = to
+	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)
+	pinEnd := len(tabs.tabs)
-	for i, t := range tabs.Tabs {
+	for i, t := range tabs.tabs {
 		if !t.pinned {
 			pinEnd = i
 			break
 		}
 	}
 
-	for _, t := range tabs.Tabs {
+	for _, t := range tabs.tabs {
-		if t.pinned && t.indexBeforePin > tabs.Selected-pinEnd {
+		if t.pinned && t.indexBeforePin > tabs.curIndex-pinEnd {
 			t.indexBeforePin -= 1
 		}
 	}
 
-	tabs.Tabs[tabs.Selected].pinned = true
+	tabs.tabs[tabs.curIndex].pinned = true
-	tabs.Tabs[tabs.Selected].indexBeforePin = tabs.Selected - pinEnd
+	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)
+	pinEnd := len(tabs.tabs)
-	for i, t := range tabs.Tabs {
+	for i, t := range tabs.tabs {
-		if i != tabs.Selected && t.pinned && t.indexBeforePin > tabs.Tabs[tabs.Selected].indexBeforePin {
+		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
+	next := tabs.curIndex + 1
-	if next >= len(tabs.Tabs) {
+	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)
+			selectedTab, ok := strip.clicked(localX, localY)
-			if !ok || selectedTab == strip.parent.Selected {
+			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)
-			}
 			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))
+	children := make([]Drawable, len(content.tabs))
-	for i, tab := range 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()
 }
 

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) {

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 {
+func (aerc *Aerc) SelectedTab() *ui.Tab {
-	return aerc.tabs.Selected
+	return aerc.tabs.Selected()
-}
-
-func (aerc *Aerc) NumTabs() int {
-	return len(aerc.tabs.Tabs)
 }
 
 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) {
+func (aerc *Aerc) MoveTab(i int, relative bool) {
-	aerc.tabs.MoveTab(i)
+	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 {
+	ok := aerc.tabs.SelectName(name)
-		if tab.Name == name {
+	if ok {
-			aerc.tabs.Select(i)
 		aerc.UpdateStatus()
-			return true
 	}
-	}
+	return ok
-	return false
 }
 
 func (aerc *Aerc) SelectTabIndex(index int) bool {
-	for i := range aerc.tabs.Tabs {
+	ok := aerc.tabs.Select(index)
-		if i == index {
+	if ok {
-			aerc.tabs.Select(i)
 		aerc.UpdateStatus()
-			return true
 	}
-	}
+	return ok
-	return false
 }
 
 func (aerc *Aerc) TabNames() []string {
-	var names []string
+	return aerc.tabs.Names()
-	for _, tab := range aerc.tabs.Tabs {
-		names = append(names, tab.Name)
-	}
-	return 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 {