From b9f22ebfc38196cace16f6b4fefca3a8f87e8f1b Mon Sep 17 00:00:00 2001 From: Sophie Herold Date: Wed, 18 Dec 2024 17:12:17 +0100 Subject: [PATCH] window: Don't store window to avoid ref cycles Use root() instead and move one constructor to on root for having the root available. --- src/application.rs | 3 ++- src/util.rs | 6 +++++ src/util/root.rs | 34 +++++++++++++++++++++++++++++ src/widgets/edit/crop.rs | 4 ++-- src/widgets/edit_window.rs | 11 ++++------ src/widgets/error_details.rs | 7 ++---- src/widgets/image_page.rs | 13 +++-------- src/widgets/image_window.rs | 23 +++++++++---------- src/widgets/image_window/actions.rs | 1 + src/widgets/print.rs | 1 + src/widgets/window.rs | 21 +++++++++--------- src/widgets/window.ui | 4 +--- 12 files changed, 77 insertions(+), 51 deletions(-) create mode 100644 src/util/root.rs diff --git a/src/application.rs b/src/application.rs index 5a07299..559b47e 100644 --- a/src/application.rs +++ b/src/application.rs @@ -82,7 +82,7 @@ mod imp { } fn activate(&self) { - log::trace!("Activate"); + log::debug!("Showing window via 'activate'"); let application = self.obj(); let window = LpWindow::new(&*application); window.present(); @@ -138,6 +138,7 @@ impl LpApplication { gio::ActionEntryBuilder::new("new-window") .activate(|app: &Self, _, _| { let win = LpWindow::new(app); + log::debug!("Showing new window"); win.present(); }) .build(), diff --git a/src/util.rs b/src/util.rs index b3e6cce..a716080 100644 --- a/src/util.rs +++ b/src/util.rs @@ -17,6 +17,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later pub mod gettext; +pub mod root; use std::ffi::CStr; use std::fmt::{Debug, Write}; @@ -261,3 +262,8 @@ pub fn locale_settings() -> LocaleSettings { locale_settings } } + +pub enum ErrorType { + Loader, + General, +} diff --git a/src/util/root.rs b/src/util/root.rs new file mode 100644 index 0000000..0c5a0be --- /dev/null +++ b/src/util/root.rs @@ -0,0 +1,34 @@ +// Copyright (c) 2024 Sophie Herold +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . +// +// SPDX-License-Identifier: GPL-3.0-or-later + +use gtk::prelude::*; + +use crate::widgets::LpWindow; + +pub trait ParentWindow: WidgetExt { + fn window(&self) -> LpWindow { + let result = self.root().and_downcast(); + + if result.is_none() { + log::error!("Couldn't find LpWindow for {self:?}"); + } + + result.unwrap() + } +} + +impl ParentWindow for T {} diff --git a/src/widgets/edit/crop.rs b/src/widgets/edit/crop.rs index 6cd5754..add0fa3 100644 --- a/src/widgets/edit/crop.rs +++ b/src/widgets/edit/crop.rs @@ -23,9 +23,9 @@ use adw::{glib, gtk}; use glycin::Operation; use crate::editing::preview::EditingError; -use crate::util::gettext::*; +use crate::util::root::ParentWindow; +use crate::util::{gettext::*, ErrorType}; use crate::widgets::edit::LpEditCropSelection; -use crate::widgets::error_details::ErrorType; use crate::widgets::{LpEditWindow, LpImage}; #[derive(Debug, Clone, Copy, Default, glib::Enum)] diff --git a/src/widgets/edit_window.rs b/src/widgets/edit_window.rs index a1cbaae..f7bb611 100644 --- a/src/widgets/edit_window.rs +++ b/src/widgets/edit_window.rs @@ -23,14 +23,14 @@ use adw::subclass::prelude::*; use glycin::Operations; use super::edit::LpEditCrop; -use super::{LpImage, LpWindow}; +use super::LpImage; use crate::deps::*; use crate::util::gettext::*; +use crate::util::root::ParentWindow; +use crate::util::ErrorType; mod imp { - use super::*; - use crate::widgets::error_details::ErrorType; #[derive(Debug, Default, gtk::CompositeTemplate, glib::Properties)] #[properties(wrapper_type = super::LpEditWindow)] @@ -43,8 +43,6 @@ mod imp { #[template_child] save: TemplateChild, - #[property(get, construct_only)] - window: OnceCell, #[property(get, construct_only)] original_image: OnceCell, @@ -170,9 +168,8 @@ glib::wrapper! { } impl LpEditWindow { - pub fn new(window: LpWindow, image: LpImage) -> Self { + pub fn new(image: LpImage) -> Self { glib::Object::builder() - .property("window", window) .property("original_image", image) .build() } diff --git a/src/widgets/error_details.rs b/src/widgets/error_details.rs index 44e4d5b..93de471 100644 --- a/src/widgets/error_details.rs +++ b/src/widgets/error_details.rs @@ -21,6 +21,7 @@ use gtk::CompositeTemplate; use crate::deps::*; use crate::util::gettext::*; +use crate::util::ErrorType; mod imp { use super::*; @@ -108,12 +109,8 @@ impl LpErrorDetails { } )); + log::debug!("Showing error details"); obj.present(Some(root)); obj } } - -pub enum ErrorType { - Loader, - General, -} diff --git a/src/widgets/image_page.rs b/src/widgets/image_page.rs index 79bcee3..589a8c5 100644 --- a/src/widgets/image_page.rs +++ b/src/widgets/image_page.rs @@ -29,6 +29,7 @@ use super::LpErrorDetails; use crate::decoder::DecoderError; use crate::deps::*; use crate::util::gettext::*; +use crate::util::ErrorType; use crate::widgets::LpImage; mod imp { @@ -255,11 +256,7 @@ impl LpImagePage { #[weak(rename_to = obj)] self, move |_| { - LpErrorDetails::new( - &obj.root().unwrap(), - &err, - super::error_details::ErrorType::Loader, - ); + LpErrorDetails::new(&obj.root().unwrap(), &err, ErrorType::Loader); } )); } @@ -271,11 +268,7 @@ impl LpImagePage { #[weak(rename_to = obj)] self, move |_| { - LpErrorDetails::new( - &obj.root().unwrap(), - &err, - super::error_details::ErrorType::Loader, - ); + LpErrorDetails::new(&obj.root().unwrap(), &err, ErrorType::Loader); } )); diff --git a/src/widgets/image_window.rs b/src/widgets/image_window.rs index 13f5cd8..121d022 100644 --- a/src/widgets/image_window.rs +++ b/src/widgets/image_window.rs @@ -42,11 +42,10 @@ use crate::application::LpApplication; use crate::config; use crate::deps::*; use crate::util::gettext::*; +use crate::util::root::ParentWindow; use crate::util::Direction; use crate::widgets::window::ActionPartGlobal; -use crate::widgets::{ - LpDragOverlay, LpImage, LpImageView, LpPropertiesView, LpWindow, LpWindowContent, -}; +use crate::widgets::{LpDragOverlay, LpImage, LpImageView, LpPropertiesView, LpWindowContent}; /// Animation duration for showing overlay buttons in milliseconds const SHOW_CONTROLS_ANIMATION_DURATION: u32 = 200; @@ -62,9 +61,6 @@ mod imp { #[properties(wrapper_type = super::LpImageWindow)] #[template(file = "image_window.ui")] pub struct LpImageWindow { - #[property(get, construct_only)] - window: OnceCell, - #[template_child] pub(super) window_content: TemplateChild, @@ -152,9 +148,11 @@ mod imp { } #[glib::derived_properties] - impl ObjectImpl for LpImageWindow { - fn constructed(&self) { - self.parent_constructed(); + impl ObjectImpl for LpImageWindow {} + + impl WidgetImpl for LpImageWindow { + fn root(&self) { + self.parent_root(); let obj = self.obj(); @@ -413,6 +411,7 @@ mod imp { } // Maybe one day this will actually work + log::debug!("Trying to focus window after drag and drop"); obj.window().present(); true @@ -421,7 +420,6 @@ mod imp { } } - impl WidgetImpl for LpImageWindow {} impl BinImpl for LpImageWindow {} impl LpImageWindow { @@ -567,7 +565,7 @@ impl LpImageWindow { /// Returns true if some text in metadata is currently selected fn has_metadata_selected(&self) -> bool { - if let Some(focus_widget) = self.window().focus() { + if let Some(focus_widget) = GtkWindowExt::focus(&self.window()) { if focus_widget.is_ancestor(&*self.imp().properties_view) { if let Ok(label) = focus_widget.downcast::() { return label.selection_bounds().is_some(); @@ -630,8 +628,7 @@ impl LpImageWindow { }); } )); - todo!(); - //self.imp().toast_overlay.add_toast(toast); + self.window().add_toast(toast); } Err(err) => { if Some(gio::IOErrorEnum::NotSupported) == err.kind::() { diff --git a/src/widgets/image_window/actions.rs b/src/widgets/image_window/actions.rs index 91eb640..6e45de5 100644 --- a/src/widgets/image_window/actions.rs +++ b/src/widgets/image_window/actions.rs @@ -22,6 +22,7 @@ use gtk::prelude::*; use strum::IntoEnumIterator; use crate::deps::*; +use crate::util::root::ParentWindow; use crate::util::{Direction, Position}; #[derive(strum::Display, strum::AsRefStr, strum::EnumIter)] diff --git a/src/widgets/print.rs b/src/widgets/print.rs index 2d948e2..25f00e7 100644 --- a/src/widgets/print.rs +++ b/src/widgets/print.rs @@ -406,6 +406,7 @@ mod imp { obj.set_orientation(orientation); drop(ui_updates_disabled); + log::debug!("Showing print dialog"); obj.present(); } diff --git a/src/widgets/window.rs b/src/widgets/window.rs index e563e06..480868c 100644 --- a/src/widgets/window.rs +++ b/src/widgets/window.rs @@ -23,12 +23,11 @@ use gio::glib::VariantTy; use gtk::CompositeTemplate; use strum::IntoEnumIterator; -use super::error_details::ErrorType; use super::LpErrorDetails; use crate::config; use crate::deps::*; use crate::util::gettext::*; -use crate::util::Direction; +use crate::util::{Direction, ErrorType}; use crate::widgets::{LpEditWindow, LpImageView, LpImageWindow}; /// Show window after X milliseconds even if image dimensions are not known yet @@ -129,6 +128,7 @@ mod imp { #[weak] obj, move || if !obj.is_visible() { + log::debug!("Showing window after timeout"); obj.present() } ), @@ -149,7 +149,7 @@ mod imp { glib::wrapper! { pub struct LpWindow(ObjectSubclass) @extends gtk::Widget, gtk::Window, gtk::ApplicationWindow, adw::ApplicationWindow, - @implements gio::ActionMap, gio::ActionGroup, gtk::Native; + @implements gio::ActionMap, gio::ActionGroup, gtk::Native, gtk::Root; } impl LpWindow { @@ -195,18 +195,18 @@ impl LpWindow { } } - pub fn show_toast(&self, text: &str, priority: adw::ToastPriority) { - let imp = self.imp(); + pub fn add_toast(&self, toast: adw::Toast) { + self.imp().toast_overlay.add_toast(toast); + } + pub fn show_toast(&self, text: &str, priority: adw::ToastPriority) { let toast = adw::Toast::new(text); toast.set_priority(priority); - imp.toast_overlay.add_toast(toast); + self.add_toast(toast); } pub fn show_error(&self, stub: &str, details: &str, type_: ErrorType) { - let imp = self.imp(); - let action = match type_ { ErrorType::General => WindowAction::ShowError, ErrorType::Loader => WindowAction::ShowLoaderError, @@ -220,7 +220,7 @@ impl LpWindow { .action_target(&details.to_variant()) .build(); - imp.toast_overlay.add_toast(toast); + self.add_toast(toast); } pub fn show_error_details(&self, details: &str) { @@ -233,6 +233,7 @@ impl LpWindow { pub async fn show_about(&self) { let about = crate::about::dialog().await; + log::debug!("Showing about dialog"); about.present(Some(self)); } @@ -246,7 +247,7 @@ impl LpWindow { if let Some(image) = self.image_view().current_image() { let edit_child = &*self.imp().edit_window_child; - edit_child.set_child(Some(&LpEditWindow::new(self.clone(), image))); + edit_child.set_child(Some(&LpEditWindow::new(image))); self.imp().stack.set_visible_child(edit_child); } else { log::error!("Can't open image editor since no current image exists"); diff --git a/src/widgets/window.ui b/src/widgets/window.ui index af3eba0..1859688 100644 --- a/src/widgets/window.ui +++ b/src/widgets/window.ui @@ -24,9 +24,7 @@ - - LpWindow - +