-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixed multiwindow closing issue again #3588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
We have to be careful not to break semver here unfortunately, so the interfaces need to remain the same but the internal details can change. IE we need to take Rc even if we downgrade it internally. I'll take a look at tweaking that on this PR. |
The multi-window example works for me on main - is there a particular repro you have? |
I tested the example on Windows and can confirm that the multiwindow example does not work. But adding let onclick = move |_| async move {
let dom = VirtualDom::new(popup);
dioxus::desktop::window().new_window(dom, Config::new().with_close_behaviour(WindowCloseBehaviour::CloseWindow));
}; |
I still see this problem on the main branch. I'm concerned that this PR is not associated with an issue against 0.7 and may have have gotten lost. Is this being tracked, or should I create a separate issue describing the problem? |
Does anyone have a good repro for this? I tested the most recent main on windows + macOS and wasn't able to repro |
Here's a minimal reproduction - macos, using main branch Dioxus. I'm experimenting with Dioxus for the first time, so this could totally be a "holding it wrong" issue - let me know. I'm on the Discord at @cortesi if you need me to try anything else on my system. use dioxus::desktop::{Config, WindowCloseBehaviour, use_window};
use dioxus::prelude::*;
fn main() {
dioxus::LaunchBuilder::desktop()
.with_cfg(
Config::new()
.with_close_behaviour(WindowCloseBehaviour::LastWindowHides)
)
.launch(WindowA);
}
#[component]
fn WindowA() -> Element {
let open_window_b = move |_| {
println!("Opening Window B");
let dom = VirtualDom::new(WindowB);
let config = Config::new()
.with_close_behaviour(WindowCloseBehaviour::CloseWindow);
dioxus::desktop::window().new_window(dom, config);
};
rsx! {
div {
h1 { "Window A" }
button {
onclick: open_window_b,
"Open Window B"
}
}
}
}
#[component]
fn WindowB() -> Element {
let window = use_window();
let close_window = move |_| {
println!("Attempting to close Window B");
window.close();
};
rsx! {
div {
h1 { "Window B" }
p { "Try closing this window with the window decoration close button" }
button {
onclick: close_window,
"Close Window B (button)"
}
}
}
} |
Thanks for the repro!!! This is a separate issue entirely - we don't need to change desktop itself, but instead manually change the drop order of the VirtualDom itself. The virtualdom and the runtime are separate objects, and the data for the I just created a new PR to fix that, making the destruction of a VirtualDom equivalent to deleting the root node. |
Like I explained in #3499 there is this issue where when creating a new window it cannot be closed again. This is due to references still pointing to the DesktopContext after being removed from webviews in app.rs. They themselves also cannot be dropped and this is not only because of the self referential structure. The problem has not been fixed with my previous pull request as half of it was simply ignored: If the DesktopContext Rc reference is upgraded again and added to other Structs it obviously cannot be dropped. Please test the examples. Also note that this fixes it for linux. For windows only partially.
Furthermore what I have noticed was that the issue was made worse because the main window cannot even be closed anymore because of the window decoration buttons not working - but this has probably nothing to do with this I'm just pointing it out...