diff --git a/apps/chanora_flutter/lib/main.dart b/apps/chanora_flutter/lib/main.dart index 87c3f56..1dc3b2e 100644 --- a/apps/chanora_flutter/lib/main.dart +++ b/apps/chanora_flutter/lib/main.dart @@ -1269,8 +1269,14 @@ class _SnapshotView extends StatelessWidget { final l10n = AppL10n.of(context); final theme = Theme.of(context); - final channels = [...snapshot.channels] - ..sort((a, b) => a.order.compareTo(b.order)); + // Channels arrive pre-sorted from the bridge: the Rust + // adapter (chanora_protocol::adapter::sort_channels_tree) + // emits them in root-first depth-first order following the + // TeamSpeak linked-list `order` predecessor pointers. We + // therefore trust the server order verbatim — re-sorting by + // `order` numerically here would re-introduce the bug fixed + // in that adapter (TS3 `order` is NOT a numeric rank). + final channels = snapshot.channels; final byChannel = >{}; for (final c in snapshot.clients) { diff --git a/crates/chanora_protocol/src/adapter.rs b/crates/chanora_protocol/src/adapter.rs index 715458a..025c89d 100644 --- a/crates/chanora_protocol/src/adapter.rs +++ b/crates/chanora_protocol/src/adapter.rs @@ -20,6 +20,7 @@ use std::time::Duration; use futures::prelude::*; +use std::collections::HashMap; use tokio::sync::{mpsc, oneshot}; use tracing::{info, warn}; @@ -551,13 +552,133 @@ fn packet_sender_id(buf: &InAudioBuf) -> Option { } } +/// Sort TeamSpeak channels by the linked-list ordering carried in +/// each channel's `order` field (predecessor pointer), producing a +/// root-first depth-first list suitable for direct UI rendering. +/// +/// Generic over `T` + an `extract` closure so unit tests can supply +/// a lightweight fixture struct without constructing a live +/// `tsclientlib::data::Channel`. +fn sort_channels_tree<'a>(channels: &'a [&'a Channel]) -> Vec<&'a Channel> { + sort_channels_tree_by(channels, |c| (c.id.0, c.parent.0, c.order.0)) +} + +/// Inner implementation that operates on any slice of items via an +/// extractor returning `(id, parent, order)` u64 triples. Pure; +/// tested directly by the `#[cfg(test)]` block below without +/// touching `tsclientlib`. +fn sort_channels_tree_by<'a, T>( + items: &'a [&'a T], + extract: impl Fn(&T) -> (u64, u64, u64), +) -> Vec<&'a T> { + // Bucket by parent and build (per parent) the predecessor->item + // lookup so we can walk the chain in O(n). + let mut by_parent: HashMap> = HashMap::new(); + for &item in items { + let (_id, parent, _order) = extract(item); + by_parent.entry(parent).or_default().push(item); + } + let mut ordered_per_parent: HashMap> = HashMap::new(); + for (parent, siblings) in by_parent.into_iter() { + let mut successor: HashMap = HashMap::with_capacity(siblings.len()); + for &c in &siblings { + let (_id, _parent, order) = extract(c); + // First-write wins: if the server emits two channels + // with the same predecessor (corrupted state), keep + // the first and fall through the leftover path for + // the duplicates. + successor.entry(order).or_insert(c); + } + let mut ordered: Vec<&'a T> = Vec::with_capacity(siblings.len()); + let mut cursor: u64 = 0; + let mut visited: std::collections::HashSet = std::collections::HashSet::new(); + while let Some(next) = successor.get(&cursor).copied() { + let (id, _parent, _order) = extract(next); + if !visited.insert(id) { + // Cycle guard. Should not happen on a well-formed + // server snapshot but cheap to defend against. + break; + } + ordered.push(next); + cursor = id; + } + // Anything we did not reach (broken predecessor pointer or + // duplicate predecessor) gets appended sorted by id so the + // UI does not silently drop channels. + let reached: std::collections::HashSet = + ordered.iter().map(|c| extract(c).0).collect(); + let mut leftover: Vec<&'a T> = siblings + .into_iter() + .filter(|c| !reached.contains(&extract(c).0)) + .collect(); + leftover.sort_by_key(|c| extract(c).0); + ordered.extend(leftover); + ordered_per_parent.insert(parent, ordered); + } + + // Emit root list first then each subtree depth-first. + let mut out: Vec<&'a T> = Vec::with_capacity(items.len()); + emit_subtree(&ordered_per_parent, 0, &mut out, &extract); + // Defensive: if a channel's `parent` does not appear anywhere + // in the emitted tree (orphaned subtree) append it so it isn't + // lost. We track emitted ids and dump anything else. + let emitted: std::collections::HashSet = + out.iter().map(|c| extract(c).0).collect(); + let mut orphans: Vec<&'a T> = items + .iter() + .copied() + .filter(|c| !emitted.contains(&extract(c).0)) + .collect(); + orphans.sort_by_key(|c| extract(c).0); + out.extend(orphans); + out +} + +fn emit_subtree<'a, T>( + by_parent: &HashMap>, + root_id: u64, + out: &mut Vec<&'a T>, + extract: &impl Fn(&T) -> (u64, u64, u64), +) { + let Some(children) = by_parent.get(&root_id) else { + return; + }; + for &ch in children { + out.push(ch); + let (child_id, _parent, _order) = extract(ch); + emit_subtree(by_parent, child_id, out, extract); + } +} + fn build_snapshot(con: &Connection) -> Result { let state: &data::Connection = con .get_state() .map_err(|e| ProtocolError::Backend(format!("get_state: {e}")))?; - let mut channels: Vec<&Channel> = state.channels.values().collect(); - channels.sort_by_key(|c| c.order.0); + // TeamSpeak channel ordering: the `order` field on a channel is + // NOT a numeric rank but the id of the channel that should + // appear immediately before this one within the same parent. + // `order == ChannelId(0)` marks the head of a parent's child + // list. The previous implementation sorted by `order.0` + // numerically, which produced a stable-but-arbitrary order + // that did not match the TS3 client display order and was + // reported by users as "channel sort in not correct". + // + // Correct algorithm: + // 1. Bucket channels by parent. + // 2. Within each bucket, walk the linked list starting from + // the entry whose `order == ChannelId(0)` and following + // each successive channel via its successor map until the + // chain terminates. + // 3. Emit channels root-first depth-first, so callers see a + // pre-ordered tree without needing to re-sort. + // + // Defensive fallback: any siblings the linked-list walk + // cannot reach (e.g. the server sent a cycle or a dangling + // predecessor) are appended at the end of the bucket sorted + // by id so the UI doesn't lose channels. + let all_channels: Vec<&Channel> = state.channels.values().collect(); + let channels: Vec<&Channel> = sort_channels_tree(&all_channels); let clients: Vec<&Client> = state.clients.values().collect(); let channels_dto: Vec = channels @@ -605,3 +726,91 @@ const _ROOT_MATCHES_UPSTREAM: () = { // also considers the root. let _ = TsChannelId(0); }; + +#[cfg(test)] +mod tests { + use super::sort_channels_tree_by; + + /// Lightweight fixture mirroring just the (id, parent, order) + /// triple that the linked-list sort needs. Avoids constructing + /// a real `tsclientlib::data::Channel` (which requires a live + /// connection) in unit tests. + #[derive(Debug, PartialEq)] + struct FakeChannel { + id: u64, + parent: u64, + order: u64, + } + + fn extract(c: &FakeChannel) -> (u64, u64, u64) { + (c.id, c.parent, c.order) + } + + #[test] + fn channel_sort_linked_list_under_one_parent() { + // Server emits four root-level channels in arbitrary HashMap + // iteration order. order=0 -> first; order=X means "comes + // after the channel with id=X". Expected emitted order is + // the linked-list walk: a -> b -> c -> d. + let a = FakeChannel { id: 100, parent: 0, order: 0 }; + let b = FakeChannel { id: 200, parent: 0, order: 100 }; + let c = FakeChannel { id: 300, parent: 0, order: 200 }; + let d = FakeChannel { id: 400, parent: 0, order: 300 }; + // Deliberately shuffled inputs. + let inputs: Vec<&FakeChannel> = vec![&c, &a, &d, &b]; + let sorted = sort_channels_tree_by(&inputs, extract); + let ids: Vec = sorted.iter().map(|c| c.id).collect(); + assert_eq!(ids, vec![100, 200, 300, 400]); + } + + #[test] + fn channel_sort_disconnected_predecessor_falls_back_by_id() { + // a is the head. b correctly chains. c claims predecessor + // = 999 which does not exist among the siblings. c must + // not be dropped — it falls back to the leftover bucket + // appended sorted by id at the end. + let a = FakeChannel { id: 100, parent: 0, order: 0 }; + let b = FakeChannel { id: 200, parent: 0, order: 100 }; + let c = FakeChannel { id: 300, parent: 0, order: 999 }; + let inputs: Vec<&FakeChannel> = vec![&c, &a, &b]; + let sorted = sort_channels_tree_by(&inputs, extract); + let ids: Vec = sorted.iter().map(|c| c.id).collect(); + assert_eq!(ids, vec![100, 200, 300]); + } + + #[test] + fn channel_sort_emits_subtree_depth_first() { + // Tree: + // root (id=0, implicit) + // ├── a (id=10, order=0) + // │ ├── a1 (id=11, parent=10, order=0) + // │ └── a2 (id=12, parent=10, order=11) + // └── b (id=20, order=10) + // Expected emission: a, a1, a2, b + let a = FakeChannel { id: 10, parent: 0, order: 0 }; + let a1 = FakeChannel { id: 11, parent: 10, order: 0 }; + let a2 = FakeChannel { id: 12, parent: 10, order: 11 }; + let b = FakeChannel { id: 20, parent: 0, order: 10 }; + let inputs: Vec<&FakeChannel> = vec![&b, &a2, &a, &a1]; + let sorted = sort_channels_tree_by(&inputs, extract); + let ids: Vec = sorted.iter().map(|c| c.id).collect(); + assert_eq!(ids, vec![10, 11, 12, 20]); + } + + #[test] + fn channel_sort_does_not_loop_on_cycle() { + // a says "comes after b"; b says "comes after a". The + // walk must terminate (cycle guard) and both channels + // must still appear in the output via the leftover path. + let a = FakeChannel { id: 1, parent: 0, order: 2 }; + let b = FakeChannel { id: 2, parent: 0, order: 1 }; + let inputs: Vec<&FakeChannel> = vec![&a, &b]; + let sorted = sort_channels_tree_by(&inputs, extract); + // Both reachable in some deterministic order (id-sorted + // in the leftover bucket since cursor=0 finds nothing). + let ids: Vec = sorted.iter().map(|c| c.id).collect(); + assert_eq!(ids.len(), 2); + assert!(ids.contains(&1)); + assert!(ids.contains(&2)); + } +}