From e9f375e8f67bbf656dbba914c2368a109f9a158a Mon Sep 17 00:00:00 2001
From: Michal Sojka <michal.sojka@cvut.cz>
Date: Sun, 28 Apr 2024 19:30:44 +0200
Subject: [PATCH] Make the order of parameters deterministic

GUI tools for working with parameters (at least Foxglove Studio, rqt
and rig_reconfigure) show the parameters in the same order as returned
by the ListParameters service. r2r stores the parameters in a HashMap,
which iterates keys and values in arbitrary order. The result is that
the GUI tools show the parameters in different order after every node
invocation, which can be quite annoying.

To make the order deterministic, we change the parameter storage from
HashMap to IndexMap, which iterates the map in insertion order.
According to the indexmap documentation, IndexMap is a drop-in
replacement of HashMap so this change should not require code changes
in applications using r2r. At least r2r examples and my projects
needed no changes.
---
 r2r/Cargo.toml        | 1 +
 r2r/src/lib.rs        | 3 ++-
 r2r/src/nodes.rs      | 9 +++++----
 r2r/src/parameters.rs | 7 ++++---
 r2r_macros/src/lib.rs | 2 +-
 5 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/r2r/Cargo.toml b/r2r/Cargo.toml
index 4fe1b8f44..ba304c241 100644
--- a/r2r/Cargo.toml
+++ b/r2r/Cargo.toml
@@ -27,6 +27,7 @@ uuid = { version = "1.2.2", features = ["serde", "v4"] }
 futures = "0.3.25"
 log = "0.4.18"
 phf = "0.11.1"
+indexmap = "2.2.6"
 
 [dev-dependencies]
 serde_json = "1.0.89"
diff --git a/r2r/src/lib.rs b/r2r/src/lib.rs
index fcf358d91..e2f1854b2 100644
--- a/r2r/src/lib.rs
+++ b/r2r/src/lib.rs
@@ -71,8 +71,9 @@
 //! }
 //! ```
 
-// otherwise crates using r2r needs to specify the same version of uuid as
+// otherwise crates using r2r needs to specify the same version of indexmap and uuid as
 // this crate depend on, which seem like bad user experience.
+pub extern crate indexmap;
 pub extern crate uuid;
 
 mod error;
diff --git a/r2r/src/nodes.rs b/r2r/src/nodes.rs
index 75d04cbe6..e0f77097f 100644
--- a/r2r/src/nodes.rs
+++ b/r2r/src/nodes.rs
@@ -3,6 +3,7 @@ use futures::{
     future::{self, join_all, FutureExt, TryFutureExt},
     stream::{Stream, StreamExt},
 };
+use indexmap::IndexMap;
 use std::{
     collections::HashMap,
     ffi::{CStr, CString},
@@ -43,7 +44,7 @@ use crate::{
 pub struct Node {
     context: Context,
     /// ROS parameters.
-    pub params: Arc<Mutex<HashMap<String, Parameter>>>,
+    pub params: Arc<Mutex<IndexMap<String, Parameter>>>,
     pub(crate) node_handle: Box<rcl_node_t>,
     // the node owns the subscribers
     pub(crate) subscribers: Vec<Box<dyn Subscriber_>>,
@@ -198,7 +199,7 @@ impl Node {
             };
 
             let mut node = Node {
-                params: Arc::new(Mutex::new(HashMap::new())),
+                params: Arc::new(Mutex::new(IndexMap::new())),
                 context: ctx,
                 node_handle,
                 subscribers: Vec::new(),
@@ -460,7 +461,7 @@ impl Node {
 
     fn handle_list_parameters(
         req: ServiceRequest<rcl_interfaces::srv::ListParameters::Service>,
-        params: &Arc<Mutex<HashMap<String, Parameter>>>,
+        params: &Arc<Mutex<IndexMap<String, Parameter>>>,
     ) -> future::Ready<()> {
         use rcl_interfaces::srv::ListParameters;
 
@@ -503,7 +504,7 @@ impl Node {
 
     fn handle_desc_parameters(
         req: ServiceRequest<rcl_interfaces::srv::DescribeParameters::Service>,
-        params: &Arc<Mutex<HashMap<String, Parameter>>>,
+        params: &Arc<Mutex<IndexMap<String, Parameter>>>,
     ) -> future::Ready<()> {
         use rcl_interfaces::{msg::ParameterDescriptor, srv::DescribeParameters};
         let mut descriptors = Vec::<ParameterDescriptor>::new();
diff --git a/r2r/src/parameters.rs b/r2r/src/parameters.rs
index d454e2825..b3453ec5f 100644
--- a/r2r/src/parameters.rs
+++ b/r2r/src/parameters.rs
@@ -1,7 +1,8 @@
 use crate::{Error, Result};
-use std::{collections::HashMap, ffi::CStr};
+use std::ffi::CStr;
 
 use crate::msg_types::generated_msgs::rcl_interfaces;
+use indexmap::IndexMap;
 use r2r_rcl::*;
 
 /// ROS parameter value.
@@ -190,7 +191,7 @@ impl Parameter {
 /// `parameters_derive.rs` example.
 pub trait RosParams {
     fn register_parameters(
-        &mut self, prefix: &str, param: Option<Parameter>, params: &mut HashMap<String, Parameter>,
+        &mut self, prefix: &str, param: Option<Parameter>, params: &mut IndexMap<String, Parameter>,
     ) -> Result<()>;
     fn get_parameter(&mut self, param_name: &str) -> Result<ParameterValue>;
     fn set_parameter(&mut self, param_name: &str, param_val: &ParameterValue) -> Result<()>;
@@ -202,7 +203,7 @@ macro_rules! impl_ros_params {
         impl RosParams for $type {
             fn register_parameters(
                 &mut self, prefix: &str, param: Option<Parameter>,
-                params: &mut HashMap<String, Parameter>,
+                params: &mut IndexMap<String, Parameter>,
             ) -> Result<()> {
                 if let Some(cli_param) = params.get(prefix) {
                     // Apply parameter value if set from command line or launch file
diff --git a/r2r_macros/src/lib.rs b/r2r_macros/src/lib.rs
index 6c14a44c9..544a6d3f0 100644
--- a/r2r_macros/src/lib.rs
+++ b/r2r_macros/src/lib.rs
@@ -26,7 +26,7 @@ pub fn derive_r2r_params(input: proc_macro::TokenStream) -> proc_macro::TokenStr
                 &mut self,
                 prefix: &str,
                 desc: ::std::option::Option<::r2r::Parameter>,
-                params: &mut ::std::collections::hash_map::HashMap<String, ::r2r::Parameter>,
+                params: &mut ::r2r::indexmap::IndexMap<String, ::r2r::Parameter>,
             ) -> ::r2r::Result<()> {
                 let prefix = if prefix.is_empty() {
                     String::from("")