split: refactor to prevent stuck splits

Refactor split logic (again...) to prevent stuck splits. Use callback
from msgstore.Select to tell the split which message to display. This
keeps the account from having to track displayed messages, which
prevents race conditions in certain situations.

Signed-off-by: Tim Culverhouse <tim@timculverhouse.com>
Tested-by: Bence Ferdinandy <bence@ferdinandy.com>

commands/account/split.go

@@ -51,10 +51,8 @@ func (Split) Execute(aerc *widgets.Aerc, args []string) error {
 		}
 		if delta {
 			n = acct.SplitSize() + n
-			// Maintain split direction when using deltas
-			if acct.SplitSize() > 0 {
-				args[0] = acct.SplitDirection()
-			}
+			acct.SetSplitSize(n)
+			return nil
 		}
 	}
 	if n == acct.SplitSize() {
@@ -66,8 +64,11 @@ func (Split) Execute(aerc *widgets.Aerc, args []string) error {
 		// Don't allow split to go negative
 		n = 1
 	}
-	if args[0] == "split" {
+	switch args[0] {
+	case "split":
 		return acct.Split(n)
+	case "vsplit":
+		return acct.Vsplit(n)
 	}
-	return acct.Vsplit(n)
+	return nil
 }

lib/messageview.go

@@ -72,6 +72,12 @@ func NewMessageStoreView(messageInfo *models.MessageInfo, setSeen bool,
 	store *MessageStore, pgp crypto.Provider, decryptKeys openpgp.PromptFunction,
 	cb func(MessageView, error),
 ) {
+	if messageInfo == nil {
+		// Call nils to the callback, the split view will use this to
+		// display an empty view
+		cb(nil, nil)
+		return
+	}
 	msv := &MessageStoreView{
 		messageInfo, store,
 		nil, nil, messageInfo.BodyStructure,

lib/msgstore.go

@@ -69,6 +69,7 @@ type MessageStore struct {
 	threadsMutex sync.Mutex
 
 	iterFactory iterator.Factory
+	onSelect    func(*models.MessageInfo)
 }
 
 const MagicUid = 0xFFFFFFFF
@@ -79,7 +80,7 @@ func NewMessageStore(worker *types.Worker,
 	thread bool, clientThreads bool, clientThreadsDelay time.Duration,
 	reverseOrder bool, reverseThreadOrder bool, sortThreadSiblings bool,
 	triggerNewEmail func(*models.MessageInfo),
-	triggerDirectoryChange func(),
+	triggerDirectoryChange func(), onSelect func(*models.MessageInfo),
 ) *MessageStore {
 	if !dirInfo.Caps.Thread {
 		clientThreads = true
@@ -116,6 +117,7 @@ func NewMessageStore(worker *types.Worker,
 		threadBuilderDelay: clientThreadsDelay,
 
 		iterFactory: iterator.NewFactory(reverseOrder),
+		onSelect:    onSelect,
 	}
 }
 
@@ -257,6 +259,9 @@ func (store *MessageStore) Update(msg types.WorkerMessage) {
 			merge(existing, msg.Info)
 		} else if msg.Info.Envelope != nil {
 			store.Messages[msg.Info.Uid] = msg.Info
+			if store.selectedUid == msg.Info.Uid {
+				store.onSelect(msg.Info)
+			}
 		}
 		if msg.NeedsFlags {
 			store.Lock()
@@ -597,7 +602,7 @@ func (store *MessageStore) Selected() *models.MessageInfo {
 func (store *MessageStore) SelectedUid() uint32 {
 	if store.selectedUid == MagicUid && len(store.Uids()) > 0 {
 		iter := store.UidsIterator()
-		store.selectedUid = store.Uids()[iter.StartIndex()]
+		store.Select(store.Uids()[iter.StartIndex()])
 	}
 	return store.selectedUid
 }
@@ -612,6 +617,9 @@ func (store *MessageStore) Select(uid uint32) {
 	if store.marker != nil {
 		store.marker.UpdateVisualMark()
 	}
+	if store.onSelect != nil {
+		store.onSelect(store.Selected())
+	}
 }
 
 func (store *MessageStore) NextPrev(delta int) {
@@ -639,7 +647,7 @@ func (store *MessageStore) NextPrev(delta int) {
 		store.threadsMutex.Lock()
 		store.threadCallback = func() {
 			if uids := store.Uids(); len(uids) > newIdx {
-				store.selectedUid = uids[newIdx]
+				store.Select(uids[newIdx])
 			}
 		}
 		store.threadsMutex.Unlock()

widgets/account.go

@@ -39,7 +39,6 @@ type AccountView struct {
 	split         *MessageViewer
 	splitSize     int
 	splitDebounce *time.Timer
-	splitUid      uint32
 	splitDir      string
 
 	// Check-mail ticker
@@ -158,9 +157,6 @@ func (acct *AccountView) Draw(ctx *ui.Context) {
 	if acct.state.SetWidth(ctx.Width()) {
 		acct.UpdateStatus()
 	}
-	if acct.SplitSize() > 0 {
-		acct.UpdateSplitView()
-	}
 	acct.grid.Draw(ctx)
 }
 
@@ -295,7 +291,9 @@ func (acct *AccountView) onMessage(msg types.WorkerMessage) {
 					if acct.dirlist.UiConfig(name).NewMessageBell {
 						acct.host.Beep()
 					}
-				})
+				},
+				acct.updateSplitView,
+			)
 			store.SetMarker(marker.New(store))
 			acct.dirlist.SetMsgStore(msg.Info.Name, store)
 		}
@@ -495,37 +493,33 @@ func (acct *AccountView) closeSplit() {
 		{Strategy: ui.SIZE_WEIGHT, Size: ui.Const(1)},
 	})
 
-	if acct.uiConf.SidebarWidth > 0 {
-		acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.uiConf))
-	}
+	acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.uiConf))
 	acct.grid.AddChild(acct.msglist).At(0, 1)
 	ui.Invalidate()
 }
 
-func (acct *AccountView) UpdateSplitView() {
-	if acct.Store() == nil {
-		return
-	}
-	if acct.Store().SelectedUid() == acct.splitUid {
+func (acct *AccountView) updateSplitView(msg *models.MessageInfo) {
+	if acct.splitSize == 0 {
 		return
 	}
 	if acct.splitDebounce != nil {
 		acct.splitDebounce.Stop()
 	}
 	fn := func() {
-		var err error
-		switch acct.SplitDirection() {
-		case "split":
-			err = acct.Split(acct.SplitSize())
-		case "vsplit":
-			err = acct.Vsplit(acct.SplitSize())
-		default:
-			return
-		}
-		if err != nil {
-			log.Errorf("could not update split: %v", err)
-		}
-		ui.Invalidate()
+		lib.NewMessageStoreView(msg, false, acct.Store(), acct.aerc.Crypto, acct.aerc.DecryptKeys,
+			func(view lib.MessageView, err error) {
+				if err != nil {
+					acct.aerc.PushError(err.Error())
+					return
+				}
+				acct.split = NewMessageViewer(acct, view)
+				switch acct.splitDir {
+				case "split":
+					acct.grid.AddChild(acct.split).At(1, 1)
+				case "vsplit":
+					acct.grid.AddChild(acct.split).At(0, 2)
+				}
+			})
 	}
 	acct.splitDebounce = time.AfterFunc(100*time.Millisecond, func() {
 		ui.QueueFunc(fn)
@@ -536,25 +530,24 @@ func (acct *AccountView) SplitSize() int {
 	return acct.splitSize
 }
 
-func (acct *AccountView) SplitDirection() string {
-	return acct.splitDir
+func (acct *AccountView) SetSplitSize(n int) {
+	if n == 0 {
+		acct.closeSplit()
+	}
+	acct.splitSize = n
 }
 
 // Split splits the message list view horizontally. The message list will be n
 // rows high. If n is 0, any existing split is removed
 func (acct *AccountView) Split(n int) error {
-	if n == 0 {
-		acct.closeSplit()
+	acct.SetSplitSize(n)
+	if acct.splitDir == "split" || n == 0 {
 		return nil
 	}
-	acct.splitSize = n
 	acct.splitDir = "split"
-	if acct.split != nil {
-		acct.split.Close()
-	}
 	acct.grid = ui.NewGrid().Rows([]ui.GridSpec{
 		// Add 1 so that the splitSize is the number of visible messages
-		{Strategy: ui.SIZE_EXACT, Size: ui.Const(acct.splitSize + 1)},
+		{Strategy: ui.SIZE_EXACT, Size: func() int { return acct.SplitSize() + 1 }},
 		{Strategy: ui.SIZE_WEIGHT, Size: ui.Const(1)},
 	}).Columns([]ui.GridSpec{
 		{Strategy: ui.SIZE_EXACT, Size: func() int {
@@ -563,83 +556,36 @@ func (acct *AccountView) Split(n int) error {
 		{Strategy: ui.SIZE_WEIGHT, Size: ui.Const(1)},
 	})
 
-	if acct.uiConf.SidebarWidth > 0 {
-		acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.uiConf)).Span(2, 1)
-	}
+	acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.uiConf)).Span(2, 1)
 	acct.grid.AddChild(ui.NewBordered(acct.msglist, ui.BORDER_BOTTOM, acct.uiConf)).At(0, 1)
-
-	if acct.msglist.Empty() {
-		acct.grid.AddChild(ui.NewFill(' ', tcell.StyleDefault)).At(1, 1)
-		ui.Invalidate()
-		return nil
-	}
-
-	msg, err := acct.SelectedMessage()
-	if err != nil {
-		return fmt.Errorf("could not create split: %w", err)
-	}
-	acct.splitUid = msg.Uid
-	lib.NewMessageStoreView(msg, false, acct.Store(), acct.aerc.Crypto, acct.aerc.DecryptKeys,
-		func(view lib.MessageView, err error) {
-			if err != nil {
-				acct.aerc.PushError(err.Error())
-				return
-			}
-			acct.split = NewMessageViewer(acct, view)
-			acct.grid.AddChild(acct.split).At(1, 1)
-		})
-	ui.Invalidate()
+	acct.split = NewMessageViewer(acct, nil)
+	acct.grid.AddChild(acct.split).At(1, 1)
+	acct.updateSplitView(acct.msglist.Selected())
 	return nil
 }
 
 // Vsplit splits the message list view vertically. The message list will be n
 // rows wide. If n is 0, any existing split is removed
 func (acct *AccountView) Vsplit(n int) error {
-	if n == 0 {
-		acct.closeSplit()
+	acct.SetSplitSize(n)
+	if acct.splitDir == "vsplit" || n == 0 {
 		return nil
 	}
-	acct.splitSize = n
 	acct.splitDir = "vsplit"
-	if acct.split != nil {
-		acct.split.Close()
-	}
 	acct.grid = ui.NewGrid().Rows([]ui.GridSpec{
 		{Strategy: ui.SIZE_WEIGHT, Size: ui.Const(1)},
 	}).Columns([]ui.GridSpec{
 		{Strategy: ui.SIZE_EXACT, Size: func() int {
 			return acct.UiConfig().SidebarWidth
 		}},
-		{Strategy: ui.SIZE_EXACT, Size: ui.Const(acct.splitSize)},
+		{Strategy: ui.SIZE_EXACT, Size: acct.SplitSize},
 		{Strategy: ui.SIZE_WEIGHT, Size: ui.Const(1)},
 	})
 
-	if acct.uiConf.SidebarWidth > 0 {
-		acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.uiConf)).At(0, 0)
-	}
+	acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.uiConf)).At(0, 0)
 	acct.grid.AddChild(ui.NewBordered(acct.msglist, ui.BORDER_RIGHT, acct.uiConf)).At(0, 1)
-
-	if acct.msglist.Empty() {
-		acct.grid.AddChild(ui.NewFill(' ', tcell.StyleDefault)).At(0, 2)
-		ui.Invalidate()
-		return nil
-	}
-
-	msg, err := acct.SelectedMessage()
-	if err != nil {
-		return fmt.Errorf("could not create split: %w", err)
-	}
-	acct.splitUid = msg.Uid
-
-	lib.NewMessageStoreView(msg, false, acct.Store(), acct.aerc.Crypto, acct.aerc.DecryptKeys,
-		func(view lib.MessageView, err error) {
-			if err != nil {
-				acct.aerc.PushError(err.Error())
-				return
-			}
-			acct.split = NewMessageViewer(acct, view)
-			acct.grid.AddChild(acct.split).At(0, 2)
-		})
-	ui.Invalidate()
+	acct.split = NewMessageViewer(acct, nil)
+	acct.grid.AddChild(acct.split).At(0, 2)
+	acct.updateSplitView(acct.msglist.Selected())
 	return nil
 }

widgets/msgviewer.go

@@ -48,6 +48,12 @@ type PartSwitcher struct {
 func NewMessageViewer(
 	acct *AccountView, msg lib.MessageView,
 ) *MessageViewer {
+	if msg == nil {
+		return &MessageViewer{
+			acct: acct,
+			err:  fmt.Errorf("(no message selected)"),
+		}
+	}
 	hf := HeaderLayoutFilter{
 		layout: HeaderLayout(config.Viewer.HeaderLayout),
 		keep: func(msg *models.MessageInfo, header string) bool {