diff --git a/admin.go b/admin.go index 2c47ec7..52d0500 100644 --- a/admin.go +++ b/admin.go @@ -65,7 +65,7 @@ func (b *Barnard) OpenAdminMenu() { return } b.adminReturnItem = b.UiTree.ActiveItem() - b.adminTargetUser = b.selectedUser + b.adminTargetUser = b.selectedUserValue() b.adminTargetChan = b.Client.Self.Channel if b.Ui.Active() == uiViewTree { switch item := b.UiTree.ActiveItem().(type) { @@ -83,7 +83,9 @@ func (b *Barnard) OpenAdminMenu() { if b.adminTargetChan != nil { b.adminTargetChan.RequestPermission() } - if root := b.Client.Channels[0]; root != nil && root != b.adminTargetChan { + var root *gumble.Channel + b.Client.Do(func() { root = b.Client.Channels[0] }) + if root != nil && root != b.adminTargetChan { root.RequestPermission() } b.UiAdmin.Rebuild() @@ -490,11 +492,21 @@ func (b *Barnard) adminACLItems() []uiterm.TreeItem { } func (b *Barnard) adminContextActionItems() []uiterm.TreeItem { - if b.Client == nil || len(b.Client.ContextActions) == 0 { + if b.Client == nil { + return []uiterm.TreeItem{adminItem{label: "No context actions available"}} + } + var actions []*gumble.ContextAction + b.Client.Do(func() { + actions = make([]*gumble.ContextAction, 0, len(b.Client.ContextActions)) + for _, action := range b.Client.ContextActions { + actions = append(actions, action) + } + }) + if len(actions) == 0 { return []uiterm.TreeItem{adminItem{label: "No context actions available"}} } items := []uiterm.TreeItem{} - for _, action := range b.Client.ContextActions { + for _, action := range actions { ca := action label := ca.Label if label == "" { @@ -854,7 +866,8 @@ func (b *Barnard) executeContextCommand(fields []string) { b.AddOutputLine("Admin: usage /admin context [server|user|channel] [target]") return } - action := b.Client.ContextActions[fields[1]] + var action *gumble.ContextAction + b.Client.Do(func() { action = b.Client.ContextActions[fields[1]] }) if action == nil { b.AddOutputLine("Admin: context action not found") return @@ -993,39 +1006,47 @@ func (b *Barnard) findOrCreateACLRule(subjectType, subject string) *gumble.ACLRu } } -func (b *Barnard) findUser(token string) *gumble.User { +func (b *Barnard) findUser(token string) (found *gumble.User) { if b.Client == nil { return nil } - if session, err := strconv.ParseUint(token, 10, 32); err == nil { - if user := b.Client.Users[uint32(session)]; user != nil { - return user + b.Client.Do(func() { + if session, err := strconv.ParseUint(token, 10, 32); err == nil { + found = b.Client.Users[uint32(session)] + if found != nil { + return + } } - } - for _, user := range b.Client.Users { - if strings.EqualFold(user.Name, token) { - return user + for _, user := range b.Client.Users { + if strings.EqualFold(user.Name, token) { + found = user + return + } } - } - return nil + }) + return found } -func (b *Barnard) findChannel(token string) *gumble.Channel { +func (b *Barnard) findChannel(token string) (found *gumble.Channel) { if b.Client == nil { return nil } token = strings.TrimSpace(token) - if id, err := strconv.ParseUint(token, 10, 32); err == nil { - if channel := b.Client.Channels[uint32(id)]; channel != nil { - return channel + b.Client.Do(func() { + if id, err := strconv.ParseUint(token, 10, 32); err == nil { + found = b.Client.Channels[uint32(id)] + if found != nil { + return + } } - } - for _, channel := range b.Client.Channels { - if strings.EqualFold(channel.Name, token) { - return channel + for _, channel := range b.Client.Channels { + if strings.EqualFold(channel.Name, token) { + found = channel + return + } } - } - return nil + }) + return found } func (b *Barnard) findRegisteredUser(token string) *gumble.RegisteredUser { @@ -1048,7 +1069,9 @@ func (b *Barnard) adminCanRoot(permission gumble.Permission) bool { if b.Client == nil { return true } - return b.adminCanChannel(b.Client.Channels[0], permission) + var root *gumble.Channel + b.Client.Do(func() { root = b.Client.Channels[0] }) + return b.adminCanChannel(root, permission) } func (b *Barnard) adminCanChannel(channel *gumble.Channel, permission gumble.Permission) bool { diff --git a/admin_test.go b/admin_test.go index 8271173..7e1c169 100644 --- a/admin_test.go +++ b/admin_test.go @@ -7,6 +7,20 @@ import ( "git.stormux.org/storm/barnard/uiterm" ) +// Regression: admin lookup helpers read Client.Users and Client.Channels while +// TCP handlers could mutate those maps. +func TestAdminLookupUsesClientSnapshot(t *testing.T) { + user := &gumble.User{Session: 7, Name: "Guest"} + channel := &gumble.Channel{ID: 4, Name: "Room"} + b := &Barnard{Client: &gumble.Client{Users: gumble.Users{7: user}, Channels: gumble.Channels{4: channel}}} + if b.findUser("guest") != user || b.findUser("7") != user { + t.Fatal("user lookup failed") + } + if b.findChannel("room") != channel || b.findChannel("4") != channel { + t.Fatal("channel lookup failed") + } +} + func TestParseToggleState(t *testing.T) { tests := []struct { name string diff --git a/barnard.go b/barnard.go index 04e1e02..43688db 100644 --- a/barnard.go +++ b/barnard.go @@ -14,8 +14,12 @@ import ( ) type TreeItem struct { - User *gumble.User - Channel *gumble.Channel + User *gumble.User + Channel *gumble.Channel + display string + userSession uint32 + channelID uint32 + snapshot bool } type Barnard struct { @@ -27,25 +31,28 @@ type Barnard struct { Address string TLSConfig tls.Config - Stream *gumbleopenal.Stream - Tx bool - AutoTransmit bool // auto-start transmission on connect - Connected bool + Stream *gumbleopenal.Stream + connectionMutex sync.RWMutex + Tx bool + AutoTransmit bool // auto-start transmission on connect + Connected bool + stateMutex sync.RWMutex - Ui *uiterm.Ui - UiOutput uiterm.Textview - UiInput uiterm.Textbox - UiStatus uiterm.Label - UiTree uiterm.Tree - UiAdmin uiterm.Tree - UiInputStatus uiterm.Label - SelectedChannel *gumble.Channel - selectedUser *gumble.User - adminTargetUser *gumble.User - adminTargetChan *gumble.Channel - adminReturnItem uiterm.TreeItem - statusText string - statusNotice bool + Ui *uiterm.Ui + UiOutput uiterm.Textview + UiInput uiterm.Textbox + UiStatus uiterm.Label + UiTree uiterm.Tree + UiAdmin uiterm.Tree + UiInputStatus uiterm.Label + SelectedChannel *gumble.Channel + selectedUser *gumble.User + selectedUserMutex sync.RWMutex + adminTargetUser *gumble.User + adminTargetChan *gumble.Channel + adminReturnItem uiterm.TreeItem + statusText string + statusNotice bool notifyChannel chan []string @@ -53,8 +60,9 @@ type Barnard struct { exitMessage string // Added for channel muting - MutedChannels map[uint32]bool - userChannels map[uint32]*gumble.Channel + MutedChannels map[uint32]bool + MutedChannelsMutex sync.RWMutex + userChannels map[uint32]*gumble.Channel // Added for noise suppression NoiseSuppressor *noise.Suppressor @@ -80,6 +88,30 @@ type Barnard struct { adminBanList gumble.BanList adminUserList gumble.RegisteredUsers adminACL *gumble.ACL + + reconnectStop chan struct{} + reconnectStopOnce sync.Once +} + +// cleanupConnectionAudio releases connection-owned audio resources before a +// reconnect replaces them. It is intentionally idempotent for repeated +// disconnect notifications. +func (b *Barnard) cleanupConnectionAudio() { + // Connection audio operations that use both resources take FileStreamMutex + // before connectionMutex, so cleanup follows that order as well. + b.FileStreamMutex.Lock() + if b.FileStream != nil { + _ = b.FileStream.Stop() + b.FileStream = nil + } + b.FileStreamMutex.Unlock() + b.connectionMutex.Lock() + if b.Stream != nil { + stream := b.Stream + b.Stream = nil + stream.Destroy() + } + b.connectionMutex.Unlock() } func (b *Barnard) cleanupToneTestAudio() { @@ -93,12 +125,113 @@ func (b *Barnard) cleanupToneTestAudio() { } } +func (b *Barnard) updateUserGain(user *gumble.User) { + b.withStream(func(stream *gumbleopenal.Stream) { + stream.UpdateUserGain(user) + }) +} + +// withStream keeps a connection-owned stream alive for the complete operation. +// Reconnect cleanup takes the write lock before destroying or replacing it. +func (b *Barnard) withStream(action func(*gumbleopenal.Stream)) bool { + b.connectionMutex.RLock() + defer b.connectionMutex.RUnlock() + if b.Stream == nil { + return false + } + action(b.Stream) + return true +} + +func (b *Barnard) isChannelMuted(channelID uint32) bool { + b.MutedChannelsMutex.RLock() + defer b.MutedChannelsMutex.RUnlock() + return b.MutedChannels[channelID] +} + +func (b *Barnard) setChannelMuted(channelID uint32, muted bool) { + b.MutedChannelsMutex.Lock() + defer b.MutedChannelsMutex.Unlock() + if b.MutedChannels == nil { + b.MutedChannels = make(map[uint32]bool) + } + if muted { + b.MutedChannels[channelID] = true + } else { + delete(b.MutedChannels, channelID) + } +} + +func (b *Barnard) selectedUserValue() *gumble.User { + b.selectedUserMutex.RLock() + defer b.selectedUserMutex.RUnlock() + return b.selectedUser +} + +func (b *Barnard) setSelectedUserValue(user *gumble.User) { + b.selectedUserMutex.Lock() + b.selectedUser = user + b.selectedUserMutex.Unlock() +} + +func (b *Barnard) isTransmitting() bool { + b.stateMutex.RLock() + defer b.stateMutex.RUnlock() + return b.Tx +} + +func (b *Barnard) setTransmitting(transmitting bool) { + b.stateMutex.Lock() + b.Tx = transmitting + b.stateMutex.Unlock() +} + +func (b *Barnard) isConnected() bool { + b.stateMutex.RLock() + defer b.stateMutex.RUnlock() + return b.Connected +} + +func (b *Barnard) setConnected(connected bool) { + b.stateMutex.Lock() + b.Connected = connected + b.stateMutex.Unlock() +} + +func (b *Barnard) stopReconnects() { + b.reconnectStopOnce.Do(func() { + if b.reconnectStop != nil { + close(b.reconnectStop) + } + }) +} + +func (b *Barnard) reconnectCanceled() bool { + if b.reconnectStop == nil { + return false + } + select { + case <-b.reconnectStop: + return true + default: + return false + } +} + func (b *Barnard) StopTransmission() { - if b.Tx { + if b.isTransmitting() { b.Notify("micdown", "me", "") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus(" Idle ", false) - b.Stream.StopSource() + if b.ToneTest { + // Stop the tone generator. + if b.toneTestStop != nil { + close(b.toneTestStop) + b.toneTestStop = nil + } + } else { + b.withStream(func(stream *gumbleopenal.Stream) { _ = stream.StopSource() }) + } } } @@ -114,7 +247,7 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm b.GotoChat() } if treeItem.User != nil { - if b.selectedUser == treeItem.User { + if b.selectedUserValue() == treeItem.User { b.SetSelectedUser(nil) b.GotoChat() } else { @@ -128,36 +261,29 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm if treeItem.Channel != nil { if key == *b.Hotkeys.MuteToggle { // Determine new channel mute state - channelWillBeMuted := !b.MutedChannels[treeItem.Channel.ID] + channelWillBeMuted := !b.isChannelMuted(treeItem.Channel.ID) // Set all users in channel to the same mute state users := makeUsersArray(treeItem.Channel.Users) for _, u := range users { // Explicitly set user mute state to match channel state - if channelWillBeMuted != u.LocallyMuted() { + if channelWillBeMuted && !u.LocallyMuted() { + if err := b.UserConfig.ToggleMute(u); err != nil { + b.AddOutputLine("Mute: could not save setting: " + err.Error()) + } + } else if !channelWillBeMuted && u.LocallyMuted() { if err := b.UserConfig.ToggleMute(u); err != nil { b.AddOutputLine("Mute: could not save setting: " + err.Error()) } } - if source := u.AudioSource(); source != nil { - if u.LocallyMuted() { - source.SetGain(0) - } else { - source.SetGain(u.Volume()) - } - } + b.updateUserGain(u) } // Update channel mute state - if channelWillBeMuted { - b.MutedChannels[treeItem.Channel.ID] = true - // If this is the current channel, stop transmission - if b.Client.Self.Channel.ID == treeItem.Channel.ID && b.Tx { - b.StopTransmission() - } - } else { - delete(b.MutedChannels, treeItem.Channel.ID) + b.setChannelMuted(treeItem.Channel.ID, channelWillBeMuted) + if channelWillBeMuted && b.Client.Self.Channel.ID == treeItem.Channel.ID && b.isTransmitting() { + b.StopTransmission() } b.RebuildUserChannelTreePreservingSelection() @@ -180,13 +306,7 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm if err := b.UserConfig.ToggleMute(treeItem.User); err != nil { b.AddOutputLine("Mute: could not save setting: " + err.Error()) } - if source := treeItem.User.AudioSource(); source != nil { - if treeItem.User.LocallyMuted() { - source.SetGain(0) - } else { - source.SetGain(treeItem.User.Volume()) - } - } + b.updateUserGain(treeItem.User) b.RebuildUserChannelTreePreservingSelection() b.Ui.Refresh() } diff --git a/client.go b/client.go index a5f8ac8..3d4e7a4 100644 --- a/client.go +++ b/client.go @@ -14,6 +14,7 @@ import ( ) func (b *Barnard) start() { + b.reconnectStop = make(chan struct{}) b.Config.Attach(gumbleutil.AutoBitrate) b.Config.Attach(b) b.Config.Address = b.Address @@ -46,7 +47,7 @@ func (b *Barnard) exitWithError(err error) { func (b *Barnard) connect(reconnect bool) bool { var err error - _, err = gumble.DialWithDialer(new(net.Dialer), b.Config, &b.TLSConfig) + _, err = gumble.DialWithDialer(&net.Dialer{Timeout: 15 * time.Second}, b.Config, &b.TLSConfig) if err != nil { if reconnect { b.Log(err.Error()) @@ -70,11 +71,11 @@ func (b *Barnard) connect(reconnect bool) bool { b.toneTestSaver = saver b.toneTestSaverDetach = b.Client.Config.AttachAudio(saver) - b.Connected = true + b.setConnected(true) if b.toneTestAutoTransmit() { b.toneTestStop = make(chan struct{}) go StartToneGenerator(b.Client, b.toneTestStop) - b.Tx = true + b.setTransmitting(true) b.UpdateGeneralStatus(" Tx ", true) b.AddOutputLine("Tone test transmission started") } @@ -86,11 +87,17 @@ func (b *Barnard) connect(reconnect bool) bool { b.exitWithError(err) return false } - b.Stream = stream - b.Stream.SetMicVolume(b.UserConfig.GetMicVolume(), false) - b.Stream.AttachStream(b.Client) - b.Stream.SetNoiseProcessor(b.NoiseSuppressor) - b.Stream.SetAGCEnabled(b.UserConfig.GetAGCEnabled()) + stream.SetMicVolume(b.UserConfig.GetMicVolume(), false) + stream.AttachStream(b.Client) + stream.SetNoiseProcessor(b.NoiseSuppressor) + stream.SetAGCEnabled(b.UserConfig.GetAGCEnabled()) + stream.SetErrorFunc(func(err error) { + if err != nil { + b.AddOutputLine(fmt.Sprintf("Microphone: %s", err.Error())) + } else { + b.AddOutputLine("Microphone: recovered") + } + }) // Initialize stereo encoder for file playback b.Client.SetStereoEncoder(opus.NewStereoEncoder()) @@ -103,10 +110,13 @@ func (b *Barnard) connect(reconnect bool) bool { b.Client.DisableStereoEncoder() b.AddOutputLine(fmt.Sprintf("File playback: %s", err.Error())) }) - b.Stream.SetFilePlayer(b.FileStream) + stream.SetFilePlayer(b.FileStream) b.FileStreamMutex.Unlock() + b.connectionMutex.Lock() + b.Stream = stream + b.connectionMutex.Unlock() - b.Connected = true + b.setConnected(true) // Dial delivers OnConnect before connect creates the OpenAL stream, so // start auto-transmit here as well for initial connections and reconnects. b.startAutoTransmit() @@ -117,18 +127,29 @@ func (b *Barnard) OnConnect(e *gumble.ConnectEvent) { b.Client = e.Client // Reset muted channels state on connect + b.MutedChannelsMutex.Lock() b.MutedChannels = make(map[uint32]bool) + b.MutedChannelsMutex.Unlock() b.userChannels = make(map[uint32]*gumble.Channel) b.RecordingMutex.Lock() b.recordingAllowed = nil b.recordingStarting = false b.RecordingMutex.Unlock() - b.Ui.SetActive(uiViewInput) - b.UiTree.Rebuild() - b.Ui.Refresh() + b.postUI(func() { + b.Ui.SetActive(uiViewInput) + b.UiTree.Rebuild() + b.Ui.Refresh() + }) - for _, u := range b.Client.Users { + var users []*gumble.User + b.Client.Do(func() { + users = make([]*gumble.User, 0, len(b.Client.Users)) + for _, u := range b.Client.Users { + users = append(users, u) + } + }) + for _, u := range users { b.UserConfig.UpdateUser(u) b.rememberUserChannel(u) } @@ -143,22 +164,26 @@ func (b *Barnard) OnConnect(e *gumble.ConnectEvent) { if wmsg != "" { b.AddOutputLine(fmt.Sprintf("Welcome message: %s", wmsg)) } - b.Ui.Refresh() b.startAutoTransmit() } func (b *Barnard) startAutoTransmit() { - if !b.AutoTransmit || b.Tx || b.Stream == nil { + if !b.AutoTransmit || b.isTransmitting() { return } - if err := b.Stream.StartSource(b.UserConfig.GetInputDevice()); err != nil { - b.AddOutputLine(fmt.Sprintf("auto-transmit failed: %s", err.Error())) + started := b.withStream(func(stream *gumbleopenal.Stream) { + if err := stream.StartSource(b.UserConfig.GetInputDevice()); err != nil { + b.AddOutputLine(fmt.Sprintf("auto-transmit failed: %s", err.Error())) + return + } + b.setTransmitting(true) + b.UpdateGeneralStatus(" AutoTx ", true) + b.AddOutputLine("Auto-transmit started") + }) + if !started { return } - b.Tx = true - b.UpdateGeneralStatus(" AutoTx ", true) - b.AddOutputLine("Auto-transmit started") } func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { @@ -175,6 +200,7 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { reason = e.String } b.stopRecordingForDisconnect() + b.cleanupConnectionAudio() // Tone test cleanup if b.ToneTest { @@ -191,20 +217,25 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { } else { b.AddOutputLine("Disconnected: " + reason) } - b.Tx = false - b.Connected = false - b.UiTree.Rebuild() - b.Ui.Refresh() + b.setTransmitting(false) + b.setConnected(false) + b.postUI(func() { + b.UiTree.Rebuild() + b.Ui.Refresh() + }) go b.reconnectGoroutine() } func (b *Barnard) reconnectGoroutine() { - for { - res := b.connect(true) - if res == true { - break + for !b.reconnectCanceled() { + if b.connect(true) { + return + } + select { + case <-b.reconnectStop: + return + case <-time.After(15 * time.Second): } - time.Sleep(15 * time.Second) } } @@ -213,15 +244,12 @@ func (b *Barnard) Log(s string) { } func (b *Barnard) OnTextMessage(e *gumble.TextMessageEvent) { - var public = false - for _, c := range e.Channels { - if c.Name == b.Client.Self.Channel.Name { - public = true - break + if b.isPublicTextMessage(e) { + sender := "Server" + if e.Sender != nil { + sender = e.Sender.Name } - } - if public { - b.Notify("msg", e.Sender.Name, e.Message) + b.Notify("msg", sender, e.Message) b.AddOutputMessage(e.Sender, e.Message) } else { var sender string @@ -235,6 +263,28 @@ func (b *Barnard) OnTextMessage(e *gumble.TextMessageEvent) { } } +// isPublicTextMessage reports whether a message targets the current channel, +// either directly or through a recursive channel-tree recipient. +func (b *Barnard) isPublicTextMessage(e *gumble.TextMessageEvent) bool { + if e == nil || b.Client == nil || b.Client.Self == nil || b.Client.Self.Channel == nil { + return false + } + current := b.Client.Self.Channel + for _, channel := range e.Channels { + if sameChannel(channel, current) { + return true + } + } + for _, root := range e.Trees { + for channel := current; channel != nil; channel = channel.Parent { + if sameChannel(channel, root) { + return true + } + } + } + return false +} + func (b *Barnard) OnUserChange(e *gumble.UserChangeEvent) { notification, hasNotification := b.userChangeNotification(e) if e.User != nil { @@ -243,20 +293,20 @@ func (b *Barnard) OnUserChange(e *gumble.UserChangeEvent) { // Check if user is joining a muted channel if e.Type.Has(gumble.UserChangeConnected) || e.Type.Has(gumble.UserChangeChannel) { // If the channel is muted, ensure the user is muted - if b.MutedChannels[e.User.Channel.ID] { + if b.isChannelMuted(e.User.Channel.ID) { // Only mute if not already muted if !e.User.LocallyMuted() { - b.UserConfig.ToggleMute(e.User) - } - if source := e.User.AudioSource(); source != nil { - source.SetGain(0) + if err := b.UserConfig.ToggleMute(e.User); err != nil { + b.AddOutputLine("Mute: could not save setting: " + err.Error()) + } } + b.updateUserGain(e.User) } } } if e.Type.Has(gumble.UserChangeDisconnected) { - if e.User == b.selectedUser { + if e.User == b.selectedUserValue() { b.SetSelectedUser(nil) } } @@ -281,8 +331,10 @@ func (b *Barnard) OnUserChange(e *gumble.UserChangeEvent) { b.AddOutputLine(formatUserStats(e.User)) } b.updateUserChannel(e) - b.RebuildUserChannelTreePreservingSelection() - b.Ui.Refresh() + b.postUI(func() { + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() + }) } type userChangeNotification struct { @@ -382,8 +434,10 @@ func (b *Barnard) OnChannelChange(e *gumble.ChannelChangeEvent) { b.AddOutputLine(fmt.Sprintf("Channel permissions for %s: %s", e.Channel.Name, permissionList(*permission))) } } - b.RebuildUserChannelTreePreservingSelection() - b.Ui.Refresh() + b.postUI(func() { + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() + }) } func formatUserStats(user *gumble.User) string { @@ -461,34 +515,27 @@ func (b *Barnard) OnPermissionDenied(e *gumble.PermissionDeniedEvent) { } func (b *Barnard) OnUserList(e *gumble.UserListEvent) { - b.adminUserList = e.UserList b.AddOutputLine(fmt.Sprintf("Admin: received %d registered users", len(e.UserList))) - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.adminUserList = e.UserList; b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnACL(e *gumble.ACLEvent) { - b.adminACL = e.ACL if e.ACL != nil && e.ACL.Channel != nil { b.AddOutputLine(fmt.Sprintf("Admin: received ACLs for %s", e.ACL.Channel.Name)) } - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.adminACL = e.ACL; b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnBanList(e *gumble.BanListEvent) { - b.adminBanList = e.BanList b.AddOutputLine(fmt.Sprintf("Admin: received %d bans", len(e.BanList))) - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.adminBanList = e.BanList; b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnContextActionChange(e *gumble.ContextActionChangeEvent) { if e.ContextAction != nil { b.AddOutputLine(fmt.Sprintf("Admin: context action updated: %s", e.ContextAction.Name)) } - b.UiAdmin.Rebuild() - b.Ui.Refresh() + b.postUI(func() { b.UiAdmin.Rebuild(); b.Ui.Refresh() }) } func (b *Barnard) OnServerConfig(e *gumble.ServerConfigEvent) { diff --git a/client_notification_test.go b/client_notification_test.go index 9eb0f74..9cbde63 100644 --- a/client_notification_test.go +++ b/client_notification_test.go @@ -1,11 +1,255 @@ package main import ( + "fmt" + "io" + "strings" + "sync" "testing" + "time" + "unicode/utf8" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" ) +// Regression: HTML escaping left terminal control sequences in server text, +// allowing ANSI/OSC sequences to alter the terminal that rendered it. +// Regression: a capture-device open error left the application alive with no +// usable microphone. These errors are fatal and use the post-TUI stderr path. +func TestFatalAudioOpenError(t *testing.T) { + for _, err := range []error{gumbleopenal.ErrMic, gumbleopenal.ErrInputDevice, gumbleopenal.ErrOutputDevice, fmt.Errorf("wrapped: %w", gumbleopenal.ErrMic)} { + if !fatalAudioOpenError(err) { + t.Fatalf("%v was not fatal", err) + } + } + if fatalAudioOpenError(gumbleopenal.ErrState) { + t.Fatal("state error should remain recoverable") + } +} + +func TestEscRemovesTerminalControlSequences(t *testing.T) { + got := esc("name\x1b]0;spoof\a\x7f\u202e") + if got != "name]0;spoof" { + t.Fatalf("unsafe terminal text %q", got) + } +} + +type testReadCloser struct{ io.Reader } + +func (testReadCloser) Close() error { return nil } + +// Regression: an EOF from the FIFO was ignored and caused an unbounded busy +// loop. The reader must deliver a final command then close its output. +func TestReadFIFOStopsOnEOF(t *testing.T) { + out := make(chan string) + go readFIFO(testReadCloser{strings.NewReader("command\n")}, out) + if got := <-out; got != "command" { + t.Fatalf("got %q", got) + } + if _, ok := <-out; ok { + t.Fatal("FIFO output remained open after EOF") + } +} + +// Regression: sequential substitutions re-expanded placeholders embedded in +// server-provided fields, and a slow notifier blocked callback goroutines. +func TestNotificationExpansionIsSinglePassAndNotifyDoesNotBlock(t *testing.T) { + got := expandNotification("%event %what", []string{"event", "who", "%event"}) + if got != "event %event" { + t.Fatalf("unexpected expansion %q", got) + } + b := &Barnard{notifyChannel: make(chan []string, 1)} + b.Notify("one", "", "") + done := make(chan struct{}) + go func() { b.Notify("two", "", ""); close(done) }() + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("Notify blocked on a full queue") + } +} + +func TestAudioIntervalDuration(t *testing.T) { + for _, milliseconds := range []int{10, 20, 40, 60} { + got, err := audioIntervalDuration(milliseconds) + if err != nil { + t.Errorf("audioIntervalDuration(%d): %v", milliseconds, err) + continue + } + if got != time.Duration(milliseconds)*time.Millisecond { + t.Errorf("audioIntervalDuration(%d) = %v", milliseconds, got) + } + } + if _, err := audioIntervalDuration(30); err == nil { + t.Fatal("audioIntervalDuration accepted unsupported duration") + } +} + +func TestJitterBufferDuration(t *testing.T) { + for _, milliseconds := range []int{0, 20, 40, 60} { + got, err := jitterBufferDuration(milliseconds) + if err != nil { + t.Errorf("jitterBufferDuration(%d): %v", milliseconds, err) + continue + } + if got != time.Duration(milliseconds)*time.Millisecond { + t.Errorf("jitterBufferDuration(%d) = %v", milliseconds, got) + } + } + if _, err := jitterBufferDuration(10); err == nil { + t.Fatal("jitterBufferDuration accepted unsupported duration") + } +} + +func TestServerAddressDefaultsPortWithoutBreakingIPv6(t *testing.T) { + for input, want := range map[string]string{ + "server": "server:64738", + "server:64739": "server:64739", + "::1": "[::1]:64738", + "[2001:db8::1]": "[2001:db8::1]:64738", + "[2001:db8::1]:9": "[2001:db8::1]:9", + } { + if got := serverAddress(input); got != want { + t.Errorf("serverAddress(%q) = %q, want %q", input, got, want) + } + } +} + +func TestConcurrentConnectionStateAccess(t *testing.T) { + b := &Barnard{} + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func(value bool) { + defer wg.Done() + b.setConnected(value) + b.setTransmitting(value) + _ = b.isConnected() + _ = b.isTransmitting() + }(i%2 == 0) + } + wg.Wait() +} + +func TestConcurrentSelectedUserAccess(t *testing.T) { + b := &Barnard{} + user := &gumble.User{Session: 1} + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func(user *gumble.User) { + defer wg.Done() + b.setSelectedUserValue(user) + _ = b.selectedUserValue() + }(user) + } + wg.Wait() +} + +func TestConcurrentMutedChannelAccess(t *testing.T) { + b := &Barnard{MutedChannels: make(map[uint32]bool)} + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + b.setChannelMuted(uint32(i%3), i%2 == 0) + _ = b.isChannelMuted(uint32((i + 1) % 3)) + }(i) + } + wg.Wait() +} + +func TestPublicTextMessageTargetsChannelIDsAndTrees(t *testing.T) { + root := &gumble.Channel{ID: 1, Name: "Root"} + current := &gumble.Channel{ID: 2, Name: "Room", Parent: root} + b := &Barnard{Client: &gumble.Client{Self: &gumble.User{Channel: current}}} + + if !b.isPublicTextMessage(&gumble.TextMessageEvent{TextMessage: gumble.TextMessage{Trees: []*gumble.Channel{root}}}) { + t.Fatal("recursive message to an ancestor was not public") + } + other := &gumble.Channel{ID: 3, Name: "Room"} + if b.isPublicTextMessage(&gumble.TextMessageEvent{TextMessage: gumble.TextMessage{Channels: []*gumble.Channel{other}}}) { + t.Fatal("message to a different channel with the same name was public") + } +} + +func TestPublicServerMessageDoesNotPanic(t *testing.T) { + channel := &gumble.Channel{ID: 1, Name: "Current"} + b := &Barnard{ + Client: &gumble.Client{Self: &gumble.User{Channel: channel}}, + notifyChannel: make(chan []string, 1), + } + + b.OnTextMessage(&gumble.TextMessageEvent{ + Client: b.Client, + TextMessage: gumble.TextMessage{ + Channels: []*gumble.Channel{channel}, + Message: "server announcement", + }, + }) + + got := <-b.notifyChannel + want := []string{"msg", "Server", "server announcement"} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] || got[2] != want[2] { + t.Fatalf("notification = %#v, want %#v", got, want) + } +} + +// Regression: reconnect replaced Stream without destroying the old capture +// and renderer resources. Cleanup must be safe for repeated disconnects. +// Regression: user and channel names in the navigation tree bypassed message +// escaping and could still carry terminal control characters. +// Regression: status truncation used byte indexes and could create invalid +// UTF-8 when a non-ASCII user or channel name exceeded the display limit. +func TestTruncateInputStatusPreservesUTF8(t *testing.T) { + got := truncateInputStatus(strings.Repeat("é", 21)) + if !utf8.ValidString(got) || utf8.RuneCountInString(got) != 21 { + t.Fatalf("invalid truncation %q", got) + } +} + +func TestTreeItemSanitizesServerNames(t *testing.T) { + item := TreeItem{Channel: &gumble.Channel{Name: "\x1b[2Jroom"}} + if got := item.String(); got != "#[2Jroom" { + t.Fatalf("got %q", got) + } +} + +func TestCleanupConnectionAudioIsIdempotent(t *testing.T) { + b := &Barnard{} + b.cleanupConnectionAudio() + b.cleanupConnectionAudio() +} + +// Tone test mode intentionally does not create an OpenAL stream. Tree +// controls must therefore keep local mute state without trying to update one. +func TestReconnectCancellationIsSafeBeforeStartup(t *testing.T) { + b := &Barnard{} + b.stopReconnects() + if b.reconnectCanceled() { + t.Fatal("nil reconnect channel should not report cancellation") + } +} + +func TestReconnectCancellationStopsWaiters(t *testing.T) { + b := &Barnard{reconnectStop: make(chan struct{})} + b.stopReconnects() + if !b.reconnectCanceled() { + t.Fatal("expected reconnect cancellation") + } + b.stopReconnects() // repeated shutdown must not panic +} + +func TestUpdateUserGainAllowsToneTestWithoutStream(t *testing.T) { + (&Barnard{ToneTest: true}).updateUserGain(&gumble.User{}) +} + +func TestToneTestRejectsFilePlayback(t *testing.T) { + (&Barnard{ToneTest: true, Connected: true}).CommandPlayFile(nil, "https://example.invalid/audio") +} + func TestUserChangeNotification(t *testing.T) { current := &gumble.Channel{ID: 1, Name: "Current"} other := &gumble.Channel{ID: 2, Name: "Other"} diff --git a/connection_resource_test.go b/connection_resource_test.go new file mode 100644 index 0000000..5415eae --- /dev/null +++ b/connection_resource_test.go @@ -0,0 +1,13 @@ +package main + +import ( + "testing" + + "git.stormux.org/storm/barnard/gumble/gumbleopenal" +) + +func TestWithStreamHandlesAbsentConnectionResource(t *testing.T) { + if (&Barnard{}).withStream(func(*gumbleopenal.Stream) {}) { + t.Fatal("nil connection stream was treated as available") + } +} diff --git a/recording_control.go b/recording_control.go index 6e20f16..614effc 100644 --- a/recording_control.go +++ b/recording_control.go @@ -6,6 +6,7 @@ import ( "time" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" "git.stormux.org/storm/barnard/recording" "git.stormux.org/storm/barnard/uiterm" ) @@ -179,10 +180,10 @@ func (b *Barnard) finishRecordingStart() { } b.Recorder = recorder b.recordingStarting = false + // Recorder operations take RecordingMutex before connectionMutex. This + // prevents disconnect cleanup from destroying a stream during attachment. + b.withStream(func(stream *gumbleopenal.Stream) { stream.SetRecorder(recorder) }) b.RecordingMutex.Unlock() - if b.Stream != nil { - b.Stream.SetRecorder(recorder) - } b.AddOutputLine(fmt.Sprintf("Recording started: %s", recorder.Path())) b.Notify("recordstart", "me", recorder.Path()) b.renderGeneralStatus() @@ -205,9 +206,7 @@ func (b *Barnard) detachRecorder() (*recording.Recorder, string, bool) { } b.Recorder = nil b.recordingStarting = false - if b.Stream != nil { - b.Stream.SetRecorder(nil) - } + b.withStream(func(stream *gumbleopenal.Stream) { stream.SetRecorder(nil) }) return recorder, path, wasPending } diff --git a/ui.go b/ui.go index e66848e..6d2964e 100644 --- a/ui.go +++ b/ui.go @@ -1,6 +1,7 @@ package main import ( + "errors" "fmt" "os" "strings" @@ -8,6 +9,7 @@ import ( "unicode" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" "git.stormux.org/storm/barnard/uiterm" "github.com/kennygrant/sanitize" "github.com/nsf/termbox-go" @@ -37,6 +39,14 @@ func esc(str string) string { return sanitize.HTML(clean) } +// postUI is the only path network and audio callbacks use to touch terminal +// widgets. Work is dropped during shutdown or queue overload. +func (b *Barnard) postUI(fn func()) { + if b.Ui != nil { + b.Ui.Post(fn) + } +} + func (b *Barnard) Notify(event string, who string, what string) { // Notifications are best-effort: a slow external command must not block a // UI or network callback. New events are dropped once the bounded queue is full. @@ -47,7 +57,7 @@ func (b *Barnard) Notify(event string, who string, what string) { } func (b *Barnard) SetSelectedUser(user *gumble.User) { - b.selectedUser = user + b.setSelectedUserValue(user) if user == nil { if len(b.UiInput.Text) > 0 { } @@ -63,12 +73,21 @@ func (b *Barnard) GetInputStatus() string { func (b *Barnard) UpdateInputStatus(status string) { status = truncateInputStatus(status) - b.UiInputStatus.Text = status - b.RebuildUserChannelTreePreservingSelection() - b.Ui.Refresh() + if b.Ui == nil { + return + } + b.Ui.Post(func() { + b.UiInputStatus.Text = status + // The initial connection status arrives after Run's first layout. Relayout + // so the prompt has cells to draw before focus is changed. + width, height := termbox.Size() + b.OnUiResize(b.Ui, width, height) + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() + }) } -// truncateInputStatus shortens the prompt without splitting a multi-byte rune. +// truncateInputStatus limits terminal cells without splitting UTF-8 runes. func truncateInputStatus(status string) string { chars := []rune(status) if len(chars) > 20 { @@ -79,7 +98,11 @@ func truncateInputStatus(status string) string { func (b *Barnard) AddOutputLine(line string) { now := time.Now() - b.UiOutput.AddLine(fmt.Sprintf("%s [%02d:%02d:%02d]", line, now.Hour(), now.Minute(), now.Second())) + formatted := fmt.Sprintf("%s [%02d:%02d:%02d]", line, now.Hour(), now.Minute(), now.Second()) + if b.Ui == nil { + return + } + b.Ui.Post(func() { b.UiOutput.AddLine(formatted) }) } func (b *Barnard) AddOutputMessage(sender *gumble.User, message string) { @@ -135,19 +158,26 @@ func (b *Barnard) toggleAGC() bool { if err := b.UserConfig.SetAGCEnabled(enabled); err != nil { b.AddOutputLine("AGC: could not save setting: " + err.Error()) } - if b.Stream != nil { - b.Stream.SetAGCEnabled(enabled) - } + b.withStream(func(stream *gumbleopenal.Stream) { + stream.SetAGCEnabled(enabled) + }) return enabled } func (b *Barnard) UpdateGeneralStatus(text string, notice bool) { - b.statusText = text - b.statusNotice = notice - b.renderGeneralStatus() + b.postUI(func() { + b.statusText = text + b.statusNotice = notice + b.renderGeneralStatusNow() + }) } func (b *Barnard) renderGeneralStatus() { + b.postUI(func() { b.renderGeneralStatusNow() }) +} + +// renderGeneralStatusNow must run on the UI-owning goroutine. +func (b *Barnard) renderGeneralStatusNow() { text := b.statusText notice := b.statusNotice if notice { @@ -241,7 +271,7 @@ func (b *Barnard) CommandPlayFile(ui *uiterm.Ui, cmd string) { } } - if !b.Connected { + if !b.isConnected() { b.AddOutputLine("Not connected to server") return } @@ -252,8 +282,12 @@ func (b *Barnard) CommandPlayFile(ui *uiterm.Ui, cmd string) { b.FileStreamMutex.Lock() defer b.FileStreamMutex.Unlock() + if b.FileStream == nil { + b.AddOutputLine("File playback is unavailable while reconnecting") + return + } - if b.FileStream != nil && b.FileStream.IsPlaying() { + if b.FileStream.IsPlaying() { b.AddOutputLine("Already playing a file. Use /stop first.") return } @@ -267,16 +301,23 @@ func (b *Barnard) CommandPlayFile(ui *uiterm.Ui, cmd string) { // Enable stereo encoder for file playback b.Client.EnableStereoEncoder() - // Auto-start transmission if not already transmitting - if !b.Tx { - err := b.Stream.StartSource(b.UserConfig.GetInputDevice()) - if err != nil { - b.AddOutputLine(fmt.Sprintf("Error starting transmission: %s", err.Error())) + // Auto-start transmission if not already transmitting. FileStreamMutex is + // held here, before withStream's connection mutex, matching cleanup. + if !b.isTransmitting() { + var startErr error + started := b.withStream(func(stream *gumbleopenal.Stream) { + startErr = stream.StartSource(b.UserConfig.GetInputDevice()) + }) + if !started { + startErr = errors.New("audio unavailable while reconnecting") + } + if startErr != nil { + b.AddOutputLine(fmt.Sprintf("Error starting transmission: %s", startErr)) b.FileStream.Stop() b.Client.DisableStereoEncoder() return } - b.Tx = true + b.setTransmitting(true) b.UpdateGeneralStatus(" File ", true) } @@ -313,15 +354,15 @@ func (b *Barnard) CommandStopFile(ui *uiterm.Ui, cmd string) { } func (b *Barnard) setTransmit(ui *uiterm.Ui, val int) { - if b.Tx && val == 1 { + if b.isTransmitting() && val == 1 { return } - if b.Tx == false && val == 0 { + if !b.isTransmitting() && val == 0 { return } - if b.Tx { + if b.isTransmitting() { b.Notify("micdown", "me", "") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus(" Idle ", false) if b.ToneTest { if b.toneTestStop != nil { @@ -329,73 +370,96 @@ func (b *Barnard) setTransmit(ui *uiterm.Ui, val int) { b.toneTestStop = nil } } else { - b.Stream.StopSource() + b.withStream(func(stream *gumbleopenal.Stream) { _ = stream.StopSource() }) } - } else if b.Connected == false { + } else if !b.isConnected() { b.Notify("error", "me", "no tx while disconnected") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus("no tx while disconnected", true) - } else if b.MutedChannels[b.Client.Self.Channel.ID] { + } else if b.isChannelMuted(b.Client.Self.Channel.ID) { // Check if current channel is muted b.Notify("error", "me", "cannot transmit in muted channel") - b.Tx = false + b.setTransmitting(false) b.UpdateGeneralStatus("cannot transmit in muted channel", true) } else { - b.Tx = true + b.setTransmitting(true) if b.ToneTest { b.toneTestStop = make(chan struct{}) go StartToneGenerator(b.Client, b.toneTestStop) b.Notify("micup", "me", "") b.UpdateGeneralStatus(" Tx ", true) } else { - err := b.Stream.StartSource(b.UserConfig.GetInputDevice()) - if err != nil { - b.Notify("error", "me", err.Error()) - b.UpdateGeneralStatus(err.Error(), true) - } else { + started := b.withStream(func(stream *gumbleopenal.Stream) { + err := stream.StartSource(b.UserConfig.GetInputDevice()) + if err != nil { + b.setTransmitting(false) + if fatalAudioOpenError(err) { + // A missing capture device cannot recover through normal + // transmission controls; exit so option 1 reports it on stderr. + b.exitWithError(fmt.Errorf("audio device initialization failed: %w", err)) + return + } + b.Notify("error", "me", err.Error()) + b.UpdateGeneralStatus(err.Error(), true) + return + } b.Notify("micup", "me", "") b.UpdateGeneralStatus(" Tx ", true) + }) + if !started { + b.setTransmitting(false) + b.UpdateGeneralStatus("audio unavailable while reconnecting", true) } } } } +func fatalAudioOpenError(err error) bool { + return errors.Is(err, gumbleopenal.ErrMic) || errors.Is(err, gumbleopenal.ErrInputDevice) || errors.Is(err, gumbleopenal.ErrOutputDevice) +} + func (b *Barnard) OnMicVolumeDown(ui *uiterm.Ui, key uiterm.Key) { if b.ToneTest { return } - b.Stream.SetMicVolume(-0.1, true) - b.UserConfig.SetMicVolume(b.Stream.GetMicVolume()) - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Microphone: could not save volume: " + err.Error()) - } + b.withStream(func(stream *gumbleopenal.Stream) { + stream.SetMicVolume(-0.1, true) + b.UserConfig.SetMicVolume(stream.GetMicVolume()) + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Microphone: could not save volume: " + err.Error()) + } + }) } func (b *Barnard) OnMicVolumeUp(ui *uiterm.Ui, key uiterm.Key) { if b.ToneTest { return } - b.Stream.SetMicVolume(0.1, true) - b.UserConfig.SetMicVolume(b.Stream.GetMicVolume()) - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Microphone: could not save volume: " + err.Error()) - } + b.withStream(func(stream *gumbleopenal.Stream) { + stream.SetMicVolume(0.1, true) + b.UserConfig.SetMicVolume(stream.GetMicVolume()) + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Microphone: could not save volume: " + err.Error()) + } + }) } func (b *Barnard) OnQuitPress(ui *uiterm.Ui, key uiterm.Key) { + b.stopReconnects() b.StopRecordingIfActive(true) b.Client.Disconnect() b.Ui.Close() } func (b *Barnard) CommandExit(ui *uiterm.Ui, cmd string) { + b.stopReconnects() b.StopRecordingIfActive(true) b.Client.Disconnect() b.Ui.Close() } func (b *Barnard) CommandStatus(ui *uiterm.Ui, cmd string) { - if b.Tx { + if b.isTransmitting() { b.Notify("status", "me", "transmitting") } else { b.Notify("status", "me", "not transmitting") @@ -485,9 +549,9 @@ func (b *Barnard) OnTextInput(ui *uiterm.Ui, textbox *uiterm.Textbox, text strin // Not a command, send as chat message if b.Client != nil && b.Client.Self != nil { - if b.selectedUser != nil { - b.selectedUser.Send(text) - b.AddOutputPrivateMessage(b.Client.Self, b.selectedUser, text) + if selectedUser := b.selectedUserValue(); selectedUser != nil { + selectedUser.Send(text) + b.AddOutputPrivateMessage(b.Client.Self, selectedUser, text) } else { b.Client.Self.Channel.Send(text, false) b.AddOutputMessage(b.Client.Self, text) @@ -527,8 +591,9 @@ func (b *Barnard) OnUiInitialize(ui *uiterm.Ui) { ui.Add(uiViewInput, &b.UiInput) b.UiInputStatus = uiterm.Label{ - Fg: uiterm.ColorBlack, - Bg: uiterm.ColorWhite, + Text: "[root]", + Fg: uiterm.ColorBlack, + Bg: uiterm.ColorWhite, } ui.Add(uiViewInputStatus, &b.UiInputStatus) @@ -575,6 +640,7 @@ func (b *Barnard) OnUiInitialize(ui *uiterm.Ui) { b.Ui.AddKeyListener(b.OnNoiseSuppressionToggle, b.Hotkeys.NoiseSuppressionToggle) b.Ui.AddKeyListener(b.OnAGCToggle, b.Hotkeys.AGCToggle) b.Ui.AddKeyListener(b.OnRecordingToggle, b.Hotkeys.RecordToggle) + b.Ui.AddKeyListener(b.OnClearPress, b.Hotkeys.ClearOutput) b.Ui.AddKeyListener(b.OnQuitPress, b.Hotkeys.Exit) b.Ui.AddKeyListener(b.OnScrollOutputUp, b.Hotkeys.ScrollUp) b.Ui.AddKeyListener(b.OnScrollOutputDown, b.Hotkeys.ScrollDown) diff --git a/ui_tree.go b/ui_tree.go index ce2e556..04af22e 100644 --- a/ui_tree.go +++ b/ui_tree.go @@ -3,11 +3,15 @@ package main import ( "fmt" "git.stormux.org/storm/barnard/gumble/gumble" + "git.stormux.org/storm/barnard/gumble/gumbleopenal" "git.stormux.org/storm/barnard/uiterm" "sort" ) func (ti TreeItem) String() string { + if ti.display != "" { + return ti.display + } if ti.User != nil { if ti.User.LocallyMuted() { return "[MUTED] " + esc(ti.User.Name) @@ -34,59 +38,61 @@ func (ti TreeItem) TreeItemStyle(fg, bg uiterm.Attribute, active bool) (uiterm.A } func (b *Barnard) changeVolume(users []*gumble.User, change float32) { - for _, u := range users { - au := u.AudioSource() - if au == nil { - continue + changed := b.withStream(func(stream *gumbleopenal.Stream) { + for _, u := range users { + var boost uint16 + var ng float32 + curboost := float32((u.Boost() - 1)) / 10 + ng = u.Volume() + curboost + change + boost = uint16(1) + if ng > 1.0 { + perc := uint16((ng * 10)) - 10 + perc += 1 + boost = perc + ng = 1.0 + } + if ng < 0 { + ng = 0.0 + } + u.SetBoost(boost) + u.SetVolume(ng) + stream.UpdateUserGain(u) + b.UserConfig.UpdateConfig(u) } - var boost uint16 - var cv float32 - var ng float32 - var curboost float32 - curboost = float32((u.Boost() - 1)) / 10 - cv = au.GetGain() + curboost - ng = cv + change - boost = uint16(1) - if ng > 1.0 { - perc := uint16((ng * 10)) - 10 - perc += 1 - boost = perc - ng = 1.0 + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Volume: could not save setting: " + err.Error()) } - if ng < 0 { - ng = 0.0 - } - u.SetBoost(boost) - u.SetVolume(ng) - if !u.LocallyMuted() { - au.SetGain(ng) - } - b.UserConfig.UpdateConfig(u) - } - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Volume: could not save setting: " + err.Error()) + }) + if changed { + b.refreshVolumeDisplay() } } func (b *Barnard) resetVolume(users []*gumble.User) { - for _, u := range users { - au := u.AudioSource() - if au == nil { - continue + changed := b.withStream(func(stream *gumbleopenal.Stream) { + for _, u := range users { + // Reset to original volume (1.0) and boost (1) + u.SetBoost(uint16(1)) + u.SetVolume(1.0) + stream.UpdateUserGain(u) + b.UserConfig.UpdateConfig(u) } - // Reset to original volume (1.0) and boost (1) - u.SetBoost(uint16(1)) - u.SetVolume(1.0) - if !u.LocallyMuted() { - au.SetGain(1.0) + if err := b.UserConfig.SaveConfig(); err != nil { + b.AddOutputLine("Volume: could not save setting: " + err.Error()) } - b.UserConfig.UpdateConfig(u) - } - if err := b.UserConfig.SaveConfig(); err != nil { - b.AddOutputLine("Volume: could not save setting: " + err.Error()) + }) + if changed { + b.refreshVolumeDisplay() } } +// Tree items render a display string snapshotted at build time, so a volume +// change is only visible after the tree is rebuilt. +func (b *Barnard) refreshVolumeDisplay() { + b.RebuildUserChannelTreePreservingSelection() + b.Ui.Refresh() +} + func makeUsersArray(users gumble.Users) []*gumble.User { t := make([]*gumble.User, 0, len(users)) for _, u := range users { @@ -102,15 +108,18 @@ func (b *Barnard) TreeItemBuild(item uiterm.TreeItem) []uiterm.TreeItem { var treeItem TreeItem if ti, ok := item.(TreeItem); !ok { - root := b.Client.Channels[0] + var root *gumble.Channel + b.Client.Do(func() { root = b.Client.Channels[0] }) if root == nil { return nil } - return []uiterm.TreeItem{ - TreeItem{ - Channel: root, - }, - } + var display string + var channelID uint32 + b.Client.Do(func() { + display = "#" + esc(root.Name) + channelID = root.ID + }) + return []uiterm.TreeItem{TreeItem{Channel: root, display: display, channelID: channelID, snapshot: true}} } else { treeItem = ti } @@ -120,39 +129,52 @@ func (b *Barnard) TreeItemBuild(item uiterm.TreeItem) []uiterm.TreeItem { } users := []uiterm.TreeItem{} - ul := []*gumble.User{} - for _, user := range treeItem.Channel.Users { - ul = append(ul, user) - var u = ul[len(ul)-1] - _ = u + type userDisplay struct { + user *gumble.User + display string + name string + session uint32 } + type channelDisplay struct { + channel *gumble.Channel + name string + id uint32 + } + ul := []userDisplay{} + cl := []channelDisplay{} + // TCP handlers mutate both maps; snapshot them while Client.Do holds its + // read lock, then sort/render outside the protocol critical section. + b.Client.Do(func() { + for _, user := range treeItem.Channel.Users { + boostPercent := float32(user.Boost()-1) * 10 + totalVolume := user.Volume()*100 + boostPercent + display := fmt.Sprintf("%s [%.0f%%]", esc(user.Name), totalVolume) + if user.LocallyMuted() { + display = "[MUTED] " + display + } + ul = append(ul, userDisplay{user: user, name: user.Name, session: user.Session, display: display}) + } + for _, subchannel := range treeItem.Channel.Children { + cl = append(cl, channelDisplay{channel: subchannel, name: subchannel.Name, id: subchannel.ID}) + } + }) sort.Slice(ul, func(i, j int) bool { - return ul[i].Name < ul[j].Name + return ul[i].name < ul[j].name }) for _, user := range ul { - users = append(users, TreeItem{ - User: user, - }) + users = append(users, TreeItem{User: user.user, display: user.display, userSession: user.session, snapshot: true}) } channels := []uiterm.TreeItem{} - cl := []*gumble.Channel{} - for _, subchannel := range treeItem.Channel.Children { - cl = append(cl, subchannel) - } sort.Slice(cl, func(i, j int) bool { - return cl[i].Name < cl[j].Name + return cl[i].name < cl[j].name }) for _, subchannel := range cl { - displayName := subchannel.Name - if b.MutedChannels[subchannel.ID] { - displayName = "[MUTED] #" + displayName - } else { - displayName = "#" + displayName + displayName := "#" + esc(subchannel.name) + if b.isChannelMuted(subchannel.id) { + displayName = "[MUTED] " + displayName } - channels = append(channels, TreeItem{ - Channel: subchannel, - }) + channels = append(channels, TreeItem{Channel: subchannel.channel, display: displayName, channelID: subchannel.id, snapshot: true}) } return append(users, channels...) @@ -172,9 +194,15 @@ func sameUserChannelTreeItem(previous, current uiterm.TreeItem) bool { return false } if prev.User != nil && cur.User != nil { + if prev.snapshot && cur.snapshot { + return prev.userSession == cur.userSession + } return prev.User.Session == cur.User.Session } if prev.Channel != nil && cur.Channel != nil { + if prev.snapshot && cur.snapshot { + return prev.channelID == cur.channelID + } return prev.Channel.ID == cur.Channel.ID } return false diff --git a/ui_tree_snapshot_test.go b/ui_tree_snapshot_test.go new file mode 100644 index 0000000..219c2ad --- /dev/null +++ b/ui_tree_snapshot_test.go @@ -0,0 +1,16 @@ +package main + +import ( + "testing" + + "git.stormux.org/storm/barnard/gumble/gumble" +) + +func TestTreeItemUsesCapturedDisplaySnapshot(t *testing.T) { + user := &gumble.User{Name: "before"} + item := TreeItem{User: user, display: "before [100%]"} + user.Name = "after" + if got := item.String(); got != "before [100%]" { + t.Fatalf("tree display read mutable user state: %q", got) + } +} diff --git a/ui_tree_test.go b/ui_tree_test.go new file mode 100644 index 0000000..69db060 --- /dev/null +++ b/ui_tree_test.go @@ -0,0 +1,22 @@ +package main + +import ( + "testing" + + "git.stormux.org/storm/barnard/gumble/gumble" +) + +// Regression: rebuilding the channel tree ranged protocol-owned maps without +// Client.Do while TCP handlers could add or remove users/channels. +func TestTreeItemBuildReadsMapsUnderClientSnapshot(t *testing.T) { + root := &gumble.Channel{ID: 0, Users: gumble.Users{}, Children: gumble.Channels{}} + user := &gumble.User{Session: 1, Name: "user"} + child := &gumble.Channel{ID: 2, Name: "child", Users: gumble.Users{}, Children: gumble.Channels{}} + root.Users[user.Session] = user + root.Children[child.ID] = child + b := &Barnard{Client: &gumble.Client{Channels: gumble.Channels{0: root}}, MutedChannels: map[uint32]bool{}} + items := b.TreeItemBuild(TreeItem{Channel: root}) + if len(items) != 2 { + t.Fatalf("got %d items", len(items)) + } +} diff --git a/uiterm/ui.go b/uiterm/ui.go index d34c74c..c6041b9 100644 --- a/uiterm/ui.go +++ b/uiterm/ui.go @@ -3,7 +3,9 @@ package uiterm import ( "errors" "strings" + "sync" "sync/atomic" + "time" "github.com/nsf/termbox-go" ) @@ -20,8 +22,10 @@ type UiManager interface { type Ui struct { Fg, Bg Attribute - close chan bool - manager UiManager + close chan struct{} + closeOnce sync.Once + events chan func() + manager UiManager drawCount int32 elements map[string]*uiElement @@ -39,7 +43,8 @@ type uiElement struct { func New(manager UiManager) *Ui { ui := &Ui{ - close: make(chan bool, 10), + close: make(chan struct{}), + events: make(chan func(), 256), elements: make(map[string]*uiElement), manager: manager, keyListeners: make(map[Key][]KeyListener), @@ -48,9 +53,24 @@ func New(manager UiManager) *Ui { return ui } +// Close is safe to call repeatedly and never blocks a caller. func (ui *Ui) Close() { - if termbox.IsInit { - ui.close <- true + ui.closeOnce.Do(func() { close(ui.close) }) +} + +// Post schedules UI work on Run's owning goroutine. It is deliberately +// bounded: network callbacks must not block behind slow terminal rendering. +func (ui *Ui) Post(fn func()) bool { + if fn == nil { + return true + } + select { + case <-ui.close: + return false + case ui.events <- fn: + return true + default: + return false } } @@ -97,15 +117,37 @@ func (ui *Ui) Run(cmds chan string) error { return nil } if err := termbox.Init(); err != nil { - return nil + return err } - defer termbox.Close() termbox.SetInputMode(termbox.InputAlt) - events := make(chan termbox.Event) + // Closing termbox wakes PollEvent. Keep delivery cancellable so the polling + // goroutine cannot become stranded trying to send after Run returns. + events := make(chan termbox.Event, 1) + pollDone := make(chan struct{}) go func() { + defer close(pollDone) for { - events <- termbox.PollEvent() + event := termbox.PollEvent() + select { + case <-ui.close: + return + default: + } + select { + case events <- event: + case <-ui.close: + return + } + } + }() + defer func() { + termbox.Close() + // Some termbox backends do not wake PollEvent promptly on Close. A fatal + // startup failure must print its stderr error instead of hanging here. + select { + case <-pollDone: + case <-time.After(100 * time.Millisecond): } }() @@ -119,7 +161,13 @@ func (ui *Ui) Run(cmds chan string) error { select { case <-ui.close: return nil - case cmd := <-cmds: + case fn := <-ui.events: + fn() + case cmd, ok := <-cmds: + if !ok { + cmds = nil + continue + } ui.onCommandEvent(cmd) case event := <-events: switch event.Type { diff --git a/uiterm/ui_regression_test.go b/uiterm/ui_regression_test.go new file mode 100644 index 0000000..4598959 --- /dev/null +++ b/uiterm/ui_regression_test.go @@ -0,0 +1,45 @@ +package uiterm + +import "testing" + +// Regression: Close sent to a bounded channel and could block or enqueue +// duplicate shutdowns when called more than once. +// Regression: network callbacks modified terminal state directly. Post gives +// them a bounded handoff to the UI-owning Run goroutine instead of blocking. +func TestPostIsBoundedAndRejectsClosedUI(t *testing.T) { + ui := New(nil) + for i := 0; i < cap(ui.events); i++ { + if !ui.Post(func() {}) { + t.Fatal("queue filled too early") + } + } + if ui.Post(func() {}) { + t.Fatal("Post accepted work past queue capacity") + } + ui.Close() + if ui.Post(func() {}) { + t.Fatal("Post accepted work after close") + } +} + +func TestCloseIsNonblockingAndIdempotent(t *testing.T) { + ui := New(nil) + ui.Close() + ui.Close() + select { + case <-ui.close: + default: + t.Fatal("Close did not signal shutdown") + } +} + +func TestSafeRuneRemovesTerminalControlCharacters(t *testing.T) { + for _, r := range []rune{'\x1b', '\x7f', '\u202e'} { + if got := safeRune(r); got != ' ' { + t.Errorf("safeRune(%U) = %U, want space", r, got) + } + } + if got := safeRune('A'); got != 'A' { + t.Fatalf("safeRune altered printable text: %U", got) + } +}