From 6deef7148fad472e148a91d57e7cd95375140e30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Commaille?= Date: Wed, 23 Oct 2024 19:15:26 +0200 Subject: [PATCH] utils: Remove ImageError::None It is only here to be able to use it as a GEnum for simplicity in a few types, but is doesn't make sense. --- src/components/avatar/editable.rs | 308 ++++++++++++++++-------------- src/components/avatar/image.rs | 188 ++++++++++-------- src/utils/media/image/mod.rs | 26 +-- 3 files changed, 277 insertions(+), 245 deletions(-) diff --git a/src/components/avatar/editable.rs b/src/components/avatar/editable.rs index eab568f2..2f879a5a 100644 --- a/src/components/avatar/editable.rs +++ b/src/components/avatar/editable.rs @@ -17,7 +17,7 @@ use crate::{ utils::{ expression, media::image::{ImageDimensions, ImageError, IMAGE_QUEUE}, - CountedRef, + BoundObject, BoundObjectWeakRef, CountedRef, }, }; @@ -49,45 +49,45 @@ mod imp { #[template(resource = "/org/gnome/Fractal/ui/components/avatar/editable.ui")] #[properties(wrapper_type = super::EditableAvatar)] pub struct EditableAvatar { + #[template_child] + stack: TemplateChild, + #[template_child] + temp_avatar: TemplateChild, + #[template_child] + error_img: TemplateChild, + #[template_child] + button_remove: TemplateChild, + #[template_child] + button_edit: TemplateChild, /// The [`AvatarData`] to display. #[property(get, set = Self::set_data, explicit_notify)] - pub data: RefCell>, + data: BoundObject, + /// The avatar image to watch. + #[property(get)] + image: BoundObjectWeakRef, /// Whether this avatar is changeable. #[property(get, set = Self::set_editable, explicit_notify)] - pub editable: Cell, + editable: Cell, /// Whether to prevent the remove button from showing. #[property(get, set = Self::set_inhibit_remove, explicit_notify)] - pub inhibit_remove: Cell, + inhibit_remove: Cell, /// The current state of the edit. #[property(get, set = Self::set_state, explicit_notify, builder(EditableAvatarState::default()))] - pub state: Cell, + state: Cell, /// The state of the avatar edit. - pub edit_state: Cell, + edit_state: Cell, /// Whether the edit button is sensitive. - pub edit_sensitive: Cell, - /// Whether this avatar is removable. - pub removable: Cell, + edit_sensitive: Cell, /// The state of the avatar removal. - pub remove_state: Cell, + remove_state: Cell, /// Whether the remove button is sensitive. - pub remove_sensitive: Cell, + remove_sensitive: Cell, /// A temporary paintable to show instead of the avatar. #[property(get)] - pub temp_paintable: RefCell>, + temp_paintable: RefCell>, /// The error encountered when loading the temporary avatar, if any. - #[property(get, builder(ImageError::default()))] - pub temp_error: Cell, + temp_error: Cell>, temp_paintable_animation_ref: RefCell>, - #[template_child] - pub stack: TemplateChild, - #[template_child] - pub temp_avatar: TemplateChild, - #[template_child] - pub error_img: TemplateChild, - #[template_child] - pub button_remove: TemplateChild, - #[template_child] - pub button_edit: TemplateChild, } #[glib::object_subclass] @@ -170,21 +170,6 @@ mod imp { imp.update_temp_paintable_state(); } )); - - // Watch errors for the avatar data. - obj.property_expression("data") - .chain_property::("image") - .chain_property::("error") - .watch( - None::<&glib::Object>, - clone!( - #[weak(rename_to = imp)] - self, - move || { - imp.update_error(); - } - ), - ); } } @@ -194,15 +179,54 @@ mod imp { impl EditableAvatar { /// Set the [`AvatarData`] to display. fn set_data(&self, data: Option) { - if *self.data.borrow() == data { + if self.data.obj() == data { return; } - self.data.replace(data); - self.update_error(); + self.data.disconnect_signals(); + + if let Some(data) = data { + let image_handler = data.connect_image_notify(clone!( + #[weak(rename_to = imp)] + self, + move |_| { + imp.update_image(); + } + )); + + self.data.set(data, vec![image_handler]); + } + + self.update_image(); self.obj().notify_data(); } + /// Update the avatar image to watch. + fn update_image(&self) { + let image = self.data.obj().and_then(|data| data.image()); + + if self.image.obj() == image { + return; + } + + self.image.disconnect_signals(); + + if let Some(image) = &image { + let error_handler = image.connect_error_changed(clone!( + #[weak(rename_to = imp)] + self, + move |_| { + imp.update_error(); + } + )); + + self.image.set(image, vec![error_handler]); + } + + self.update_error(); + self.obj().notify_image(); + } + /// Set whether this avatar is editable. fn set_editable(&self, editable: bool) { if self.editable.get() == editable { @@ -224,61 +248,60 @@ mod imp { } /// Set the state of the edit. - fn set_state(&self, state: EditableAvatarState) { + pub(super) fn set_state(&self, state: EditableAvatarState) { if self.state.get() == state { return; } - let obj = self.obj(); match state { EditableAvatarState::Default => { self.show_temp_paintable(false); - obj.set_edit_state(ActionState::Default); - obj.set_edit_sensitive(true); - obj.set_remove_state(ActionState::Default); - obj.set_remove_sensitive(true); + self.set_edit_state(ActionState::Default); + self.set_edit_sensitive(true); + self.set_remove_state(ActionState::Default); + self.set_remove_sensitive(true); self.set_temp_paintable(Ok(None)); } EditableAvatarState::EditInProgress => { self.show_temp_paintable(true); - obj.set_edit_state(ActionState::Loading); - obj.set_edit_sensitive(true); - obj.set_remove_state(ActionState::Default); - obj.set_remove_sensitive(false); + self.set_edit_state(ActionState::Loading); + self.set_edit_sensitive(true); + self.set_remove_state(ActionState::Default); + self.set_remove_sensitive(false); } EditableAvatarState::EditSuccessful => { self.show_temp_paintable(false); - obj.set_edit_sensitive(true); - obj.set_remove_state(ActionState::Default); - obj.set_remove_sensitive(true); + self.set_edit_sensitive(true); + self.set_remove_state(ActionState::Default); + self.set_remove_sensitive(true); self.set_temp_paintable(Ok(None)); // Animation for success. - obj.set_edit_state(ActionState::Success); + self.set_edit_state(ActionState::Success); glib::timeout_add_local_once( Duration::from_secs(2), clone!( - #[weak] - obj, + #[weak(rename_to =imp)] + self, move || { - obj.set_state(EditableAvatarState::Default); + imp.set_state(EditableAvatarState::Default); } ), ); } EditableAvatarState::RemovalInProgress => { self.show_temp_paintable(true); - obj.set_edit_state(ActionState::Default); - obj.set_edit_sensitive(false); - obj.set_remove_state(ActionState::Loading); - obj.set_remove_sensitive(true); + self.set_edit_state(ActionState::Default); + self.set_edit_sensitive(false); + self.set_remove_state(ActionState::Loading); + self.set_remove_sensitive(true); } } self.state.set(state); - obj.notify_state(); + self.obj().notify_state(); } /// The dimensions of the avatar in this widget. @@ -305,8 +328,8 @@ mod imp { /// Set the temporary paintable. fn set_temp_paintable(&self, paintable: Result, ImageError>) { let (paintable, error) = match paintable { - Ok(paintable) => (paintable, ImageError::None), - Err(error) => (None, error), + Ok(paintable) => (paintable, None), + Err(error) => (None, Some(error)), }; if *self.temp_paintable.borrow() == paintable { @@ -347,7 +370,7 @@ mod imp { } /// Set the error encountered when loading the temporary avatar, if any. - fn set_temp_error(&self, error: ImageError) { + fn set_temp_error(&self, error: Option) { if self.temp_error.get() == error { return; } @@ -355,7 +378,6 @@ mod imp { self.temp_error.set(error); self.update_error(); - self.obj().notify_temp_error(); } /// Update the error that is displayed. @@ -365,20 +387,71 @@ mod imp { .visible_child_name() .is_some_and(|name| name == "default") { - self.data - .borrow() - .as_ref() - .and_then(|data| data.image()) - .map(|image| image.error()) - .unwrap_or_default() + self.image.obj().and_then(|image| image.error()) } else { self.temp_error.get() }; - if error.is_error() { + if let Some(error) = error { self.error_img.set_tooltip_text(Some(&error.to_string())); } - self.error_img.set_visible(error.is_error()); + self.error_img.set_visible(error.is_some()); + } + + /// The state of the avatar edit. + pub(super) fn edit_state(&self) -> ActionState { + self.edit_state.get() + } + + /// Set the state of the avatar edit. + fn set_edit_state(&self, state: ActionState) { + if self.edit_state() == state { + return; + } + + self.edit_state.set(state); + } + + /// Whether the edit button is sensitive. + fn edit_sensitive(&self) -> bool { + self.edit_sensitive.get() + } + + /// Set whether the edit button is sensitive. + fn set_edit_sensitive(&self, sensitive: bool) { + if self.edit_sensitive() == sensitive { + return; + } + + self.edit_sensitive.set(sensitive); + } + + /// The state of the avatar removal. + pub(super) fn remove_state(&self) -> ActionState { + self.remove_state.get() + } + + /// Set the state of the avatar removal. + fn set_remove_state(&self, state: ActionState) { + if self.remove_state() == state { + return; + } + + self.remove_state.set(state); + } + + /// Whether the remove button is sensitive. + fn remove_sensitive(&self) -> bool { + self.remove_sensitive.get() + } + + /// Set whether the remove button is sensitive. + fn set_remove_sensitive(&self, sensitive: bool) { + if self.remove_sensitive() == sensitive { + return; + } + + self.remove_sensitive.set(sensitive); } } } @@ -395,91 +468,36 @@ impl EditableAvatar { } /// Reset the state of the avatar. - pub fn reset(&self) { - self.set_state(EditableAvatarState::Default); + pub(crate) fn reset(&self) { + self.imp().set_state(EditableAvatarState::Default); } /// Show that an edit is in progress. - pub fn edit_in_progress(&self) { - self.set_state(EditableAvatarState::EditInProgress); + pub(crate) fn edit_in_progress(&self) { + self.imp().set_state(EditableAvatarState::EditInProgress); } /// Show that a removal is in progress. - pub fn removal_in_progress(&self) { - self.set_state(EditableAvatarState::RemovalInProgress); + pub(crate) fn removal_in_progress(&self) { + self.imp().set_state(EditableAvatarState::RemovalInProgress); } /// Show that the current ongoing action was successful. /// /// This is has no effect if no action is ongoing. - pub fn success(&self) { - if self.edit_state() == ActionState::Loading { - self.set_state(EditableAvatarState::EditSuccessful); - } else if self.remove_state() == ActionState::Loading { + pub(crate) fn success(&self) { + let imp = self.imp(); + if imp.edit_state() == ActionState::Loading { + imp.set_state(EditableAvatarState::EditSuccessful); + } else if imp.remove_state() == ActionState::Loading { // The remove button is hidden as soon as the avatar is gone so we // don't need a state when it succeeds. - self.set_state(EditableAvatarState::Default); + imp.set_state(EditableAvatarState::Default); } } - /// The state of the avatar edit. - fn edit_state(&self) -> ActionState { - self.imp().edit_state.get() - } - - /// Set the state of the avatar edit. - fn set_edit_state(&self, state: ActionState) { - if self.edit_state() == state { - return; - } - - self.imp().edit_state.set(state); - } - - /// Whether the edit button is sensitive. - fn edit_sensitive(&self) -> bool { - self.imp().edit_sensitive.get() - } - - /// Set whether the edit button is sensitive. - fn set_edit_sensitive(&self, sensitive: bool) { - if self.edit_sensitive() == sensitive { - return; - } - - self.imp().edit_sensitive.set(sensitive); - } - - /// The state of the avatar removal. - fn remove_state(&self) -> ActionState { - self.imp().remove_state.get() - } - - /// Set the state of the avatar removal. - fn set_remove_state(&self, state: ActionState) { - if self.remove_state() == state { - return; - } - - self.imp().remove_state.set(state); - } - - /// Whether the remove button is sensitive. - fn remove_sensitive(&self) -> bool { - self.imp().remove_sensitive.get() - } - - /// Set whether the remove button is sensitive. - fn set_remove_sensitive(&self, sensitive: bool) { - if self.remove_sensitive() == sensitive { - return; - } - - self.imp().remove_sensitive.set(sensitive); - } - /// Choose a new avatar. - async fn choose_avatar(&self) { + pub(super) async fn choose_avatar(&self) { let filters = gio::ListStore::new::(); let image_filter = gtk::FileFilter::new(); @@ -524,7 +542,7 @@ impl EditableAvatar { self.imp().set_temp_paintable_from_file(file.clone()).await; self.emit_by_name::<()>("edit-avatar", &[&file]); } else { - error!("The chosen file is not an image"); + error!("Expected an image, got {content_type}"); toast!(self, gettext("The chosen file is not an image")); } } else { diff --git a/src/components/avatar/image.rs b/src/components/avatar/image.rs index 1b9a570a..f5dc5c61 100644 --- a/src/components/avatar/image.rs +++ b/src/components/avatar/image.rs @@ -1,4 +1,9 @@ -use gtk::{gdk, glib, glib::clone, prelude::*, subclass::prelude::*}; +use gtk::{ + gdk, glib, + glib::{clone, closure_local}, + prelude::*, + subclass::prelude::*, +}; use ruma::{ api::client::media::get_content_thumbnail::v3::Method, events::room::avatar::ImageInfo, OwnedMxcUri, @@ -31,6 +36,9 @@ mod imp { marker::PhantomData, }; + use glib::subclass::Signal; + use once_cell::sync::Lazy; + use super::*; #[derive(Debug, Default, glib::Properties)] @@ -38,28 +46,27 @@ mod imp { pub struct AvatarImage { /// The image content as a paintable, if any. #[property(get)] - pub paintable: RefCell>, + paintable: RefCell>, /// The biggest needed size of the user-defined image. /// /// If this is `0`, no image will be loaded. #[property(get, set = Self::set_needed_size, explicit_notify, minimum = 0)] - pub needed_size: Cell, + needed_size: Cell, /// The Matrix URI of the avatar. - pub(super) uri: RefCell>, + uri: RefCell>, /// The Matrix URI of the `AvatarImage`, as a string. #[property(get = Self::uri_string)] uri_string: PhantomData>, /// Information about the avatar. - pub(super) info: RefCell>, + info: RefCell>, /// The source of the avatar's URI. #[property(get, construct_only, builder(AvatarUriSource::default()))] - pub uri_source: Cell, + uri_source: Cell, /// The current session. #[property(get, construct_only)] - pub session: OnceCell, + session: OnceCell, /// The error encountered when loading the avatar, if any. - #[property(get, builder(ImageError::default()))] - pub error: Cell, + pub(super) error: Cell>, } #[glib::object_subclass] @@ -69,7 +76,13 @@ mod imp { } #[glib::derived_properties] - impl ObjectImpl for AvatarImage {} + impl ObjectImpl for AvatarImage { + fn signals() -> &'static [Signal] { + static SIGNALS: Lazy> = + Lazy::new(|| vec![Signal::builder("error-changed").build()]); + SIGNALS.as_ref() + } + } impl AvatarImage { /// Set the needed size of the user-defined image. @@ -79,11 +92,10 @@ mod imp { if self.needed_size.get() >= size { return; } - let obj = self.obj(); self.needed_size.set(size); - obj.load(); - obj.notify_needed_size(); + self.load(); + self.obj().notify_needed_size(); } /// The Matrix URI of the `AvatarImage`. @@ -124,8 +136,8 @@ mod imp { /// loading the avatar. pub(super) fn set_paintable(&self, paintable: Result, ImageError>) { let (paintable, error) = match paintable { - Ok(paintable) => (paintable, ImageError::None), - Err(error) => (None, error), + Ok(paintable) => (paintable, None), + Err(error) => (None, Some(error)), }; if *self.paintable.borrow() != paintable { @@ -137,13 +149,72 @@ mod imp { } /// Set the error encountered when loading the avatar, if any. - fn set_error(&self, error: ImageError) { + fn set_error(&self, error: Option) { if self.error.get() == error { return; } self.error.set(error); - self.obj().notify_error(); + self.obj().emit_by_name::<()>("error-changed", &[]); + } + + /// Load the image with the current settings. + pub(super) fn load(&self) { + if self.needed_size.get() == 0 { + // We do not need the avatar. + self.set_paintable(Ok(None)); + return; + } + + let Some(uri) = self.uri() else { + // We do not have an avatar. + self.set_paintable(Ok(None)); + return; + }; + + spawn!( + glib::Priority::LOW, + clone!( + #[weak(rename_to = imp)] + self, + async move { + imp.load_inner(uri).await; + } + ) + ); + } + + async fn load_inner(&self, uri: OwnedMxcUri) { + let client = self.session.get().expect("session is initialized").client(); + let info = self.info(); + + let needed_size = self.needed_size.get(); + let dimensions = ImageDimensions { + width: needed_size, + height: needed_size, + }; + + let downloader = ThumbnailDownloader { + main: ImageSource { + source: (&uri).into(), + info: info.as_ref().map(Into::into), + }, + // Avatars are not encrypted so we should always get the thumbnail from the + // original. + alt: None, + }; + let settings = ThumbnailSettings { + dimensions, + method: Method::Crop, + animated: true, + prefer_thumbnail: true, + }; + + // TODO: Change priority depending on size? + let result = downloader + .download(client, settings, ImageRequestPriority::Low) + .await; + self.set_paintable(result.map(|image| Some(image.into()))); } } } @@ -156,7 +227,7 @@ glib::wrapper! { impl AvatarImage { /// Construct a new `AvatarImage` with the given session, Matrix URI and /// avatar info. - pub fn new( + pub(crate) fn new( session: &Session, uri_source: AvatarUriSource, uri: Option, @@ -172,83 +243,38 @@ impl AvatarImage { } /// Set the Matrix URI and information of the avatar. - pub fn set_uri_and_info(&self, uri: Option, info: Option) { + pub(crate) fn set_uri_and_info(&self, uri: Option, info: Option) { let imp = self.imp(); let changed = imp.set_uri(uri); imp.set_info(info); if changed { - self.load(); + imp.load(); } } /// The Matrix URI of the avatar. - pub fn uri(&self) -> Option { + pub(crate) fn uri(&self) -> Option { self.imp().uri() } - /// Information about the avatar. - pub fn info(&self) -> Option { - self.imp().info() - } - - /// Load the image with the current settings. - fn load(&self) { - if self.needed_size() == 0 { - // We do not need the avatar. - self.imp().set_paintable(Ok(None)); - return; - } - - let Some(uri) = self.uri() else { - // We do not have an avatar. - self.imp().set_paintable(Ok(None)); - return; - }; - - spawn!( - glib::Priority::LOW, - clone!( - #[weak(rename_to = obj)] - self, - async move { - obj.load_inner(uri).await; - } - ) - ); + /// The error encountered when loading the avatar, if any. + pub(crate) fn error(&self) -> Option { + self.imp().error.get() } - async fn load_inner(&self, uri: OwnedMxcUri) { - let imp = self.imp(); - let client = self.session().client(); - let info = self.info(); - - let needed_size = self.needed_size(); - let dimensions = ImageDimensions { - width: needed_size, - height: needed_size, - }; - - let downloader = ThumbnailDownloader { - main: ImageSource { - source: (&uri).into(), - info: info.as_ref().map(Into::into), - }, - // Avatars are not encrypted so we should always get the thumbnail from the original. - alt: None, - }; - let settings = ThumbnailSettings { - dimensions, - method: Method::Crop, - animated: true, - prefer_thumbnail: true, - }; - - // TODO: Change priority depending on size? - let result = downloader - .download(client, settings, ImageRequestPriority::Low) - .await; - imp.set_paintable(result.map(|image| Some(image.into()))); + /// Connect to the signal emitted when the error changed. + pub(crate) fn connect_error_changed( + &self, + f: F, + ) -> glib::SignalHandlerId { + self.connect_closure( + "error-changed", + true, + closure_local!(|obj: Self| { + f(&obj); + }), + ) } } diff --git a/src/utils/media/image/mod.rs b/src/utils/media/image/mod.rs index 6cc2bdac..03d4fdcc 100644 --- a/src/utils/media/image/mod.rs +++ b/src/utils/media/image/mod.rs @@ -1,9 +1,9 @@ //! Collection of methods for images. -use std::{fmt, str::FromStr, sync::Arc}; +use std::{error::Error, fmt, str::FromStr, sync::Arc}; use gettextrs::gettext; -use gtk::{gdk, gio, glib, prelude::*}; +use gtk::{gdk, gio, prelude::*}; use image::{ColorType, DynamicImage, ImageDecoder, ImageResult}; use matrix_sdk::{ attachment::{BaseImageInfo, BaseThumbnailInfo, Thumbnail}, @@ -776,16 +776,8 @@ impl From for MediaThumbnailSettings { } /// An error encountered when loading an image. -/// -/// This type implements `Display` with localized messages for the actual -/// errors, but the implementation crashes if it is called for the `None` -/// variant. -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, glib::Enum)] -#[enum_type(name = "ImageError")] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ImageError { - /// There is no error. - #[default] - None, /// Could not download the image. Download, /// Could not save the image to a temporary file. @@ -800,20 +792,16 @@ pub enum ImageError { Aborted, } -impl ImageError { - /// Whether this is an actual error. - pub fn is_error(&self) -> bool { - !matches!(self, Self::None) - } -} +impl Error for ImageError {} impl fmt::Display for ImageError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let s = match self { - Self::None | Self::Aborted => unimplemented!(), Self::Download => gettext("Could not retrieve media"), Self::UnsupportedFormat => gettext("Image format not supported"), - Self::File | Self::Io | Self::Unknown => gettext("An unexpected error occurred"), + Self::File | Self::Io | Self::Unknown | Self::Aborted => { + gettext("An unexpected error occurred") + } }; f.write_str(&s)