Lay out grids at runtime rather than at compile time

[dfaure-kdab]

This opens the door for using computed values in "row"/"col" properties, and later on for dynamic grid layouts (for/if support)

PROBLEM: it's missing the "row" and "col" properties from my previous commit... This is WIP, so we can discuss how to bring those properties back.

[ogoffart]

I think this is going in the right direction.

We'll have to have good documentation for the exact behavior and tests.

[dfaure-kdab]

I think this is going in the right direction.

Phew ;-)

We'll have to have good documentation for the exact behavior and tests.

Documentation, I agree (will do).
Tests, I want to reassure you, we do already. While working on this, I have had a LOT of feedback from the existing layout tests.

[dfaure-kdab]

Now including row and col properties, implemented using the layout cache.
It works, but it has an issue: if e.g. a widget's text property is set to "row" or "col", it creates an infinite recursion on the layout-cache binding, incorrectly. It makes sense to x,y,width,height to depend on the widget text, but not for row and col. So I should create a different kind of cache for that?

The other issue is the one mentioned in the commit log: how to react to changes to row/col and trigger layouting again?
I'll dig into it, but hints welcome.

[ogoffart]

It works, but it has an issue: if e.g. a widget's text property is set to "row" or "col", it creates an infinite recursion on the layout-cache binding, incorrectly.

I see, because the row and col are both input and output of the layout computation?

It makes sense to x,y,width,height to depend on the widget text, but not for row and col. So I should create a different kind of cache for that?

Perhaps it would help to have another property to cache these values instead of doing it all in solve_layout, yes.
(Then we would also not need this cache if the row and col are entirely known at compile time)

But this can be a known issue and doesn't need to be solve, as long as it is detected at compile time and the logic in the binding_analysis that detect the loop is catching that properly.

[dfaure-kdab]

Perhaps it would help to have another property to cache these values instead of doing it all in solve_layout, yes

Hmm, solve_grid_layout returns the x,y,width,height cache (for use in the property binding for layoutcache) and takes the spec from the user as input, so row/col determination has to happen in there too (it's obviously needed to determine x,y,width,height)... but the result of that row/col determination needs to be stored into another property... I wonder how.

The current solution is already to duplicate the input data between layoutcache (the call to solve_grid_layout) and layoutinfo (the call to grid_layout_info). Somehow we should be able to extract that into a function that returns a Vec, that I could also use from the third property that is computed from that same data?
I means doing some processing 3 times but I guess this is unavoidable, e.g. to compute the binding dependencies correctly... or we could use the output of the first step as input to the other two (layoutcache and layoutinfo), to optimize this.

(Then we would also not need this cache if the row and col are entirely known at compile time)

That is an excellent idea, it would address your concern about memory usage.

But this can be a known issue and doesn't need to be solve, as long as it is detected at compile time and the logic in the binding_analysis that detect the loop is catching that properly.

Yes, it's caught properly, but it's still a bug, and one that requires a big refactoring of the code in this MR. But indeed if we want the feature already (or if you simply want to avoid re-reviewing the same thing many times), you can merge this and I can build on top.

[dfaure-kdab]

The other issue is the one mentioned in the commit log: how to react to changes to row/col and trigger layouting again? I'll dig into it, but hints welcome.

I was wrong, this works well in an interactive testcase. It just doesn't work in a unittest like
grid_variable_row_col.slint.txt
(extension hacked so github allows it). get_final_grid_ok() fails. But in an interactive testcase with a button to call change_grid(), it works fine. I guess one needs to wait for the relayout to happens before testing the new properties, any idea how to do that? EDIT - PEBCAK, sorry for the noise.

[ogoffart]

   out property <bool> test: {
       initial_grid_ok
       // how to call change_grid??
   }

Maybe something like

init => {
   if !initial_grid_ok { root.error = "!initial_grid_ok"; }
   change_grid()
}
out property <string> error;
out property <bool> test: error == "" && final_grid_ok;

[dfaure-kdab]

Maybe something like [...]

Thanks, this worked, now the new test passes (both when compiled and when interpreted)

[dfaure-kdab]

we could use the output of the first step as input to the other two (layoutcache and layoutinfo), to optimize this.

I quite like this idea, actually. I am currently implementing this solution, on top of this MR, to make this cleaner and solve the binding loop.

pub struct GridLayoutInputData {
    /// whether this cell is the first one in a Row element
    pub new_row: bool,
    /// col and row number (u16::MAX means auto).
    pub col: u16,
    pub row: u16,
    /// colspan and rowspan
    pub colspan: u16,
    pub rowspan: u16,
}

/// The organized layout data for a GridLayout, after row/col determination:
/// For each cell, stores col, colspan, row, rowspan
pub type GridLayoutOrganizedData = SharedVector<Coord>;
}

A new property "layout-organized-data" contains the result of the call that uses the input data to determine row/col values and store that into the organized data (which is a SharedVector in order to reuse the LayoutCacheAccess mechanism, just on a different cache property), and this (plus layout constraints...) will be the source of information for both solve_grid_layout and grid_layout_info. Currently only implemented for the interpreter (works 100% - grid_layout_data() reads from the organized data and adds the constraints, it all works including a text property that uses row and col), I will now work on the compiler side of things.

[dfaure-kdab]

A new property "layout-organized-data" contains the result of the call that uses the input data to determine row/col values and store that into the organized data [...]

This is implemented now. I quite like the result. This cache of row/col/rowspan/colspan values is only recomputed when needed (fixing the binding loop mentioned previously), and is used for everything: for the user-accessible properties, and for the two layouting algorithms (solve_grid_layout and grid_layout_info). And the logic for reordering dialog buttons is no longer a hack applied into the data structures used for layouting, it's now the implementation of "organize" for the case of dialogs (which don't need the default "organize" step, the one which deals with auto numbering and with runtime expressions for row/col).

The whole thing can be quite a bit confusing, so I made this diagram:
http://www.davidfaure.fr/slint/gridlayout_structs.png

[ogoffart]

Thanks.
Just a note about the fact that we added expression tothe grid layout structure and we don't visit them.

I think this might cause the compiler to not see them when trying to see if everything is used or detecting binding loops.

Another test could be a test with a binding loop in
col: self.x > 10px ? 3 : 4; or something like that (it should correctly be detected as a binding loop.)

[ogoffart]

could you also have an extra test that does foo.col = xyz in a callback and that mix that with auto-numbering?

[dfaure-kdab]

Hmm, so the error about mixing auto with runtime-expr needs to be a runtime error too, not just a compile time error? Or do you think we can detect such an issue at compile time already? For compile-time I'd need to iterate over all cells any time the compiler is seeing a statement like foo.col = xyz (after detecting this special case of assigning to a row or col property). Do we have anything like this already, for inspiration?

[ogoffart]

I'd say you can use the PropertyAnalysis::is_set for that property to know that this property is set by some binding. and therefore disallow mixing

[dfaure-kdab]

Thanks, I didn't know about PropertyAnalysis. Unfortunately it runs after lower_layout so it's not available there...

One thing about this testcase:

• if foo doesn't define col, then nothing happens when setting foo.col because the generated code treated the value of col as a constant, so it generated the_struct.r#col = 65535f64 as _;, not reading a property.
• if foo has col: 1; or col: <runtime_expr> then the generated code reads from the foo_col property, so changing it works (but we only want that if all row and col properties in the layout are set, otherwise we're back to the what-happens-to-neighbors issues).

OK, so actually, all we need is to store into the GridLayout in which mode it is: only-auto-and-literal or explicit-row-col-everywhere (in other words: uses auto yes/no). Then during property analysis we can warn if changing a row or col of a grid layout that uses auto anywhere.

[dfaure-kdab]

But PropertyAnalysis::is_set won't allow to point the error to the location where it's set...

Apart from that, it works.

[dfaure-kdab]

What's the reason for the CI failure? I don't see any error message.

[tronical]

The compiler crashing I think:(. Seems to trigger often with rust 1.88. cleared the cache, re-run, now it's green :)