From d0bf0fba50f9bf41042b78cf894392d327a50df3 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 1 Apr 2022 12:17:12 +0200 Subject: [PATCH] Fix a bunch more issue with PopupWindow * Make sure that the compiler don't panic if the parent of a PopupWindow is optimized (by not optiizing such element) * Ensure that we can call popup.show() from within a deeper repeater * Ensure that the parent element of the popup is the right one in case of repeater (and not the node in the parent component) This partially revert ad5991f8fad9936016ce77c103f41f1fd93fa53d and 6c7a7aed0e1a0fedd0a7164dc21d5be2255f61d5 because we must do the lower_popup adter the repeater pass, because otherwise the parent element of the created component for the PopupWindow might be wrong and it is not easy to adjust (we would have to make Component::parent_element a RefCell or duplicate it again. Fixes #1132 --- internal/compiler/generator/cpp.rs | 16 +++++++++++--- internal/compiler/generator/rust.rs | 13 ++++++++++-- internal/compiler/object_tree.rs | 4 ++++ internal/compiler/passes.rs | 2 +- internal/compiler/passes/ensure_window.rs | 1 + internal/compiler/passes/inlining.rs | 1 + internal/compiler/passes/lower_popups.rs | 16 +++++++------- .../passes/optimize_useless_rectangles.rs | 2 +- .../compiler/passes/repeater_component.rs | 21 +------------------ .../crashes/issue1113_popup_in_repeater.slint | 21 +++++++++++++++++++ 10 files changed, 62 insertions(+), 35 deletions(-) diff --git a/internal/compiler/generator/cpp.rs b/internal/compiler/generator/cpp.rs index 8d920a94476..1e2180f76f9 100644 --- a/internal/compiler/generator/cpp.rs +++ b/internal/compiler/generator/cpp.rs @@ -2320,16 +2320,26 @@ fn compile_builtin_function_call( if let [llr::Expression::NumberLiteral(popup_index), x, y, llr::Expression::PropertyReference(parent_ref)] = arguments { + let mut parent_ctx = ctx; + let mut component_access = "self".into(); + + if let llr::PropertyReference::InParent { level, .. } = parent_ref { + for _ in 0..level.get() { + component_access = format!("{}->parent", component_access); + parent_ctx = parent_ctx.parent.as_ref().unwrap().ctx; + } + }; + let window = access_window_field(ctx); - let current_sub_component = ctx.current_sub_component.unwrap(); + let current_sub_component = parent_ctx.current_sub_component.unwrap(); let popup_window_id = ident(¤t_sub_component.popup_windows[*popup_index as usize].root.name); let parent_component = access_item_rc(parent_ref, ctx); let x = compile_expression(x, ctx); let y = compile_expression(y, ctx); format!( - "{}.show_popup<{}>(self, {{ {}, {} }}, {{ {} }})", - window, popup_window_id, x, y, parent_component, + "{}.show_popup<{}>({}, {{ {}, {} }}, {{ {} }})", + window, popup_window_id, component_access, x, y, parent_component, ) } else { panic!("internal error: invalid args to ShowPopupWindow {:?}", arguments) diff --git a/internal/compiler/generator/rust.rs b/internal/compiler/generator/rust.rs index 08a3a15c80c..f1623f589a7 100644 --- a/internal/compiler/generator/rust.rs +++ b/internal/compiler/generator/rust.rs @@ -1886,7 +1886,16 @@ fn compile_builtin_function_call( if let [Expression::NumberLiteral(popup_index), x, y, Expression::PropertyReference(parent_ref)] = arguments { - let current_sub_component = ctx.current_sub_component.unwrap(); + let mut parent_ctx = ctx; + let mut component_access_tokens = quote!(_self); + if let llr::PropertyReference::InParent { level, .. } = parent_ref { + for _ in 0..level.get() { + component_access_tokens = + quote!(#component_access_tokens.parent.upgrade().unwrap().as_pin_ref()); + parent_ctx = parent_ctx.parent.as_ref().unwrap().ctx; + } + } + let current_sub_component = parent_ctx.current_sub_component.unwrap(); let popup_window_id = inner_component_id( ¤t_sub_component.popup_windows[*popup_index as usize].root, ); @@ -1896,7 +1905,7 @@ fn compile_builtin_function_call( let window_tokens = access_window_field(ctx); quote!( #window_tokens.show_popup( - &VRc::into_dyn(#popup_window_id::new(_self.self_weak.get().unwrap().clone()).into()), + &VRc::into_dyn(#popup_window_id::new(#component_access_tokens.self_weak.get().unwrap().clone()).into()), Point::new(#x as f32, #y as f32), #parent_component ); diff --git a/internal/compiler/object_tree.rs b/internal/compiler/object_tree.rs index f227f09c6b3..0ca0fad8f8c 100644 --- a/internal/compiler/object_tree.rs +++ b/internal/compiler/object_tree.rs @@ -458,6 +458,10 @@ pub struct Element { /// true if this Element is the fake Flickable viewport pub is_flickable_viewport: bool, + /// true if this Element may have a popup as child meaning it cannot be optimized + /// because the popup references it. + pub has_popup_child: bool, + /// This is the component-local index of this item in the item tree array. /// It is generated after the last pass and before the generators run. pub item_index: once_cell::unsync::OnceCell, diff --git a/internal/compiler/passes.rs b/internal/compiler/passes.rs index 08168beda4a..3d7fa389bf0 100644 --- a/internal/compiler/passes.rs +++ b/internal/compiler/passes.rs @@ -116,8 +116,8 @@ pub async fn run_passes( .chain(std::iter::once(root_component)) { flickable::handle_flickable(component, &global_type_registry.borrow()); - lower_popups::lower_popups(component, &doc.local_registry, diag); repeater_component::process_repeater_components(component); + lower_popups::lower_popups(component, &doc.local_registry, diag); lower_layout::lower_layouts(component, type_loader, diag).await; default_geometry::default_geometry(component, diag); z_order::reorder_by_z_order(component, diag); diff --git a/internal/compiler/passes/ensure_window.rs b/internal/compiler/passes/ensure_window.rs index 5608c09aba7..f5cf6e4718f 100644 --- a/internal/compiler/passes/ensure_window.rs +++ b/internal/compiler/passes/ensure_window.rs @@ -41,6 +41,7 @@ pub fn ensure_window( states: Default::default(), transitions: Default::default(), child_of_layout: false, + has_popup_child: false, layout_info_prop: Default::default(), is_flickable_viewport: false, item_index: Default::default(), diff --git a/internal/compiler/passes/inlining.rs b/internal/compiler/passes/inlining.rs index a9f0dcc5181..02d0dc817a9 100644 --- a/internal/compiler/passes/inlining.rs +++ b/internal/compiler/passes/inlining.rs @@ -218,6 +218,7 @@ fn duplicate_element_with_mapping( item_index: Default::default(), // Not determined yet item_index_of_first_children: Default::default(), is_flickable_viewport: elem.is_flickable_viewport, + has_popup_child: elem.has_popup_child, inline_depth: elem.inline_depth + 1, })); mapping.insert(element_key(element.clone()), new.clone()); diff --git a/internal/compiler/passes/lower_popups.rs b/internal/compiler/passes/lower_popups.rs index 5a402dcca9e..14c357945ca 100644 --- a/internal/compiler/passes/lower_popups.rs +++ b/internal/compiler/passes/lower_popups.rs @@ -37,14 +37,6 @@ fn lower_popup_window( window_type: &Type, diag: &mut BuildDiagnostics, ) { - if popup_window_element.borrow().repeated.is_some() { - diag.push_error( - "PopupWindow cannot be directly repeated or conditional".into(), - &*popup_window_element.borrow(), - ); - return; - } - let parent_element = match parent_element { None => { diag.push_error( @@ -57,6 +49,13 @@ fn lower_popup_window( }; let parent_component = popup_window_element.borrow().enclosing_component.upgrade().unwrap(); + if Rc::ptr_eq(&parent_component.root_element, popup_window_element) { + diag.push_error( + "PopupWindow cannot be directly repeated or conditional".into(), + &*popup_window_element.borrow(), + ); + return; + } // Remove the popup_window_element from its parent let old_size = parent_element.borrow().children.len(); @@ -66,6 +65,7 @@ fn lower_popup_window( old_size, "Exactly one child must be removed (the popup itself)" ); + parent_element.borrow_mut().has_popup_child = true; popup_window_element.borrow_mut().base_type = window_type.clone(); diff --git a/internal/compiler/passes/optimize_useless_rectangles.rs b/internal/compiler/passes/optimize_useless_rectangles.rs index 5544de44714..09241d77f64 100644 --- a/internal/compiler/passes/optimize_useless_rectangles.rs +++ b/internal/compiler/passes/optimize_useless_rectangles.rs @@ -36,7 +36,7 @@ pub fn optimize_useless_rectangles(root_component: &Rc) { /// Check that this is a element we can optimize fn can_optimize(elem: &ElementRc) -> bool { let e = elem.borrow(); - if e.is_flickable_viewport { + if e.is_flickable_viewport || e.has_popup_child { return false; }; diff --git a/internal/compiler/passes/repeater_component.rs b/internal/compiler/passes/repeater_component.rs index db3ac99c15c..331c9ac31b8 100644 --- a/internal/compiler/passes/repeater_component.rs +++ b/internal/compiler/passes/repeater_component.rs @@ -14,7 +14,6 @@ use std::rc::Rc; pub fn process_repeater_components(component: &Rc) { create_repeater_components(component); adjust_references(component); - adjust_popups(component); } fn create_repeater_components(component: &Rc) { @@ -43,6 +42,7 @@ fn create_repeater_components(component: &Rc) { child_of_layout: elem.child_of_layout || is_listview.is_some(), layout_info_prop: elem.layout_info_prop.take(), is_flickable_viewport: elem.is_flickable_viewport, + has_popup_child: elem.has_popup_child, item_index: Default::default(), // Not determined yet item_index_of_first_children: Default::default(), inline_depth: 0, @@ -114,22 +114,3 @@ fn adjust_references(comp: &Rc) { }) }); } - -fn adjust_popups(component: &Rc) { - component.popup_windows.borrow_mut().retain(|popup| { - let parent = popup - .component - .parent_element - .upgrade() - .unwrap() - .borrow() - .enclosing_component - .upgrade() - .unwrap(); - if Rc::ptr_eq(&parent, component) { - return true; - } - parent.popup_windows.borrow_mut().push(popup.clone()); - false - }); -} diff --git a/tests/cases/crashes/issue1113_popup_in_repeater.slint b/tests/cases/crashes/issue1113_popup_in_repeater.slint index ee19bf7ff90..9535deeca45 100644 --- a/tests/cases/crashes/issue1113_popup_in_repeater.slint +++ b/tests/cases/crashes/issue1113_popup_in_repeater.slint @@ -12,5 +12,26 @@ App := Window { } } } + + if true : Rectangle { + Rectangle { + TouchArea { + clicked => { + popup2.show(); + } + } + popup2 := PopupWindow { } + } + } + + // Issue #1132 + for menu-item in [0] : TouchArea { + clicked => { popup3.show(); } + if true: TouchArea { + clicked => { popup3.show(); } + } + popup3 := PopupWindow { } + } + }