gui: fix channel sort order (#17) (#20)

# Summary
Channel ordering is currently broken as described in #17.  This change makes sorting work correctly and cleans up the logic a bit.

# Changes
- creates a `ChannelsState` wrapper struct to handle this behavior
- moves the logic for handling `ChannelState` processing, including data update and parent-child tree sorting, into the impl for `ChannelsState`
- moves the logic for handling `ChannelRemove` into this impl

Parent child sorting properly applies the position values, which are arbitrary integers that are supposed to be sorted in numerical order. Lexicographical sorting is use for tiebreaking, which lines up (at least in my testing) with the official client's behavior. We may handle some lexicographical edge cases differently (spaces, symbols, etc) but 1. the Desktop client compliance is best effort and 2. users should use the position fields instead of relying on text sort order. Some compatibility is still helpful for matching temporary channel positioning, especially for servers with automated channel creation workflows.

This code is a bit complicated, as the mumble protocol makes no guarantees which order the channels will be sent. It ended up being simpler to just bulk recreate the children anytime any channel update is sent. I don't expect this to ever have performance issues, though maybe someday some server with 10,000 channels will send us a bug report 😆

# Testing
I tested this change by creating a bunch of channel with various sort orders and names. I compared the behavior with the official desktop client and our client seemed to follow along.

Reviewed-on: #20

gui/src/app.rs

@@ -4,7 +4,7 @@ use dioxus::prelude::*;
 use mime_guess::Mime;
 use mumble_web2_common::{ClientConfig, ServerStatus};
 use ordermap::OrderSet;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 
 use crate::imp;
 
@@ -54,14 +54,6 @@ pub enum Command {
 use Command::*;
 use ConnectionState::*;
 
-#[derive(Default)]
-pub struct ChannelState {
-    pub name: String,
-    pub children: OrderSet<ChannelId>,
-    pub users: OrderSet<UserId>,
-    pub parent: Option<ChannelId>,
-}
-
 #[derive(Default)]
 pub struct UserState {
     pub name: String,
@@ -94,8 +86,121 @@ pub struct Chat {
 }
 
 #[derive(Default)]
-pub struct ServerState {
+pub struct ChannelState {
+    pub name: String,
+    pub children: OrderSet<ChannelId>,
+    pub users: OrderSet<UserId>,
+    pub parent: Option<ChannelId>,
+    pub position: i32,
+}
+
+impl ChannelState {
+    pub fn update_from_channel_state(
+        &mut self,
+        channel_state: &mumble_protocol::control::msgs::ChannelState,
+    ) {
+        if channel_state.has_position() {
+            self.position = channel_state.get_position();
+        }
+        if channel_state.has_parent() {
+            self.parent = Some(channel_state.get_parent());
+        }
+        if channel_state.has_name() {
+            self.name = channel_state.get_name().to_string();
+        }
+    }
+}
+
+#[derive(Default)]
+pub struct ChannelsState {
     pub channels: HashMap<ChannelId, ChannelState>,
+}
+
+impl ChannelsState {
+    pub fn update_from_channel_state(
+        &mut self,
+        channel_state: &mumble_protocol::control::msgs::ChannelState,
+    ) {
+        self.channels
+            .entry(channel_state.get_channel_id())
+            .or_default()
+            .update_from_channel_state(channel_state);
+
+        self.update_channel_parents();
+    }
+
+    pub fn update_from_channel_remove(
+        &mut self,
+        channel_remove: &mumble_protocol::control::msgs::ChannelRemove,
+    ) {
+        self.channels.remove(&channel_remove.get_channel_id());
+        self.update_channel_parents();
+    }
+
+    pub fn update_channel_parents(&mut self) {
+        // Zero out existing children
+        for state in self.channels.values_mut() {
+            state.children.clear();
+        }
+
+        let mut to_sort: Vec<(ChannelId, Option<ChannelId>, i32, String)> = Vec::new();
+        for (id, state) in self.channels.iter() {
+            // Handle channels with no parent (the root channel)
+            let Some(parent_id) = state.parent else {
+                to_sort.push((*id, None, 0, state.name.clone()));
+                continue;
+            };
+
+            // If a channel has a parent that we haven't gotten a channel
+            // state packet for, ignore it
+            if !self.channels.contains_key(&parent_id) {
+                continue;
+            }
+
+            to_sort.push((*id, Some(parent_id), state.position, state.name.clone()));
+        }
+
+        let pos_name: HashMap<ChannelId, (i32, String)> = self
+            .channels
+            .iter()
+            .map(|(&id, state)| (id, (state.position, state.name.clone())))
+            .collect();
+
+        let mut updated: HashSet<ChannelId> = HashSet::new();
+
+        while updated.len() < to_sort.len() {
+            for &(id, ref parent_id, position, ref name) in &to_sort {
+                let Some(parent_id) = parent_id else {
+                    updated.insert(id);
+                    continue;
+                };
+
+                if updated.contains(&id) || !updated.contains(&parent_id) {
+                    continue;
+                }
+
+                // Unwrap should never fail here since we pre filter
+                let parent = self.channels.get_mut(&parent_id).unwrap();
+
+                let mut insert_index = parent.children.len();
+                for (i, &child) in parent.children.iter().enumerate() {
+                    let (p, ref n) = pos_name[&child];
+                    if (position == p && name < n) || p > position {
+                        insert_index = i;
+                        break;
+                    }
+                }
+
+                parent.children.insert_before(insert_index, id);
+                updated.insert(id);
+            }
+        }
+    }
+}
+
+#[derive(Default)]
+pub struct ServerState {
+    pub channels_state: ChannelsState,
     pub users: HashMap<UserId, UserState>,
     pub chat: Vec<Chat>,
     pub session: Option<UserId>,
@@ -182,7 +287,7 @@ pub fn Channel(id: ChannelId) -> Element {
     let net: Coroutine<Command> = use_coroutine_handle();
     let server = STATE.server.read();
     let user = server.session.unwrap();
-    let Some(state) = server.channels.get(&id) else {
+    let Some(state) = server.channels_state.channels.get(&id) else {
         return rsx!("missing channel {id}");
     };
 
@@ -336,7 +441,7 @@ pub fn ControlView(config: Resource<ClientConfig>) -> Element {
         return rsx!();
     };
 
-    let current_channel_name = server.channels[&channel].name.clone();
+    let current_channel_name = server.channels_state.channels[&channel].name.clone();
 
     let proxy_url = config
         .read_unchecked()
@@ -528,7 +633,7 @@ pub fn ServerView(config: Resource<ClientConfig>) -> Element {
             class: "server_grid",
             div {
                 class: "server_channel_box",
-                for (id, state) in server.channels.iter() {
+                for (id, state) in server.channels_state.channels.iter() {
                     if state.parent.is_none() {
                         Channel { id: *id }
                     }

gui/src/lib.rs

@@ -335,41 +335,11 @@ fn accept_packet(
         }
         ControlPacket::ChannelState(u) => {
             let mut server = STATE.server.write();
-            let id = u.get_channel_id();
-
-            let state = server.channels.entry(id).or_default();
-            let new_parent = if u.has_parent() {
-                if let Some(parent) = state.parent.and_then(|p| server.channels.get_mut(&p)) {
-                    parent.children.remove(&id);
-                }
-
-                let parent_id = u.get_parent();
-                let parent = server.channels.entry(parent_id).or_default();
-                if u.has_position() && u.get_position() as usize <= parent.children.len() {
-                    // TODO: what if positions are received out of order? we need to sort afterwards?
-                    parent.children.insert_before(u.get_position() as usize, id);
-                } else {
-                    parent.children.insert(id);
-                }
-                Some(parent_id)
-            } else {
-                None
-            };
-
-            let state = server.channels.entry(id).or_default();
-            state.parent = new_parent;
-            if u.has_name() {
-                state.name = u.get_name().to_string();
-            }
+            server.channels_state.update_from_channel_state(&u);
         }
         ControlPacket::ChannelRemove(u) => {
             let mut server = STATE.server.write();
-            let id = u.get_channel_id();
-            if let Some(channel) = server.channels.remove(&id) {
-                if let Some(parent) = channel.parent.and_then(|p| server.channels.get_mut(&p)) {
-                    parent.children.remove(&id);
-                }
-            }
+            server.channels_state.update_from_channel_remove(&u);
         }
         ControlPacket::UserState(u) => {
             let mut server = STATE.server.write();
@@ -381,12 +351,13 @@ fn accept_packet(
             let state = state_entry.or_default();
             // the server might now send a channel_id if the user is in channel=0
             if u.has_channel_id() || new {
-                if let Some(parent) = server.channels.get_mut(&state.channel) {
+                if let Some(parent) = server.channels_state.channels.get_mut(&state.channel) {
                     parent.users.remove(&id);
                 }
 
                 let channel_id = u.get_channel_id();
                 server
+                    .channels_state
                     .channels
                     .entry(channel_id)
                     .or_default()
@@ -418,7 +389,7 @@ fn accept_packet(
             let mut server = STATE.server.write();
             let id = u.get_session();
             if let Some(state) = server.users.remove(&id) {
-                if let Some(parent) = server.channels.get_mut(&state.channel) {
+                if let Some(parent) = server.channels_state.channels.get_mut(&state.channel) {
                     parent.users.remove(&id);
                 }
             }