Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tronical opened this issue Aug 12, 2021 · 6 comments
Open

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

tronical opened this issue Aug 12, 2021 · 6 comments
Labels
a:compiler Slint compiler internal (not the codegen, not the parser) a:layouts Related to the layouting and positioning of the elements (mO,bT) bug Something isn't working

Comments

@tronical
Copy link
Member

tronical commented Aug 12, 2021

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.
See comment #407 (comment)


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

@tronical tronical added bug Something isn't working a:compiler Slint compiler internal (not the codegen, not the parser) labels Aug 12, 2021
@ogoffart ogoffart self-assigned this Aug 12, 2021
@ogoffart
Copy link
Member

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
Copy link
Member Author

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 added a commit that referenced this issue Aug 12, 2021
…avoid compiler panic

This fixes a panic but ideally, we should merge the layout info of each children

cc #407
@ogoffart ogoffart changed the title repeater with layout inside Flickable panics compiler layouting info not used if repeater with layout inside Flickable Aug 12, 2021
@ogoffart ogoffart removed their assignment Aug 12, 2021
@ogoffart
Copy link
Member

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.

@ogoffart ogoffart added the a:layouts Related to the layouting and positioning of the elements (mO,bT) label Aug 12, 2021
@flukejones
Copy link
Contributor

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
Copy link
Contributor

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 ogoffart changed the title layouting info not used if repeater with layout inside Flickable layouting constraints not respected for items in if or for Oct 21, 2023
@ogoffart
Copy link
Member

ogoffart commented Oct 21, 2023

In order to fix this bug, one would need to address the FIXME in

if c.borrow().repeated.is_some() {
// FIXME: we should ideally add runtime code to merge layout info of all elements that are repeated (same as #407)
return None;

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:compiler Slint compiler internal (not the codegen, not the parser) a:layouts Related to the layouting and positioning of the elements (mO,bT) bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants