fix(ui): further key handling improvements

Avoids losing user destination selection when re-rendering the page,
especially after adding or removing a destination.

internal/app/app.go

@@ -120,9 +120,8 @@ func Run(ctx context.Context, params RunParams) error {
 					return fmt.Errorf("start mediaserver: %w", err)
 				}
 			}
-		case cfg = <-params.ConfigService.C():
+		case <-params.ConfigService.C():
-			applyConfig(cfg, state)
+			// No-op, config updates are handled synchronously for now.
-			updateUI()
 		case cmd, ok := <-ui.C():
 			if !ok {
 				// TODO: keep UI open until all containers have closed
@@ -143,6 +142,8 @@ func Run(ctx context.Context, params RunParams) error {
 					ui.ConfigUpdateFailed(err)
 					continue
 				}
+				cfg = newCfg
+				handleConfigUpdate(cfg, state, ui)
 				ui.DestinationAdded()
 			case terminal.CommandRemoveDestination:
 				repl.StopDestination(c.URL) // no-op if not live
@@ -155,6 +156,9 @@ func Run(ctx context.Context, params RunParams) error {
 					ui.ConfigUpdateFailed(err)
 					continue
 				}
+				cfg = newCfg
+				handleConfigUpdate(cfg, state, ui)
+				ui.DestinationRemoved()
 			case terminal.CommandStartDestination:
 				if !state.Source.Live {
 					ui.ShowSourceNotLiveModal()
@@ -186,6 +190,12 @@ func Run(ctx context.Context, params RunParams) error {
 	}
 }
 
+// handleConfigUpdate applies the config to the app state, and updates the UI.
+func handleConfigUpdate(cfg config.Config, appState *domain.AppState, ui *terminal.UI) {
+	applyConfig(cfg, appState)
+	ui.SetState(*appState)
+}
+
 // applyServerState applies the current server state to the app state.
 func applyServerState(serverState domain.Source, appState *domain.AppState) {
 	appState.Source = serverState

internal/app/integration_test.go

@@ -189,6 +189,7 @@ func TestIntegration(t *testing.T) {
 	)
 	printScreen(getContents, "After starting the destination streams")
 
+	sendKey(screen, tcell.KeyUp, ' ')
 	sendKey(screen, tcell.KeyDelete, ' ')
 	sendKey(screen, tcell.KeyEnter, ' ')
 

internal/terminal/terminal.go

@@ -68,6 +68,9 @@ type UI struct {
 	// hasDestinations is true if the UI thinks there are destinations
 	// configured.
 	hasDestinations bool
+	// lastSelectedDestIndex is the index of the last selected destination, starting
+	// at 1 (because 0 is the header).
+	lastSelectedDestIndex int
 }
 
 // Screen represents a terminal screen. This includes its desired dimensions,
@@ -242,11 +245,8 @@ func StartUI(ctx context.Context, params StartParams) (*UI, error) {
 		urlsToStartState:  make(map[string]startState),
 	}
 
-	app.SetInputCapture(ui.handleInputCapture)
+	app.SetInputCapture(ui.inputCaptureHandler)
-
+	app.SetAfterDrawFunc(ui.afterDrawHandler)
-	if ui.screenCaptureC != nil {
-		app.SetAfterDrawFunc(ui.captureScreen)
-	}
 
 	go ui.run(ctx)
 
@@ -282,7 +282,7 @@ func (ui *UI) run(ctx context.Context) {
 	}
 }
 
-func (ui *UI) handleInputCapture(event *tcell.EventKey) *tcell.EventKey {
+func (ui *UI) inputCaptureHandler(event *tcell.EventKey) *tcell.EventKey {
 	// Special case: handle CTRL-C even when a modal is visible.
 	if event.Key() == tcell.KeyCtrlC {
 		ui.confirmQuit()
@@ -398,6 +398,14 @@ func (ui *UI) ShowFatalErrorModal(err error) {
 	})
 }
 
+func (ui *UI) afterDrawHandler(screen tcell.Screen) {
+	if ui.screenCaptureC == nil {
+		return
+	}
+
+	ui.captureScreen(screen)
+}
+
 // captureScreen captures the screen and sends it to the screenCaptureC
 // channel, which must have been set in StartParams.
 //
@@ -519,26 +527,63 @@ const (
 	pageNameModalStartupCheck      = "modal-startup-check"
 )
 
-func (ui *UI) resetFocus() {
+// modalVisible returns true if any modal, including the add destination form,
-	if name, el := ui.pages.GetFrontPage(); name == pageNameMain || name == pageNameNoDestinations {
+// is visible.
-		ui.app.SetFocus(ui.destView)
-		// If we don't explicitly set the focus to some row, then sometimes no row
-		// is selected and the user must press up or down to select a row before
-		// continuing. This isn't completely a blocker but it is sometime a bit
-		// confusing and also makes integration tests less predictable. It would be
-		// nice to improve this behaviour so that e.g. if a new destination is
-		// added then that destination is selected, not the first in the row.
-		ui.destView.Select(1, 0)
-	} else {
-		ui.app.SetFocus(el)
-	}
-}
-
 func (ui *UI) modalVisible() bool {
 	pageName, _ := ui.pages.GetFrontPage()
 	return pageName != pageNameMain && pageName != pageNameNoDestinations
 }
 
+// saveSelectedDestination saves the last selected destination index to local
+// mutable state.
+//
+// This is needed so that the user's selection can be restored
+// after redrawing the screen. It may be possible to remove this if we can
+// re-render the screen more selectively instead of calling [redrawFromState]
+// every time the state changes.
+func (ui *UI) saveSelectedDestination() {
+	row, _ := ui.destView.GetSelection()
+	ui.mu.Lock()
+	ui.lastSelectedDestIndex = row
+	ui.mu.Unlock()
+}
+
+// selectPreviousDestination sets the focus to the last-selected destination.
+func (ui *UI) selectPreviousDestination() {
+	if ui.modalVisible() {
+		return
+	}
+
+	var row int
+	ui.mu.Lock()
+	row = ui.lastSelectedDestIndex
+	ui.mu.Unlock()
+
+	// If the last element has been removed, select the new last element.
+	row = min(ui.destView.GetRowCount()-1, row)
+
+	ui.app.SetFocus(ui.destView)
+
+	if row == 0 {
+		return
+	}
+
+	ui.destView.Select(row, 0)
+}
+
+// selectLastDestination sets the user selection to the last destination.
+func (ui *UI) selectLastDestination() {
+	if ui.modalVisible() {
+		return
+	}
+
+	ui.app.SetFocus(ui.destView)
+
+	if rowCount := ui.destView.GetRowCount(); rowCount > 1 {
+		ui.destView.Select(rowCount-1, 0)
+	}
+}
+
 func (ui *UI) showModal(pageName string, text string, buttons []string, doneFunc func(int, string)) {
 	if ui.pages.HasPage(pageName) {
 		return
@@ -553,17 +598,19 @@ func (ui *UI) showModal(pageName string, text string, buttons []string, doneFunc
 			ui.pages.RemovePage(pageName)
 
 			if !ui.modalVisible() {
-				ui.app.SetInputCapture(ui.handleInputCapture)
+				ui.app.SetInputCapture(ui.inputCaptureHandler)
 			}
 
-			ui.resetFocus()
-
 			if doneFunc != nil {
 				doneFunc(buttonIndex, buttonLabel)
 			}
+
+			ui.selectPreviousDestination()
 		}).
 		SetBorderStyle(tcell.StyleDefault.Background(tcell.ColorBlack).Foreground(tcell.ColorWhite))
 
+	ui.saveSelectedDestination()
+
 	ui.pages.AddPage(pageName, modal, true, true)
 }
 
@@ -784,7 +831,10 @@ func (ui *UI) addDestination() {
 				URL:             form.GetFormItemByLabel(inputLabelURL).(*tview.InputField).GetText(),
 			}
 		}).
-		AddButton("Cancel", func() { ui.closeAddDestinationForm() }).
+		AddButton("Cancel", func() {
+			ui.closeAddDestinationForm()
+			ui.selectPreviousDestination()
+		}).
 		SetFieldBackgroundColor(tcell.ColorDarkSlateGrey).
 		SetBorder(true).
 		SetTitle("Add a new destination").
@@ -792,6 +842,7 @@ func (ui *UI) addDestination() {
 		SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey {
 			if event.Key() == tcell.KeyEscape {
 				ui.closeAddDestinationForm()
+				ui.selectPreviousDestination()
 				return nil
 			}
 			return event
@@ -802,6 +853,8 @@ func (ui *UI) addDestination() {
 	ui.addingDestination = true
 	ui.mu.Unlock()
 
+	ui.saveSelectedDestination()
+
 	ui.pages.HidePage(pageNameNoDestinations)
 	ui.pages.AddPage(pageNameAddDestination, form, false, true)
 }
@@ -836,6 +889,7 @@ func (ui *UI) removeDestination() {
 	)
 }
 
+// DestinationAdded should be called when a new destination is added.
 func (ui *UI) DestinationAdded() {
 	ui.mu.Lock()
 	ui.hasDestinations = true
@@ -844,9 +898,15 @@ func (ui *UI) DestinationAdded() {
 	ui.app.QueueUpdateDraw(func() {
 		ui.pages.HidePage(pageNameNoDestinations)
 		ui.closeAddDestinationForm()
+		ui.selectLastDestination()
 	})
 }
 
+// DestinationRemoved should be called when a destination is removed.
+func (ui *UI) DestinationRemoved() {
+	ui.selectPreviousDestination()
+}
+
 func (ui *UI) closeAddDestinationForm() {
 	var hasDestinations bool
 	ui.mu.Lock()
@@ -858,7 +918,6 @@ func (ui *UI) closeAddDestinationForm() {
 	if !hasDestinations {
 		ui.pages.ShowPage(pageNameNoDestinations)
 	}
-	ui.resetFocus()
 }
 
 func (ui *UI) toggleDestination() {