From 567c644a5fe06cbfc11700a31a9c5de58eef60b3 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Tue, 19 Oct 2021 14:32:54 +0200 Subject: [PATCH] Fix PopupWindow position when all elements are not inlined Pass a reference to the parent item in the show_popup function so we can compute the exact location at runtime. --- api/sixtyfps-cpp/include/sixtyfps.h | 5 +-- sixtyfps_compiler/generator/cpp.rs | 7 +++- sixtyfps_compiler/generator/rust.rs | 5 ++- sixtyfps_compiler/object_tree.rs | 1 + sixtyfps_compiler/passes/inlining.rs | 4 +++ sixtyfps_compiler/passes/lower_popups.rs | 33 +++++++------------ sixtyfps_runtime/corelib/window.rs | 16 ++++++++- .../interpreter/dynamic_component.rs | 7 ++-- sixtyfps_runtime/interpreter/eval.rs | 15 ++++++++- 9 files changed, 61 insertions(+), 32 deletions(-) diff --git a/api/sixtyfps-cpp/include/sixtyfps.h b/api/sixtyfps-cpp/include/sixtyfps.h index 8f9ae17f1..1441cedf5 100644 --- a/api/sixtyfps-cpp/include/sixtyfps.h +++ b/api/sixtyfps-cpp/include/sixtyfps.h @@ -139,10 +139,11 @@ public: } template - void show_popup(const Parent *parent_component, cbindgen_private::Point p) const + void show_popup(const Parent *parent_component, cbindgen_private::Point p, + cbindgen_private::ItemRc parent_item) const { auto popup = Component::create(parent_component).into_dyn(); - cbindgen_private::sixtyfps_windowrc_show_popup(&inner, &popup, p); + cbindgen_private::sixtyfps_windowrc_show_popup(&inner, &popup, p, &parent_item); } private: diff --git a/sixtyfps_compiler/generator/cpp.rs b/sixtyfps_compiler/generator/cpp.rs index 8c4e5989f..da0a3b647 100644 --- a/sixtyfps_compiler/generator/cpp.rs +++ b/sixtyfps_compiler/generator/cpp.rs @@ -1753,7 +1753,12 @@ fn compile_expression( let popup = popup_list.iter().find(|p| Rc::ptr_eq(&p.component, &pop_comp)).unwrap(); let x = access_named_reference(&popup.x, component, "self"); let y = access_named_reference(&popup.y, component, "self"); - format!("self->m_window.window_handle().show_popup<{}>(self, {{ {}.get(), {}.get() }} );", popup_window_rcid, x, y) + let parent_component_ref = access_element_component(&popup.parent_element, component, "self"); + format!( + "self->m_window.window_handle().show_popup<{}>(self, {{ {}.get(), {}.get() }}, {{ {}->self_weak.lock()->into_dyn(), {} }} );", + popup_window_rcid, x, y, + parent_component_ref, popup.parent_element.borrow().item_index.get().unwrap(), + ) } else { panic!("internal error: argument to SetFocusItem must be an element") } diff --git a/sixtyfps_compiler/generator/rust.rs b/sixtyfps_compiler/generator/rust.rs index a53cff6f9..66c5f8637 100644 --- a/sixtyfps_compiler/generator/rust.rs +++ b/sixtyfps_compiler/generator/rust.rs @@ -1342,10 +1342,13 @@ fn compile_expression(expr: &Expression, component: &Rc) -> TokenStre let popup = popup_list.iter().find(|p| Rc::ptr_eq(&p.component, &pop_comp)).unwrap(); let x = access_named_reference(&popup.x, component, quote!(_self)); let y = access_named_reference(&popup.y, component, quote!(_self)); + let parent_component_ref = access_element_component(&popup.parent_element, component, quote!(_self)); + let parent_index = *popup.parent_element.borrow().item_index.get().unwrap(); quote!( _self.window.window_handle().show_popup( &VRc::into_dyn(#popup_window_id::new(_self.self_weak.get().unwrap().clone(), &_self.window.window_handle()).into()), - Point::new(#x.get(), #y.get()) + Point::new(#x.get(), #y.get()), + &ItemRc::new(VRc::into_dyn(#parent_component_ref.self_weak.get().unwrap().upgrade().unwrap()), #parent_index) ); ) } else { diff --git a/sixtyfps_compiler/object_tree.rs b/sixtyfps_compiler/object_tree.rs index 3d1a3cf79..f69feee92 100644 --- a/sixtyfps_compiler/object_tree.rs +++ b/sixtyfps_compiler/object_tree.rs @@ -171,6 +171,7 @@ pub struct PopupWindow { pub component: Rc, pub x: NamedReference, pub y: NamedReference, + pub parent_element: ElementRc, } type ChildrenInsertionPoint = (ElementRc, syntax_nodes::ChildrenPlaceholder); diff --git a/sixtyfps_compiler/passes/inlining.rs b/sixtyfps_compiler/passes/inlining.rs index d50dfd714..75a630e1b 100644 --- a/sixtyfps_compiler/passes/inlining.rs +++ b/sixtyfps_compiler/passes/inlining.rs @@ -298,6 +298,10 @@ fn duplicate_popup( x: p.x.clone(), y: p.y.clone(), component: duplicate_sub_component(&p.component, &parent, mapping), + parent_element: mapping + .get(&element_key(p.parent_element.clone())) + .expect("Parent element must be in the mapping") + .clone(), } } diff --git a/sixtyfps_compiler/passes/lower_popups.rs b/sixtyfps_compiler/passes/lower_popups.rs index 90253c359..2448feb10 100644 --- a/sixtyfps_compiler/passes/lower_popups.rs +++ b/sixtyfps_compiler/passes/lower_popups.rs @@ -25,27 +25,24 @@ pub fn lower_popups( recurse_elem_including_sub_components_no_borrow( component, - &Vec::new(), - &mut |elem, parent_stack: &Vec| { + &None, + &mut |elem, parent_element: &Option| { let is_popup = elem.borrow().base_type.to_string() == "PopupWindow"; if is_popup { - lower_popup_window(elem, parent_stack, &window_type, diag); + lower_popup_window(elem, parent_element.as_ref(), &window_type, diag); } - // this could be implemented in a better way with less cloning of the state - let mut parent_stack = parent_stack.clone(); - parent_stack.push(elem.clone()); - parent_stack + Some(elem.clone()) }, ) } fn lower_popup_window( popup_window_element: &ElementRc, - parent_stack: &[ElementRc], + parent_element: Option<&ElementRc>, window_type: &Type, diag: &mut BuildDiagnostics, ) { - let parent_element = match parent_stack.last() { + let parent_element = match parent_element { None => { diag.push_error( "PopupWindow cannot be the top level".into(), @@ -76,8 +73,8 @@ fn lower_popup_window( // Generate a x and y property, relative to the window coordinate // FIXME: this is a hack that doesn't always work, perhaps should we store an item ref or something - let coord_x = create_coordinate(&popup_comp, parent_stack, "x"); - let coord_y = create_coordinate(&popup_comp, parent_stack, "y"); + let coord_x = create_coordinate(&popup_comp, parent_element, "x"); + let coord_y = create_coordinate(&popup_comp, parent_element, "y"); // Throw error when accessing the popup from outside // FIXME: @@ -99,30 +96,22 @@ fn lower_popup_window( component: popup_comp, x: coord_x, y: coord_y, + parent_element: parent_element.clone(), }); } fn create_coordinate( popup_comp: &Rc, - parent_stack: &[ElementRc], + parent_element: &ElementRc, coord: &str, ) -> NamedReference { - let mut expression = popup_comp + let expression = popup_comp .root_element .borrow() .bindings .get(coord) .map(|e| e.expression.clone()) .unwrap_or(Expression::NumberLiteral(0., crate::expression_tree::Unit::Phx)); - - for parent in parent_stack { - expression = Expression::BinaryExpression { - lhs: Box::new(expression), - rhs: Box::new(Expression::PropertyReference(NamedReference::new(parent, coord))), - op: '+', - }; - } - let parent_element = parent_stack.last().unwrap(); let property_name = format!("{}-popup-{}", popup_comp.root_element.borrow().id, coord); parent_element .borrow_mut() diff --git a/sixtyfps_runtime/corelib/window.rs b/sixtyfps_runtime/corelib/window.rs index d2e10071d..e139b78ab 100644 --- a/sixtyfps_runtime/corelib/window.rs +++ b/sixtyfps_runtime/corelib/window.rs @@ -472,6 +472,19 @@ impl Window { size } + /// Show a popup at the given position relative to the item + pub fn show_popup(&self, popup: &ComponentRc, mut position: Point, parent_item: &ItemRc) { + let mut parent_item = parent_item.clone(); + loop { + position += parent_item.borrow().as_ref().geometry().origin.to_vector(); + parent_item = match parent_item.parent_item().upgrade() { + None => break, + Some(pi) => pi, + } + } + self.platform_window.get().unwrap().show_popup(popup, position) + } + /// Removes any active popup. pub fn close_popup(&self) { if let Some(current_popup) = self.active_popup.replace(None) { @@ -671,9 +684,10 @@ pub mod ffi { handle: *const WindowRcOpaque, popup: &ComponentRc, position: crate::graphics::Point, + parent_item: &ItemRc, ) { let window = &*(handle as *const WindowRc); - window.show_popup(popup, position); + window.show_popup(popup, position, parent_item); } /// Close the current popup pub unsafe extern "C" fn sixtyfps_windowrc_close_popup(handle: *const WindowRcOpaque) { diff --git a/sixtyfps_runtime/interpreter/dynamic_component.rs b/sixtyfps_runtime/interpreter/dynamic_component.rs index 69e675eb2..550828165 100644 --- a/sixtyfps_runtime/interpreter/dynamic_component.rs +++ b/sixtyfps_runtime/interpreter/dynamic_component.rs @@ -1592,16 +1592,15 @@ impl<'a, 'id> InstanceRef<'a, 'id> { /// Show the popup at the given location pub fn show_popup( popup: &object_tree::PopupWindow, - x: f32, - y: f32, + pos: sixtyfps_corelib::graphics::Point, parent_comp: ComponentRefPin, parent_window: &WindowRc, + parent_item: &ItemRc, ) { generativity::make_guard!(guard); // FIXME: we should compile once and keep the cached compiled component let compiled = generate_component(&popup.component, guard); let inst = instantiate(compiled, Some(parent_comp), Some(parent_window)); inst.run_setup_code(); - parent_window - .show_popup(&vtable::VRc::into_dyn(inst), sixtyfps_corelib::graphics::Point::new(x, y)); + parent_window.show_popup(&vtable::VRc::into_dyn(inst), pos, parent_item); } diff --git a/sixtyfps_runtime/interpreter/eval.rs b/sixtyfps_runtime/interpreter/eval.rs index 74809a620..4fa5beea0 100644 --- a/sixtyfps_runtime/interpreter/eval.rs +++ b/sixtyfps_runtime/interpreter/eval.rs @@ -305,7 +305,20 @@ pub fn eval_expression(expression: &Expression, local_context: &mut EvalLocalCon let popup = popup_list.iter().find(|p| Rc::ptr_eq(&p.component, &pop_comp)).unwrap(); let x = load_property_helper(local_context.component_instance, &popup.x.element(), popup.x.name()).unwrap(); let y = load_property_helper(local_context.component_instance, &popup.y.element(), popup.y.name()).unwrap(); - crate::dynamic_component::show_popup(popup, x.try_into().unwrap(), y.try_into().unwrap(), component.borrow(), window_ref(component).unwrap()); + + generativity::make_guard!(guard); + let enclosing_component = + enclosing_component_for_element(&popup.parent_element, component, guard); + let parent_item_info = &enclosing_component.component_type.items[popup.parent_element.borrow().id.as_str()]; + let parent_item_comp = enclosing_component.self_weak().get().unwrap().upgrade().unwrap(); + let parent_item = corelib::items::ItemRc::new(vtable::VRc::into_dyn(parent_item_comp), parent_item_info.item_index()); + + crate::dynamic_component::show_popup( + popup, + sixtyfps_corelib::graphics::Point::new(x.try_into().unwrap(), y.try_into().unwrap()), + component.borrow(), + window_ref(component).unwrap(), + &parent_item); Value::Void } else { panic!("internal error: argument to SetFocusItem must be an element")