Move reordering of dialog buttons to the organize step

This repairs the feature of reordering buttons, which was
temporarily left out in the previous commit.

Author: dfaure-kdab

internal/compiler/generator/cpp.rs

@@ -3555,23 +3555,6 @@ fn compile_expression(expr: &llr::Expression, ctx: &EvaluationContext) -> String
             sub_expression,
             ctx,
         ),
-        Expression::ComputeDialogLayoutCells { cells_variable, roles, unsorted_cells } => {
-            let cells_variable = ident(cells_variable);
-            let mut cells = match &**unsorted_cells {
-                Expression::Array { values, .. } => {
-                    values.iter().map(|v| compile_expression(v, ctx))
-                }
-                _ => panic!("dialog layout unsorted cells not an array"),
-            };
-            format!("slint::cbindgen_private::GridLayoutCellData {cv}_array [] = {{ {c} }};\
-                    slint::cbindgen_private::slint_reorder_dialog_button_layout({cv}_array, {r});\
-                    slint::cbindgen_private::Slice<slint::cbindgen_private::GridLayoutCellData> {cv} = slint::private_api::make_slice(std::span({cv}_array))",
-                    r = compile_expression(roles, ctx),
-                    cv = cells_variable,
-                    c = cells.join(", "),
-                )
-
-        }
         Expression::MinMax { ty, op, lhs, rhs } => {
             let ident = match op {
                 MinMaxOp::Min => "min",

internal/compiler/generator/rust.rs

@@ -2711,21 +2711,6 @@ fn compile_expression(expr: &Expression, ctx: &EvaluationContext) -> TokenStream
             sub_expression,
             ctx,
         ),
-        Expression::ComputeDialogLayoutCells { cells_variable, roles, unsorted_cells } => {
-            let cells_variable = ident(cells_variable);
-            let roles = compile_expression(roles, ctx);
-            let cells = match &**unsorted_cells {
-                Expression::Array { values, .. } => {
-                    values.iter().map(|v| compile_expression(v, ctx))
-                }
-                _ => panic!("dialog layout unsorted cells not an array"),
-            };
-            quote! {
-                let mut #cells_variable = [#(#cells),*];
-                sp::reorder_dialog_button_layout(&mut #cells_variable, &#roles);
-                let #cells_variable = sp::Slice::from_slice(&#cells_variable);
-            }
-        }
         Expression::MinMax { ty, op, lhs, rhs } => {
             let lhs = compile_expression(lhs, ctx);
             let t = rust_primitive_type(ty);

internal/compiler/llr/expression.rs

@@ -184,15 +184,6 @@ pub enum Expression {
         orientation: Orientation,
         sub_expression: Box<Expression>,
     },
-
-    ComputeDialogLayoutCells {
-        /// The local variable where the slice of cells is going to be stored
-        cells_variable: String,
-        roles: Box<Expression>,
-        /// This is an Expression::Array
-        unsorted_cells: Box<Expression>,
-    },
-
     MinMax {
         ty: Type,
         op: MinMaxOp,
@@ -320,9 +311,6 @@ impl Expression {
             Self::EnumerationValue(e) => Type::Enumeration(e.enumeration.clone()),
             Self::LayoutCacheAccess { .. } => Type::LogicalLength,
             Self::BoxLayoutFunction { sub_expression, .. } => sub_expression.ty(ctx),
-            Self::ComputeDialogLayoutCells { .. } => {
-                Type::Array(super::lower_expression::grid_layout_cell_data_ty().into())
-            }
             Self::MinMax { ty, .. } => ty.clone(),
             Self::EmptyComponentFactory => Type::ComponentFactory,
             Self::TranslationReference { .. } => Type::String,
@@ -406,10 +394,6 @@ macro_rules! visit_impl {
                 $visitor(sub_expression);
                 elements.$iter().filter_map(|x| x.$as_ref().left()).for_each($visitor);
             }
-            Expression::ComputeDialogLayoutCells { roles, unsorted_cells, .. } => {
-                $visitor(roles);
-                $visitor(unsorted_cells);
-            }
             Expression::MinMax { ty: _, op: _, lhs, rhs } => {
                 $visitor(lhs);
                 $visitor(rhs);

internal/compiler/llr/lower_expression.rs

@@ -667,10 +667,34 @@ fn organize_grid_layout(
     ctx: &mut ExpressionLoweringCtx,
 ) -> llr_Expression {
     let cells = grid_layout_input_data(layout, ctx);
-    llr_Expression::ExtraBuiltinFunctionCall {
-        function: "organize_grid_layout".into(),
-        arguments: vec![cells],
-        return_ty: Type::Array(Type::Int32.into()),
+
+    if let Some(button_roles) = &layout.dialog_button_roles {
+        let e = crate::typeregister::BUILTIN.with(|e| e.enums.DialogButtonRole.clone());
+        let roles = button_roles
+            .iter()
+            .map(|r| {
+                llr_Expression::EnumerationValue(EnumerationValue {
+                    value: e.values.iter().position(|x| x == r).unwrap() as _,
+                    enumeration: e.clone(),
+                })
+            })
+            .collect();
+        let roles_expr = llr_Expression::Array {
+            element_ty: Type::Enumeration(e),
+            values: roles,
+            as_model: false,
+        };
+        llr_Expression::ExtraBuiltinFunctionCall {
+            function: "organize_dialog_button_layout".into(),
+            arguments: vec![cells, roles_expr],
+            return_ty: Type::Array(Type::Int32.into()),
+        }
+    } else {
+        llr_Expression::ExtraBuiltinFunctionCall {
+            function: "organize_grid_layout".into(),
+            arguments: vec![cells],
+            return_ty: Type::Array(Type::Int32.into()),
+        }
     }
 }
 

internal/compiler/llr/optim_passes/inline_expressions.rs

@@ -67,7 +67,6 @@ fn expression_cost(exp: &Expression, ctx: &EvaluationContext) -> isize {
         Expression::EnumerationValue(_) => 0,
         Expression::LayoutCacheAccess { .. } => PROPERTY_ACCESS_COST,
         Expression::BoxLayoutFunction { .. } => return isize::MAX,
-        Expression::ComputeDialogLayoutCells { .. } => return isize::MAX,
         Expression::MinMax { .. } => 10,
         Expression::EmptyComponentFactory => 10,
         Expression::TranslationReference { .. } => PROPERTY_ACCESS_COST + 2 * ALLOC_COST,

internal/compiler/llr/pretty_print.rs

@@ -362,9 +362,6 @@ impl<'a, T> Display for DisplayExpression<'a, T> {
                 write!(f, "{}[{} % {}]", DisplayPropertyRef(layout_cache_prop, ctx), index, e(ri))
             }
             Expression::BoxLayoutFunction { .. } => write!(f, "BoxLayoutFunction(TODO)",),
-            Expression::ComputeDialogLayoutCells { .. } => {
-                write!(f, "ComputeDialogLayoutCells(TODO)",)
-            }
             Expression::MinMax { ty: _, op, lhs, rhs } => match op {
                 MinMaxOp::Min => write!(f, "min({}, {})", e(lhs), e(rhs)),
                 MinMaxOp::Max => write!(f, "max({}, {})", e(lhs), e(rhs)),

internal/core/layout.rs

@@ -454,7 +454,6 @@ pub struct GridLayoutData {
 /// The horizontal or vertical data for a cell of a GridLayout
 #[repr(C)]
 #[derive(Default, Debug, Clone, PartialEq)]
-// Remember to apply any changes to grid_layout_cell_data()/grid_layout_cell_data_ty()
 pub struct GridLayoutCellData {
     /// col or row (u16::MAX means auto).
     pub col_or_row: u16,
@@ -491,16 +490,111 @@ impl GridLayoutOrganizedData {
     }
 }
 
+/// Given the cells of a layout of a Dialog, re-order the buttons according to the platform
+/// This function assume that the `roles` contains the roles of the button which are the first cells in `input_data`
+pub fn organize_dialog_button_layout(
+    input_data: Slice<GridLayoutInputData>,
+    dialog_button_roles: Slice<DialogButtonRole>,
+) -> GridLayoutOrganizedData {
+    let mut organized_data = GridLayoutOrganizedData::default();
+    organized_data.reserve(input_data.len() * 4);
+
+    #[cfg(feature = "std")]
+    fn is_kde() -> bool {
+        // assume some Unix, check if XDG_CURRENT_DESKTOP starts with K
+        std::env::var("XDG_CURRENT_DESKTOP")
+            .ok()
+            .and_then(|v| v.as_bytes().first().copied())
+            .is_some_and(|x| x.eq_ignore_ascii_case(&b'K'))
+    }
+    #[cfg(not(feature = "std"))]
+    let is_kde = || true;
+
+    let expected_order: &[DialogButtonRole] = match crate::detect_operating_system() {
+        crate::items::OperatingSystemType::Windows => {
+            &[
+                DialogButtonRole::Reset,
+                DialogButtonRole::None, // spacer
+                DialogButtonRole::Accept,
+                DialogButtonRole::Action,
+                DialogButtonRole::Reject,
+                DialogButtonRole::Apply,
+                DialogButtonRole::Help,
+            ]
+        }
+        crate::items::OperatingSystemType::Macos | crate::items::OperatingSystemType::Ios => {
+            &[
+                DialogButtonRole::Help,
+                DialogButtonRole::Reset,
+                DialogButtonRole::Apply,
+                DialogButtonRole::Action,
+                DialogButtonRole::None, // spacer
+                DialogButtonRole::Reject,
+                DialogButtonRole::Accept,
+            ]
+        }
+        _ if is_kde() => {
+            // KDE variant
+            &[
+                DialogButtonRole::Help,
+                DialogButtonRole::Reset,
+                DialogButtonRole::None, // spacer
+                DialogButtonRole::Action,
+                DialogButtonRole::Accept,
+                DialogButtonRole::Apply,
+                DialogButtonRole::Reject,
+            ]
+        }
+        _ => {
+            // GNOME variant and fallback for WASM build
+            &[
+                DialogButtonRole::Help,
+                DialogButtonRole::Reset,
+                DialogButtonRole::None, // spacer
+                DialogButtonRole::Action,
+                DialogButtonRole::Accept,
+                DialogButtonRole::Apply,
+                DialogButtonRole::Reject,
+            ]
+        }
+    };
+
+    // Reorder the actual buttons according to expected_order
+    let mut column_for_input: Vec<usize> = Vec::with_capacity(dialog_button_roles.len());
+    for role in expected_order.iter() {
+        if role == &DialogButtonRole::None {
+            column_for_input.push(usize::MAX); // empty column, ensure nothing will match
+            continue;
+        }
+        for (idx, r) in dialog_button_roles.as_slice().iter().enumerate() {
+            if *r == *role {
+                column_for_input.push(idx);
+            }
+        }
+    }
+
+    for (input_index, cell) in input_data.as_slice().iter().enumerate() {
+        let col = column_for_input.iter().position(|&x| x == input_index);
+        if let Some(col) = col {
+            organized_data.push_cell(col as u16, cell.colspan, cell.row, cell.rowspan);
+        } else {
+            // This is used for the main window (which is the only cell which isn't a button)
+            // Given lower_dialog_layout(), this will always be a single cell at 0,0 with a colspan of number_of_buttons
+            organized_data.push_cell(cell.col, cell.colspan, cell.row, cell.rowspan);
+        }
+    }
+    organized_data
+}
+
 // Implement "auto" behavior for row/col numbers (unless specified in the slint file).
-pub fn organize_grid_layout(core_slice: Slice<GridLayoutInputData>) -> GridLayoutOrganizedData {
-    let data = core_slice.as_slice();
+pub fn organize_grid_layout(input_data: Slice<GridLayoutInputData>) -> GridLayoutOrganizedData {
     let mut organized_data = GridLayoutOrganizedData::default();
-    organized_data.reserve(data.len() * 4);
+    organized_data.reserve(input_data.len() * 4);
     let mut row = 0;
     let mut col = 0;
     let mut first = true;
     let auto = u16::MAX;
-    for cell in data.iter() {
+    for cell in input_data.as_slice().iter() {
         if cell.new_row && !first {
             row += 1;
             col = 0;
@@ -753,80 +847,6 @@ pub fn box_layout_info_ortho(cells: Slice<BoxLayoutCellData>, padding: &Padding)
     fold
 }
 
-/// Given the cells of a layout of a Dialog, re-order the button according to the platform
-///
-/// This function assume that the `roles` contains the roles of the button which are the first `cells`
-/// It will simply change the column field of the cell
-pub fn reorder_dialog_button_layout(cells: &mut [GridLayoutCellData], roles: &[DialogButtonRole]) {
-    fn add_buttons(
-        cells: &mut [GridLayoutCellData],
-        roles: &[DialogButtonRole],
-        idx: &mut u16,
-        role: DialogButtonRole,
-    ) {
-        for (cell, r) in cells.iter_mut().zip(roles.iter()) {
-            if *r == role {
-                cell.col_or_row = *idx;
-                *idx += 1;
-            }
-        }
-    }
-
-    #[cfg(feature = "std")]
-    fn is_kde() -> bool {
-        // assume some Unix, check if XDG_CURRENT_DESKTOP starts with K
-        std::env::var("XDG_CURRENT_DESKTOP")
-            .ok()
-            .and_then(|v| v.as_bytes().first().copied())
-            .is_some_and(|x| x.eq_ignore_ascii_case(&b'K'))
-    }
-    #[cfg(not(feature = "std"))]
-    let is_kde = || true;
-
-    let mut idx = 0;
-
-    match crate::detect_operating_system() {
-        crate::items::OperatingSystemType::Windows => {
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reset);
-            idx += 1;
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Accept);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Action);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reject);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Apply);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Help);
-        }
-        crate::items::OperatingSystemType::Macos | crate::items::OperatingSystemType::Ios => {
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Help);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reset);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Apply);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Action);
-            idx += 1;
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reject);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Accept);
-        }
-        _ if is_kde() => {
-            // KDE variant
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Help);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reset);
-            idx += 1;
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Action);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Accept);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Apply);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reject);
-        }
-        _ => {
-            // GNOME variant and fallback for everything else
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Help);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reset);
-            idx += 1;
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Action);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Apply);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Reject);
-            add_buttons(cells, roles, &mut idx, DialogButtonRole::Accept);
-        }
-    }
-}
-
 #[cfg(feature = "ffi")]
 pub(crate) mod ffi {
     #![allow(unsafe_code)]
@@ -890,16 +910,4 @@ pub(crate) mod ffi {
     ) -> LayoutInfo {
         super::box_layout_info_ortho(cells, padding)
     }
-
-    /// Calls [`reorder_dialog_button_layout`].
-    ///
-    /// Safety: `cells` must be a pointer to a mutable array of cell data, the array must have at
-    /// least `roles.len()` elements.
-    #[unsafe(no_mangle)]
-    pub unsafe extern "C" fn slint_reorder_dialog_button_layout(
-        cells: *mut GridLayoutCellData,
-        roles: Slice<DialogButtonRole>,
-    ) {
-        reorder_dialog_button_layout(core::slice::from_raw_parts_mut(cells, roles.len()), &roles);
-    }
 }

internal/interpreter/eval_layout.rs

@@ -93,7 +93,20 @@ pub(crate) fn organize_grid_layout(
     local_context: &mut EvalLocalContext,
 ) -> Value {
     let cells = grid_layout_input_data(layout, local_context);
-    core_layout::organize_grid_layout(Slice::from_slice(cells.as_slice())).into()
+
+    if let Some(buttons_roles) = &layout.dialog_button_roles {
+        let roles = buttons_roles
+            .iter()
+            .map(|r| DialogButtonRole::from_str(r).unwrap())
+            .collect::<Vec<_>>();
+        core_layout::organize_dialog_button_layout(
+            Slice::from_slice(cells.as_slice()),
+            Slice::from_slice(roles.as_slice()),
+        )
+        .into()
+    } else {
+        core_layout::organize_grid_layout(Slice::from_slice(cells.as_slice())).into()
+    }
 }
 
 pub(crate) fn solve_grid_layout(