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() {
+	// Read the name before switching. A message from the server can
-		buf := a.ui.buffers[bufIndex]
+	// add a buffer at any time, but only at the end, so bufIndex
-		if buf.Name != "(status)" {
+	// 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()

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
+		select {
-		if err != nil {
+		case err := <-done:
-			t.Errorf("run ui: %v", err)
+			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")
 		}
 	})
 }

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.
+// BufferName returns the name of the buffer at index bufIndex,
-func (ui *UI) BufferCount() int {
+// and whether there is such a buffer.
-	return len(ui.buffers)
+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("")
-
+		ui.callOnInput(text)
-		if ui.onInput != nil {
-			ui.onInput(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.callOnInput("/window " + string(r))
-					ui.onInput("/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.
 }