From da2e81fbdbe8d92baeb16b9c291278ccd1de52fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Commaille?= Date: Sun, 13 Apr 2025 12:01:14 +0200 Subject: [PATCH] timeline: Clean up TimelineItem API Remove the `selectable` property which is not used, and move properties about the header to Event, which is the only child type using it. --- src/session/model/room/timeline/event/mod.rs | 32 ++-- src/session/model/room/timeline/mod.rs | 21 ++- .../model/room/timeline/timeline_item.rs | 168 +----------------- 3 files changed, 35 insertions(+), 186 deletions(-) diff --git a/src/session/model/room/timeline/event/mod.rs b/src/session/model/room/timeline/event/mod.rs index a6690db1..9c830d5d 100644 --- a/src/session/model/room/timeline/event/mod.rs +++ b/src/session/model/room/timeline/event/mod.rs @@ -134,6 +134,9 @@ mod imp { /// Whether this event has any read receipt. #[property(get = Self::has_read_receipts)] has_read_receipts: PhantomData, + /// Whether this event should show its header in the room history. + #[property(get, set = Self::set_show_header, explicit_notify)] + show_header: Cell, } #[glib::object_subclass] @@ -160,19 +163,7 @@ mod imp { } } - impl TimelineItemImpl for Event { - fn can_hide_header(&self) -> bool { - self.item().content().can_show_header() - } - - fn event_sender_id(&self) -> Option { - Some(self.sender_id()) - } - - fn selectable(&self) -> bool { - true - } - } + impl TimelineItemImpl for Event {} impl Event { /// Set the underlying SDK timeline item. @@ -470,6 +461,16 @@ mod imp { fn has_read_receipts(&self) -> bool { self.read_receipts().n_items() > 0 } + + /// Set whether this event should show its header in the room history. + fn set_show_header(&self, show: bool) { + if self.show_header.get() == show { + return; + } + + self.show_header.set(show); + self.obj().notify_show_header(); + } } } @@ -638,6 +639,11 @@ impl Event { self.item().content().can_contain_at_room() } + /// Whether this event can show a header. + pub(crate) fn can_show_header(&self) -> bool { + self.item().content().can_show_header() + } + /// Get the ID of the event this event replies to, if any. pub(crate) fn reply_to_id(&self) -> Option { match self.item().content() { diff --git a/src/session/model/room/timeline/mod.rs b/src/session/model/room/timeline/mod.rs index e1908d2e..ab4da9eb 100644 --- a/src/session/model/room/timeline/mod.rs +++ b/src/session/model/room/timeline/mod.rs @@ -568,9 +568,9 @@ mod imp { let mut previous_sender = if pos > 0 { sdk_items .item(pos - 1) - .and_downcast::() - .filter(TimelineItem::can_hide_header) - .and_then(|item| item.event_sender_id()) + .and_downcast::() + .filter(Event::can_show_header) + .map(|event| event.sender_id()) } else { None }; @@ -580,15 +580,22 @@ mod imp { let Some(current) = self.item_at(i) else { break; }; + let Ok(current) = current.downcast::() else { + previous_sender = None; + continue; + }; - let current_sender = current.event_sender_id(); + let current_sender = current.sender_id(); - if !current.can_hide_header() { + if !current.can_show_header() { current.set_show_header(false); previous_sender = None; - } else if current_sender != previous_sender { + } else if previous_sender + .as_ref() + .is_none_or(|previous_sender| current_sender != *previous_sender) + { current.set_show_header(true); - previous_sender = current_sender; + previous_sender = Some(current_sender); } else { current.set_show_header(false); } diff --git a/src/session/model/room/timeline/timeline_item.rs b/src/session/model/room/timeline/timeline_item.rs index df950b5d..72d2d27b 100644 --- a/src/session/model/room/timeline/timeline_item.rs +++ b/src/session/model/room/timeline/timeline_item.rs @@ -1,46 +1,24 @@ use gtk::{glib, prelude::*, subclass::prelude::*}; use matrix_sdk_ui::timeline::{TimelineItem as SdkTimelineItem, TimelineItemKind}; -use ruma::OwnedUserId; use tracing::error; use super::{Event, Timeline, VirtualItem}; use crate::session::model::Room; mod imp { - use std::{ - cell::{Cell, OnceCell, RefCell}, - marker::PhantomData, - }; + use std::cell::{OnceCell, RefCell}; use super::*; #[repr(C)] pub struct TimelineItemClass { parent_class: glib::object::ObjectClass, - pub(super) selectable: fn(&super::TimelineItem) -> bool, - pub(super) can_hide_header: fn(&super::TimelineItem) -> bool, - pub(super) event_sender_id: fn(&super::TimelineItem) -> Option, } unsafe impl ClassStruct for TimelineItemClass { type Type = TimelineItem; } - pub(super) fn timeline_item_selectable(this: &super::TimelineItem) -> bool { - let klass = this.class(); - (klass.as_ref().selectable)(this) - } - - pub(super) fn timeline_item_can_hide_header(this: &super::TimelineItem) -> bool { - let klass = this.class(); - (klass.as_ref().can_hide_header)(this) - } - - pub(super) fn timeline_item_event_sender_id(this: &super::TimelineItem) -> Option { - let klass = this.class(); - (klass.as_ref().event_sender_id)(this) - } - #[derive(Debug, Default, glib::Properties)] #[properties(wrapper_type = super::TimelineItem)] pub struct TimelineItem { @@ -50,26 +28,6 @@ mod imp { /// A unique ID for this `TimelineItem` in the local timeline. #[property(get, construct_only)] timeline_id: RefCell, - /// Whether this `TimelineItem` is selectable. - /// - /// Defaults to `false`. - #[property(get = Self::selectable)] - selectable: PhantomData, - /// Whether this `TimelineItem` should show its header. - /// - /// Defaults to `false`. - #[property(get, set = Self::set_show_header, explicit_notify)] - show_header: Cell, - /// Whether this `TimelineItem` is allowed to hide its header. - /// - /// Defaults to `false`. - #[property(get = Self::can_hide_header)] - can_hide_header: PhantomData, - /// If this is a Matrix event, the sender of the event. - /// - /// Defaults to `None`. - #[property(get = Self::event_sender_id)] - event_sender_id: PhantomData>, } #[glib::object_subclass] @@ -82,39 +40,6 @@ mod imp { #[glib::derived_properties] impl ObjectImpl for TimelineItem {} - - impl TimelineItem { - /// Whether this `TimelineItem` is selectable. - /// - /// Defaults to `false`. - fn selectable(&self) -> bool { - imp::timeline_item_selectable(&self.obj()) - } - - /// Set whether this `TimelineItem` should show its header. - fn set_show_header(&self, show: bool) { - if self.show_header.get() == show { - return; - } - - self.show_header.set(show); - self.obj().notify_show_header(); - } - - /// Whether this `TimelineItem` is allowed to hide its header. - /// - /// Defaults to `false`. - fn can_hide_header(&self) -> bool { - imp::timeline_item_can_hide_header(&self.obj()) - } - - /// If this is a Matrix event, the sender of the event. - /// - /// Defaults to `None`. - fn event_sender_id(&self) -> Option { - imp::timeline_item_event_sender_id(&self.obj()).map(Into::into) - } - } } glib::wrapper! { @@ -184,29 +109,6 @@ pub(crate) trait TimelineItemExt: 'static { /// A unique ID for this `TimelineItem` in the local timeline. fn timeline_id(&self) -> String; - - /// Whether this `TimelineItem` is selectable. - /// - /// Defaults to `false`. - fn selectable(&self) -> bool; - - /// Whether this `TimelineItem` should show its header. - /// - /// Defaults to `false`. - fn show_header(&self) -> bool; - - /// Set whether this `TimelineItem` should show its header. - fn set_show_header(&self, show: bool); - - /// Whether this `TimelineItem` is allowed to hide its header. - /// - /// Defaults to `false`. - fn can_hide_header(&self) -> bool; - - /// If this is a Matrix event, the sender of the event. - /// - /// Defaults to `None`. - fn event_sender_id(&self) -> Option; } impl> TimelineItemExt for O { @@ -217,26 +119,6 @@ impl> TimelineItemExt for O { fn timeline_id(&self) -> String { self.upcast_ref().timeline_id() } - - fn selectable(&self) -> bool { - self.upcast_ref().selectable() - } - - fn show_header(&self) -> bool { - self.upcast_ref().show_header() - } - - fn set_show_header(&self, show: bool) { - self.upcast_ref().set_show_header(show); - } - - fn can_hide_header(&self) -> bool { - self.upcast_ref().can_hide_header() - } - - fn event_sender_id(&self) -> Option { - imp::timeline_item_event_sender_id(self.upcast_ref()) - } } /// Public trait that must be implemented for everything that derives from @@ -244,19 +126,7 @@ impl> TimelineItemExt for O { /// /// Overriding a method from this Trait overrides also its behavior in /// `TimelineItemExt`. -pub(crate) trait TimelineItemImpl: ObjectImpl { - fn selectable(&self) -> bool { - false - } - - fn can_hide_header(&self) -> bool { - false - } - - fn event_sender_id(&self) -> Option { - None - } -} +pub(crate) trait TimelineItemImpl: ObjectImpl {} // Make `TimelineItem` subclassable. unsafe impl IsSubclassable for TimelineItem @@ -266,39 +136,5 @@ where { fn class_init(class: &mut glib::Class) { Self::parent_class_init::(class.upcast_ref_mut()); - - let klass = class.as_mut(); - - klass.selectable = selectable_trampoline::; - klass.can_hide_header = can_hide_header_trampoline::; - klass.event_sender_id = event_sender_id_trampoline::; } } - -// Virtual method implementation trampolines. -fn selectable_trampoline(this: &TimelineItem) -> bool -where - T: ObjectSubclass + TimelineItemImpl, - T::Type: IsA, -{ - let this = this.downcast_ref::().unwrap(); - this.imp().selectable() -} - -fn can_hide_header_trampoline(this: &TimelineItem) -> bool -where - T: ObjectSubclass + TimelineItemImpl, - T::Type: IsA, -{ - let this = this.downcast_ref::().unwrap(); - this.imp().can_hide_header() -} - -fn event_sender_id_trampoline(this: &TimelineItem) -> Option -where - T: ObjectSubclass + TimelineItemImpl, - T::Type: IsA, -{ - let this = this.downcast_ref::().unwrap(); - this.imp().event_sender_id() -}