Implement support for models in the Row of a GridLayout

[dfaure-kdab]

This is only part of the overall if/for support in grid: for now it's only supported to repeat cells in a given row, one cannot repeat entire Rows this way yet.

Some implementation notes:

• GridLayoutOrganizedData (4 u16s per cell) can now contain indirection indices in the case of repeaters, like LayoutCache (2 f32s per cell). The "generator" for that is separate code due to these two differences (amount and type), while LayoutCacheAccess can be used for both (it now receives the additional information of how many entries per item, as a parameter)
• BoxLayoutFunction and BoxLayoutCellData are used by the grid layouting code too; good for code sharing, but bad naming then. There are TODOs to rename those in a followup commit. This code reuse required porting from LayoutInfo to BoxLayoutCellData (which has only one field, the LayoutInfo, I suppose this is good for future extensions...).
• Compared to handling of repeaters for box layouts, there are additional complications due to the "new_row" bool, especially with empty repeaters (the first item at compile time isn't necessarily the first item at runtime). The generated code solves that with a "running" new_row bool, to detect if the repeater had instances or not.

[dfaure-kdab]

Known issues:

• specifying a row,col,rowspan or colspan in a repeated element leads to Repeated element should be empty because of the repeater_component.rs pass (elem.debug[0].layout is none at that point, when visiting all named references from repeater_components::adjust_references, because this happens before lower_layout...)
• no documentation/example yet. Hmm, actually the if/for (model) syntax isn't documented currently in HorizontalLayout and VerticalLayout, only in the tiles example?

[dfaure-kdab]

Somewhat real-world example (at least more than the auto test):
MainWindow.slint.txt

[ogoffart]

specifying a row,col,rowspan or colspan in a repeated element leads to Repeated element should be empty because of the repeater_component.rs pass

Looks like we should put these properties not within the repeated component, but within the base of the repeated component (in the layout pass), which is the thing actually being repeated.

[ogoffart]

This would probably help future debugging to include the entries_per_item

[dfaure-kdab]

Hmm. The current output is

   for person[] in (root.model/* as model */):(base)  := Cell {  /* Cell,;Rectangle,Cell::root */
      cell-text: [email protected];
      col: <borrowed>.layout-organized-data[12 + $index];
      height: <borrowed>.layout-cache-v[7 + $index];
      row: <borrowed>.layout-organized-data[14 + $index];
      width: <borrowed>.layout-cache-h[7 + $index];
      [...]
   }

which is actually incorrect, we don't just do 12 + $index. I never noticed, I didn't use this for debugging...
Here's a fixed version. More correct, but certainly less readable?

   for person[] in (root.model/* as model */):(base)  := Cell {  /* Cell,;Rectangle,Cell::root */
      cell-text: [email protected];
      col: <borrowed>.layout-organized-data[<borrowed>.layout-organized-data[12] + $repeater_index * 4];
      height: <borrowed>.layout-cache-v[<borrowed>.layout-cache-v[7] + $repeater_index * 2];
      row: <borrowed>.layout-organized-data[<borrowed>.layout-organized-data[14] + $repeater_index * 4];
      width: <borrowed>.layout-cache-h[<borrowed>.layout-cache-h[7] + $repeater_index * 2];

[ogoffart]

I guess you did consider trying to merge this with box_layout_function, and that wasn't worth it?

[dfaure-kdab]

You guess right ;)

• The input arguments differ
  ** box layout wants an orientation, grid doesn't
  ** for each repeater, boxlayout only needs the repeater number, grid also needs the new_row value (and I'm guessing we'll need more information for the next feature, repeating Rows), which is why the second type in the Either has changed
• the code being pushed for each instance is different obviously, including a variable outside the for loop, for grids -- I'm not sure how this could be passed as parameter given that stuff inside quote! refers to local variables outside of it, coming from the shared logic

Maybe instead of a single function with a ton of parameters, there could be shared helpers between the two? The last 13 lines are identical, for instance. I can look into factorizing that if you think it's a good idea (however it'll be a function with 7 variables, AFAICS: repeater_idx, cells_variable, sub_expression, fixed_count, repeated_count, push_code)

Or maybe this is a job for a macro? It can provide the beginning and end, and let the two functions implement the inner stuff differently. But the difference in type for repeater makes this tricky.

[ogoffart]

Why did you delete this file. We should still get some errors for it so i believe it should stay.

[dfaure-kdab]

No, there are no errors anymore in that file. if/for are supported when used directly under a GridLayout.

The remaining error is the one in if_in_grid_row.slint.

[ogoffart]

yes, that's a better name.

[dfaure-kdab]

OK, I will do the renamings in a followup MR, to make review easier.

[dfaure-kdab]

Shall I add the example in slint/examples?

I'm wondering if it wouldn't be more useful if the model came from some rust code rather than being hardcoded in the slint file? (but then C++ users wouldn't find it that great...)

[dfaure-kdab]

specifying a row,col,rowspan or colspan in a repeated element leads to Repeated element should be empty because of the repeater_component.rs pass

Looks like we should put these properties not within the repeated component, but within the base of the repeated component (in the layout pass), which is the thing actually being repeated.

There are multiple issues. I fixed the first few, but I could use your help with what I hope is the last one.

• The error above actually came from materialize_fake_properties() adding colspan to the repeater, I simply added a if elem.repeated.is_some() { return; } there.
• Then I realized what's missing is to actually use the 4 properties when generating code in grid_input_function. I added them to GridLayoutRepeatedElement. But: now I hit the unreachable in map_property_reference because it's a property on a LoweredElement::Repeated element. I think that's because it started being used (by lower_layout) after the code in repeater_components that adjusts references to properties in repeated elements. So my idea was to do a similar adjustment in lower_layout.rs:

--- i/internal/compiler/passes/lower_layout.rs
+++ w/internal/compiler/passes/lower_layout.rs
@@ -389,7 +389,25 @@ fn add_element(
         }
 
         let propref_or_default = |name: &'static str| -> Option<RowColExpr> {
-            crate::layout::binding_reference(item_element, name).map(RowColExpr::Named)
+            let nr = crate::layout::binding_reference(item_element, name).map(|nr| {
+                // similar to adjust_references in repeater_component.rs
+                let e = nr.element();
+                let mut nr = nr.clone();
+                if e.borrow().repeated.is_some()
+                    && let crate::langtype::ElementType::Component(c) = e.borrow().base_type.clone()
+                {
+                    nr = NamedReference::new(&c.root_element, nr.name().clone())
+                };
+                nr
+            });
+            nr.map(RowColExpr::Named)
         };

This "works" but everything falls apart when this named reference is used. I hit the unwrap() in map_property_reference because enclosing isn't set? I don't really understand what enclosing is, but I see that it's also not set when repeater_components does similar adjustments, it gets set later on?

[...]
adjust_references: Adjusting reference to property height in repeated element "optionalText" to refer to component root "optionalText_repeated". Enclosing=""
grid_layout_input_data: adjusting reference to property colspan in repeated element "optionalText" to refer to component root "optionalText_repeated". Enclosing=""
[...]

(the first one is the existing code, the second one is my new code)
which leads to

Mapping property reference root-1.empty-5-layout-organized-data in component "MainWindow". Enclosing="MainWindow"
Mapping property reference root-1.empty-5-height in component "MainWindow". Enclosing="MainWindow"
Mapping property reference root-1.empty-5-layout-organized-data in component "MainWindow". Enclosing="MainWindow"
Mapping property reference optionalText_repeated-6.colspan in component "MainWindow". Enclosing=""
=> unwrap on None

Should I keep digging in this direction, or am I missing a much simpler way to solve this? There's some complex interaction between the various compiler passes... any hint welcome.
(I think this can all be done in a followup PR, but that's up to you)

[dfaure-kdab]

I think I understand what the problem is. The ExpressionLoweringCtx is for the MainWindow component, while the property is in the repeated component, which is fully separate? AFAIU this means it's inaccessible from there? Like the problem that we can't access properties of popups, maybe?

[ogoffart]

right, the col, row and span property need to be located in the repeated component itself, not on the placeholder element in the parent.
And their expression is dependent of the repeated instance at runtime (each instance might have a different value).
So we need a way to access it like we have RepeatedItemTree::box_layout_data and we should either add the cell information there, or add a RepeatedItemTree::grid_layout_cell_data function to get this.