slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
17.56k stars 604 forks source link

layouting constraints not respected for items in `if` or `for` #407

Open tronical opened 3 years ago

tronical commented 3 years ago

Edit: The panic is fixed, but the remaining bug is that element in if or for don't propagate their layout constraint to the parent when not in a layout.

That is:

    Flickable {
            HorizontalLayout { Rectangle { height: 55px; } }
    }  

Works, but

    Flickable {
            if true: HorizontalLayout { Rectangle { height: 55px; } }
    }  

Doesn't work as the geometry of the child is not used

Workaround: use a layout: https://github.com/slint-ui/slint/issues/407#issuecomment-2258778337

The Slint code, tests, and docs reference this issue (git grep #407 to find them) Notably, the fix should be around there: https://github.com/slint-ui/slint/blob/32dd94973235ab4dcfb44ee1f4309f180a0d4ca9/internal/compiler/passes/default_geometry.rs#L186


Original issue:

The following minimal test-case causes a panic in the compiler in the generators/interpreter:

export Testcase := Window {
    preferred-width: 640px;
    preferred-height: 480px;
    Flickable {
            for blah in 1: HorizontalLayout {}
    }      
}

A call to unwrap() at the end of access_member panics:

  access_member(
            element,
            name,
            &component
                .parent_element
                .upgrade()
                .unwrap() // <-- this panics
                .borrow()
                .enclosing_component
                .upgrade()
                .unwrap(),
            quote!(#component_rust.parent.upgrade().unwrap().as_pin_ref()),
            is_special,
  )

Edit: Panic was fixed, but the layouting info of the inner repeated elements are not taken into account when they should See FIXME in tests/cases/layout/issue_407_for_layout_in_flickable.60

ogoffart commented 3 years ago

The problem is trying to estimate the size constraints of the Flickable based on the inside, but the inside is a repeated expression. Easy fix would be to ignore repeated elements when computing size constraints.

More complicated fix would be to take them into account.

tronical commented 3 years ago

I wonder what "taking them into account" would look like anyway. All constraints simply merged?

But even if the above would generate an error ("not currently supported") instead of panic it would be an improvement :)

ogoffart commented 3 years ago

I've fixed the panic, but we should still take into account the size. See tests/cases/layout/issue_407_for_layout_in_flickable.60

It is strange that

    Flickable {
            if (true): HorizontalLayout { Rectangle { height: 55px; } }
    }  

and

    Flickable {
            HorizontalLayout { Rectangle { height: 55px; } }
    }  

have different preferred-height or viewport-height.

flukejones commented 1 year ago

I'm hitting issues with this I think, but in my case it's with the fact I want a vertical list generated from a for loop to be vertically flickable. Every attempt unless I set the parent size ends up with compacting my items vertically and making the flickable horizontal only.

I really don't desire to set the parent size and would like the total size to be vertical view constrained but fit to width of largest item.

flukejones commented 1 year ago

Actually the precise issue I have is that I need the children of the for loop to set the parent rectangle width:

Like so (removed spacing, paddings etc):

component ParameterViewBox inherits VerticalLayout {
    in property <length> item-height: 64px;
    in property <[ParameterChoice]> choices;

    for parm [idx] in root.choices: Rectangle {
        Rectangle {
            height: item-height;
            HorizontalBox {
                if parm.show_icon: Image {
                    width: self.height;
                    source: parm.icon;
                }

                Text {
                    text: parm.label;
                    font-size: 18px;
                    vertical-alignment: center;
                }
            }
        }
    }
}
    Rectangle {
        width: 200px; // THIS WIDTH NEEDS TO BE SET OR THE WIDTH IS TOO NARROW
        Flickable { // REMOVING THE FLICKABLE AND WIDTH SETS THE WIDTH CORRECTLY BY AUTO
            ParameterViewBox {
                item-height: root.item-height;
                choices: root.choices;
            }
        }
    }

I've worked around it by specialcasing a set list length, but it's not ideal at all. Flickable works great in other places where I've got a set dimension, but not here where I really want it to have width automatically set.

ogoffart commented 1 year ago

In order to fix this bug, one would need to address the FIXME in https://github.com/slint-ui/slint/blob/b8c3bbef74a36669d51b874a08cb3b467712f62c/internal/compiler/passes/default_geometry.rs#L185-L187

Grepping for #407 in the codebase shows commented out tests that would be fixed if one fix this bug

DASPRiD commented 3 months ago

What would be the current workaround if the items generated by a for loop don't have a predefined height?

ogoffart commented 3 months ago

What would be the current workaround

Use an explicit layout.

Instead of

    if foobar: Xxx {}

Use

    HorizontalLayout { // or VerticalLayout, it should be the same.
       if foobar: Xxx {}
    }