Skip to content

Commit

Permalink
Fix regression when overriding the with of a Component that specifies…
Browse files Browse the repository at this point in the history
… limits

We were comparing the priorities of different component, which does
not make sense

This is a regression found with the crater run
  • Loading branch information
ogoffart committed Nov 24, 2021
1 parent 2c4cebd commit 84e394a
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 26 deletions.
62 changes: 36 additions & 26 deletions sixtyfps_compiler/layout.rs
Expand Up @@ -12,10 +12,10 @@ LICENSE END */
use crate::diagnostics::BuildDiagnostics;
use crate::expression_tree::*;
use crate::langtype::{PropertyLookupResult, Type};
use crate::object_tree::ElementRc;
use crate::object_tree::{Component, ElementRc};

use std::cell::RefCell;
use std::rc::Rc;
use std::rc::{Rc, Weak};

#[derive(Clone, Debug, Copy, Eq, PartialEq)]
pub enum Orientation {
Expand Down Expand Up @@ -156,36 +156,46 @@ impl LayoutConstraints {
fixed_height: false,
};
let mut apply_size_constraint =
|prop, binding: &BindingExpression, depth, op: &mut Option<NamedReference>| {
|prop,
binding: &BindingExpression,
enclosing1: &Weak<Component>,
depth,
op: &mut Option<NamedReference>| {
if let Some(other_prop) = op {
find_binding(&other_prop.element(), other_prop.name(), |old, d2| {
let old_priority = old.priority.saturating_add(d2);
if old_priority <= binding.priority.saturating_add(depth) {
diag.push_error(
format!(
"Cannot specify both '{}' and '{}'",
prop,
other_prop.name()
),
binding,
)
}
});
find_binding(
&other_prop.element(),
other_prop.name(),
|old, enclosing2, d2| {
if Weak::ptr_eq(enclosing1, enclosing2)
&& old.priority.saturating_add(d2)
<= binding.priority.saturating_add(depth)
{
diag.push_error(
format!(
"Cannot specify both '{}' and '{}'",
prop,
other_prop.name()
),
binding,
)
}
},
);
}
*op = Some(NamedReference::new(element, prop))
};
find_binding(element, "height", |s, depth| {
find_binding(element, "height", |s, enclosing, depth| {
constraints.fixed_height = true;
apply_size_constraint("height", s, depth, &mut constraints.min_height);
apply_size_constraint("height", s, depth, &mut constraints.max_height);
apply_size_constraint("height", s, enclosing, depth, &mut constraints.min_height);
apply_size_constraint("height", s, enclosing, depth, &mut constraints.max_height);
});
find_binding(element, "width", |s, depth| {
find_binding(element, "width", |s, enclosing, depth| {
if s.expression.ty() == Type::Percent {
apply_size_constraint("width", s, depth, &mut constraints.min_width);
apply_size_constraint("width", s, enclosing, depth, &mut constraints.min_width);
} else {
constraints.fixed_width = true;
apply_size_constraint("width", s, depth, &mut constraints.min_width);
apply_size_constraint("width", s, depth, &mut constraints.max_width);
apply_size_constraint("width", s, enclosing, depth, &mut constraints.min_width);
apply_size_constraint("width", s, enclosing, depth, &mut constraints.max_width);
}
});

Expand Down Expand Up @@ -364,13 +374,13 @@ impl LayoutGeometry {
fn find_binding<R>(
element: &ElementRc,
name: &str,
f: impl FnOnce(&BindingExpression, i32) -> R,
f: impl FnOnce(&BindingExpression, &Weak<Component>, i32) -> R,
) -> Option<R> {
let mut element = element.clone();
let mut depth = 0;
loop {
if let Some(b) = element.borrow().bindings.get(name) {
return Some(f(&b.borrow(), depth));
return Some(f(&b.borrow(), &element.borrow().enclosing_component, depth));
}
let e = match &element.borrow().base_type {
Type::Component(base) => base.root_element.clone(),
Expand All @@ -383,7 +393,7 @@ fn find_binding<R>(

/// Return a named reference to a property if a binding is set on that property
fn binding_reference(element: &ElementRc, name: &str) -> Option<NamedReference> {
find_binding(element, name, |_, _| NamedReference::new(&element, name))
find_binding(element, name, |_, _, _| NamedReference::new(&element, name))
}

fn init_fake_property(
Expand Down
45 changes: 45 additions & 0 deletions tests/cases/layout/override_from_parent.60
@@ -0,0 +1,45 @@
/* LICENSE BEGIN
This file is part of the SixtyFPS Project -- https://sixtyfps.io
Copyright (c) 2021 Olivier Goffart <olivier.goffart@sixtyfps.io>
Copyright (c) 2021 Simon Hausmann <simon.hausmann@sixtyfps.io>

SPDX-License-Identifier: GPL-3.0-only
This file is also available under commercial licensing terms.
Please contact info@sixtyfps.io for more information.
LICENSE END */

ComboBox := Rectangle {
min-width: 60px;
}

SubComp1 := Rectangle {
HorizontalLayout {
ComboBox {
width: 200px;
}
}
}

SubComp2 := HorizontalLayout {
ComboBox {
width: 200px;
}
}

SubComp3 := HorizontalLayout {
max-width: 500px;
Rectangle { }
}


TestCase := Rectangle {
width: 300phx;
height: 300phx;

sc1 := SubComp1 {}
sc2 := SubComp2 {}
// FIXME: the HorizontalLayout is required here because the sc3.max-width takes the existing binding instead of being re-materialized
sc3 := HorizontalLayout { SubComp3 { width: 200px; } }
property<bool> test: sc1.min-width == 200px && sc2.min-width == 200px && sc3.max-width == 200px;
}

0 comments on commit 84e394a

Please sign in to comment.