diff --git a/internal/cli/app.go b/internal/cli/app.go index acadfc3..c8a1fb4 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -610,21 +610,24 @@ func (a *App) cmdWindow(args string) { _, _ = fmt.Sscanf(args, "%d", &bufIndex) - a.ui.SwitchBuffer(bufIndex) - a.mu.Lock() nick := a.nick a.mu.Unlock() - if bufIndex >= 0 && bufIndex < a.ui.BufferCount() { - buf := a.ui.buffers[bufIndex] - if buf.Name != "(status)" { + // Read the name before switching. A message from the server can + // add a buffer at any time, but only at the end, so bufIndex + // still names this buffer when SwitchBuffer runs. + name, found := a.ui.BufferName(bufIndex) + if found { + a.ui.SwitchBuffer(bufIndex) + + if name != "(status)" { a.mu.Lock() - a.target = buf.Name + a.target = name a.mu.Unlock() a.ui.SetStatus( - nick, buf.Name, "connected", + nick, name, "connected", ) } else { a.mu.Lock() diff --git a/internal/cli/app_internal_test.go b/internal/cli/app_internal_test.go index 54586f2..3f133a5 100644 --- a/internal/cli/app_internal_test.go +++ b/internal/cli/app_internal_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/gdamore/tcell/v2" api "sneak.berlin/go/neoirc/internal/cli/api" @@ -20,6 +21,14 @@ const ( testText = "hello" ) +// eventLoopTimeout is how long a test waits for a typed line to +// be handled, or for the UI to stop, before it fails. +const eventLoopTimeout = 5 * time.Second + +// holdTime is how long TestLinesKeepOrder holds the first line, +// which gives the second line time to start too early. +const holdTime = 200 * time.Millisecond + func TestMemberPrefix(t *testing.T) { t.Parallel() @@ -251,7 +260,7 @@ func TestAltDigit(t *testing.T) { t.Parallel() app := &App{ui: NewUI(), nick: testNick, target: testOtherNick} - app.ui.OnInput(app.handleInput) + handled := handledLines(app) runUI(t, app.ui) // Window 1 is the channel. Window 2, the one shown, is @@ -260,7 +269,8 @@ func TestAltDigit(t *testing.T) { app.ui.SwitchToBuffer(testOtherNick) key := tcell.NewEventKey(tcell.KeyRune, test.digit, tcell.ModAlt) - app.ui.app.GetInputCapture()(key) + app.ui.app.QueueEvent(key) + waitHandled(t, handled, "/window "+string(test.digit)) var shown string @@ -276,6 +286,68 @@ func TestAltDigit(t *testing.T) { } } +// TestEventLoopKeepsRunning sends Alt and 1, then a typed line and +// Enter, to a running UI. It checks that both are handled, so the +// event loop did not stop on either. +func TestEventLoopKeepsRunning(t *testing.T) { + t.Parallel() + + app := &App{ui: NewUI(), nick: testNick} + handled := handledLines(app) + runUI(t, app.ui) + + // Window 1 is the channel, and window 0, the status window, is + // shown. The typed line adds a line to the status window, as + // the client is not connected. + app.ui.SwitchToBuffer(testChannel) + app.ui.SwitchBuffer(0) + + app.ui.app.QueueEvent(tcell.NewEventKey(tcell.KeyRune, '1', tcell.ModAlt)) + + for _, r := range testText { + app.ui.app.QueueEvent(tcell.NewEventKey(tcell.KeyRune, r, tcell.ModNone)) + } + + app.ui.app.QueueEvent(tcell.NewEventKey(tcell.KeyEnter, 0, tcell.ModNone)) + + waitHandled(t, handled, "/window 1") + waitHandled(t, handled, testText) +} + +// TestLinesKeepOrder holds the handling of a first line until a +// second line has been passed in. It fails if the second line is +// handled while the first is still held. +func TestLinesKeepOrder(t *testing.T) { + t.Parallel() + + const firstLine, secondLine = "one", "two" + + ui := NewUI() + handled := make(chan string) + release := make(chan struct{}) + + ui.OnInput(func(text string) { + if text == firstLine { + <-release + } + + handled <- text + }) + + ui.callOnInput(firstLine) + ui.callOnInput(secondLine) + + select { + case got := <-handled: + t.Fatalf("handled %q while the first line was held", got) + case <-time.After(holdTime): + } + + close(release) + waitHandled(t, handled, firstLine) + waitHandled(t, handled, secondLine) +} + // TestOwnEventFromOtherClient checks that a message, notice, // join or part that the user's session makes from another // client shows as one line in the buffer it belongs to. @@ -358,6 +430,36 @@ func bufferState(ui *UI, name string) (int, int) { return lines, unread } +// handledLines makes app.ui pass each typed line to +// app.handleInput, as Run does, and returns a channel that +// receives each line once it has been handled. +func handledLines(app *App) <-chan string { + handled := make(chan string) + + app.ui.OnInput(func(text string) { + app.handleInput(text) + + handled <- text + }) + + return handled +} + +// waitHandled fails the test unless want is the next line handled, +// within eventLoopTimeout. +func waitHandled(t *testing.T, handled <-chan string, want string) { + t.Helper() + + select { + case got := <-handled: + if got != want { + t.Fatalf("handled %q, want %q", got, want) + } + case <-time.After(eventLoopTimeout): + t.Fatalf("%q was not handled within %v", want, eventLoopTimeout) + } +} + // newServer starts a server that answers every request with // body until the test ends. func newServer(t *testing.T, body string) *httptest.Server { @@ -379,16 +481,22 @@ func runUI(t *testing.T, ui *UI) { ui.app.SetScreen(tcell.NewSimulationScreen("")) - done := make(chan error) + // One slot, so that Run's goroutine can end even after the + // cleanup below has stopped waiting for it. + done := make(chan error, 1) go func() { done <- ui.Run() }() t.Cleanup(func() { ui.Stop() - err := <-done - if err != nil { - t.Errorf("run ui: %v", err) + select { + case err := <-done: + if err != nil { + t.Errorf("run ui: %v", err) + } + case <-time.After(eventLoopTimeout): + t.Error("the UI did not stop: its event loop stopped responding") } }) } diff --git a/internal/cli/ui.go b/internal/cli/ui.go index 581ba0b..563df0d 100644 --- a/internal/cli/ui.go +++ b/internal/cli/ui.go @@ -16,7 +16,10 @@ type Buffer struct { Unread int } -// UI manages the terminal interface. +// UI manages the terminal interface. Most of its methods wait for +// the tview event loop to run their update. Code that runs on that +// loop, such as a key handler, must not call them: it would wait +// for itself. type UI struct { app *tview.Application messages *tview.TextView @@ -28,6 +31,10 @@ type UI struct { currentBuffer int onInput func(string) + // inputDone is closed once the line last passed to onInput + // has been handled. Only callOnInput, on the event loop, uses + // it. + inputDone chan struct{} } // NewUI creates the tview-based IRC-like UI. @@ -176,9 +183,24 @@ func (ui *UI) SetStatus( }) } -// BufferCount returns the number of buffers. -func (ui *UI) BufferCount() int { - return len(ui.buffers) +// BufferName returns the name of the buffer at index bufIndex, +// and whether there is such a buffer. +func (ui *UI) BufferName(bufIndex int) (string, bool) { + var ( + name string + found bool + ) + + ui.app.QueueUpdate(func() { + if bufIndex < 0 || bufIndex >= len(ui.buffers) { + return + } + + name = ui.buffers[bufIndex].Name + found = true + }) + + return name, found } // BufferIndex returns the index of a named buffer. @@ -226,10 +248,7 @@ func (ui *UI) initInput() { } ui.input.SetText("") - - if ui.onInput != nil { - ui.onInput(text) - } + ui.callOnInput(text) }) } @@ -244,9 +263,7 @@ func (ui *UI) initKeyCapture() { // SwitchBuffer alone would leave the target as it was. r := event.Rune() if r >= '0' && r <= '9' { - if ui.onInput != nil { - ui.onInput("/window " + string(r)) - } + ui.callOnInput("/window " + string(r)) return nil } @@ -256,6 +273,31 @@ func (ui *UI) initKeyCapture() { ) } +// callOnInput passes text to the input callback on a new +// goroutine. The key handlers call it on the event loop, and the +// callback waits for that loop to update the screen, so they must +// not wait for the callback. The goroutine first waits for the +// line before it to be handled, which keeps lines in the order +// they were typed. +func (ui *UI) callOnInput(text string) { + if ui.onInput == nil { + return + } + + previous := ui.inputDone + done := make(chan struct{}) + ui.inputDone = done + + go func() { + if previous != nil { + <-previous + } + + ui.onInput(text) + close(done) + }() +} + func (ui *UI) refreshStatusBar() { // Placeholder; full refresh needs nick/target context. }