From 25a64d97c0065adc7273abc3dbaa879c80470bd6 Mon Sep 17 00:00:00 2001 From: sertschgi <58653179+sertschgi@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:10:22 +0100 Subject: [PATCH] Fixed the multiple window closing issue, where the two multiwindow examples weren't working. (#3499) * fixed window when multiple windows not closing * fixed window when multiple windows not closing * fixed accidental includes * polished * fix last window closing hiding app and rc hardcount * find the suspect strong count holders and fix them * Use the proper type alias * fmt --------- Co-authored-by: Jonathan Kelley --- examples/multiwindow.rs | 6 +++++- packages/desktop/src/app.rs | 21 ++++++++++++++------- packages/desktop/src/desktop_context.rs | 7 ++++++- packages/desktop/src/document.rs | 18 +++++++++++++----- packages/desktop/src/lib.rs | 2 +- packages/desktop/src/webview.rs | 10 ++++++---- 6 files changed, 45 insertions(+), 19 deletions(-) diff --git a/examples/multiwindow.rs b/examples/multiwindow.rs index 32e116c59..c22049b3d 100644 --- a/examples/multiwindow.rs +++ b/examples/multiwindow.rs @@ -5,9 +5,13 @@ //! own context, root elements, etc. use dioxus::prelude::*; +use dioxus::{desktop::Config, desktop::WindowCloseBehaviour}; fn main() { - dioxus::LaunchBuilder::desktop().launch(app); + dioxus::LaunchBuilder::desktop() + // We can choose the close behavior of the last window to hide. See WindowCloseBehaviour for more options. + .with_cfg(Config::new().with_close_behaviour(WindowCloseBehaviour::LastWindowHides)) + .launch(app); } fn app() -> Element { diff --git a/packages/desktop/src/app.rs b/packages/desktop/src/app.rs index d41f55250..dac264182 100644 --- a/packages/desktop/src/app.rs +++ b/packages/desktop/src/app.rs @@ -20,7 +20,7 @@ use tao::{ dpi::PhysicalSize, event::Event, event_loop::{ControlFlow, EventLoop, EventLoopBuilder, EventLoopProxy, EventLoopWindowTarget}, - window::WindowId, + window::{Window, WindowId}, }; /// The single top-level object that manages all the running windows, assets, shortcuts, etc @@ -198,11 +198,14 @@ impl App { } } + LastWindowHides if self.webviews.len() > 1 => { + self.webviews.remove(&id); + } + LastWindowHides => { - let Some(webview) = self.webviews.get(&id) else { - return; - }; - hide_app_window(&webview.desktop_context.webview); + if let Some(webview) = self.webviews.get(&id) { + hide_last_window(&webview.desktop_context.window); + } } CloseWindow => { @@ -529,9 +532,13 @@ struct PreservedWindowState { monitor: String, } -/// Different hide implementations per platform +/// Hide the last window when using LastWindowHides. +/// +/// On macOS, if we use `set_visibility(false)` on the window, it will hide the window but not show +/// it again when the user switches back to the app. `NSApplication::hide:` has the correct behaviour, +/// so we need to special case it. #[allow(unused)] -pub fn hide_app_window(window: &wry::WebView) { +fn hide_last_window(window: &Window) { #[cfg(target_os = "windows")] { use tao::platform::windows::WindowExtWindows; diff --git a/packages/desktop/src/desktop_context.rs b/packages/desktop/src/desktop_context.rs index 9480f9e74..1966b0a7e 100644 --- a/packages/desktop/src/desktop_context.rs +++ b/packages/desktop/src/desktop_context.rs @@ -35,6 +35,11 @@ pub fn window() -> DesktopContext { /// A handle to the [`DesktopService`] that can be passed around. pub type DesktopContext = Rc; +/// A weak handle to the [`DesktopService`] to ensure safe passing. +/// The problem without this is that the tao window is never dropped and therefore cannot be closed. +/// This was due to the Rc that had still references because of multiple copies when creating a webview. +pub type WeakDesktopContext = Weak; + /// An imperative interface to the current window. /// /// To get a handle to the current window, use the [`use_window`] hook. @@ -101,7 +106,7 @@ impl DesktopService { /// You can use this to control other windows from the current window. /// /// Be careful to not create a cycle of windows, or you might leak memory. - pub fn new_window(&self, dom: VirtualDom, cfg: Config) -> Weak { + pub fn new_window(&self, dom: VirtualDom, cfg: Config) -> WeakDesktopContext { let window = WebviewInstance::new(cfg, dom, self.shared.clone()); let cx = window.dom.in_runtime(|| { diff --git a/packages/desktop/src/document.rs b/packages/desktop/src/document.rs index 0c8121b08..b5c46ed67 100644 --- a/packages/desktop/src/document.rs +++ b/packages/desktop/src/document.rs @@ -1,11 +1,11 @@ +use crate::{query::Query, DesktopContext, WeakDesktopContext}; use dioxus_core::prelude::queue_effect; use dioxus_document::{ create_element_in_head, Document, Eval, EvalError, Evaluator, LinkProps, MetaProps, ScriptProps, StyleProps, }; -use generational_box::{AnyStorage, GenerationalBox, UnsyncStorage}; -use crate::{query::Query, DesktopContext}; +use generational_box::{AnyStorage, GenerationalBox, UnsyncStorage}; /// Code for the Dioxus channel used to communicate between the dioxus and javascript code pub const NATIVE_EVAL_JS: &str = include_str!("./js/native_eval.js"); @@ -13,22 +13,30 @@ pub const NATIVE_EVAL_JS: &str = include_str!("./js/native_eval.js"); /// Represents the desktop-target's provider of evaluators. #[derive(Clone)] pub struct DesktopDocument { - pub(crate) desktop_ctx: DesktopContext, + pub(crate) desktop_ctx: WeakDesktopContext, } impl DesktopDocument { pub fn new(desktop_ctx: DesktopContext) -> Self { + let desktop_ctx = std::rc::Rc::downgrade(&desktop_ctx); Self { desktop_ctx } } } impl Document for DesktopDocument { fn eval(&self, js: String) -> Eval { - Eval::new(DesktopEvaluator::create(self.desktop_ctx.clone(), js)) + Eval::new(DesktopEvaluator::create( + self.desktop_ctx + .upgrade() + .expect("Window to exist when document is alive"), + js, + )) } fn set_title(&self, title: String) { - self.desktop_ctx.window.set_title(&title); + if let Some(ctx) = self.desktop_ctx.upgrade() { + ctx.set_title(&title); + } } /// Create a new meta tag in the head diff --git a/packages/desktop/src/lib.rs b/packages/desktop/src/lib.rs index d71c3934e..9eecdc6fe 100644 --- a/packages/desktop/src/lib.rs +++ b/packages/desktop/src/lib.rs @@ -48,7 +48,7 @@ pub mod trayicon; // Public exports pub use assets::AssetRequest; pub use config::{Config, WindowCloseBehaviour}; -pub use desktop_context::{window, DesktopContext, DesktopService}; +pub use desktop_context::{window, DesktopContext, DesktopService, WeakDesktopContext}; pub use event_handlers::WryEventHandler; pub use hooks::*; pub use shortcut::{ShortcutHandle, ShortcutRegistryError}; diff --git a/packages/desktop/src/webview.rs b/packages/desktop/src/webview.rs index 440807d94..b1f8c72a8 100644 --- a/packages/desktop/src/webview.rs +++ b/packages/desktop/src/webview.rs @@ -1,4 +1,3 @@ -use crate::document::DesktopDocument; use crate::element::DesktopElement; use crate::file_upload::DesktopFileDragEvent; use crate::menubar::DioxusMenu; @@ -12,6 +11,7 @@ use crate::{ waker::tao_waker, Config, DesktopContext, DesktopService, }; +use crate::{document::DesktopDocument, WeakDesktopContext}; use base64::prelude::BASE64_STANDARD; use dioxus_core::{Runtime, ScopeId, VirtualDom}; use dioxus_document::Document; @@ -28,7 +28,7 @@ use wry::{DragDropEvent, RequestAsyncResponder, WebContext, WebViewBuilder}; pub(crate) struct WebviewEdits { runtime: Rc, pub wry_queue: WryQueue, - desktop_context: Rc>, + desktop_context: Rc>, } impl WebviewEdits { @@ -40,7 +40,7 @@ impl WebviewEdits { } } - fn set_desktop_context(&self, context: DesktopContext) { + fn set_desktop_context(&self, context: WeakDesktopContext) { _ = self.desktop_context.set(context); } @@ -115,6 +115,8 @@ impl WebviewEdits { return Default::default(); }; + let desktop_context = desktop_context.upgrade().unwrap(); + let query = desktop_context.query.clone(); let recent_file = desktop_context.file_hover.clone(); @@ -412,7 +414,7 @@ impl WebviewInstance { )); // Provide the desktop context to the virtual dom and edit handler - edits.set_desktop_context(desktop_context.clone()); + edits.set_desktop_context(Rc::downgrade(&desktop_context)); let provider: Rc = Rc::new(DesktopDocument::new(desktop_context.clone())); let history_provider: Rc = Rc::new(MemoryHistory::default()); dom.in_runtime(|| {