room-history: Fix enabling and disabling copy-image action

We now track all the way down if there is currently a texture and
update whenever the texture is added/removed.

This prevents a race condition when the image is loaded before the child
is added to its parent and works with showing and hiding media previews
too.

src/session/view/content/room_history/event_row.rs

@@ -1,7 +1,6 @@
 use adw::{prelude::*, subclass::prelude::*};
 use gtk::{gdk, gio, glib, glib::clone};
 use matrix_sdk_ui::timeline::TimelineEventItemId;
-use tracing::error;
 
 use super::{EventActionsGroup, MessageRow, RoomHistory, StateRow};
 use crate::{
@@ -41,31 +40,6 @@ mod imp {
         fn class_init(klass: &mut Self::Class) {
             klass.set_css_name("event-row");
             klass.set_accessible_role(gtk::AccessibleRole::ListItem);
-
-            klass.install_action(
-                "event-row.enable-copy-image",
-                Some(&bool::static_variant_type()),
-                |obj, _, param| {
-                    let enable = param
-                        .and_then(glib::Variant::get::<bool>)
-                        .expect("The parameter should be a boolean");
-                    let imp = obj.imp();
-
-                    let Some(action_group) = imp.action_group.borrow().clone() else {
-                        error!("Could not change state of copy-image action: no action group");
-                        return;
-                    };
-                    let Some(action) = action_group.lookup_action("copy-image") else {
-                        error!("Could not change state of copy-image action: action not found");
-                        return;
-                    };
-                    let Some(action) = action.downcast_ref::<gio::SimpleAction>() else {
-                        error!("Could not change state of copy-image action: not a GSimpleAction");
-                        return;
-                    };
-                    action.set_enabled(enable);
-                },
-            );
         }
     }
 
@@ -295,7 +269,30 @@ mod imp {
                 let child = obj.child_or_default::<StateRow>();
                 child.set_event(event);
             } else {
-                let child = obj.child_or_default::<MessageRow>();
+                let child = obj.child_or_else::<MessageRow>(|| {
+                    let child = MessageRow::default();
+
+                    child.connect_texture_notify(clone!(
+                        #[weak(rename_to = imp)]
+                        self,
+                        move |child| {
+                            let Some(copy_image_action) = imp
+                                .action_group
+                                .borrow()
+                                .as_ref()
+                                .and_then(|action_group| action_group.lookup_action("copy-image"))
+                                .and_downcast::<gio::SimpleAction>()
+                            else {
+                                return;
+                            };
+
+                            copy_image_action.set_enabled(child.texture().is_some());
+                        }
+                    ));
+
+                    child
+                });
+
                 child.set_event(event);
             }
         }
@@ -333,6 +330,14 @@ mod imp {
             let action_group = self.event_actions_group();
             let has_context_menu = action_group.is_some();
 
+            if let Some(copy_image_action) = action_group
+                .as_ref()
+                .and_then(|action_group| action_group.lookup_action("copy-image"))
+                .and_downcast::<gio::SimpleAction>()
+            {
+                copy_image_action.set_enabled(self.texture().is_some());
+            }
+
             obj.insert_action_group("event", action_group.as_ref());
             self.action_group.replace(action_group);
             obj.set_has_context_menu(has_context_menu);

src/session/view/content/room_history/message_row/content.rs

@@ -41,7 +41,7 @@ pub enum ContentFormat {
 }
 
 mod imp {
-    use std::cell::Cell;
+    use std::{cell::Cell, marker::PhantomData};
 
     use super::*;
 
@@ -51,6 +51,12 @@ mod imp {
         /// The displayed format of the message.
         #[property(get, set = Self::set_format, explicit_notify, builder(ContentFormat::default()))]
         format: Cell<ContentFormat>,
+        /// The texture of the image preview displayed by the descendant of this
+        /// widget, if any.
+        #[property(get = Self::texture)]
+        texture: PhantomData<Option<gdk::Texture>>,
+        /// The widget with the visual media content of the event, if any.
+        pub(super) visual_media_widget: glib::WeakRef<MessageVisualMedia>,
     }
 
     #[glib::object_subclass]
@@ -76,6 +82,62 @@ mod imp {
             self.format.set(format);
             self.obj().notify_format();
         }
+
+        /// The texture of the image preview displayed by the descendant of this
+        /// widget, if any.
+        fn texture(&self) -> Option<gdk::Texture> {
+            self.visual_media_widget.upgrade()?.texture()
+        }
+
+        /// Update the current visual media widget if necessary.
+        pub(super) fn update_visual_media_widget(&self) {
+            let prev_widget = self.visual_media_widget.upgrade();
+            let current_widget = self.visual_media_widget();
+
+            if prev_widget == current_widget {
+                return;
+            }
+
+            let obj = self.obj();
+
+            if let Some(visual_media) = &current_widget {
+                visual_media.connect_texture_notify(clone!(
+                    #[weak]
+                    obj,
+                    move |_| {
+                        obj.notify_texture();
+                    }
+                ));
+            }
+            self.visual_media_widget.set(current_widget.as_ref());
+
+            let prev_texture = prev_widget.and_then(|visual_media| visual_media.texture());
+            let current_texture = current_widget.and_then(|visual_media| visual_media.texture());
+            if prev_texture != current_texture {
+                obj.notify_texture();
+            }
+        }
+
+        /// The widget with the visual media content of the event, if any.
+        ///
+        /// This allows to access the descendant content while discarding the
+        /// content of a related message, like a replied-to event, or the
+        /// caption of the event.
+        fn visual_media_widget(&self) -> Option<MessageVisualMedia> {
+            let mut child = self.obj().child()?;
+
+            // If it is a reply, the media is in the main content.
+            if let Some(reply) = child.downcast_ref::<MessageReply>() {
+                child = reply.content().child()?;
+            }
+
+            // If it is a caption, the media is the child of the caption.
+            if let Some(caption) = child.downcast_ref::<MessageCaption>() {
+                child = caption.child()?;
+            }
+
+            child.downcast::<MessageVisualMedia>().ok()
+        }
     }
 }
 
@@ -90,25 +152,9 @@ impl MessageContent {
         glib::Object::new()
     }
 
-    /// Access the widget with the visual media content of the event, if any.
-    ///
-    /// This allows to access the descendant content while discarding the
-    /// content of a related message, like a replied-to event, or the caption of
-    /// the event.
+    /// The widget with the visual media content of the event, if any.
     pub(crate) fn visual_media_widget(&self) -> Option<MessageVisualMedia> {
-        let mut child = BinExt::child(self)?;
-
-        // If it is a reply, the media is in the main content.
-        if let Some(reply) = child.downcast_ref::<MessageReply>() {
-            child = BinExt::child(reply.content())?;
-        }
-
-        // If it is a caption, the media is the child of the caption.
-        if let Some(caption) = child.downcast_ref::<MessageCaption>() {
-            child = caption.child()?;
-        }
-
-        child.downcast::<MessageVisualMedia>().ok()
+        self.imp().visual_media_widget.upgrade()
     }
 
     /// Update this widget to present the given `Event`.
@@ -136,7 +182,9 @@ impl MessageContent {
                     TimelineDetails::Error(error) => {
                         error!(
                             "Could not fetch replied to event '{}': {error}",
-                            event.reply_to_id().unwrap()
+                            event
+                                .reply_to_id()
+                                .expect("reply event should have replied-to event ID")
                         );
                     }
                     TimelineDetails::Ready(replied_to_event) => {
@@ -169,7 +217,9 @@ impl MessageContent {
                             event.transaction_id(),
                             event.event_id(),
                         );
-                        BinExt::set_child(self, Some(&reply));
+                        self.set_child(Some(&reply));
+
+                        self.imp().update_visual_media_widget();
 
                         return;
                     }
@@ -186,6 +236,8 @@ impl MessageContent {
             event.transaction_id(),
             event.event_id(),
         );
+
+        self.imp().update_visual_media_widget();
     }
 
     /// Update this widget to present the given related event.
@@ -209,11 +261,6 @@ impl MessageContent {
             },
         );
     }
-
-    /// Get the texture displayed by this widget, if any.
-    pub(crate) fn texture(&self) -> Option<gdk::Texture> {
-        self.visual_media_widget()?.texture()
-    }
 }
 
 impl IsABin for MessageContent {}

src/session/view/content/room_history/message_row/mod.rs

@@ -61,6 +61,10 @@ mod imp {
         /// This is ignored if this event doesn’t have a header.
         #[property(get = Self::show_header, set = Self::set_show_header, explicit_notify)]
         show_header: PhantomData<bool>,
+        /// The texture of the image preview displayed by the descendant of this
+        /// widget, if any.
+        #[property(get = Self::texture)]
+        texture: PhantomData<Option<gdk::Texture>>,
     }
 
     #[glib::object_subclass]
@@ -86,6 +90,7 @@ mod imp {
     impl ObjectImpl for MessageRow {
         fn constructed(&self) {
             self.parent_constructed();
+            let obj = self.obj();
 
             self.content.connect_format_notify(clone!(
                 #[weak(rename_to = imp)]
@@ -97,6 +102,13 @@ mod imp {
                     ));
                 }
             ));
+            self.content.connect_texture_notify(clone!(
+                #[weak]
+                obj,
+                move |_| {
+                    obj.notify_texture();
+                }
+            ));
 
             let system_settings = Application::default().system_settings();
             let system_settings_handler = system_settings.connect_clock_format_notify(clone!(
@@ -281,11 +293,6 @@ impl MessageRow {
     pub fn new() -> Self {
         glib::Object::new()
     }
-
-    /// Get the texture displayed by this widget, if any.
-    pub(crate) fn texture(&self) -> Option<gdk::Texture> {
-        self.imp().texture()
-    }
 }
 
 impl Default for MessageRow {

src/session/view/content/room_history/message_row/visual_media.rs

@@ -39,7 +39,10 @@ const PLACEHOLDER_PAGE: &str = "placeholder";
 const MEDIA_PAGE: &str = "media";
 
 mod imp {
-    use std::cell::{Cell, RefCell};
+    use std::{
+        cell::{Cell, RefCell},
+        marker::PhantomData,
+    };
 
     use glib::subclass::InitializingObject;
 
@@ -97,6 +100,9 @@ mod imp {
         /// The current video file, if any.
         file: RefCell<Option<File>>,
         paintable_animation_ref: RefCell<Option<CountedRef>>,
+        /// The texture of the current image preview, if any.
+        #[property(get = Self::texture)]
+        texture: PhantomData<Option<gdk::Texture>>,
     }
 
     #[glib::object_subclass]
@@ -239,6 +245,8 @@ mod imp {
         ///
         /// Removes the previous media child if one was set.
         fn set_media_child(&self, child: Option<&impl IsA<gtk::Widget>>) {
+            let prev_texture = self.texture();
+
             if let Some(prev_child) = self.stack.child_by_name(MEDIA_PAGE) {
                 self.stack.remove(&prev_child);
             }
@@ -246,6 +254,10 @@ mod imp {
             if let Some(child) = child {
                 self.stack.add_named(child, Some(MEDIA_PAGE));
             }
+
+            if self.texture() != prev_texture {
+                self.obj().notify_texture();
+            }
         }
 
         /// Set the state of the media.
@@ -409,6 +421,19 @@ mod imp {
             self.update_visible_page();
         }
 
+        /// The texture of the current image preview, if any.
+        fn texture(&self) -> Option<gdk::Texture> {
+            let paintable = self
+                .media_child::<gtk::Picture>()
+                .and_then(|p| p.paintable())?;
+
+            if let Some(paintable) = paintable.downcast_ref::<AnimatedImagePaintable>() {
+                paintable.current_texture()
+            } else {
+                paintable.downcast().ok()
+            }
+        }
+
         /// Set the visual media message to display.
         pub(super) fn set_media_message(
             &self,
@@ -627,11 +652,6 @@ mod imp {
                 return;
             }
 
-            // Disable the copy-image action while the image is loading.
-            if matches!(media_message, VisualMediaMessage::Image(_)) {
-                self.enable_copy_image_action(false);
-            }
-
             let scale_factor = self.obj().scale_factor();
             let settings = ThumbnailSettings {
                 dimensions: FrameDimensions::thumbnail_max_dimensions(scale_factor),
@@ -657,16 +677,18 @@ mod imp {
                 return;
             }
 
-            let child = if let Some(child) = self.media_child::<gtk::Picture>() {
-                child
+            let paintable = gdk::Paintable::from(image);
+
+            if let Some(child) = self.media_child::<gtk::Picture>() {
+                child.set_paintable(Some(&paintable));
+                self.obj().notify_texture();
             } else {
                 let child = gtk::Picture::builder()
                     .content_fit(gtk::ContentFit::ScaleDown)
                     .build();
+                child.set_paintable(Some(&paintable));
                 self.set_media_child(Some(&child));
-                child
-            };
-            child.set_paintable(Some(&gdk::Paintable::from(image)));
+            }
 
             if matches!(&media_message, VisualMediaMessage::Sticker(_)) {
                 self.overlay.remove_css_class("opaque-bg");
@@ -675,27 +697,6 @@ mod imp {
             }
 
             self.set_state(LoadingState::Ready);
-
-            // Enable the copy-image action now that the image is loaded.
-            if matches!(media_message, VisualMediaMessage::Image(_)) {
-                self.enable_copy_image_action(true);
-            }
-        }
-
-        /// Enable or disable the context menu action to copy the image.
-        fn enable_copy_image_action(&self, enable: bool) {
-            if self.compact.get() {
-                // In its compact form the message does not have actions.
-                return;
-            }
-
-            if self
-                .obj()
-                .activate_action("event-row.enable-copy-image", Some(&enable.to_variant()))
-                .is_err()
-            {
-                error!("Could not change state of copy-image action: `event-row.enable-copy-image` action not found");
-            }
         }
 
         /// Build the content for the video in the given media message.
@@ -828,20 +829,6 @@ impl MessageVisualMedia {
         self.imp()
             .set_media_message(media_message, room, format, cache_key);
     }
-
-    /// Get the texture displayed by this widget, if any.
-    pub(crate) fn texture(&self) -> Option<gdk::Texture> {
-        let paintable = self
-            .imp()
-            .media_child::<gtk::Picture>()
-            .and_then(|p| p.paintable())?;
-
-        if let Some(paintable) = paintable.downcast_ref::<AnimatedImagePaintable>() {
-            paintable.current_texture()
-        } else {
-            paintable.downcast().ok()
-        }
-    }
 }
 
 impl Default for MessageVisualMedia {