Change output sorting to match make/model/serial first

We can do this now that we have libdisplay-info.

6 files changed, 102 insertions, 18 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 131a0c75..6b10a793 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2304,6 +2304,7 @@ version = "0.1.8"
 dependencies = [
  "bitflags 2.6.0",
  "csscolorparser",
+ "k9",
  "knuffel",
  "miette",
  "niri-ipc",
diff --git a/Cargo.toml b/Cargo.toml
index e8a4574b..edb6df3a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -13,6 +13,7 @@ repository = "https://github.com/YaLTeR/niri"
 anyhow = "1.0.86"
 bitflags = "2.6.0"
 clap = { version = "4.5.14", features = ["derive"] }
+k9 = "0.12.0"
 serde = { version = "1.0.205", features = ["derive"] }
 serde_json = "1.0.122"
 tracing = { version = "0.1.40", features = ["max_level_trace", "release_max_level_debug"] }
@@ -104,7 +105,7 @@ features = [
 
 [dev-dependencies]
 approx = "0.5.1"
-k9 = "0.12.0"
+k9.workspace = true
 proptest = "1.5.0"
 proptest-derive = "0.5.0"
 xshell = "0.2.6"
diff --git a/niri-config/Cargo.toml b/niri-config/Cargo.toml
index 93ddc9aa..acb301dd 100644
--- a/niri-config/Cargo.toml
+++ b/niri-config/Cargo.toml
@@ -19,5 +19,6 @@ tracing.workspace = true
 tracy-client.workspace = true
 
 [dev-dependencies]
-pretty_assertions = "1.4.0"
+k9.workspace = true
 miette = { version = "5.10.0", features = ["fancy"] }
+pretty_assertions = "1.4.0"
diff --git a/niri-config/src/lib.rs b/niri-config/src/lib.rs
index d9d55583..0820a018 100644
--- a/niri-config/src/lib.rs
+++ b/niri-config/src/lib.rs
@@ -1821,6 +1821,26 @@ impl OutputName {
 
         true
     }
+
+    // Similar in spirit to Ord, but I don't want to derive Eq to avoid mistakes (you should use
+    // `Self::match`, not Eq).
+    pub fn compare(&self, other: &Self) -> std::cmp::Ordering {
+        let self_missing_mms = self.make.is_none() && self.model.is_none() && self.serial.is_none();
+        let other_missing_mms =
+            other.make.is_none() && other.model.is_none() && other.serial.is_none();
+
+        match (self_missing_mms, other_missing_mms) {
+            (true, true) => self.connector.cmp(&other.connector),
+            (true, false) => std::cmp::Ordering::Greater,
+            (false, true) => std::cmp::Ordering::Less,
+            (false, false) => self
+                .make
+                .cmp(&other.make)
+                .then_with(|| self.model.cmp(&other.model))
+                .then_with(|| self.serial.cmp(&other.serial))
+                .then_with(|| self.connector.cmp(&other.connector)),
+        }
+    }
 }
 
 impl<S> knuffel::Decode<S> for DefaultColumnWidth
@@ -2753,6 +2773,7 @@ pub fn set_miette_hook() -> Result<(), miette::InstallError> {
 
 #[cfg(test)]
 mod tests {
+    use k9::snapshot;
     use pretty_assertions::assert_eq;
 
     use super::*;
@@ -3445,6 +3466,20 @@ mod tests {
         assert_eq!(config.input.keyboard.repeat_rate, 25);
     }
 
+    fn make_output_name(
+        connector: &str,
+        make: Option<&str>,
+        model: Option<&str>,
+        serial: Option<&str>,
+    ) -> OutputName {
+        OutputName {
+            connector: connector.to_string(),
+            make: make.map(|x| x.to_string()),
+            model: model.map(|x| x.to_string()),
+            serial: serial.map(|x| x.to_string()),
+        }
+    }
+
     #[test]
     fn test_output_name_match() {
         fn check(
@@ -3454,12 +3489,7 @@ mod tests {
             model: Option<&str>,
             serial: Option<&str>,
         ) -> bool {
-            let name = OutputName {
-                connector: connector.to_string(),
-                make: make.map(|x| x.to_string()),
-                model: model.map(|x| x.to_string()),
-                serial: serial.map(|x| x.to_string()),
-            };
+            let name = make_output_name(connector, make, model, serial);
             name.matches(target)
         }
 
@@ -3510,4 +3540,48 @@ mod tests {
         ));
         assert!(!check("unknown unknown unknown", "DP-2", None, None, None));
     }
+
+    #[test]
+    fn test_output_name_sorting() {
+        let mut names = vec![
+            make_output_name("DP-2", None, None, None),
+            make_output_name("DP-1", None, None, None),
+            make_output_name("DP-3", Some("B"), Some("A"), Some("A")),
+            make_output_name("DP-3", Some("A"), Some("B"), Some("A")),
+            make_output_name("DP-3", Some("A"), Some("A"), Some("B")),
+            make_output_name("DP-3", None, Some("A"), Some("A")),
+            make_output_name("DP-3", Some("A"), None, Some("A")),
+            make_output_name("DP-3", Some("A"), Some("A"), None),
+            make_output_name("DP-5", Some("A"), Some("A"), Some("A")),
+            make_output_name("DP-4", Some("A"), Some("A"), Some("A")),
+        ];
+        names.sort_by(|a, b| a.compare(b));
+        let names = names
+            .into_iter()
+            .map(|name| {
+                format!(
+                    "{} | {}",
+                    name.format_make_model_serial_or_connector(),
+                    name.connector,
+                )
+            })
+            .collect::<Vec<_>>();
+        snapshot!(
+            names,
+            r#"
+[
+    "Unknown A A | DP-3",
+    "A Unknown A | DP-3",
+    "A A Unknown | DP-3",
+    "A A A | DP-4",
+    "A A A | DP-5",
+    "A A B | DP-3",
+    "A B A | DP-3",
+    "B A A | DP-3",
+    "DP-1 | DP-1",
+    "DP-2 | DP-2",
+]
+"#
+        );
+    }
 }
diff --git a/src/ipc/client.rs b/src/ipc/client.rs
index 692b39b1..fcbf4b8c 100644
--- a/src/ipc/client.rs
+++ b/src/ipc/client.rs
@@ -1,4 +1,5 @@
 use anyhow::{anyhow, bail, Context};
+use niri_config::OutputName;
 use niri_ipc::socket::Socket;
 use niri_ipc::{
     Event, KeyboardLayouts, LogicalOutput, Mode, Output, OutputConfigChanged, Request, Response,
@@ -120,8 +121,11 @@ pub fn handle_msg(msg: Msg, json: bool) -> anyhow::Result<()> {
                 return Ok(());
             }
 
-            let mut outputs = outputs.into_iter().collect::<Vec<_>>();
-            outputs.sort_unstable_by(|a, b| a.0.cmp(&b.0));
+            let mut outputs = outputs
+                .into_values()
+                .map(|out| (OutputName::from_ipc_output(&out), out))
+                .collect::<Vec<_>>();
+            outputs.sort_unstable_by(|a, b| a.0.compare(&b.0));
 
             for (_name, output) in outputs.into_iter() {
                 print_output(output)?;
diff --git a/src/niri.rs b/src/niri.rs
index e7e23789..f7963742 100644
--- a/src/niri.rs
+++ b/src/niri.rs
@@ -1904,7 +1904,7 @@ impl Niri {
         #[derive(Debug)]
         struct Data {
             output: Output,
-            name: String,
+            name: OutputName,
             position: Option<Point<i32, Logical>>,
             config: Option<niri_config::Position>,
         }
@@ -1918,7 +1918,7 @@ impl Niri {
 
             outputs.push(Data {
                 output: output.clone(),
-                name: name.connector.clone(),
+                name: name.clone(),
                 position,
                 config,
             });
@@ -1932,15 +1932,17 @@ impl Niri {
         // Connectors can appear in udev in any order. If we sort by name then we get output
         // positioning that does not depend on the order they appeared.
         //
-        // All outputs must have different (connector) names.
-        outputs.sort_unstable_by(|a, b| Ord::cmp(&a.name, &b.name));
+        // This sorting first compares by make/model/serial so that it is stable regardless of the
+        // connector name. However, if make/model/serial is equal or unknown, then it does fall
+        // back to comparing the connector name, which should always be unique.
+        outputs.sort_unstable_by(|a, b| a.name.compare(&b.name));
 
         // Place all outputs with explicitly configured position first, then the unconfigured ones.
         outputs.sort_by_key(|d| d.config.is_none());
 
         trace!(
             "placing outputs in order: {:?}",
-            outputs.iter().map(|d| &d.name)
+            outputs.iter().map(|d| &d.name.connector)
         );
 
         for data in outputs.into_iter() {
@@ -1967,9 +1969,10 @@ impl Niri {
 
                     if let Some(overlap) = overlap {
                         warn!(
-                            "output {name} at x={} y={} sized {}x{} \
+                            "output {} at x={} y={} sized {}x{} \
                              overlaps an existing output at x={} y={} sized {}x{}, \
                              falling back to automatic placement",
+                            name.connector,
                             pos.x,
                             pos.y,
                             size.w,
@@ -2003,8 +2006,8 @@ impl Niri {
             // in global_space, we ensure that this branch always runs for it.
             if Some(new_position) != position {
                 debug!(
-                    "putting output {name} at x={} y={}",
-                    new_position.x, new_position.y
+                    "putting output {} at x={} y={}",
+                    name.connector, new_position.x, new_position.y
                 );
                 output.change_current_state(None, None, None, Some(new_position));
                 self.ipc_outputs_changed = true;