fix: address audit findings — blur channel mismatch, logout quit, config error logging
- Fix BGRA→RGBA channel swap in apply_blur so image::RgbaImage semantics match the actual pixel data from GDK texture download - Logout now calls app.quit() like lock does, via new quit_after field on ActionDef (replaces fragile magic string comparison) - Log TOML parse errors to stderr instead of silently ignoring - Remove pointless zlib compression of JPEG wallpaper in GResource - Add tests for quit_after behavior and config error handling
This commit is contained in:
parent
478caed8e0
commit
412ed159a4
13
CHANGELOG.md
13
CHANGELOG.md
@ -3,6 +3,19 @@
|
||||
All notable changes to this project will be documented in this file.
|
||||
Format based on [Keep a Changelog](https://keepachangelog.com/).
|
||||
|
||||
## [0.4.1] - 2026-03-28
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fix pixel format mismatch in blur path — `texture.download()` yields BGRA but was passed to `RgbaImage` without channel swap, now explicitly converts B↔R
|
||||
- Logout action now calls `app.quit()` to dismiss the menu immediately (previously only Lock did)
|
||||
- Log TOML parse errors to stderr instead of silently falling back to defaults
|
||||
|
||||
### Changed
|
||||
|
||||
- Replace magic string `"lock"` comparison with `quit_after` field on `ActionDef` for type-safe action dispatch
|
||||
- Remove `compressed="true"` from JPEG wallpaper in GResource — JPEG is already compressed, zlib overhead hurts startup for negligible size savings
|
||||
|
||||
## [0.4.0] - 2026-03-28
|
||||
|
||||
### Added
|
||||
|
||||
@ -1,6 +1,6 @@
|
||||
[package]
|
||||
name = "moonset"
|
||||
version = "0.4.0"
|
||||
version = "0.4.1"
|
||||
edition = "2024"
|
||||
description = "Wayland session power menu with GTK4 and Layer Shell"
|
||||
license = "MIT"
|
||||
|
||||
@ -2,6 +2,13 @@
|
||||
|
||||
Architectural and design decisions for Moonset, in reverse chronological order.
|
||||
|
||||
## 2026-03-28 – Replace action name dispatch with `quit_after` field
|
||||
|
||||
- **Who**: Hekate, Dom
|
||||
- **Why**: Post-action behavior (quit the app or not) was controlled by comparing `action_name == "lock"` — a magic string duplicated from the action definition. Renaming an action would silently break the dispatch.
|
||||
- **Tradeoffs**: Adds a field to `ActionDef` that most actions set to `false`. Acceptable because it makes the contract explicit and testable.
|
||||
- **How**: `ActionDef.quit_after: bool` — `true` for lock and logout, `false` for hibernate/reboot/shutdown.
|
||||
|
||||
## 2026-03-28 – Optional background blur via `image` crate
|
||||
|
||||
- **Who**: Hekate, Dom
|
||||
|
||||
@ -2,7 +2,7 @@
|
||||
<gresources>
|
||||
<gresource prefix="/dev/moonarch/moonset">
|
||||
<file>style.css</file>
|
||||
<file compressed="true">wallpaper.jpg</file>
|
||||
<file>wallpaper.jpg</file>
|
||||
<file>default-avatar.svg</file>
|
||||
</gresource>
|
||||
</gresources>
|
||||
|
||||
@ -31,12 +31,17 @@ pub fn load_config(config_paths: Option<&[PathBuf]>) -> Config {
|
||||
let mut merged = Config::default();
|
||||
for path in paths {
|
||||
if let Ok(content) = fs::read_to_string(path) {
|
||||
if let Ok(parsed) = toml::from_str::<Config>(&content) {
|
||||
if parsed.background_path.is_some() {
|
||||
merged.background_path = parsed.background_path;
|
||||
match toml::from_str::<Config>(&content) {
|
||||
Ok(parsed) => {
|
||||
if parsed.background_path.is_some() {
|
||||
merged.background_path = parsed.background_path;
|
||||
}
|
||||
if parsed.background_blur.is_some() {
|
||||
merged.background_blur = parsed.background_blur;
|
||||
}
|
||||
}
|
||||
if parsed.background_blur.is_some() {
|
||||
merged.background_blur = parsed.background_blur;
|
||||
Err(e) => {
|
||||
eprintln!("Warning: failed to parse {}: {e}", path.display());
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -185,4 +190,35 @@ mod tests {
|
||||
let result = resolve_background_path_with(&config, Path::new("/nonexistent"));
|
||||
assert!(result.to_str().unwrap().contains("wallpaper.jpg"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_config_ignores_invalid_toml_syntax() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let conf = dir.path().join("bad.toml");
|
||||
fs::write(&conf, "this is not valid [[[ toml").unwrap();
|
||||
let paths = vec![conf];
|
||||
let config = load_config(Some(&paths));
|
||||
assert!(config.background_path.is_none());
|
||||
assert!(config.background_blur.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_config_ignores_wrong_field_types() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let conf = dir.path().join("wrong_type.toml");
|
||||
fs::write(&conf, "background_blur = \"not_a_number\"\n").unwrap();
|
||||
let paths = vec![conf];
|
||||
let config = load_config(Some(&paths));
|
||||
assert!(config.background_blur.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_config_accepts_negative_blur() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let conf = dir.path().join("negative.toml");
|
||||
fs::write(&conf, "background_blur = -5.0\n").unwrap();
|
||||
let paths = vec![conf];
|
||||
let config = load_config(Some(&paths));
|
||||
assert_eq!(config.background_blur, Some(-5.0));
|
||||
}
|
||||
}
|
||||
|
||||
33
src/panel.rs
33
src/panel.rs
@ -31,6 +31,7 @@ pub struct ActionDef {
|
||||
pub label_attr: fn(&Strings) -> &'static str,
|
||||
pub error_attr: fn(&Strings) -> &'static str,
|
||||
pub confirm_attr: Option<fn(&Strings) -> &'static str>,
|
||||
pub quit_after: bool,
|
||||
}
|
||||
|
||||
/// All 5 power action definitions.
|
||||
@ -44,6 +45,7 @@ pub fn action_definitions() -> Vec<ActionDef> {
|
||||
label_attr: |s| s.lock_label,
|
||||
error_attr: |s| s.lock_failed,
|
||||
confirm_attr: None,
|
||||
quit_after: true,
|
||||
},
|
||||
ActionDef {
|
||||
name: "logout",
|
||||
@ -53,6 +55,7 @@ pub fn action_definitions() -> Vec<ActionDef> {
|
||||
label_attr: |s| s.logout_label,
|
||||
error_attr: |s| s.logout_failed,
|
||||
confirm_attr: Some(|s| s.logout_confirm),
|
||||
quit_after: true,
|
||||
},
|
||||
ActionDef {
|
||||
name: "hibernate",
|
||||
@ -62,6 +65,7 @@ pub fn action_definitions() -> Vec<ActionDef> {
|
||||
label_attr: |s| s.hibernate_label,
|
||||
error_attr: |s| s.hibernate_failed,
|
||||
confirm_attr: Some(|s| s.hibernate_confirm),
|
||||
quit_after: false,
|
||||
},
|
||||
ActionDef {
|
||||
name: "reboot",
|
||||
@ -71,6 +75,7 @@ pub fn action_definitions() -> Vec<ActionDef> {
|
||||
label_attr: |s| s.reboot_label,
|
||||
error_attr: |s| s.reboot_failed,
|
||||
confirm_attr: Some(|s| s.reboot_confirm),
|
||||
quit_after: false,
|
||||
},
|
||||
ActionDef {
|
||||
name: "shutdown",
|
||||
@ -80,6 +85,7 @@ pub fn action_definitions() -> Vec<ActionDef> {
|
||||
label_attr: |s| s.shutdown_label,
|
||||
error_attr: |s| s.shutdown_failed,
|
||||
confirm_attr: Some(|s| s.shutdown_confirm),
|
||||
quit_after: false,
|
||||
},
|
||||
]
|
||||
}
|
||||
@ -218,8 +224,14 @@ fn apply_blur(texture: &gdk::Texture, sigma: f32) -> gdk::Texture {
|
||||
let height = texture.height() as u32;
|
||||
let stride = width as usize * 4;
|
||||
let mut pixel_data = vec![0u8; stride * height as usize];
|
||||
// download() yields GDK_MEMORY_DEFAULT = B8G8R8A8_PREMULTIPLIED (BGRA byte order).
|
||||
texture.download(&mut pixel_data, stride);
|
||||
|
||||
// Swap B↔R so image::RgbaImage channel semantics are correct.
|
||||
for pixel in pixel_data.chunks_exact_mut(4) {
|
||||
pixel.swap(0, 2);
|
||||
}
|
||||
|
||||
let img = image::RgbaImage::from_raw(width, height, pixel_data)
|
||||
.expect("pixel buffer size matches texture dimensions");
|
||||
let blurred = imageops::blur(&image::DynamicImage::ImageRgba8(img), sigma);
|
||||
@ -228,7 +240,7 @@ fn apply_blur(texture: &gdk::Texture, sigma: f32) -> gdk::Texture {
|
||||
let mem_texture = gdk::MemoryTexture::new(
|
||||
width as i32,
|
||||
height as i32,
|
||||
gdk::MemoryFormat::B8g8r8a8Premultiplied,
|
||||
gdk::MemoryFormat::R8g8b8a8Premultiplied,
|
||||
&bytes,
|
||||
stride,
|
||||
);
|
||||
@ -564,6 +576,7 @@ fn execute_action(
|
||||
|
||||
let action_fn = action_def.action_fn;
|
||||
let action_name = action_def.name;
|
||||
let quit_after = action_def.quit_after;
|
||||
let error_message = (action_def.error_attr)(strings).to_string();
|
||||
|
||||
// Use glib::spawn_future_local + gio::spawn_blocking to avoid Send issues
|
||||
@ -579,8 +592,7 @@ fn execute_action(
|
||||
|
||||
match result {
|
||||
Ok(Ok(())) => {
|
||||
// Lock action: quit after successful execution
|
||||
if action_name == "lock" {
|
||||
if quit_after {
|
||||
app.quit();
|
||||
}
|
||||
}
|
||||
@ -739,6 +751,21 @@ mod tests {
|
||||
assert_eq!(confirm_fn(strings), "Wirklich abmelden?");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lock_and_logout_quit_after() {
|
||||
let defs = action_definitions();
|
||||
assert!(defs[0].quit_after, "lock should quit after");
|
||||
assert!(defs[1].quit_after, "logout should quit after");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_actions_do_not_quit_after() {
|
||||
let defs = action_definitions();
|
||||
for def in &defs[2..] {
|
||||
assert!(!def.quit_after, "{} should not quit after", def.name);
|
||||
}
|
||||
}
|
||||
|
||||
// -- Blur cache tests --
|
||||
|
||||
#[test]
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user