From 2ff346f3fd1db95b4aae65592bf44755441c44f3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Commaille?= <zecakeh@tedomum.fr>
Date: Sat, 18 Jan 2025 15:10:10 +0100
Subject: [PATCH] account-switcher: Refactor and fix visibility

---
 .../account_switcher_button.rs                | 158 ++++++++----------
 .../account_switcher_button.ui                |  12 ++
 .../account_switcher_popover.rs               | 115 +++++++------
 src/account_switcher/avatar_with_selection.rs |  20 +--
 src/account_switcher/mod.rs                   |   2 +-
 src/account_switcher/session_item.rs          |  54 +++---
 6 files changed, 174 insertions(+), 187 deletions(-)

diff --git a/src/account_switcher/account_switcher_button.rs b/src/account_switcher/account_switcher_button.rs
index 08bb8937..5017317f 100644
--- a/src/account_switcher/account_switcher_button.rs
+++ b/src/account_switcher/account_switcher_button.rs
@@ -1,9 +1,4 @@
-use gtk::{
-    glib::{self, clone, closure},
-    prelude::*,
-    subclass::prelude::*,
-    CompositeTemplate,
-};
+use gtk::{glib, glib::clone, prelude::*, subclass::prelude::*, CompositeTemplate};
 
 use super::AccountSwitcherPopover;
 use crate::{
@@ -14,17 +9,17 @@ use crate::{
 };
 
 mod imp {
-    use std::cell::RefCell;
-
     use glib::subclass::InitializingObject;
 
     use super::*;
 
-    #[derive(Debug, Default, CompositeTemplate)]
+    #[derive(Debug, Default, CompositeTemplate, glib::Properties)]
     #[template(resource = "/org/gnome/Fractal/ui/account_switcher/account_switcher_button.ui")]
+    #[properties(wrapper_type = super::AccountSwitcherButton)]
     pub struct AccountSwitcherButton {
-        pub popover: BoundObjectWeakRef<AccountSwitcherPopover>,
-        pub watch: RefCell<Option<gtk::ExpressionWatch>>,
+        /// The popover of this button.
+        #[property(get, set = Self::set_popover, explicit_notify, nullable)]
+        popover: BoundObjectWeakRef<AccountSwitcherPopover>,
     }
 
     #[glib::object_subclass]
@@ -38,6 +33,7 @@ mod imp {
             SessionInfo::ensure_type();
 
             Self::bind_template(klass);
+            Self::bind_template_callbacks(klass);
             TemplateCallbacks::bind_template_callbacks(klass);
         }
 
@@ -46,102 +42,86 @@ mod imp {
         }
     }
 
-    impl ObjectImpl for AccountSwitcherButton {
-        fn constructed(&self) {
-            self.parent_constructed();
-            let obj = self.obj();
-
-            obj.connect_toggled(|obj| {
-                obj.handle_toggled();
-            });
-
-            let watch = obj
-                .property_expression("root")
-                .chain_property::<Window>("session-selection")
-                .chain_property::<gtk::SingleSelection>("n-items")
-                .chain_closure::<bool>(closure!(|_: Option<glib::Object>, n_items: u32| {
-                    n_items > 0
-                }))
-                .bind(&*obj, "visible", glib::Object::NONE);
-            self.watch.replace(Some(watch));
-        }
-
-        fn dispose(&self) {
-            if let Some(watch) = self.watch.take() {
-                watch.unwatch();
-            }
-        }
-    }
+    #[glib::derived_properties]
+    impl ObjectImpl for AccountSwitcherButton {}
 
     impl WidgetImpl for AccountSwitcherButton {}
     impl ButtonImpl for AccountSwitcherButton {}
     impl ToggleButtonImpl for AccountSwitcherButton {}
-}
 
-glib::wrapper! {
-    /// A button showing the currently selected account and opening the account switcher popover.
-    pub struct AccountSwitcherButton(ObjectSubclass<imp::AccountSwitcherButton>)
-        @extends gtk::Widget, gtk::Button, gtk::ToggleButton, @implements gtk::Accessible;
-}
+    #[gtk::template_callbacks]
+    impl AccountSwitcherButton {
+        /// Set the popover of this button.
+        fn set_popover(&self, popover: Option<&AccountSwitcherPopover>) {
+            let old_popover = self.popover.obj();
 
-#[gtk::template_callbacks]
-impl AccountSwitcherButton {
-    pub fn new() -> Self {
-        glib::Object::new()
-    }
-
-    pub fn popover(&self) -> Option<AccountSwitcherPopover> {
-        self.imp().popover.obj()
-    }
+            if old_popover.as_ref() == popover {
+                return;
+            }
+            let obj = self.obj();
 
-    pub fn set_popover(&self, popover: Option<&AccountSwitcherPopover>) {
-        let old_popover = self.popover();
+            // Reset the state.
+            if let Some(popover) = old_popover {
+                popover.unparent();
+            }
+            self.popover.disconnect_signals();
+            obj.set_active(false);
+
+            if let Some(popover) = popover {
+                // We need to remove the popover from the previous button, if any.
+                if let Some(parent) = popover
+                    .parent()
+                    .and_downcast::<super::AccountSwitcherButton>()
+                {
+                    parent.set_popover(None::<AccountSwitcherPopover>);
+                }
 
-        if old_popover.as_ref() == popover {
-            return;
-        }
+                let closed_handler = popover.connect_closed(clone!(
+                    #[weak]
+                    obj,
+                    move |_| {
+                        obj.set_active(false);
+                    }
+                ));
 
-        let imp = self.imp();
+                popover.set_parent(&*obj);
+                self.popover.set(popover, vec![closed_handler]);
+            }
 
-        // Reset the state.
-        if let Some(popover) = old_popover {
-            popover.unparent();
+            obj.notify_popover();
         }
-        imp.popover.disconnect_signals();
-        self.set_active(false);
 
-        if let Some(popover) = popover {
-            // We need to remove the popover from the previous button, if any.
-            if let Some(parent) = popover.parent().and_downcast::<AccountSwitcherButton>() {
-                parent.set_popover(None);
-            }
+        /// Toggle the popover of this button.
+        #[template_callback]
+        fn toggle_popover(&self) {
+            let obj = self.obj();
 
-            let closed_handler = popover.connect_closed(clone!(
-                #[weak(rename_to = obj)]
-                self,
-                move |_| {
-                    obj.set_active(false);
-                }
-            ));
+            if obj.is_active() {
+                let Some(window) = obj.root().and_downcast::<Window>() else {
+                    return;
+                };
+
+                let popover = window.account_switcher();
+                self.set_popover(Some(popover));
 
-            popover.set_parent(self);
-            imp.popover.set(popover, vec![closed_handler]);
+                popover.popup();
+            } else if let Some(popover) = self.popover.obj() {
+                popover.popdown();
+            }
         }
     }
+}
 
-    fn handle_toggled(&self) {
-        if self.is_active() {
-            let Some(window) = self.root().and_downcast::<Window>() else {
-                return;
-            };
-
-            let popover = window.account_switcher();
-            self.set_popover(Some(popover));
+glib::wrapper! {
+    /// A button showing the currently selected session and opening the account switcher popover.
+    pub struct AccountSwitcherButton(ObjectSubclass<imp::AccountSwitcherButton>)
+        @extends gtk::Widget, gtk::Button, gtk::ToggleButton, @implements gtk::Accessible;
+}
 
-            popover.popup();
-        } else if let Some(popover) = self.popover() {
-            popover.popdown();
-        }
+#[gtk::template_callbacks]
+impl AccountSwitcherButton {
+    pub fn new() -> Self {
+        glib::Object::new()
     }
 }
 
diff --git a/src/account_switcher/account_switcher_button.ui b/src/account_switcher/account_switcher_button.ui
index 073f17a8..a9921ad3 100644
--- a/src/account_switcher/account_switcher_button.ui
+++ b/src/account_switcher/account_switcher_button.ui
@@ -1,6 +1,18 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <interface>
   <template class="AccountSwitcherButton" parent="GtkToggleButton">
+    <signal name="toggled" handler="toggle_popover" swapped="yes"/>
+    <binding name="visible">
+      <closure type="gboolean" function="invert_boolean">
+        <closure type="gboolean" function="guint_is_zero">
+          <lookup name="n-items" type="GtkSingleSelection">
+            <lookup name="session-selection" type="Window">
+              <lookup name="root">AccountSwitcherButton</lookup>
+            </lookup>
+          </lookup>
+        </closure>
+      </closure>
+    </binding>
     <binding name="tooltip-text">
       <lookup name="user-id-string" type="SessionInfo">
         <lookup name="selected-item" type="GtkSingleSelection">
diff --git a/src/account_switcher/account_switcher_popover.rs b/src/account_switcher/account_switcher_popover.rs
index b9635c3d..2e3f699a 100644
--- a/src/account_switcher/account_switcher_popover.rs
+++ b/src/account_switcher/account_switcher_popover.rs
@@ -18,12 +18,12 @@ mod imp {
     #[properties(wrapper_type = super::AccountSwitcherPopover)]
     pub struct AccountSwitcherPopover {
         #[template_child]
-        pub sessions: TemplateChild<gtk::ListBox>,
+        sessions: TemplateChild<gtk::ListBox>,
         /// The model containing the logged-in sessions selection.
         #[property(get, set = Self::set_session_selection, explicit_notify, nullable)]
-        pub session_selection: BoundObjectWeakRef<gtk::SingleSelection>,
+        session_selection: BoundObjectWeakRef<gtk::SingleSelection>,
         /// The selected row.
-        pub selected_row: glib::WeakRef<SessionItemRow>,
+        selected_row: glib::WeakRef<SessionItemRow>,
     }
 
     #[glib::object_subclass]
@@ -34,7 +34,7 @@ mod imp {
 
         fn class_init(klass: &mut Self::Class) {
             Self::bind_template(klass);
-            Self::Type::bind_template_callbacks(klass);
+            Self::bind_template_callbacks(klass);
 
             klass.install_action("account-switcher.close", None, |obj, _, _| {
                 obj.popdown();
@@ -52,97 +52,94 @@ mod imp {
     impl WidgetImpl for AccountSwitcherPopover {}
     impl PopoverImpl for AccountSwitcherPopover {}
 
+    #[gtk::template_callbacks]
     impl AccountSwitcherPopover {
         /// Set the model containing the logged-in sessions selection.
         fn set_session_selection(&self, selection: Option<&gtk::SingleSelection>) {
             if selection == self.session_selection.obj().as_ref() {
                 return;
             }
-            let obj = self.obj();
 
             self.session_selection.disconnect_signals();
 
             self.sessions.bind_model(selection, |session| {
-                let row = SessionItemRow::new(session.downcast_ref().unwrap());
+                let row = SessionItemRow::new(
+                    session
+                        .downcast_ref()
+                        .expect("sessions list box item should be a Session"),
+                );
                 row.upcast()
             });
 
             if let Some(selection) = selection {
                 let selected_handler = selection.connect_selected_item_notify(clone!(
-                    #[weak]
-                    obj,
+                    #[weak(rename_to = imp)]
+                    self,
                     move |selection| {
-                        obj.update_selected_item(selection.selected());
+                        imp.update_selected_item(selection.selected());
                     }
                 ));
-                obj.update_selected_item(selection.selected());
+                self.update_selected_item(selection.selected());
 
                 self.session_selection
                     .set(selection, vec![selected_handler]);
             }
 
-            obj.notify_session_selection();
+            self.obj().notify_session_selection();
+        }
+
+        /// Select the given row in the session list.
+        #[template_callback]
+        fn select_row(&self, row: &gtk::ListBoxRow) {
+            self.obj().popdown();
+
+            let Some(selection) = self.session_selection.obj() else {
+                return;
+            };
+
+            let index = row.index().try_into().expect("selected row has an index");
+            selection.set_selected(index);
+        }
+
+        /// Update the selected item in the session list.
+        fn update_selected_item(&self, selected: u32) {
+            let old_selected = self.selected_row.upgrade();
+            let new_selected = if selected == gtk::INVALID_LIST_POSITION {
+                None
+            } else {
+                let index = selected.try_into().expect("item index should fit into i32");
+                self.sessions
+                    .row_at_index(index)
+                    .and_downcast::<SessionItemRow>()
+            };
+
+            if old_selected == new_selected {
+                return;
+            }
+
+            if let Some(row) = &old_selected {
+                row.set_selected(false);
+            }
+            if let Some(row) = &new_selected {
+                row.set_selected(true);
+            }
+
+            self.selected_row.set(new_selected.as_ref());
         }
     }
 }
 
 glib::wrapper! {
+    /// A popover allowing to switch between the available sessions, to open their
+    /// account settings, or to log into a new account.
     pub struct AccountSwitcherPopover(ObjectSubclass<imp::AccountSwitcherPopover>)
         @extends gtk::Widget, gtk::Popover, @implements gtk::Accessible;
 }
 
-#[gtk::template_callbacks]
 impl AccountSwitcherPopover {
     pub fn new() -> Self {
         glib::Object::new()
     }
-
-    fn selected_row(&self) -> Option<SessionItemRow> {
-        self.imp().selected_row.upgrade()
-    }
-
-    /// Select the given row in the session list.
-    #[template_callback]
-    fn select_row(&self, row: &gtk::ListBoxRow) {
-        self.popdown();
-
-        let Some(selection) = self.session_selection() else {
-            return;
-        };
-
-        let index = row.index().try_into().expect("selected row has an index");
-        selection.set_selected(index);
-    }
-
-    /// Update the selected item in the session list.
-    fn update_selected_item(&self, selected: u32) {
-        let imp = self.imp();
-
-        let old_selected = self.selected_row();
-        let new_selected = if selected == gtk::INVALID_LIST_POSITION {
-            None
-        } else {
-            let index = selected
-                .try_into()
-                .expect("item index always fits into i32");
-            imp.sessions
-                .row_at_index(index)
-                .and_downcast::<SessionItemRow>()
-        };
-
-        if old_selected == new_selected {
-            return;
-        }
-
-        if let Some(row) = &old_selected {
-            row.set_selected(false);
-        }
-        if let Some(row) = &new_selected {
-            row.set_selected(true);
-        }
-
-        imp.selected_row.set(new_selected.as_ref());
-    }
 }
 
 impl Default for AccountSwitcherPopover {
diff --git a/src/account_switcher/avatar_with_selection.rs b/src/account_switcher/avatar_with_selection.rs
index 43d32a02..30557788 100644
--- a/src/account_switcher/avatar_with_selection.rs
+++ b/src/account_switcher/avatar_with_selection.rs
@@ -1,5 +1,5 @@
-use adw::subclass::prelude::*;
-use gtk::{glib, prelude::*, CompositeTemplate};
+use adw::{prelude::*, subclass::prelude::*};
+use gtk::{glib, CompositeTemplate};
 
 use crate::components::{Avatar, AvatarData};
 
@@ -15,18 +15,18 @@ mod imp {
     #[properties(wrapper_type = super::AvatarWithSelection)]
     pub struct AvatarWithSelection {
         #[template_child]
-        pub child_avatar: TemplateChild<Avatar>,
+        child_avatar: TemplateChild<Avatar>,
         #[template_child]
-        pub checkmark: TemplateChild<gtk::Image>,
+        checkmark: TemplateChild<gtk::Image>,
         /// The [`AvatarData`] displayed by this widget.
         #[property(get = Self::data, set = Self::set_data, explicit_notify, nullable)]
-        pub data: PhantomData<Option<AvatarData>>,
+        data: PhantomData<Option<AvatarData>>,
         /// The size of the Avatar.
         #[property(get = Self::size, set = Self::set_size, minimum = -1, default = -1)]
-        pub size: PhantomData<i32>,
+        size: PhantomData<i32>,
         /// Whether this avatar is selected.
         #[property(get = Self::is_selected, set = Self::set_selected, explicit_notify)]
-        pub selected: PhantomData<bool>,
+        selected: PhantomData<bool>,
     }
 
     #[glib::object_subclass]
@@ -103,7 +103,7 @@ mod imp {
 }
 
 glib::wrapper! {
-    /// A widget displaying an `Avatar` for a `Room` or `User` and an optional selected effect.
+    /// A widget displaying an [`Avatar`] and an optional selected effect.
     pub struct AvatarWithSelection(ObjectSubclass<imp::AvatarWithSelection>)
         @extends gtk::Widget, adw::Bin, @implements gtk::Accessible;
 }
@@ -112,8 +112,4 @@ impl AvatarWithSelection {
     pub fn new() -> Self {
         glib::Object::new()
     }
-
-    pub fn avatar(&self) -> &Avatar {
-        &self.imp().child_avatar
-    }
 }
diff --git a/src/account_switcher/mod.rs b/src/account_switcher/mod.rs
index fd9aac96..c0489317 100644
--- a/src/account_switcher/mod.rs
+++ b/src/account_switcher/mod.rs
@@ -3,7 +3,7 @@ mod account_switcher_popover;
 mod avatar_with_selection;
 mod session_item;
 
-pub use self::{
+pub(crate) use self::{
     account_switcher_button::AccountSwitcherButton,
     account_switcher_popover::AccountSwitcherPopover,
 };
diff --git a/src/account_switcher/session_item.rs b/src/account_switcher/session_item.rs
index eba623e8..48b4ebcf 100644
--- a/src/account_switcher/session_item.rs
+++ b/src/account_switcher/session_item.rs
@@ -1,4 +1,4 @@
-use gtk::{self, glib, prelude::*, subclass::prelude::*, CompositeTemplate};
+use gtk::{glib, prelude::*, subclass::prelude::*, CompositeTemplate};
 
 use super::avatar_with_selection::AvatarWithSelection;
 use crate::{
@@ -20,22 +20,22 @@ mod imp {
     #[properties(wrapper_type = super::SessionItemRow)]
     pub struct SessionItemRow {
         #[template_child]
-        pub avatar: TemplateChild<AvatarWithSelection>,
+        avatar: TemplateChild<AvatarWithSelection>,
         #[template_child]
-        pub display_name: TemplateChild<gtk::Label>,
+        display_name: TemplateChild<gtk::Label>,
         #[template_child]
-        pub user_id: TemplateChild<gtk::Label>,
+        user_id: TemplateChild<gtk::Label>,
         #[template_child]
-        pub state_stack: TemplateChild<gtk::Stack>,
+        state_stack: TemplateChild<gtk::Stack>,
         #[template_child]
-        pub error_image: TemplateChild<gtk::Image>,
+        error_image: TemplateChild<gtk::Image>,
         /// The session this item represents.
         #[property(get, set = Self::set_session, explicit_notify)]
-        pub session: glib::WeakRef<SessionInfo>,
-        pub user_bindings: RefCell<Vec<glib::Binding>>,
+        session: glib::WeakRef<SessionInfo>,
+        user_bindings: RefCell<Vec<glib::Binding>>,
         /// Whether this session is selected.
         #[property(get = Self::is_selected, set = Self::set_selected, explicit_notify)]
-        pub selected: PhantomData<bool>,
+        selected: PhantomData<bool>,
     }
 
     #[glib::object_subclass]
@@ -46,7 +46,7 @@ mod imp {
 
         fn class_init(klass: &mut Self::Class) {
             Self::bind_template(klass);
-            Self::Type::bind_template_callbacks(klass);
+            Self::bind_template_callbacks(klass);
         }
 
         fn instance_init(obj: &InitializingObject<Self>) {
@@ -66,6 +66,7 @@ mod imp {
     impl WidgetImpl for SessionItemRow {}
     impl ListBoxRowImpl for SessionItemRow {}
 
+    #[gtk::template_callbacks]
     impl SessionItemRow {
         /// Whether this session is selected.
         fn is_selected(&self) -> bool {
@@ -145,6 +146,23 @@ mod imp {
             self.session.set(session);
             self.obj().notify_session();
         }
+
+        /// Show the account settings for the session of this row.
+        #[template_callback]
+        fn show_account_settings(&self) {
+            let Some(session) = self.session.upgrade() else {
+                return;
+            };
+
+            let obj = self.obj();
+            obj.activate_action("account-switcher.close", None)
+                .expect("`account-switcher.close` action should exist");
+            obj.activate_action(
+                "win.open-account-settings",
+                Some(&session.session_id().to_variant()),
+            )
+            .expect("`win.open-account-settings` action should exist");
+        }
     }
 }
 
@@ -154,24 +172,8 @@ glib::wrapper! {
         @extends gtk::Widget, gtk::ListBoxRow, @implements gtk::Accessible;
 }
 
-#[gtk::template_callbacks]
 impl SessionItemRow {
     pub fn new(session: &SessionInfo) -> Self {
         glib::Object::builder().property("session", session).build()
     }
-
-    #[template_callback]
-    pub fn show_account_settings(&self) {
-        let Some(session) = self.session() else {
-            return;
-        };
-
-        self.activate_action("account-switcher.close", None)
-            .unwrap();
-        self.activate_action(
-            "win.open-account-settings",
-            Some(&session.session_id().to_variant()),
-        )
-        .unwrap();
-    }
 }