Avoid cloning Config and mutable state

Prior to this commit, configs are shared between at least two structs without any nice way to coordinate updates. For the background config, this could lead to expensive clones of an internal HashMap or wrapping the config in an Arc and a Mutex. The latter could lead to lock contention if multiple interfaces are called that need the config. This commit makes use of the portal's messaging passing infrastructure to handle configs cleaner (at least for the background for now). Interface implementations may stall or deadlock with zbus if the function takes `&mut self`. Another benefit of this commit is that the interfaces for the background are now all immutable.
pop-os · Aug 4, 2024 · 5d6e3f2 · 5d6e3f2
1 parent e99d456
commit 5d6e3f2
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 31 deletions.
diff --git a/src/app.rs b/src/app.rs
@@ -196,6 +196,28 @@ impl cosmic::Application for CosmicPortal {
                 subscription::Event::Background(args) => {
                     background::update_args(self, args).map(cosmic::app::Message::App)
                 }
+                subscription::Event::BackgroundGetAppPerm(app_id, tx) => {
+                    let perm = match self.config.background.default_perm {
+                        config::background::PermissionDialog::Allow => {
+                            background::ConfigAppPerm::DefaultAllow
+                        }
+                        config::background::PermissionDialog::Deny => {
+                            background::ConfigAppPerm::DefaultDeny
+                        }
+                        _ => match self.config.background.apps.get(&app_id) {
+                            Some(true) => background::ConfigAppPerm::UserAllow,
+                            Some(false) => background::ConfigAppPerm::UserDeny,
+                            None => background::ConfigAppPerm::Unset,
+                        },
+                    };
+                    cosmic::Command::perform(
+                        async move {
+                            let _ = tx.send(perm).await;
+                            cosmic::app::message::none()
+                        },
+                        |x| x,
+                    )
+                }
                 subscription::Event::Config(config) => self.update(Msg::ConfigSubUpdate(config)),
                 subscription::Event::Accent(_)
                 | subscription::Event::IsDark(_)

diff --git a/src/background.rs b/src/background.rs
@@ -2,7 +2,7 @@
 
 use std::collections::{hash_map::Entry, HashMap};
 
-use cosmic::{iced::window, iced_runtime::command::Action, widget};
+use cosmic::{iced::window, widget};
 use futures::{FutureExt, TryFutureExt};
 use tokio::sync::mpsc::Sender;
 use zbus::{fdo, object_server::SignalContext, zvariant};
@@ -21,17 +21,12 @@ const POP_SHELL_PATH: &str = "/com.System76.PopShell";
 /// https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.impl.portal.Background.html
 pub struct Background {
     tx: Sender<subscription::Event>,
-    config: config::background::Background,
 }
 
 impl Background {
-    pub fn new(tx: Sender<subscription::Event>) -> Self {
-        // FIXME: Will need to change this to handle external updates (e.g. from cosmic-settings)
-        let config = config::Config::load().0.background;
-        Self { tx, config }
+    pub const fn new(tx: Sender<subscription::Event>) -> Self {
+        Self { tx }
     }
-
-    // fn dialog(name: &str) -> widget::Dialog<'_, PermissionResponse> {}
 }
 
 #[zbus::interface(name = "org.freedesktop.impl.portal.Background")]
@@ -41,14 +36,14 @@ impl Background {
         &self,
         // #[zbus(connection)] connection: &zbus::Connection,
     ) -> fdo::Result<HashMap<String, AppStatus>> {
-        // TODO: How do I get running programs?
+        // TODO: Subscribe to Wayland window open events for running apps
         log::warn!("[background] GetAppState is currently unimplemented");
         Ok(HashMap::default())
     }
 
     /// Notifies the user that an app is running in the background
     async fn notify_background(
-        &mut self,
+        &self,
         // #[zbus(connection)] connection: &zbus::Connection,
         #[zbus(signal_context)] context: SignalContext<'_>,
         handle: zvariant::ObjectPath<'_>,
@@ -57,30 +52,43 @@ impl Background {
     ) -> PortalResponse<NotifyBackgroundResult> {
         log::debug!("[background] Request handle: {handle:?}");
 
-        match self.config.apps.entry(app_id) {
+        // Request only what's needed to avoid cloning and receiving the entire config
+        // This is also cleaner than storing the config because it's difficult to keep it
+        // updated without synch primitives and we also avoid &mut self
+        let (tx, mut rx) = tokio::sync::mpsc::channel(1);
+        let config = self
+            .tx
+            .send(subscription::Event::BackgroundGetAppPerm(
+                tx,
+                app_id.clone(),
+            ))
+            .inspect_err(|e| {
+                log::error!("[background] Failed receiving background config from main app {e:?}")
+            })
+            .and_then(|_| rx.recv())
+            .unwrap_or_default()
+            .await;
+
+        match config {
             // Skip dialog based on default response set in configs
-            _ if self.config.default_perm == PermissionDialog::Allow => {
+            ConfigAppPerm::DefaultAllow => {
                 log::debug!("[background] AUTO ALLOW {name} based on default permission");
                 PortalResponse::Success(NotifyBackgroundResult {
                     result: PermissionResponse::Allow,
                 })
             }
-            _ if self.config.default_perm == PermissionDialog::Deny => {
+            ConfigAppPerm::DefaultDeny => {
                 log::debug!("[background] AUTO DENY {name} based on default permission");
                 PortalResponse::Success(NotifyBackgroundResult {
                     result: PermissionResponse::Deny,
                 })
             }
             // Dialog
-            Entry::Vacant(entry) => {
-                log::debug!(
-                    "[background] Requesting permission for {} ({name})",
-                    entry.key()
-                );
+            ConfigAppPerm::Unset => {
+                log::debug!("[background] Requesting permission for {app_id} ({name})",);
 
                 let handle = handle.to_owned();
                 let id = window::Id::unique();
-                let app_id = entry.key().clone();
                 let (tx, mut rx) = tokio::sync::mpsc::channel(1);
                 self.tx
                     .send(subscription::Event::Background(Args {
@@ -99,20 +107,14 @@ impl Background {
                     .await
             }
             // We asked the user about this app already
-            Entry::Occupied(entry) if *entry.get() => {
-                log::debug!(
-                    "[background] AUTO ALLOW {} ({name}) based on cached response",
-                    entry.key()
-                );
+            ConfigAppPerm::UserAllow => {
+                log::debug!("[background] AUTO ALLOW {app_id} ({name}) based on cached response");
                 PortalResponse::Success(NotifyBackgroundResult {
                     result: PermissionResponse::Allow,
                 })
             }
-            Entry::Occupied(entry) => {
-                log::debug!(
-                    "[background] AUTO DENY {} ({name}) based on cached response",
-                    entry.key()
-                );
+            ConfigAppPerm::UserDeny => {
+                log::debug!("[background] AUTO DENY {app_id} ({name}) based on cached response");
                 PortalResponse::Success(NotifyBackgroundResult {
                     result: PermissionResponse::Deny,
                 })
@@ -176,6 +178,18 @@ pub enum PermissionResponse {
     AllowOnce,
 }
 
+/// Evaluated permissions from background config
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub enum ConfigAppPerm {
+    DefaultAllow,
+    DefaultDeny,
+    #[default]
+    Unset,
+    UserAllow,
+    UserDeny,
+}
+
+/// Background permissions dialog state
 #[derive(Clone)]
 pub struct Args {
     pub handle: zvariant::ObjectPath<'static>,
@@ -184,6 +198,7 @@ pub struct Args {
     tx: Sender<PortalResponse<NotifyBackgroundResult>>,
 }
 
+/// Background permissions dialog response
 #[derive(Debug, Clone)]
 pub enum Msg {
     Response {

diff --git a/src/subscription.rs b/src/subscription.rs
@@ -7,7 +7,7 @@ use std::{
 
 use cosmic::{cosmic_theme::palette::Srgba, iced::subscription};
 use futures::{future, SinkExt};
-use tokio::sync::mpsc::Receiver;
+use tokio::sync::mpsc::{Receiver, Sender};
 use zbus::{zvariant, Connection};
 
 use crate::{
@@ -24,6 +24,7 @@ pub enum Event {
     Screencast(crate::screencast_dialog::Args),
     CancelScreencast(zvariant::ObjectPath<'static>),
     Background(crate::background::Args),
+    BackgroundGetAppPerm(String, Sender<crate::background::ConfigAppPerm>),
     Accent(Srgba),
     IsDark(bool),
     HighContrast(bool),
@@ -191,7 +192,12 @@ pub(crate) async fn process_changes(
                     }
                     Event::Background(args) => {
                         if let Err(err) = output.send(Event::Background(args)).await {
-                            log::error!("Error sending background event: {err:?}")
+                            log::error!("Error sending background event: {:?}", err);
+                        }
+                    }
+                    Event::BackgroundGetAppPerm(app_id, tx) => {
+                        if let Err(err) = output.send(Event::BackgroundGetAppPerm(app_id, tx)) {
+                            log::error!("Error sending background config request event: {:?}", err);
                         }
                     }
                     Event::Accent(a) => {