Popup: Set correct window adapter for the component#11352
Popup: Set correct window adapter for the component#11352Murmele wants to merge 17 commits intoslint-ui:masterfrom
Conversation
Reason: Previously always the main window adapter is used for popups as well. This is not correct, because then acting on the wrong renderer caches is done and the application might crash or memory gets not freed. To solve this problem. Showing the popup is split up in multiple steps. 1) Creating the window adapter for the popup if possible. 2) Assigning this window adapter to the popup instance so that the tree will be setup with the newly created window adapter. 3) Determine the popup location and show the created popup
Murmele
force-pushed
the
mm/window_adapter
branch
from
610922e to
f2b74e4
Compare
|
|
||
| /// Shows the popup created with create_popup_window_adapter() | ||
| /// `geometry` is the location and size of the popup in the window coordinate | ||
| fn show_popup(&self, _window_adapter: Rc<dyn WindowAdapter>, _geometry: LogicalRect) {} |
There was a problem hiding this comment.
Long term, we'll want to make this interface public, and i'd like to avoid having too many functions for this in the WindowAdapter.
What's the reason this can't be done by the normal show function?
There was a problem hiding this comment.
I changed to use the set_visible() function so we don't need this trait function. fd13575
Tested with qt, winit ChildWindow and winit TopLevelWindow (on another branch)
| if let Some(s) = self.window_adapter().internal(crate::InternalToken) { | ||
| return s.create_popup_window_adapter(); | ||
| } | ||
| None |
There was a problem hiding this comment.
nitpick: This could be simplified with a .map(...)
| ItemTreeRc::borrow_pin(popup_componentrc) | ||
| .as_ref() | ||
| .window_adapter(false, &mut popup_window_adapter); | ||
| let popup_window_adapter = popup_window_adapter.unwrap(); // It must be there because we set the global |
There was a problem hiding this comment.
nitpick: could be an expect(...) instead of a comment
| WindowInner::from_pub(window_adapter.window()).set_component(popup_componentrc); | ||
| PopupWindowLocation::TopLevel(window_adapter) | ||
| } | ||
| let location = if Rc::ptr_eq(&parent_window_adapter, &popup_window_adapter) { |
There was a problem hiding this comment.
I'm thinking this could potentially be a problem if we had a popup child of another popup and one (custom) platform would only support one layer of popup. But maybe there is no such thing.
There was a problem hiding this comment.
You mean a mix of ChildWindow and TopLevelWindow?
| #(#global_names : self.#global_names.clone(),)* | ||
| #(#from_library_global_names : self.#from_library_global_names.clone(),)* | ||
| window_adapter: ::core::default::Default::default(), | ||
| root_item_tree_weak: self.root_item_tree_weak.clone(), |
There was a problem hiding this comment.
I think this also need to passed in
There was a problem hiding this comment.
I can do. It is only required to init the window_adapter, but since we initialized already, self.window_adapter.get_or_try_init will never call the init branch
There was a problem hiding this comment.
The problem here is that we need the global to create the item, but on the other side we need the global to create the popup item
| let parent_item = #parent_item; | ||
| let popup_instance = #popup_window_id::new(#component_access_tokens.self_weak.get().unwrap().clone()).unwrap(); | ||
| // Use the newly created window adapter if we are able to create one. Otherwise use the parent's one | ||
| let globals = if let Some(window_adapter) = sp::WindowInner::from_pub(#window_adapter_tokens.window()).create_popup_window_adapter() { |
There was a problem hiding this comment.
I guess a similar change in the C++ and interpreter code will be required.
There was a problem hiding this comment.
yep there I have to add as well
#Conflicts: # internal/backends/winit/winitwindowadapter.rs # internal/core/window.rs
2 participants
Reason: Previously always the main window adapter is used for popups as well. This is not correct, because then acting on the wrong renderer caches is done and the application might crash or memory gets not freed.
To solve this problem, showing the popup is split up in multiple steps.