0.6.1: fix Firewall silent failures and Snapshots error messaging

Firewall's fetch_status() discarded stderr/exit code on failure, so a
failed pkexec call (e.g. no polkit agent in the session) left the page
silently stuck on "Status not loaded" with no indication anything went
wrong. Now surfaces the actual failure reason, and attaches the
previously-invisible per-action log buffer to a visible TextView
(matching the Packages page's existing pattern).

Snapshots' "No snapshots yet" empty state was shown identically whether
snapper was genuinely unconfigured or the user just lacked permissions
(a "No permissions." stderr) -- now distinguishes the two.

Cargo.lock

@@ -28,7 +28,7 @@ checksum = "b4388bee8683e3d04af747c73422af53102d2bd24d9eadb6cbc100baef4b43f8"
 
 [[package]]
 name = "bos-settings"
-version = "0.6.0"
+version = "0.6.1"
 dependencies = [
  "async-channel",
  "bread-theme",

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "bos-settings"
-version = "0.6.0"
+version = "0.6.1"
 edition = "2021"
 
 [dependencies]

src/ui/views/firewall.rs

@@ -14,7 +14,7 @@
 //! action instead of forcing it on app open.
 
 use gtk4::prelude::*;
-use gtk4::{Box as GBox, Button, Entry, Label, ListBox, ListBoxRow, Orientation, ScrolledWindow, Switch};
+use gtk4::{Box as GBox, Button, Entry, Label, ListBox, ListBoxRow, Orientation, ScrolledWindow, Switch, TextView};
 use std::cell::Cell;
 use std::rc::Rc;
 
@@ -33,13 +33,28 @@ struct Status {
 
 /// One `pkexec ufw status numbered` call, parsed for both the active/inactive
 /// line and the numbered rules — a single privileged read instead of two.
-fn fetch_status() -> Option<Status> {
+/// `Err` carries stderr (or a description of the exec failure) so callers can
+/// show *why* it failed instead of leaving the page silently on "Status not
+/// loaded" forever — previously this returned `Option<Status>`, discarding
+/// the reason entirely, so a failed pkexec call (wrong password, no polkit
+/// agent running in the session, ufw missing, ...) looked identical to
+/// simply never having clicked Refresh.
+fn fetch_status() -> Result<Status, String> {
     let output = std::process::Command::new("pkexec")
         .args(["ufw", "status", "numbered"])
         .output()
-        .ok()?;
+        .map_err(|e| format!("couldn't run pkexec: {e}"))?;
     if !output.status.success() {
-        return None;
+        let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
+        return Err(if stderr.is_empty() {
+            match output.status.code() {
+                Some(127) => "no polkit authentication agent is available in this session".to_string(),
+                Some(code) => format!("pkexec exited with status {code}"),
+                None => "pkexec was terminated by a signal".to_string(),
+            }
+        } else {
+            stderr
+        });
     }
     let text = String::from_utf8_lossy(&output.stdout);
     let active = text.lines().next().is_some_and(|l| l.trim() == "Status: active");
@@ -55,10 +70,10 @@ fn fetch_status() -> Option<Status> {
             Some(Rule { number, text: rest.trim().to_string() })
         })
         .collect();
-    Some(Status { active, rules })
+    Ok(Status { active, rules })
 }
 
-fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc<Cell<bool>>) {
+fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc<Cell<bool>>, log_buf: &gtk4::TextBuffer, log_view: &TextView) {
     while let Some(child) = list.first_child() {
         list.remove(&child);
     }
@@ -92,14 +107,19 @@ fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc<Cell<bool>>) {
             let list = list.clone();
             let number = rule.number.clone();
             let programmatic = programmatic.clone();
+            let log_buf = log_buf.clone();
+            let log_view = log_view.clone();
             remove_btn.connect_clicked(move |_| {
-                let log_buf = gtk4::TextBuffer::new(None);
+                log_buf.set_text("");
+                log_view.set_visible(true);
                 let list2 = list.clone();
                 let programmatic2 = programmatic.clone();
+                let log_buf2 = log_buf.clone();
+                let log_view2 = log_view.clone();
                 w::stream_command_then(
                     &["pkexec", "ufw", "--force", "delete", &number],
-                    log_buf,
-                    move || refresh(&list2, None, &programmatic2),
+                    log_buf.clone(),
+                    move || refresh(&list2, None, &programmatic2, &log_buf2, &log_view2),
                 );
             });
         }
@@ -111,6 +131,19 @@ fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc<Cell<bool>>) {
     }
 }
 
+/// Shows `message` as the list's only row, in place of the rule list —
+/// used both for the initial "not loaded yet" state and for a failed
+/// Refresh, so a failure looks like a state, not a no-op.
+fn render_message(list: &ListBox, icon: &str, title: &str, detail: &str) {
+    while let Some(child) = list.first_child() {
+        list.remove(&child);
+    }
+    let row = ListBoxRow::new();
+    row.set_selectable(false);
+    row.set_child(Some(&w::empty_state(icon, title, detail)));
+    list.append(&row);
+}
+
 /// Re-fetch status on a background thread and update the list (+ switch, if
 /// given) on completion. `enabled_sw` is `None` when called from a row
 /// action (delete/add) where the enabled state can't have changed.
@@ -120,22 +153,42 @@ fn render_rules(list: &ListBox, rules: &[Rule], programmatic: &Rc<Cell<bool>>) {
 /// `disable`) — without it, the very first Refresh after ufw turns out to
 /// already be active would immediately fire an unwanted `ufw enable` the
 /// moment `set_active(true)` flips a switch that just became sensitive.
-fn refresh(list: &ListBox, enabled_sw: Option<&Switch>, programmatic: &Rc<Cell<bool>>) {
-    let (tx, rx) = async_channel::bounded::<Option<Status>>(1);
+fn refresh(list: &ListBox, enabled_sw: Option<&Switch>, programmatic: &Rc<Cell<bool>>, log_buf: &gtk4::TextBuffer, log_view: &TextView) {
+    let (tx, rx) = async_channel::bounded::<Result<Status, String>>(1);
     std::thread::spawn(move || {
         let _ = tx.send_blocking(fetch_status());
     });
     let list = list.clone();
     let enabled_sw = enabled_sw.cloned();
     let programmatic = programmatic.clone();
+    let log_buf = log_buf.clone();
+    let log_view = log_view.clone();
     glib::spawn_future_local(async move {
-        if let Ok(Some(status)) = rx.recv().await {
-            render_rules(&list, &status.rules, &programmatic);
-            if let Some(sw) = &enabled_sw {
-                programmatic.set(true);
-                sw.set_sensitive(true);
-                sw.set_active(status.active);
-                programmatic.set(false);
+        match rx.recv().await {
+            Ok(Ok(status)) => {
+                render_rules(&list, &status.rules, &programmatic, &log_buf, &log_view);
+                if let Some(sw) = &enabled_sw {
+                    programmatic.set(true);
+                    sw.set_sensitive(true);
+                    sw.set_active(status.active);
+                    programmatic.set(false);
+                }
+            }
+            Ok(Err(reason)) => {
+                render_message(
+                    &list,
+                    "dialog-warning-symbolic",
+                    "Couldn't read firewall status",
+                    &reason,
+                );
+            }
+            Err(_) => {
+                render_message(
+                    &list,
+                    "dialog-warning-symbolic",
+                    "Couldn't read firewall status",
+                    "the background check never reported back",
+                );
             }
         }
     });
@@ -161,34 +214,47 @@ pub fn build() -> GBox {
 
     let list = ListBox::new();
     list.set_selection_mode(gtk4::SelectionMode::None);
-    {
-        let row = ListBoxRow::new();
-        row.set_selectable(false);
-        row.set_child(Some(&w::empty_state(
-            "security-high-symbolic",
-            "Status not loaded",
-            "Click Refresh below to check the firewall's current state.",
-        )));
-        list.append(&row);
-    }
+    render_message(
+        &list,
+        "security-high-symbolic",
+        "Status not loaded",
+        "Click Refresh below to check the firewall's current state.",
+    );
     let scroll = ScrolledWindow::new();
     scroll.set_vexpand(true);
     scroll.set_min_content_height(260);
     scroll.set_child(Some(&list));
     content.append(&scroll);
 
+    // Shared log view for every pkexec/ufw call on this page (refresh,
+    // enable/disable, add, remove) — previously each action created its own
+    // throwaway `TextBuffer::new(None)` that was never attached to any
+    // visible widget, so stderr/stdout from a failing command (including
+    // pkexec itself failing) went nowhere the user could see.
+    let log_buf = gtk4::TextBuffer::new(None);
+    let log_view = TextView::with_buffer(&log_buf);
+    log_view.set_editable(false);
+    log_view.set_monospace(true);
+    log_view.set_height_request(140);
+    log_view.set_margin_top(8);
+    log_view.set_visible(false);
+
     let refresh_btn = Button::with_label("Refresh status");
     {
         let list = list.clone();
         let enabled_sw = enabled_sw.clone();
         let programmatic = programmatic.clone();
-        refresh_btn.connect_clicked(move |_| refresh(&list, Some(&enabled_sw), &programmatic));
+        let log_buf = log_buf.clone();
+        let log_view = log_view.clone();
+        refresh_btn.connect_clicked(move |_| refresh(&list, Some(&enabled_sw), &programmatic, &log_buf, &log_view));
     }
     content.append(&refresh_btn);
 
     {
         let list = list.clone();
         let programmatic = programmatic.clone();
+        let log_buf = log_buf.clone();
+        let log_view = log_view.clone();
         enabled_sw.connect_active_notify(move |s| {
             // Skip both the pre-refresh insensitive state and any
             // programmatic set_active() from refresh() itself — only a
@@ -197,11 +263,14 @@ pub fn build() -> GBox {
                 return;
             }
             let verb = if s.is_active() { "enable" } else { "disable" };
-            let log_buf = gtk4::TextBuffer::new(None);
+            log_buf.set_text("");
+            log_view.set_visible(true);
             let list2 = list.clone();
             let programmatic2 = programmatic.clone();
-            w::stream_command_then(&["pkexec", "ufw", "--force", verb], log_buf, move || {
-                refresh(&list2, None, &programmatic2);
+            let log_buf2 = log_buf.clone();
+            let log_view2 = log_view.clone();
+            w::stream_command_then(&["pkexec", "ufw", "--force", verb], log_buf.clone(), move || {
+                refresh(&list2, None, &programmatic2, &log_buf2, &log_view2);
             });
         });
     }
@@ -219,21 +288,26 @@ pub fn build() -> GBox {
         let list = list.clone();
         let add_entry = add_entry.clone();
         let programmatic = programmatic.clone();
+        let log_buf = log_buf.clone();
+        let log_view = log_view.clone();
         add_btn.connect_clicked(move |_| {
             let rule = add_entry.text().to_string();
             if rule.trim().is_empty() {
                 return;
             }
-            let log_buf = gtk4::TextBuffer::new(None);
+            log_buf.set_text("");
+            log_view.set_visible(true);
             let list2 = list.clone();
             let add_entry2 = add_entry.clone();
             let programmatic2 = programmatic.clone();
+            let log_buf2 = log_buf.clone();
+            let log_view2 = log_view.clone();
             w::stream_command_then(
                 &["pkexec", "ufw", "allow", rule.trim()],
-                log_buf,
+                log_buf.clone(),
                 move || {
                     add_entry2.set_text("");
-                    refresh(&list2, None, &programmatic2);
+                    refresh(&list2, None, &programmatic2, &log_buf2, &log_view2);
                 },
             );
         });
@@ -242,5 +316,7 @@ pub fn build() -> GBox {
     add_row.append(&add_btn);
     content.append(&add_row);
 
+    content.append(&log_view);
+
     outer
 }

src/ui/views/snapshots.rs

@@ -13,27 +13,28 @@ struct SnapshotRow {
     description: String,
 }
 
-fn list_snapshots() -> Vec<SnapshotRow> {
+/// `Err` carries snapper's trimmed stderr — distinct from `Ok(vec![])`
+/// (snapper works fine, there just aren't any snapshots yet), so the caller
+/// can tell a real failure from a legitimately empty list instead of
+/// collapsing both into the same generic "not configured yet" message.
+fn list_snapshots() -> Result<Vec<SnapshotRow>, String> {
     // NOTE: the real flag is --columns, not --output-cols (which snapper
     // rejects outright with "Unknown option") — confirmed against snapper
     // 0.13's own --help. With the wrong flag this always failed and the
     // panel silently showed "No snapshots found" on every install.
-    let Ok(output) = Command::new("snapper")
+    let output = Command::new("snapper")
         .args(["list", "--columns", "number,date,description"])
         .output()
-    else {
-        return Vec::new();
-    };
+        .map_err(|e| e.to_string())?;
     if !output.status.success() {
-        eprintln!(
-            "bos-settings: snapper list failed: {}",
-            String::from_utf8_lossy(&output.stderr).trim()
-        );
-        return Vec::new();
+        let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
+        eprintln!("bos-settings: snapper list failed: {stderr}");
+        return Err(stderr);
     }
 
     let text = String::from_utf8_lossy(&output.stdout);
-    text.lines()
+    Ok(text
+        .lines()
         .skip(2) // header + separator
         .filter_map(|line| {
             let mut cols = line.splitn(3, '|');
@@ -49,7 +50,7 @@ fn list_snapshots() -> Vec<SnapshotRow> {
                 description: cols.next()?.trim().to_string(),
             })
         })
-        .collect()
+        .collect())
 }
 
 /// Returns whether the list ended up empty, so callers can disable the
@@ -58,15 +59,39 @@ fn populate_list(list: &ListBox) -> bool {
     while let Some(child) = list.first_child() {
         list.remove(&child);
     }
-    let snapshots = list_snapshots();
+    let snapshots = match list_snapshots() {
+        Ok(snapshots) => snapshots,
+        Err(stderr) => {
+            let lower = stderr.to_lowercase();
+            let (title, detail) = if lower.contains("no permission") {
+                (
+                    "No permission to read snapshots",
+                    "This user isn't allowed to run snapper. Check ALLOW_USERS in \
+                     /etc/snapper/configs/root — it should list your username.",
+                )
+            } else if lower.contains("unknown config") || lower.contains("no such file") {
+                (
+                    "Snapper isn't configured",
+                    "No snapper config exists for root yet, so nothing is being \
+                     snapshotted. This should be set up automatically at install.",
+                )
+            } else {
+                ("Couldn't read snapshots", stderr.as_str())
+            };
+            let row = ListBoxRow::new();
+            row.set_selectable(false);
+            row.set_child(Some(&w::empty_state("dialog-warning-symbolic", title, detail)));
+            list.append(&row);
+            return true;
+        }
+    };
     if snapshots.is_empty() {
         let row = ListBoxRow::new();
         row.set_selectable(false);
         row.set_child(Some(&w::empty_state(
             "document-open-recent-symbolic",
             "No snapshots yet",
-            "Snapshots are created automatically on every pacman transaction \
-             (snapper may not be configured yet).",
+            "Snapshots are created automatically on every pacman transaction.",
         )));
         list.append(&row);
         return true;