Fix re-instentiating if elements when the condition is dirty

Fixes #3953

api/cpp/include/slint_models.h

@@ -816,8 +816,6 @@ namespace private_api {
 template<typename C, typename ModelData>
 class Repeater
 {
-    private_api::Property<std::shared_ptr<Model<ModelData>>> model;
-
     struct RepeaterInner : ModelChangeListener
     {
         enum class State { Clean, Dirty };
@@ -866,10 +864,16 @@ class Repeater
         }
     };
 
-public:
+    private_api::Property<std::shared_ptr<Model<ModelData>>> model;
-    // FIXME: should be private, but layouting code uses it.
     mutable std::shared_ptr<RepeaterInner> inner;
 
+    vtable::VRef<private_api::ItemTreeVTable> item_at(int i) const
+    {
+        const auto &x = inner->data.at(i);
+        return { &C::static_vtable, const_cast<C *>(&(**x.ptr)) };
+    }
+
+public:
     template<typename F>
     void set_model_binding(F &&binding) const
     {
@@ -947,12 +951,6 @@ public:
         return std::numeric_limits<uint64_t>::max();
     }
 
-    vtable::VRef<private_api::ItemTreeVTable> item_at(int i) const
-    {
-        const auto &x = inner->data.at(i);
-        return { &C::static_vtable, const_cast<C *>(&(**x.ptr)) };
-    }
-
     vtable::VWeak<private_api::ItemTreeVTable> instance_at(std::size_t i) const
     {
         if (i >= inner->data.size()) {
@@ -967,6 +965,8 @@ public:
         return private_api::IndexRange { 0, inner->data.size() };
     }
 
+    std::size_t len() const { return inner ? inner->data.size() : 0; }
+
     float compute_layout_listview(const private_api::Property<float> *viewport_width,
                                   float listview_width, float viewport_y) const
     {
@@ -992,6 +992,72 @@ public:
             }
         }
     }
+
+    void for_each(auto &&f) const
+    {
+        if (inner) {
+            for (auto &&x : inner->data) {
+                f(*x.ptr);
+            }
+        }
+    }
+};
+
+template<typename C>
+class Conditional
+{
+    private_api::Property<bool> model;
+    mutable std::optional<ComponentHandle<C>> instance;
+
+public:
+    template<typename F>
+    void set_model_binding(F &&binding) const
+    {
+        model.set_binding(std::forward<F>(binding));
+    }
+
+    template<typename Parent>
+    void ensure_updated(const Parent *parent) const
+    {
+        if (!model.get()) {
+            instance = std::nullopt;
+        } else if (!instance) {
+            instance = C::create(parent);
+            (*instance)->init();
+        }
+    }
+
+    uint64_t visit(TraversalOrder order, private_api::ItemVisitorRefMut visitor) const
+    {
+        if (instance) {
+            vtable::VRef<private_api::ItemTreeVTable> ref { &C::static_vtable,
+                                                            const_cast<C *>(&(**instance)) };
+            if (ref.vtable->visit_children_item(ref, -1, order, visitor)
+                != std::numeric_limits<uint64_t>::max()) {
+                return 0;
+            }
+        }
+        return std::numeric_limits<uint64_t>::max();
+    }
+
+    vtable::VWeak<private_api::ItemTreeVTable> instance_at(std::size_t i) const
+    {
+        if (i != 0 || !instance) {
+            return {};
+        }
+        return vtable::VWeak<private_api::ItemTreeVTable> { instance->into_dyn() };
+    }
+
+    private_api::IndexRange index_range() const { return private_api::IndexRange { 0, len() }; }
+
+    std::size_t len() const { return instance ? 1 : 0; }
+
+    void for_each(auto &&f) const
+    {
+        if (instance) {
+            f(*instance);
+        }
+    }
 };
 
 } // namespace private_api

internal/compiler/generator/cpp.rs

@@ -2079,17 +2079,13 @@ fn generate_sub_component(
 
     for (idx, repeated) in component.repeated.iter_enumerated() {
         let sc = &root.sub_components[repeated.sub_tree.root];
-        let data_type = if let Some(data_prop) = repeated.data_prop {
+        let data_type = repeated.data_prop.map(|data_prop| sc.properties[data_prop].ty.clone());
-            sc.properties[data_prop].ty.clone()
-        } else {
-            Type::Int32
-        };
 
         generate_repeated_component(
             repeated,
             root,
             ParentCtx::new(&ctx, Some(idx)),
-            &data_type,
+            data_type.as_ref(),
             file,
             conditional_includes,
         );
@@ -2097,12 +2093,7 @@ fn generate_sub_component(
         let idx = usize::from(idx);
         let repeater_id = format_smolstr!("repeater_{}", idx);
 
-        let mut model = compile_expression(&repeated.model.borrow(), &ctx);
+        let model = compile_expression(&repeated.model.borrow(), &ctx);
-        if repeated.model.ty(&ctx) == Type::Bool {
-            // bool converts to int
-            // FIXME: don't do a heap allocation here
-            model = format!("std::make_shared<slint::private_api::UIntModel>({model})")
-        }
 
         // FIXME: optimize  if repeated.model.is_constant()
         properties_init_code.push(format!(
@@ -2143,17 +2134,19 @@ fn generate_sub_component(
             }}",
         ));
 
-        target_struct.members.push((
+        let rep_type = match data_type {
-            field_access,
+            Some(data_type) => {
-            Declaration::Var(Var {
+                format_smolstr!(
-                ty: format_smolstr!(
                     "slint::private_api::Repeater<class {}, {}>",
                     ident(&sc.name),
-                    data_type.cpp_type().unwrap(),
+                    data_type.cpp_type().unwrap()
-                ),
+                )
-                name: repeater_id,
+            }
-                ..Default::default()
+            None => format_smolstr!("slint::private_api::Conditional<class {}>", ident(&sc.name)),
-            }),
+        };
+        target_struct.members.push((
+            field_access,
+            Declaration::Var(Var { ty: rep_type, name: repeater_id, ..Default::default() }),
         ));
     }
 
@@ -2438,7 +2431,7 @@ fn generate_repeated_component(
     repeated: &llr::RepeatedElement,
     root: &llr::CompilationUnit,
     parent_ctx: ParentCtx,
-    model_data_type: &Type,
+    model_data_type: Option<&Type>,
     file: &mut File,
     conditional_includes: &ConditionalIncludes,
 ) {
@@ -2474,22 +2467,24 @@ fn generate_repeated_component(
     let index_prop = repeated.index_prop.iter().map(access_prop);
     let data_prop = repeated.data_prop.iter().map(access_prop);
 
-    let mut update_statements = vec!["[[maybe_unused]] auto self = this;".into()];
+    if let Some(model_data_type) = model_data_type {
-    update_statements.extend(index_prop.map(|prop| format!("{prop}.set(i);")));
+        let mut update_statements = vec!["[[maybe_unused]] auto self = this;".into()];
-    update_statements.extend(data_prop.map(|prop| format!("{prop}.set(data);")));
+        update_statements.extend(index_prop.map(|prop| format!("{prop}.set(i);")));
+        update_statements.extend(data_prop.map(|prop| format!("{prop}.set(data);")));
 
-    repeater_struct.members.push((
+        repeater_struct.members.push((
-        Access::Public, // Because Repeater accesses it
+            Access::Public, // Because Repeater accesses it
-        Declaration::Function(Function {
+            Declaration::Function(Function {
-            name: "update_data".into(),
+                name: "update_data".into(),
-            signature: format!(
+                signature: format!(
-                "([[maybe_unused]] int i, [[maybe_unused]] const {} &data) const -> void",
+                    "([[maybe_unused]] int i, [[maybe_unused]] const {} &data) const -> void",
-                model_data_type.cpp_type().unwrap()
+                    model_data_type.cpp_type().unwrap()
-            ),
+                ),
-            statements: Some(update_statements),
+                statements: Some(update_statements),
-            ..Function::default()
+                ..Function::default()
-        }),
+            }),
-    ));
+        ));
+    }
 
     repeater_struct.members.push((
         Access::Public, // Because Repeater accesses it
@@ -3972,19 +3967,19 @@ fn box_layout_function(
                 if let Some(ri) = &repeated_indices {
                     write!(push_code, "{}_array[{}] = cells_vector.size();", ri, repeater_idx * 2)
                         .unwrap();
-                    write!(push_code,
+                    write!(
-                        "{ri}_array[{c}] = self->repeater_{id}.inner ? self->repeater_{id}.inner->data.size() : 0;",
+                        push_code,
+                        "{ri}_array[{c}] = self->repeater_{id}.len();",
                         ri = ri,
                         c = repeater_idx * 2 + 1,
                         id = repeater,
-                    ).unwrap();
+                    )
+                    .unwrap();
                 }
                 repeater_idx += 1;
                 write!(
                     push_code,
-                    "if (self->repeater_{id}.inner) \
+                    "self->repeater_{id}.for_each([&](const auto &sub_comp){{ cells_vector.push_back(sub_comp->box_layout_data({o})); }});",
-                        for (auto &&sub_comp : self->repeater_{id}.inner->data) \
-                           cells_vector.push_back((*sub_comp.ptr)->box_layout_data({o}));",
                     id = repeater,
                     o = to_cpp_orientation(orientation),
                 )

internal/compiler/generator/rust.rs

@@ -766,9 +766,8 @@ fn generate_sub_component(
         }
     }
 
-    let mut repeated_element_names: Vec<Ident> = vec![];
     let mut repeated_visit_branch: Vec<TokenStream> = vec![];
-    let mut repeated_element_components: Vec<Ident> = vec![];
+    let mut repeated_element_components: Vec<TokenStream> = vec![];
     let mut repeated_subtree_ranges: Vec<TokenStream> = vec![];
     let mut repeated_subtree_components: Vec<TokenStream> = vec![];
 
@@ -783,11 +782,7 @@ fn generate_sub_component(
         let rep_inner_component_id =
             self::inner_component_id(&root.sub_components[repeated.sub_tree.root]);
 
-        let mut model = compile_expression(&repeated.model.borrow(), &ctx);
+        let model = compile_expression(&repeated.model.borrow(), &ctx);
-        if repeated.model.ty(&ctx) == Type::Bool {
-            model = quote!(sp::ModelRc::new(#model as bool))
-        }
-
         init.push(quote! {
             _self.#repeater_id.set_model_binding({
                 let self_weak = sp::VRcMapped::downgrade(&self_rc);
@@ -838,8 +833,11 @@ fn generate_sub_component(
                 }
             }
         ));
-        repeated_element_names.push(repeater_id);
+        repeated_element_components.push(if repeated.index_prop.is_some() {
-        repeated_element_components.push(rep_inner_component_id);
+            quote!(#repeater_id: sp::Repeater<#rep_inner_component_id>)
+        } else {
+            quote!(#repeater_id: sp::Conditional<#rep_inner_component_id>)
+        });
     }
 
     // Use ids following the real repeaters to piggyback on their forwarding through sub-components!
@@ -1149,7 +1147,7 @@ fn generate_sub_component(
             #(#popup_id_names : ::core::cell::Cell<sp::Option<::core::num::NonZeroU32>>,)*
             #(#declared_property_vars : sp::Property<#declared_property_types>,)*
             #(#declared_callbacks : sp::Callback<(#(#declared_callbacks_types,)*), #declared_callbacks_ret>,)*
-            #(#repeated_element_names : sp::Repeater<#repeated_element_components>,)*
+            #(#repeated_element_components,)*
             #(#change_tracker_names : sp::ChangeTracker,)*
             #(#timer_names : sp::Timer,)*
             self_weak : sp::OnceCell<sp::VWeakMapped<sp::ItemTreeVTable, #inner_component_id>>,

internal/compiler/llr/item_tree.rs

@@ -157,9 +157,9 @@ pub struct ListViewInfo {
 #[derive(Debug)]
 pub struct RepeatedElement {
     pub model: MutExpression,
-    /// Within the sub_tree's root component
+    /// Within the sub_tree's root component. None for `if`
     pub index_prop: Option<PropertyIdx>,
-    /// Within the sub_tree's root component
+    /// Within the sub_tree's root component. None for `if`
     pub data_prop: Option<PropertyIdx>,
     pub sub_tree: ItemTree,
     /// The index of the item node in the parent tree

internal/core/model.rs

@@ -1274,6 +1274,91 @@ impl<C: RepeatedItemTree + 'static> Repeater<C> {
     }
 }
 
+#[pin_project]
+pub struct Conditional<C: RepeatedItemTree> {
+    #[pin]
+    model: Property<bool>,
+    instance: RefCell<Option<ItemTreeRc<C>>>,
+}
+
+impl<C: RepeatedItemTree> Default for Conditional<C> {
+    fn default() -> Self {
+        Self {
+            model: Property::new_named(false, "i_slint_core::Conditional::model"),
+            instance: RefCell::new(None),
+        }
+    }
+}
+
+impl<C: RepeatedItemTree + 'static> Conditional<C> {
+    /// Call this function to make sure that the model is updated.
+    /// The init function is the function to create a ItemTree
+    pub fn ensure_updated(self: Pin<&Self>, init: impl Fn() -> ItemTreeRc<C>) {
+        let model = self.project_ref().model.get();
+
+        if !model {
+            drop(self.instance.replace(None));
+        } else if self.instance.borrow().is_none() {
+            let i = init();
+            self.instance.replace(Some(i.clone()));
+            i.init();
+        }
+    }
+
+    /// Set the model binding
+    pub fn set_model_binding(&self, binding: impl Fn() -> bool + 'static) {
+        self.model.set_binding(binding);
+    }
+
+    /// Call the visitor for the root of each instance
+    pub fn visit(
+        &self,
+        order: TraversalOrder,
+        mut visitor: crate::item_tree::ItemVisitorRefMut,
+    ) -> crate::item_tree::VisitChildrenResult {
+        // We can't keep self.inner borrowed because the event might modify the model
+        let instance = self.instance.borrow().clone();
+        if let Some(c) = instance {
+            if c.as_pin_ref().visit_children_item(-1, order, visitor.borrow_mut()).has_aborted() {
+                return crate::item_tree::VisitChildrenResult::abort(0, 0);
+            }
+        }
+
+        crate::item_tree::VisitChildrenResult::CONTINUE
+    }
+
+    /// Return the amount of instances (1 if the conditional is active, 0 otherwise)
+    pub fn len(&self) -> usize {
+        self.instance.borrow().is_some() as usize
+    }
+
+    /// Return the range of indices used by this Conditional.
+    ///
+    /// Similar to Repeater::range, but the range is always [0, 1] if the Conditional is active.
+    pub fn range(&self) -> core::ops::Range<usize> {
+        0..self.len()
+    }
+
+    /// Return the instance for the given model index.
+    /// The index should be within [`Self::range()`]
+    pub fn instance_at(&self, index: usize) -> Option<ItemTreeRc<C>> {
+        if index != 0 {
+            return None;
+        }
+        self.instance.borrow().clone()
+    }
+
+    /// Return true if the Repeater as empty
+    pub fn is_empty(&self) -> bool {
+        self.len() == 0
+    }
+
+    /// Returns a vector containing all instances
+    pub fn instances_vec(&self) -> Vec<ItemTreeRc<C>> {
+        self.instance.borrow().clone().into_iter().collect()
+    }
+}
+
 impl From<SharedString> for StandardListViewItem {
     fn from(value: SharedString) -> Self {
         StandardListViewItem { text: value }

internal/interpreter/dynamic_item_tree.rs

@@ -118,6 +118,8 @@ pub(crate) struct RepeaterWithinItemTree<'par_id, 'sub_id> {
     pub(crate) model: Expression,
     /// Offset of the `Repeater`
     offset: FieldOffset<Instance<'par_id>, Repeater<ErasedItemTreeBox>>,
+    /// Whether this is a `if` or a `for`
+    is_conditional: bool,
 }
 
 impl RepeatedItemTree for ErasedItemTreeBox {
@@ -1052,6 +1054,7 @@ pub(crate) fn generate_item_tree<'id>(
             let base_component = item.base_type.as_component();
             self.repeater_names.insert(item.id.clone(), self.repeater.len());
             generativity::make_guard!(guard);
+            let repeated_element_info = item.repeated.as_ref().unwrap();
             self.repeater.push(
                 RepeaterWithinItemTree {
                     item_tree_to_repeat: generate_item_tree(
@@ -1062,7 +1065,8 @@ pub(crate) fn generate_item_tree<'id>(
                         guard,
                     ),
                     offset: self.type_builder.add_field_type::<Repeater<ErasedItemTreeBox>>(),
-                    model: item.repeated.as_ref().unwrap().model.clone(),
+                    model: repeated_element_info.model.clone(),
+                    is_conditional: repeated_element_info.is_conditional_element,
                 }
                 .into(),
             );
@@ -1701,14 +1705,23 @@ pub fn instantiate(
         let repeater = rep_in_comp.offset.apply_pin(instance_ref.instance);
         let expr = rep_in_comp.model.clone();
         let model_binding_closure = make_binding_eval_closure(expr, &self_weak);
-        repeater.set_model_binding(move || {
+        if rep_in_comp.is_conditional {
-            let m = model_binding_closure();
+            let bool_model = Rc::new(crate::value_model::BoolModel::default());
-            if let Value::Model(m) = m {
+            repeater.set_model_binding(move || {
-                m.clone()
+                let v = model_binding_closure();
-            } else {
+                bool_model.set_value(v.try_into().expect("condition model is bool"));
-                ModelRc::new(crate::value_model::ValueModel::new(m))
+                ModelRc::from(bool_model.clone())
-            }
+            });
-        });
+        } else {
+            repeater.set_model_binding(move || {
+                let m = model_binding_closure();
+                if let Value::Model(m) = m {
+                    m.clone()
+                } else {
+                    ModelRc::new(crate::value_model::ValueModel::new(m))
+                }
+            });
+        }
     }
     self_rc
 }

internal/interpreter/value_model.rs

@@ -2,7 +2,8 @@
 // SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
 
 use crate::api::Value;
-use i_slint_core::model::{Model, ModelTracker};
+use core::cell::Cell;
+use i_slint_core::model::{Model, ModelNotify, ModelTracker};
 
 pub struct ValueModel {
     value: Value,
@@ -81,3 +82,36 @@ impl Model for ValueModel {
         self
     }
 }
+
+/// A model for conditional elements
+#[derive(Default)]
+pub(crate) struct BoolModel {
+    value: Cell<bool>,
+    notify: ModelNotify,
+}
+
+impl Model for BoolModel {
+    type Data = Value;
+    fn row_count(&self) -> usize {
+        if self.value.get() {
+            1
+        } else {
+            0
+        }
+    }
+    fn row_data(&self, row: usize) -> Option<Self::Data> {
+        (row == 0 && self.value.get()).then_some(Value::Void)
+    }
+    fn model_tracker(&self) -> &dyn ModelTracker {
+        &self.notify
+    }
+}
+
+impl BoolModel {
+    pub fn set_value(&self, val: bool) {
+        let old = self.value.replace(val);
+        if old != val {
+            self.notify.reset();
+        }
+    }
+}

tests/cases/models/if_dirty.slint

@@ -0,0 +1,146 @@
+// Copyright © SixtyFPS GmbH <info@slint.dev>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
+
+// Test for 3953
+
+export component TestCase inherits Window {
+    width: 100phx;
+    height: 100phx;
+
+    in property<int> value: 42;
+    in-out property <string> r;
+
+    if value > 0 : TouchArea {
+        property <int> cL;
+        width: 50%;
+        x: 0px;
+        clicked => {
+            cL += 1;
+            r += "clickL(" + cL + ").";
+        }
+        init => {
+            r += "initL(" + cL + ").";
+        }
+
+    }
+
+    if value.mod(2) == 0 : TouchArea {
+        property <int> cR;
+        width: 50%;
+        x: parent.width - self.width;
+        clicked => {
+            cR += 1;
+            r += "clickR(" + cR + ").";
+        }
+        init => {
+            r += "initR(" + cR + ").";
+        }
+    }
+
+}
+
+
+
+
+/*
+```cpp
+auto handle = TestCase::create();
+const TestCase &instance = *handle;
+
+// condition is false
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq(instance.get_r(), "initR(0).initL(0).clickL(1).");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq(instance.get_r(), "initR(0).initL(0).clickL(1).clickR(1).");
+instance.set_r("");
+
+instance.set_value(-42);
+assert_eq(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq(instance.get_r(), "clickR(2).");
+instance.set_r("");
+
+instance.set_value(48);
+assert_eq(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq(instance.get_r(), "initL(0).clickL(1).");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq(instance.get_r(), "initL(0).clickL(1).clickR(3).");
+instance.set_r("");
+
+instance.set_value(49);
+assert_eq(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq(instance.get_r(), "clickL(2).");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq(instance.get_r(), "clickL(2).");
+```
+
+
+```rust
+let instance = TestCase::new().unwrap();
+
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq!(instance.get_r(), "initR(0).initL(0).clickL(1).");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq!(instance.get_r(), "initR(0).initL(0).clickL(1).clickR(1).");
+instance.set_r("".into());
+
+instance.set_value(-42);
+assert_eq!(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq!(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq!(instance.get_r(), "clickR(2).");
+instance.set_r("".into());
+
+instance.set_value(48);
+assert_eq!(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq!(instance.get_r(), "initL(0).clickL(1).");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq!(instance.get_r(), "initL(0).clickL(1).clickR(3).");
+instance.set_r("".into());
+
+instance.set_value(49);
+assert_eq!(instance.get_r(), "");
+slint_testing::send_mouse_click(&instance, 5., 5.);
+assert_eq!(instance.get_r(), "clickL(2).");
+slint_testing::send_mouse_click(&instance, 95., 5.);
+assert_eq!(instance.get_r(), "clickL(2).");
+```
+
+```js
+var instance = new slint.TestCase();
+slintlib.private_api.send_mouse_click(instance, 5., 5.);
+assert.equal(instance.r, "initR(0).initL(0).clickL(1).");
+slintlib.private_api.send_mouse_click(instance, 95., 5.);
+assert.equal(instance.r, "initR(0).initL(0).clickL(1).clickR(1).");
+instance.r = "";
+
+instance.value = -42;
+assert.equal(instance.r, "");
+slintlib.private_api.send_mouse_click(instance, 5., 5.);
+assert.equal(instance.r, "");
+slintlib.private_api.send_mouse_click(instance, 95., 5.);
+assert.equal(instance.r, "clickR(2).");
+instance.r = "";
+
+instance.value = 48;
+assert.equal(instance.r, "");
+slintlib.private_api.send_mouse_click(instance, 5., 5.);
+assert.equal(instance.r, "initL(0).clickL(1).");
+slintlib.private_api.send_mouse_click(instance, 95., 5.);
+assert.equal(instance.r, "initL(0).clickL(1).clickR(3).");
+instance.r = "";
+
+instance.value = 49;
+assert.equal(instance.r, "");
+slintlib.private_api.send_mouse_click(instance, 5., 5.);
+assert.equal(instance.r, "clickL(2).");
+slintlib.private_api.send_mouse_click(instance, 95., 5.);
+assert.equal(instance.r, "clickL(2).");
+```
+*/