edison/chanora
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(audio,protocol,ui): TC-2.3 + TC-10 + TC-13 + channel tree hierarchy

Browse Source 
Five user-reported defects + one auto-test regression-catcher.

== TC-2.3: capability badge stuck at L0Focused on Korean Win 11 ==
Root cause: in WindowsRawInputBackend::start() (and the parallel
WindowsHookBackend), the worker thread's armed.store(ok, ...) only
ran AFTER GetMessageW returned (i.e. on WM_QUIT). During normal
arming the message pump runs forever, so armed stayed at its
initial false value, and descriptor() reported L0Focused even
though RegisterRawInputDevices had succeeded.

Fix: run_raw_input_loop and run_hook_loop now take armed as a
parameter and flip it to true inside the loop right after the
successful registration, before blocking on GetMessageW. The
outer setter is kept as a belt-and-braces clear-on-failure path.

Also drops the redundant outer 'Raw Input armed' / 'low-level
hook armed' log lines — the in-loop 'Raw Input devices
registered' / 'low-level hooks installed' messages already
convey arming success with full context.

New tests raw_input_backend_start_flips_armed_to_l2 and
hook_backend_start_flips_armed_to_l2 call real start(), sleep
80 ms, assert descriptor().level == L2GlobalHoldToTalk. Replaces
the previous #[ignore]'d real-start smoke test which never
asserted on the descriptor.

== TC-10: no-permission channel rejection invisible ==
Root cause 1: chanora_protocol::adapter::move_self_to used the
fire-and-forget send() on the client_move command. TS3 server
replies with a typed error event the adapter discarded, and
move_to_channel returned Ok regardless.

Root cause 2: even when chanora_core::voice_join detected the
non-confirmation via snapshot polling, the rolled-back error
flowed into the connect-form-area _error string which is hidden
post-connect. The user saw no feedback.

Fix:
* New ProtocolError::ServerRejected { code: u32, message: String }
  carries the canonical TS3 error code per the official catalogue
  at https://github.com/ReSpeak/tsdeclarations (Errors.csv).
* move_self_to now uses send_with_result, returns a MessageHandle.
  The connection-task loop holds a pending_moves HashMap keyed by
  MessageHandle, services StreamItem::MessageResult by looking up
  and resolving the reply with either Ok or the typed
  ServerRejected.
* Pending entries have a 3 s deadline so a server that never
  replies doesn't leak the reply channel — expired entries fall
  back to Ok and let the snapshot poll handle confirmation.
* voice_join short-circuits on ServerRejected (no need for the
  full snapshot poll), still polls for confirmation as a
  belt-and-braces fallback for legacy servers; on poll failure
  emits ServerRejected with sentinel code 0x0001 (undefined).
* New BridgeError::ServerRejected mirror with the same fields;
  CoreError → BridgeError mapping preserves the typed variant.
* Flutter _onJoinChannel shows a floating SnackBar with a
  localised message selected by error code (channelJoinFailed*
  l10n entries). 6 known codes mapped to specific messages
  (insufficient permission, wrong password, channel full,
  family limit, private channel, timeout); everything else
  falls back to the server-supplied generic message.

== TC-13: mouse side-button capture only works on text field ==
The _PttBindingCaptureDialog wrapped its Column with a Listener
using the default HitTestBehavior.deferToChild. Pointer events
landing on the dialog's empty padding regions weren't claimed by
any child and so were never delivered to the Listener.

Fix: explicit HitTestBehavior.opaque so the entire dialog area
catches PointerDown events regardless of where the cursor sits.

== Channel tree hierarchy ==
Reported issue: tree rendered as flat list, no indication of
parent-child nesting. The bridge already carries the
field; the renderer just ignored it.

Fix in _SnapshotView: walk the (already DFS-sorted) channel list
and compute each row's depth from its parent's depth. Render
left-padding of depth * 18 dp. Cap depth at 6 to keep deep
hierarchies visually bounded; the cap plateaus silently (no
glyph, channel still tappable, data carries the real depth).

== Responsive layout ==
Connected layout is now LayoutBuilder-driven. Below 840 dp wide
(Material's tablet/desktop breakpoint) the original stacked
column layout is used (Voice Bar on top, channel tree below).
At 840 dp and above the layout becomes a side-by-side Row with
the Voice Bar pinned at 320 dp on the left and the channel tree
Expanded on the right.

Verification
- cargo check --workspace: clean.
- cargo test --workspace --lib: 80 / 0 / 1 (unchanged Linux total;
  +2 new Windows-only tests not counted here).
- flutter analyze: clean (6 pre-existing Radio.groupValue infos).
- FRB bindings regenerated to expose BridgeError_ServerRejected.
This commit is contained in:
EdisonJwa
2026-05-16 03:27:25 +08:00
parent 0b6ea11077
commit 2511b24982
15 changed files with 644 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/chanora_audio/src/ptt_backends/windows.rs
+84 -26
View File
@@ -268,8 +268,22 @@ impl DesktopPttBackend for WindowsRawInputBackend {
// — otherwise the main thread's readiness probe
// times out (the signal would only fire when
// WM_QUIT was eventually delivered).
let ok = unsafe { run_raw_input_loop(init_tx) };
armed.store(ok, Ordering::Release);
//
// Also pass `armed` so the loop can flip the flag
// to true inside the same critical window — the
// outer `armed.store(ok, ...)` only runs after the
// message pump returns (i.e. on WM_QUIT), which
// never happens during normal arming. Without the
// in-loop set, `descriptor()` would always report
// L0Focused even though the backend is correctly
// armed and receiving WM_INPUT events.
let ok = unsafe { run_raw_input_loop(init_tx, armed.clone()) };
// Belt-and-braces: if the loop's normal exit
// (WM_QUIT) happens before stop() runs the outer
// set, clear the flag here too.
if !ok {
armed.store(false, Ordering::Release);
}
// If init failed, exit immediately. If it
// succeeded, run_raw_input_loop already ran the
@@ -289,15 +303,9 @@ impl DesktopPttBackend for WindowsRawInputBackend {
// this just lets us log accurately.
match init_rx.recv_timeout(std::time::Duration::from_secs(2)) {
Ok(true) => {
info!(
target: "chanora_audio",
bound_input_class = ?match self.binding.class() {
2 => "mouse-side-button",
1 => "keyboard",
_ => "none",
},
"windows ptt: Raw Input armed"
);
// Success — the in-loop log line already wrote
// "Raw Input devices registered" with full context.
// Don't double-log; just continue.
}
Ok(false) => {
warn!(
@@ -370,7 +378,10 @@ impl Drop for WindowsRawInputBackend {
/// # Safety
/// Calls into Win32 directly. Must be invoked on the thread that
/// owns the message window (created here).
unsafe fn run_raw_input_loop(init_tx: std::sync::mpsc::SyncSender<bool>) -> bool {
unsafe fn run_raw_input_loop(
init_tx: std::sync::mpsc::SyncSender<bool>,
armed: Arc<AtomicBool>,
) -> bool {
// Helper that signals the readiness state to the main thread.
// We send exactly once at the first decisive moment (either an
// early-fail return or right after a successful
@@ -473,6 +484,14 @@ unsafe fn run_raw_input_loop(init_tx: std::sync::mpsc::SyncSender<bool>) -> bool
target: "chanora_audio",
"windows ptt: Raw Input devices registered (keyboard + mouse, INPUTSINK)"
);
// Flip armed = true here, inside the loop, BEFORE blocking on
// GetMessageW. The outer worker closure's armed.store(ok, ...)
// only runs on WM_QUIT and so never fires during normal use.
// Without this in-loop store, descriptor() would always report
// L0Focused even though the backend is correctly receiving
// WM_INPUT events — the bug that landed the capability badge
// stuck at L0Focused on Korean Win 11 in TC-2.3.
armed.store(true, Ordering::Release);
// Signal readiness NOW (before we block on GetMessageW). The
// main thread's init probe is waiting for this; the loop runs
// until WM_QUIT and the return value at end-of-life is no
@@ -721,8 +740,10 @@ impl DesktopPttBackend for WindowsHookBackend {
});
});
let ok = unsafe { run_hook_loop(init_tx) };
armed.store(ok, Ordering::Release);
let ok = unsafe { run_hook_loop(init_tx, armed.clone()) };
if !ok {
armed.store(false, Ordering::Release);
}
HOOK_CTX.with(|cell| {
*cell.borrow_mut() = None;
@@ -731,10 +752,10 @@ impl DesktopPttBackend for WindowsHookBackend {
.map_err(|e| PttBackendError::Init(format!("llhook thread: {e}")))?;
match init_rx.recv_timeout(std::time::Duration::from_secs(2)) {
Ok(true) => info!(
target: "chanora_audio",
"windows ptt: low-level hook armed"
),
Ok(true) => {
// Success — the in-loop log line already wrote
// "low-level hooks installed"; no need to repeat.
}
Ok(false) => warn!(
target: "chanora_audio",
"windows ptt: SetWindowsHookEx failed; descriptor will report L0"
@@ -795,7 +816,10 @@ impl Drop for WindowsHookBackend {
/// # Safety
/// Calls Win32 directly; must run on the thread that owns the
/// hook handles.
unsafe fn run_hook_loop(init_tx: std::sync::mpsc::SyncSender<bool>) -> bool {
unsafe fn run_hook_loop(
init_tx: std::sync::mpsc::SyncSender<bool>,
armed: Arc<AtomicBool>,
) -> bool {
let mut signal = Some(init_tx);
macro_rules! report {
($v:expr) => {
@@ -851,6 +875,9 @@ unsafe fn run_hook_loop(init_tx: std::sync::mpsc::SyncSender<bool>) -> bool {
target: "chanora_audio",
"windows ptt: low-level hooks installed (WH_KEYBOARD_LL + WH_MOUSE_LL)"
);
// Flip armed = true here, before blocking on GetMessageW.
// Same rationale as run_raw_input_loop.
armed.store(true, Ordering::Release);
// Signal readiness now, before blocking on GetMessageW. The
// return value at end-of-loop is no longer used by the init
// probe.
@@ -1313,18 +1340,49 @@ mod tests {
assert_eq!(b.binding.class(), 0);
}
/// Real-runtime arm of the Raw Input backend. Requires a
/// Windows message pump and Raw Input registration access, so
/// the test is gated `#[ignore]` and only runs when explicitly
/// requested with `cargo test -- --ignored` on the Korean
/// host.
/// Full start → descriptor → stop cycle on the real Windows
/// runtime. Catches the regression where `armed` was only
/// flipped after `WM_QUIT` (i.e. never during normal use), so
/// `descriptor()` reported `L0Focused` even though the
/// `RegisterRawInputDevices` call had succeeded.
///
/// Not ignored — must run on every Windows test pass.
#[test]
#[ignore = "requires real Windows runtime; runs on the host smoke pass"]
fn raw_input_backend_real_start_succeeds() {
fn raw_input_backend_start_flips_armed_to_l2() {
let mut b = WindowsRawInputBackend::try_new().unwrap();
let gate = AudioTransmitGate::new(false);
b.start(gate, binding(PttInputClass::Keyboard, "Space"))
.expect("real RegisterRawInputDevices should succeed on a Windows desktop");
// The worker thread flips armed inside the message-pump
// loop. Give it a tiny window to do so.
std::thread::sleep(std::time::Duration::from_millis(80));
let d = b.descriptor();
assert_eq!(
d.level,
PttCapabilityLevel::L2GlobalHoldToTalk,
"armed must flip to true while the loop is running"
);
assert_eq!(d.backend_id, "raw-input");
assert_eq!(d.bound_input_class, Some("keyboard"));
b.stop();
}
/// Same as above but for the WH_KEYBOARD_LL / WH_MOUSE_LL hook
/// backend. Catches the parallel armed-flag regression there.
#[test]
fn hook_backend_start_flips_armed_to_l2() {
let mut b = WindowsHookBackend::try_new().unwrap();
let gate = AudioTransmitGate::new(false);
b.start(gate, binding(PttInputClass::Keyboard, "Space"))
.expect("real SetWindowsHookExW should succeed on a Windows desktop");
std::thread::sleep(std::time::Duration::from_millis(80));
let d = b.descriptor();
assert_eq!(
d.level,
PttCapabilityLevel::L2GlobalHoldToTalk,
"armed must flip to true while the hook is running"
);
assert_eq!(d.backend_id, "low-level-hook");
b.stop();
}
crates/chanora_bridge/src/frb_generated.rs
+21 -2
View File
@@ -1232,6 +1232,14 @@ impl SseDecode for crate::BridgeError {
return crate::BridgeError::AlreadyConnected;
}
5 => {
let mut var_code = <u32>::sse_decode(deserializer);
let mut var_message = <String>::sse_decode(deserializer);
return crate::BridgeError::ServerRejected {
code: var_code,
message: var_message,
};
}
6 => {
let mut var_field0 = <String>::sse_decode(deserializer);
return crate::BridgeError::Unmapped(var_field0);
}
@@ -1636,8 +1644,14 @@ impl flutter_rust_bridge::IntoDart for crate::BridgeError {
}
crate::BridgeError::NotConnected => [3.into_dart()].into_dart(),
crate::BridgeError::AlreadyConnected => [4.into_dart()].into_dart(),
crate::BridgeError::ServerRejected { code, message } => [
5.into_dart(),
code.into_into_dart().into_dart(),
message.into_into_dart().into_dart(),
]
.into_dart(),
crate::BridgeError::Unmapped(field0) => {
[5.into_dart(), field0.into_into_dart().into_dart()].into_dart()
[6.into_dart(), field0.into_into_dart().into_dart()].into_dart()
}
_ => {
unimplemented!("");
@@ -1897,8 +1911,13 @@ impl SseEncode for crate::BridgeError {
crate::BridgeError::AlreadyConnected => {
<i32>::sse_encode(4, serializer);
}
crate::BridgeError::Unmapped(field0) => {
crate::BridgeError::ServerRejected { code, message } => {
<i32>::sse_encode(5, serializer);
<u32>::sse_encode(code, serializer);
<String>::sse_encode(message, serializer);
}
crate::BridgeError::Unmapped(field0) => {
<i32>::sse_encode(6, serializer);
<String>::sse_encode(field0, serializer);
}
_ => {
crates/chanora_bridge/src/lib.rs
+15
View File
@@ -66,6 +66,18 @@ pub enum BridgeError {
/// connection.
#[error("already connected")]
AlreadyConnected,
/// A server command was rejected by the TeamSpeak server. The
/// `code` is the canonical TS3 error number (see
/// https://github.com/ReSpeak/tsdeclarations Errors.csv); the
/// `message` is the server-supplied text. The UI uses `code`
/// to look up a localised explanation.
#[error("server rejected (code {code}): {message}")]
ServerRejected {
/// Raw TS3 error code.
code: u32,
/// Server-supplied message text.
message: String,
},
/// An unmapped error escaped the subsystem boundary. Production
/// callers should never see this; if they do, it is a mapping
/// bug here.
@@ -85,6 +97,9 @@ impl From<chanora_core::CoreError> for BridgeError {
host,
reason,
}) => BridgeError::DnsFailed { host, reason },
chanora_core::CoreError::Protocol(
chanora_protocol::ProtocolError::ServerRejected { code, message },
) => BridgeError::ServerRejected { code, message },
chanora_core::CoreError::Protocol(p) => BridgeError::Connection(format!("{p}")),
chanora_core::CoreError::Audio(a) => BridgeError::Connection(format!("audio: {a}")),
chanora_core::CoreError::Storage(s) => {
crates/chanora_protocol/src/adapter.rs
+100 -19
View File
@@ -27,7 +27,8 @@ use tracing::{info, warn};
use tsclientlib::data::{self, Channel, Client};
use tsclientlib::prelude::*;
use tsclientlib::{
ChannelId as TsChannelId, Connection, DisconnectOptions, Identity, OutCommandExt, StreamItem,
ChannelId as TsChannelId, Connection, DisconnectOptions, Identity, MessageHandle,
OutCommandExt, StreamItem,
};
use tsproto_packets::packets::{InAudioBuf, OutPacket};
@@ -410,6 +411,20 @@ async fn connection_task(
let _ = ready_tx.send(Ok(()));
// Pending `client_move` requests: each one is keyed by the
// `MessageHandle` tsclientlib returns from `send_with_result`.
// When the corresponding `StreamItem::MessageResult` arrives we
// resolve the oneshot back to the caller. Entries also carry a
// deadline so a server that never replies doesn't leak the
// reply channel — at most 3 s of pending state per move.
let mut pending_moves: HashMap<
MessageHandle,
(
oneshot::Sender<Result<(), ProtocolError>>,
std::time::Instant,
),
> = HashMap::new();
// Main loop: pump events, service requests, forward voice.
loop {
// 1. Drain any outbound voice packets first — they're time-sensitive.
@@ -426,20 +441,49 @@ async fn connection_task(
};
match pump.await {
Ok(Some(Ok(item))) => {
if let StreamItem::Audio(buf) = item {
// Extract `from` client id then forward.
let from = packet_sender_id(&buf);
if let Some(from) = from {
if voice_in_tx
.try_send(InboundVoice {
from_client: from,
packet: buf,
})
.is_err()
{
// Subscriber is too slow or absent; drop.
match item {
StreamItem::Audio(buf) => {
let from = packet_sender_id(&buf);
if let Some(from) = from {
if voice_in_tx
.try_send(InboundVoice {
from_client: from,
packet: buf,
})
.is_err()
{
// Subscriber is too slow or absent; drop.
}
}
}
StreamItem::MessageResult(handle, result) => {
if let Some((reply, _deadline)) = pending_moves.remove(&handle) {
let mapped = match result {
Ok(()) => Ok(()),
Err(cmd_err) => {
// tsclientlib's CommandError carries a
// typed `TsError` (the canonical TS3
// error code) plus an optional missing
// permission. We convert to our typed
// ProtocolError::ServerRejected so the
// upper layers can render a localised
// explanation by code instead of a
// generic backend string.
let code = cmd_err.error as u32;
let message = cmd_err.error.to_string();
info!(
target: "chanora_protocol",
code,
message = %message,
"server rejected client_move"
);
Err(ProtocolError::ServerRejected { code, message })
}
};
let _ = reply.send(mapped);
}
}
_ => { /* book / message / other events: ignore */ }
}
}
Ok(Some(Err(e))) => {
@@ -454,6 +498,28 @@ async fn connection_task(
Err(_) => { /* no event in 20 ms */ }
}
// 2b. Sweep stale pending_moves whose deadline has passed.
// The server should always reply within ~1 s; 3 s is a
// generous ceiling. Expired entries get an Ok() so the
// caller's snapshot-confirmation polling still has a chance
// to detect success (fall back to the previous optimistic
// behaviour rather than blocking the user with a fake
// ServerRejected).
if !pending_moves.is_empty() {
let now = std::time::Instant::now();
pending_moves.retain(|_, (reply, deadline)| {
if now >= *deadline {
// Cannot move `reply` out of `&mut` cleanly here
// without an intermediate take(); use a sentinel
// sender so retain's signature works.
let _ = std::mem::replace(reply, oneshot::channel().0).send(Ok(()));
false
} else {
true
}
});
}
// 3. Service at most one control request (non-blocking).
match rx.try_recv() {
Ok(Request::Snapshot(reply)) => {
@@ -461,8 +527,18 @@ async fn connection_task(
let _ = reply.send(snap);
}
Ok(Request::MoveToChannel { channel_id, password, reply }) => {
let r = move_self_to(&mut con, channel_id, password.as_deref());
let _ = reply.send(r);
match move_self_to(&mut con, channel_id, password.as_deref()) {
Ok(handle) => {
let deadline =
std::time::Instant::now() + Duration::from_secs(3);
pending_moves.insert(handle, (reply, deadline));
}
Err(e) => {
// Couldn't even send the command; report
// immediately.
let _ = reply.send(Err(e));
}
}
}
Ok(Request::SetMuted { input, output, reply }) => {
let r = set_self_muted(&mut con, input, output);
@@ -488,12 +564,16 @@ async fn connection_task(
/// Move our own client into `channel_id` with an optional password.
/// Looks up our `own_client` in the current state and dispatches the
/// generated `client_move` command via the `OutCommandExt` trait.
/// generated `client_move` command via `send_with_result`. The
/// returned `MessageHandle` is correlated by the connection loop
/// against the next `StreamItem::MessageResult` so we can surface
/// typed `ServerRejected` errors (no permission, wrong password,
/// channel full, etc.) per the TS3 error catalogue.
fn move_self_to(
con: &mut Connection,
channel_id: u64,
password: Option<&str>,
) -> Result<(), ProtocolError> {
) -> Result<MessageHandle, ProtocolError> {
let state = con
.get_state()
.map_err(|e| ProtocolError::Backend(format!("get_state: {e}")))?;
@@ -507,10 +587,11 @@ fn move_self_to(
if let Some(pw) = password {
part = part.set_password(pw);
}
part.send(con)
let handle = part
.send_with_result(con)
.map_err(|e| ProtocolError::Backend(format!("client_move send: {e}")))?;
info!(target: "chanora_protocol", channel_id, "client_move sent");
Ok(())
Ok(handle)
}
/// Send a `clientupdate` with the requested mute fields set. `None`
crates/chanora_protocol/src/lib.rs
+15
View File
@@ -95,6 +95,21 @@ pub enum ProtocolError {
#[error("protocol timeout")]
Timeout,
/// A server command was rejected by the TeamSpeak server with
/// a typed error code. The `code` is the raw TS3 error number
/// (see https://github.com/ReSpeak/tsdeclarations Errors.csv),
/// and `message` is the server-supplied human-readable text.
/// Distinguishing this from `Backend` lets the UI surface a
/// localised explanation (insufficient permission, wrong
/// channel password, etc.) instead of a generic failure.
#[error("server rejected (code {code}): {message}")]
ServerRejected {
/// Raw TS3 error code (e.g. 0x0a08 = `permissions_client_insufficient`).
code: u32,
/// Server-supplied message text.
message: String,
},
/// A backend error escaped the mapping. Production callers
/// should never see this; if they do, it is a mapping bug here.
#[error("protocol backend: {0}")]