From d332a78af1053e9af5893c245b69c8a96a61fe58 Mon Sep 17 00:00:00 2001 From: Rob Watson Date: Thu, 10 Apr 2025 07:04:26 +0200 Subject: [PATCH] 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 | 16 ++++- internal/app/integration_test.go | 1 + internal/terminal/terminal.go | 111 +++++++++++++++++++++++-------- 3 files changed, 99 insertions(+), 29 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 3dc26db..482a87a 100644 --- a/internal/app/app.go +++ b/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(): - applyConfig(cfg, state) - updateUI() + case <-params.ConfigService.C(): + // No-op, config updates are handled synchronously for now. 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 diff --git a/internal/app/integration_test.go b/internal/app/integration_test.go index 851614e..e08b547 100644 --- a/internal/app/integration_test.go +++ b/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, ' ') diff --git a/internal/terminal/terminal.go b/internal/terminal/terminal.go index 6b900e4..d0d77b0 100644 --- a/internal/terminal/terminal.go +++ b/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) - - if ui.screenCaptureC != nil { - app.SetAfterDrawFunc(ui.captureScreen) - } + app.SetInputCapture(ui.inputCaptureHandler) + app.SetAfterDrawFunc(ui.afterDrawHandler) 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() { - if name, el := ui.pages.GetFrontPage(); name == pageNameMain || name == pageNameNoDestinations { - 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) - } -} - +// modalVisible returns true if any modal, including the add destination form, +// is visible. 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() {