Merge PR #44 from 'feat/order-of-nested-params'

Preserve the order of non-grouped nested parameters

Cargo.lock

@@ -2640,6 +2640,8 @@ source = "git+https://github.com/robbert-vdh/nih_plug_assets.git#a04e327923e120b
 name = "nih_plug_derive"
 version = "0.1.0"
 dependencies = [
+ "nih_plug",
+ "proc-macro2",
  "quote",
  "syn",
 ]

nih_plug_derive/Cargo.toml

@@ -11,3 +11,7 @@ proc-macro = true
 [dependencies]
 syn = { version = "1.0", features = ["extra-traits"] }
 quote = "1.0"
+proc-macro2 = "1.0"
+
+[dev-dependencies]
+nih_plug = { path = ".." }

nih_plug_derive/src/params.rs

@@ -31,9 +31,9 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
     // keys at compile time.
     // TODO: This duplication check doesn't work for nested fields since we don't know anything
     //       about the fields on the nested structs
-    let mut params: Vec<Param> = Vec::new();
+    let mut params: Vec<AnyParam> = Vec::new();
     let mut persistent_fields: Vec<PersistentField> = Vec::new();
-    let mut nested_params: Vec<NestedParams> = Vec::new();
+    let mut group_params: Vec<NestedParams> = Vec::new();
     for field in fields.named {
         let field_name = match &field.ident {
             Some(ident) => ident,
@@ -62,7 +62,10 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
                         // This is a vector since we want to preserve the order. If structs get
                         // large enough to the point where a linear search starts being expensive,
                         // then the plugin should probably start splitting up their parameters.
-                        if params.iter().any(|p| p.id == s) {
+                        if params.iter().any(|p| match p {
+                            AnyParam::Single(param) => param.id == s,
+                            _ => false,
+                        }) {
                             return syn::Error::new(
                                 field.span(),
                                 "Multiple parameters with the same ID found",
@@ -71,10 +74,10 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
                             .into();
                         }
 
-                        params.push(Param {
+                        params.push(AnyParam::Single(Param {
                             id: s,
                             field: field_name.clone(),
-                        });
+                        }));
 
                         processed_attribute = true;
                     }
@@ -204,7 +207,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
                             }
                         }
 
-                        nested_params.push(match (nested_array, nested_id_prefix) {
+                        let param = match (nested_array, nested_id_prefix) {
                             (true, None) => NestedParams::Array {
                                 field: field_name.clone(),
                                 group: nested_group,
@@ -226,7 +229,20 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
                                 .to_compile_error()
                                 .into()
                             }
-                        });
+                        };
+
+                        // Grouped parameters are handled differently than nested parameters:
+                        // DAWs that support grouped parameters usually display parameter groups below "regular" ones
+                        // in a tree view. To keep the order consistent with those, group parameters are collected
+                        // separately, so they can be inserted at the end of the parameter list.
+                        // Nested parameters without a group on the other hand are merely an implementation detail of
+                        // the plugin and thus should not behave different than regular parameters to the user. Because
+                        // of this, they are inserted alongside the "regular" ones.
+                        if param.is_group() {
+                            group_params.push(param);
+                        } else {
+                            params.push(AnyParam::Nested(param));
+                        }
 
                         processed_attribute = true;
                     }
@@ -247,97 +263,17 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
     // The next step is build the gathered information into tokens that can be spliced into a
     // `Params` implementation
     let param_map_tokens = {
-        // `param_map` adds the parameters from this struct, and then handles the nested tokens.
+        let param_mapping_self_tokens = params.into_iter().map(|p| p.to_token());
-        let param_mapping_self_tokens = params.into_iter().map(
+        let param_mapping_group_tokens = group_params.iter().map(|p| p.to_token());
-            |Param {field, id}| quote! { (String::from(#id), self.#field.as_ptr(), String::new()) },
-        );
-
-        // How nested parameters are handled depends on the `NestedParams` variant.
-        // These are pairs of `(parameter_id, param_ptr, param_group)`. The specific
-        // parameter types know how to convert themselves into the correct ParamPtr variant.
-        // Top-level parameters have no group, and we'll prefix the group name specified in
-        // the `#[nested(...)]` attribute to fields coming from nested groups
-        let param_mapping_nested_tokens = nested_params.iter().map(|nested| match nested {
-            // TODO: No idea how to splice this as an `Option<&str>`, so this involves some
-            //       copy-pasting
-            NestedParams::Inline { field, group: Some(group) } => quote! {
-                param_map.extend(self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| {
-                    if nested_group_name.is_empty() {
-                        (param_id, param_ptr, String::from(#group))
-                    } else {
-                        (param_id, param_ptr, format!("{}/{}", #group, nested_group_name))
-                    }
-                }));
-            },
-            NestedParams::Inline { field, group: None } => quote! {
-                param_map.extend(self.#field.param_map());
-            },
-            NestedParams::Prefixed {
-                field,
-                id_prefix,
-                group: Some(group),
-            } => quote! {
-                param_map.extend(self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| {
-                    let param_id = format!("{}_{}", #id_prefix, param_id);
-
-                    if nested_group_name.is_empty() {
-                        (param_id, param_ptr, String::from(#group))
-                    } else {
-                        (param_id, param_ptr, format!("{}/{}", #group, nested_group_name))
-                    }
-                }));
-            },
-            NestedParams::Prefixed {
-                field,
-                id_prefix,
-                group: None,
-            } => quote! {
-                param_map.extend(self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| {
-                    let param_id = format!("{}_{}", #id_prefix, param_id);
-
-                    (param_id, param_ptr, nested_group_name)
-                }));
-            },
-            // We'll start at index 1 for display purposes. Both the group and the parameter ID get
-            // a suffix matching the array index.
-            NestedParams::Array { field, group: Some(group) } => quote! {
-                param_map.extend(self.#field.iter().enumerate().flat_map(|(idx, params)| {
-                    let idx = idx + 1;
-
-                    params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| {
-                        let param_id = format!("{}_{}", param_id, idx);
-                        let group = format!("{} {}", #group, idx);
-
-                        // Note that this is different from the other variants
-                        if nested_group_name.is_empty() {
-                            (param_id, param_ptr, group)
-                        } else {
-                            (param_id, param_ptr, format!("{}/{}", group, nested_group_name))
-                        }
-                    })
-                }));
-            },
-            NestedParams::Array { field, group: None } => quote! {
-                param_map.extend(self.#field.iter().enumerate().flat_map(|(idx, params)| {
-                    let idx = idx + 1;
-
-                    params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| {
-                        let param_id = format!("{}_{}", param_id, idx);
-
-                        (param_id, param_ptr, nested_group_name)
-                    })
-                }));
-            },
-        });
 
         quote! {
             // This may not be in scope otherwise, used to call .as_ptr()
             use ::nih_plug::params::Param;
 
             #[allow(unused_mut)]
-            let mut param_map = vec![#(#param_mapping_self_tokens),*];
+            let mut param_map = Vec::new();
-
+            #(param_map.extend(#param_mapping_self_tokens); )*
-            #(#param_mapping_nested_tokens);*
+            #(param_map.extend(#param_mapping_group_tokens); )*
 
             param_map
         }
@@ -394,7 +330,7 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
                 .unzip();
 
         let (serialize_fields_nested_tokens, deserialize_fields_nested_tokens): (Vec<_>, Vec<_>) =
-            nested_params
+            group_params
                 .iter()
                 .map(|nested| match nested {
                     NestedParams::Inline { field, .. } | NestedParams::Prefixed { field, .. } => (
@@ -466,6 +402,23 @@ pub fn derive_params(input: TokenStream) -> TokenStream {
     .into()
 }
 
+#[derive(Debug)]
+enum AnyParam {
+    Single(Param),
+    Nested(NestedParams),
+}
+
+impl AnyParam {
+    fn to_token(&self) -> proc_macro2::TokenStream {
+        match self {
+            AnyParam::Single(Param { field, id }) => {
+                quote! { [(String::from(#id), self.#field.as_ptr(), String::new())] }
+            }
+            AnyParam::Nested(params) => params.to_token(),
+        }
+    }
+}
+
 /// A parameter that should be added to the parameter map.
 #[derive(Debug)]
 struct Param {
@@ -508,3 +461,106 @@ enum NestedParams {
         group: Option<syn::LitStr>,
     },
 }
+
+impl NestedParams {
+    fn is_group(&self) -> bool {
+        let group = match self {
+            NestedParams::Inline { field: _, group } => group,
+            NestedParams::Prefixed {
+                field: _,
+                id_prefix: _,
+                group,
+            } => group,
+            NestedParams::Array { field: _, group } => group,
+        };
+
+        group.is_some()
+    }
+
+    fn to_token(&self) -> proc_macro2::TokenStream {
+        // How nested parameters are handled depends on the `NestedParams` variant.
+        // These are pairs of `(parameter_id, param_ptr, param_group)`. The specific
+        // parameter types know how to convert themselves into the correct ParamPtr variant.
+        // Top-level parameters have no group, and we'll prefix the group name specified in
+        // the `#[nested(...)]` attribute to fields coming from nested groups
+
+        match self {
+            // TODO: No idea how to splice this as an `Option<&str>`, so this involves some
+            //       copy-pasting
+            NestedParams::Inline {
+                field,
+                group: Some(group),
+            } => quote! {
+                self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| {
+                    if nested_group_name.is_empty() {
+                        (param_id, param_ptr, String::from(#group))
+                    } else {
+                        (param_id, param_ptr, format!("{}/{}", #group, nested_group_name))
+                    }
+                })
+            },
+            NestedParams::Inline { field, group: None } => quote! {
+                self.#field.param_map();
+            },
+            NestedParams::Prefixed {
+                field,
+                id_prefix,
+                group: Some(group),
+            } => quote! {
+                self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| {
+                    let param_id = format!("{}_{}", #id_prefix, param_id);
+
+                    if nested_group_name.is_empty() {
+                        (param_id, param_ptr, String::from(#group))
+                    } else {
+                        (param_id, param_ptr, format!("{}/{}", #group, nested_group_name))
+                    }
+                })
+            },
+            NestedParams::Prefixed {
+                field,
+                id_prefix,
+                group: None,
+            } => quote! {
+                self.#field.param_map().into_iter().map(|(param_id, param_ptr, nested_group_name)| {
+                    let param_id = format!("{}_{}", #id_prefix, param_id);
+
+                    (param_id, param_ptr, nested_group_name)
+                })
+            },
+            // We'll start at index 1 for display purposes. Both the group and the parameter ID get
+            // a suffix matching the array index.
+            NestedParams::Array {
+                field,
+                group: Some(group),
+            } => quote! {
+                self.#field.iter().enumerate().flat_map(|(idx, params)| {
+                    let idx = idx + 1;
+
+                    params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| {
+                        let param_id = format!("{}_{}", param_id, idx);
+                        let group = format!("{} {}", #group, idx);
+
+                        // Note that this is different from the other variants
+                        if nested_group_name.is_empty() {
+                            (param_id, param_ptr, group)
+                        } else {
+                            (param_id, param_ptr, format!("{}/{}", group, nested_group_name))
+                        }
+                    })
+                })
+            },
+            NestedParams::Array { field, group: None } => quote! {
+                self.#field.iter().enumerate().flat_map(|(idx, params)| {
+                    let idx = idx + 1;
+
+                    params.param_map().into_iter().map(move |(param_id, param_ptr, nested_group_name)| {
+                        let param_id = format!("{}_{}", param_id, idx);
+
+                        (param_id, param_ptr, nested_group_name)
+                    })
+                })
+            },
+        }
+    }
+}

nih_plug_derive/tests/params.rs

@@ -0,0 +1,117 @@
+use nih_plug::prelude::*;
+
+#[derive(Params)]
+struct FlatParams {
+    #[id = "one"]
+    pub one: BoolParam,
+
+    #[id = "two"]
+    pub two: FloatParam,
+
+    #[id = "three"]
+    pub three: IntParam,
+}
+
+impl Default for FlatParams {
+    fn default() -> Self {
+        FlatParams {
+            one: BoolParam::new("one", true),
+            two: FloatParam::new("two", 0.0, FloatRange::Linear { min: 0.0, max: 1.0 }),
+            three: IntParam::new("three", 0, IntRange::Linear { min: 0, max: 100 }),
+        }
+    }
+}
+
+#[derive(Params)]
+struct GroupedParams {
+    #[id = "one"]
+    pub one: BoolParam,
+
+    #[nested(group = "Some Group", id_prefix = "group1")]
+    pub group1: FlatParams,
+
+    #[id = "three"]
+    pub three: IntParam,
+
+    #[nested(group = "Another Group", id_prefix = "group2")]
+    pub group2: FlatParams,
+}
+
+impl Default for GroupedParams {
+    fn default() -> Self {
+        GroupedParams {
+            one: BoolParam::new("one", true),
+            group1: FlatParams::default(),
+            three: IntParam::new("three", 0, IntRange::Linear { min: 0, max: 100 }),
+            group2: FlatParams::default(),
+        }
+    }
+}
+
+#[derive(Params)]
+struct NestedParams {
+    #[id = "one"]
+    pub one: BoolParam,
+
+    #[nested(id_prefix = "two")]
+    pub two: FlatParams,
+
+    #[id = "three"]
+    pub three: IntParam,
+}
+
+impl Default for NestedParams {
+    fn default() -> Self {
+        NestedParams {
+            one: BoolParam::new("one", true),
+            two: FlatParams::default(),
+            three: IntParam::new("three", 0, IntRange::Linear { min: 0, max: 100 }),
+        }
+    }
+}
+
+mod param_order {
+    use super::*;
+    #[test]
+    fn flat() {
+        let p = FlatParams::default();
+
+        // Parameters must have the same order as they are defined in
+        let param_ids: Vec<String> = p.param_map().into_iter().map(|(id, _, _)| id).collect();
+        assert_eq!(param_ids, ["one", "two", "three",]);
+    }
+
+    #[test]
+    fn grouped() {
+        let p = GroupedParams::default();
+
+        // Parameters must have the same order as they are defined in. Groups are put in the end though.
+        let param_ids: Vec<String> = p.param_map().into_iter().map(|(id, _, _)| id).collect();
+        assert_eq!(
+            param_ids,
+            [
+                "one",
+                "three",
+                "group1_one",
+                "group1_two",
+                "group1_three",
+                "group2_one",
+                "group2_two",
+                "group2_three",
+            ]
+        );
+    }
+
+    #[test]
+    fn nested() {
+        let p = NestedParams::default();
+
+        // Parameters must have the same order as they are defined in. The position of nested parameters which are not
+        // grouped explicitly is preserved.
+        let param_ids: Vec<String> = p.param_map().into_iter().map(|(id, _, _)| id).collect();
+        assert_eq!(
+            param_ids,
+            ["one", "two_one", "two_two", "two_three", "three",]
+        );
+    }
+}