diff options
Diffstat (limited to 'crates')
| -rw-r--r-- | crates/core/src/context_menu.rs | 238 | ||||
| -rw-r--r-- | crates/wayland/src/decoration.rs | 110 | ||||
| -rw-r--r-- | crates/wayland/src/decoration/tests.rs | 68 | ||||
| -rw-r--r-- | crates/wayland/src/input/pointer.rs | 17 | ||||
| -rw-r--r-- | crates/wayland/src/state/desktop_icons.rs | 18 | ||||
| -rw-r--r-- | crates/wayland/src/state/menu.rs | 63 | ||||
| -rw-r--r-- | crates/x11/src/platform/actions.rs | 2 | ||||
| -rw-r--r-- | crates/x11/src/platform/context_menu.rs | 43 | ||||
| -rw-r--r-- | crates/x11/src/platform/events.rs | 2 |
9 files changed, 447 insertions, 114 deletions
diff --git a/crates/core/src/context_menu.rs b/crates/core/src/context_menu.rs index 30b1e8f..f56c7b9 100644 --- a/crates/core/src/context_menu.rs +++ b/crates/core/src/context_menu.rs @@ -10,8 +10,18 @@ //! drifting copy of the same logic - this data has no Wayland-specific //! content at all, so duplicating it would just be two copies of the same //! bug waiting to happen. +//! +//! Redesigned after being reported live as "looks very ugly and some of +//! it doesn't make sense": every row, including a separator, used to be a +//! full `TITLEBAR_HEIGHT`-tall slot (a hairline sitting in the middle of +//! 32px of empty space), and "Move to Workspace" faked a section header +//! by embedding box-drawing characters directly in a clickable-looking +//! item label (`"─── Move to Workspace ───"`) rather than being a real, +//! non-interactive row. Both are fixed below: separators and headers get +//! their own, much smaller row heights, and [`MenuAction::Header`] is a +//! real non-clickable row type instead of a label-text hack. -use crate::{WindowManager, WindowId, WorkspaceId, TITLEBAR_HEIGHT}; +use crate::{WindowManager, WindowId, WorkspaceId}; #[derive(Clone, Copy)] pub enum MenuAction { @@ -21,13 +31,41 @@ pub enum MenuAction { ToggleFloating, ToggleAlwaysOnTop, MoveToWorkspace(WorkspaceId), + /// Cycles `ThemeConfig::traffic_light_buttons` - the same live knob + /// `srd set button_style` flips, but applied here with an immediate + /// redraw of every open window's titlebar (see `run_context_menu_ + /// action`'s own doc comment for why that's only possible from + /// backend-specific code, not the generic IPC path). A theme setting + /// is shared across every window by definition, not a per-window + /// property - cycling it from one window's menu restyles all of + /// them, the same way changing it in a settings app would. + CycleButtonStyle, + /// Cycles `ThemeConfig::buttons_left` - same shape as `CycleButtonStyle`. + CycleButtonSide, Close, - /// Not a real action - a purely visual divider row. `row_at` still - /// resolves a click on one to `Some(index)` (it occupies a real row, - /// same as any other), so the dispatch site is what actually no-ops - /// on it, same "the row exists but does nothing" contract a real - /// desktop's own menu separators have. + /// A purely visual divider row - no label, no click behaviour. `row_ + /// at` still resolves a click on one to `Some(index)` (it occupies + /// real space, same as any other row), so the dispatch site is what + /// actually no-ops on it, same "the row exists but does nothing" + /// contract a real desktop's own menu separators have. Separator, + /// A non-interactive section label (`"Move to Workspace"`, `"Customize"`) + /// - dimmer, smaller text, never highlighted, click is a no-op just + /// like [`Self::Separator`]. Replaces an earlier hack that embedded + /// box-drawing characters directly in an ordinary item's label, which + /// rendered (and behaved, right up until the dispatch site's own + /// special-case) exactly like a clickable row that happened to do + /// nothing - confusing on both counts. + Header, +} + +impl MenuAction { + /// Whether this row can ever be the target of a real click - shared + /// by both backends' click-dispatch sites so `Separator`/`Header` + /// can't drift out of sync with each other on which rows are inert. + pub fn is_interactive(&self) -> bool { + !matches!(self, MenuAction::Separator | MenuAction::Header) + } } pub struct ContextMenu { @@ -36,11 +74,29 @@ pub struct ContextMenu { /// `Window.geometry` and every other rendered element's position uses. pub pos: (i32, i32), pub width: u32, + /// A real, clickable item's own row height. `Separator`/`Header` rows + /// use [`SEPARATOR_HEIGHT`]/[`HEADER_HEIGHT`] instead, regardless of + /// this value - see [`Self::row_height_for`]. pub row_height: u32, pub items: Vec<(&'static str, MenuAction)>, } const MENU_WIDTH: u32 = 170; +/// A real item's own row height, in logical pixels. Its own constant +/// rather than reusing `TITLEBAR_HEIGHT`: this is a stand-alone popup, not +/// a titlebar, and the two heights never needed to match - they just +/// happened to, which made a hairline separator take a full 32px slot to +/// draw a 1px line in. Sized against GNOME/KDE's own popover menu row +/// height (32-36px and 28-30px respectively), not this compositor's own +/// titlebar band. +pub const MENU_ROW_HEIGHT: u32 = 28; +/// A divider's own row height - just enough room for the hairline plus a +/// little breathing room on each side, not a full item row. +pub const SEPARATOR_HEIGHT: u32 = 9; +/// A section-label row's own height - shorter than a real item (it holds +/// smaller, dimmer text with nothing to click), taller than a plain +/// separator (it has to fit that text). +pub const HEADER_HEIGHT: u32 = 22; impl ContextMenu { /// Builds the menu for `window`, opening with its top-left corner at @@ -53,15 +109,23 @@ impl ContextMenu { let w = wm.window(window)?; let maximize_label = if w.maximized { "Restore" } else { "Maximize" }; let fullscreen_label = if w.fullscreen { "Exit Fullscreen" } else { "Fullscreen" }; - let floating_label = if w.floating { "\u{2713} Floating" } else { "Floating" }; let pin_label = if w.always_on_top { "\u{2713} Always on Top" } else { "Always on Top" }; - let mut items = vec![ - ("Minimize", MenuAction::Minimize), - (maximize_label, MenuAction::ToggleMaximize), - (fullscreen_label, MenuAction::ToggleFullscreen), - (floating_label, MenuAction::ToggleFloating), - (pin_label, MenuAction::ToggleAlwaysOnTop), - ]; + let mut items = vec![("Minimize", MenuAction::Minimize), (maximize_label, MenuAction::ToggleMaximize), (fullscreen_label, MenuAction::ToggleFullscreen)]; + // "Floating" only means something under a layout that actually + // tiles - `arrange_workspace` is the only reader of `Window:: + // floating` anywhere in this compositor, and it only runs for the + // "tiling" layout (`"dynamic"`/`"floating"` never reposition + // anyone regardless of this flag). Reported live as "doesn't make + // sense": toggling it while running this project's own default + // dynamic layout visibly does nothing at all, which reads as a + // broken menu item rather than an inapplicable one. Shown only + // when it would actually change something on screen. + let layout_is_tiling = wm.workspace(w.workspace).map(|ws| ws.layout == "tiling").unwrap_or(false); + if layout_is_tiling { + let floating_label = if w.floating { "\u{2713} Floating" } else { "Floating" }; + items.push((floating_label, MenuAction::ToggleFloating)); + } + items.push((pin_label, MenuAction::ToggleAlwaysOnTop)); // One row per *other* workspace - skips the window's own current // one, since "move to the workspace it's already on" isn't a real // action. Flattened rather than a real submenu - `workspace.count` @@ -69,7 +133,8 @@ impl ContextMenu { // so the menu stays a reasonable height without one. let others: Vec<&crate::Workspace> = wm.workspaces().iter().filter(|ws| ws.id != w.workspace).collect(); if !others.is_empty() { - items.push(("\u{2500}\u{2500}\u{2500} Move to Workspace \u{2500}\u{2500}\u{2500}", MenuAction::Separator)); + items.push(("", MenuAction::Separator)); + items.push(("Move to Workspace", MenuAction::Header)); for ws in others { // `Workspace.name` is `&'static`-incompatible (a real // `String`, user-configurable via `workspace.names`) -- @@ -79,13 +144,50 @@ impl ContextMenu { items.push((label, MenuAction::MoveToWorkspace(ws.id))); } } - items.push(("\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}", MenuAction::Separator)); + // Quick-access theme customization - reported live as a direct + // ask ("allow customizing from there as well"), scoped to the two + // knobs the same live request already named by name (traffic- + // light vs. traditional buttons, which side they sit on): both + // are shared theme settings, not per-window ones, so cycling + // either restyles every open window's titlebar at once, same as + // changing it in a config reload would - see `CycleButtonStyle`/ + // `CycleButtonSide`'s own doc comments for why that's still an + // *immediate* redraw here rather than "takes effect next time you + // open a window" the way the plain `srd set` path is scoped to. + items.push(("", MenuAction::Separator)); + items.push(("Customize", MenuAction::Header)); + let button_style_label: &'static str = if wm.theme.traffic_light_buttons { "Button Style: Traffic Lights" } else { "Button Style: Traditional" }; + let button_side_label: &'static str = if wm.theme.buttons_left { "Button Side: Left" } else { "Button Side: Right" }; + items.push((button_style_label, MenuAction::CycleButtonStyle)); + items.push((button_side_label, MenuAction::CycleButtonSide)); + items.push(("", MenuAction::Separator)); items.push(("Close", MenuAction::Close)); - Some(Self { window, pos, width: MENU_WIDTH, row_height: TITLEBAR_HEIGHT, items }) + Some(Self { window, pos, width: MENU_WIDTH, row_height: MENU_ROW_HEIGHT, items }) + } + + /// The height of row `index`, or `self.row_height` (a real item's own + /// height) for an out-of-range index - callers that already know + /// `index` is valid (every real one does) never hit that fallback; it + /// exists so this can't panic if a future caller ever gets it wrong. + pub fn row_height_for(&self, index: usize) -> u32 { + match self.items.get(index) { + Some((_, MenuAction::Separator)) => SEPARATOR_HEIGHT, + Some((_, MenuAction::Header)) => HEADER_HEIGHT, + _ => self.row_height, + } + } + + /// The y-offset row `index` starts at, relative to the menu's own top + /// - every row height up to (not including) `index`, summed. `row_ + /// at`/rendering both walk rows this same way, so a mismatch between + /// "where a row is drawn" and "where a click resolves to" can't creep + /// in from computing the two differently. + pub fn row_y(&self, index: usize) -> i32 { + (0..index.min(self.items.len())).map(|i| self.row_height_for(i)).sum::<u32>() as i32 } pub fn height(&self) -> i32 { - self.row_height as i32 * self.items.len() as i32 + self.row_y(self.items.len()) } /// Which row (if any) global-space point `(x, y)` falls on. @@ -97,7 +199,15 @@ impl ContextMenu { if rel_y < 0 || rel_y >= self.height() { return None; } - Some((rel_y / self.row_height as i32) as usize) + let mut top = 0; + for (i, _) in self.items.iter().enumerate() { + let h = self.row_height_for(i) as i32; + if rel_y < top + h { + return Some(i); + } + top += h; + } + None } } @@ -114,6 +224,15 @@ mod tests { (wm, id) } + fn wm_with_tiling_window() -> (WindowManager, WindowId) { + let mut wm = WindowManager::new(); + wm.set_monitors(vec![crate::Monitor::new(0, "primary", crate::Rect::new(0, 0, 1920, 1080))]); + wm.set_layout(wm.current_workspace(), "tiling"); + let id = wm.alloc_window_id(); + wm.add_window(Window::new(id, "a")); + (wm, id) + } + #[test] fn open_labels_maximize_action_by_current_state() { let (mut wm, id) = wm_with_window(); @@ -126,15 +245,29 @@ mod tests { } #[test] + fn floating_row_is_hidden_outside_tiling_layout() { + let (wm, id) = wm_with_window(); + let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); + assert!(!menu.items.iter().any(|(label, _)| label.contains("Floating")), "dynamic layout: toggling floating has no visible effect, so it must not be offered"); + } + + #[test] + fn floating_row_is_shown_under_tiling_layout() { + let (wm, id) = wm_with_tiling_window(); + let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); + assert!(menu.items.iter().any(|(label, _)| label.contains("Floating")), "tiling layout: toggling floating genuinely changes the window's geometry"); + } + + #[test] fn open_marks_pinned_state_on_the_always_on_top_row() { let (mut wm, id) = wm_with_window(); let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); - // Minimize, Maximize, Fullscreen, Floating, then Always on Top. - assert_eq!(menu.items[4].0, "Always on Top"); + // Minimize, Maximize, Fullscreen (no Floating - dynamic layout), Always on Top. + assert_eq!(menu.items[3].0, "Always on Top"); wm.toggle_always_on_top(id); let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); - assert!(menu.items[4].0.starts_with('\u{2713}'), "pinned state must be visible on the label itself"); + assert!(menu.items[3].0.starts_with('\u{2713}'), "pinned state must be visible on the label itself"); } #[test] @@ -148,7 +281,6 @@ mod tests { let (wm, id) = wm_with_window(); let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); assert!(!menu.items.iter().any(|(label, _)| label.contains("Workspace"))); - assert_eq!(menu.items.len(), 7, "Minimize, Maximize, Fullscreen, Floating, Always on Top, one separator, Close"); } #[test] @@ -165,22 +297,68 @@ mod tests { } #[test] - fn clicking_a_separator_row_is_distinguishable_from_a_real_action() { + fn header_rows_are_not_interactive() { + let (wm, id) = wm_with_window(); + let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); + let headers: Vec<_> = menu.items.iter().filter(|(_, a)| matches!(a, MenuAction::Header)).collect(); + assert!(!headers.is_empty()); + for (_, action) in headers { + assert!(!action.is_interactive()); + } + } + + #[test] + fn customize_section_reflects_current_theme_state() { let (mut wm, id) = wm_with_window(); - wm.add_workspace("2", "dynamic"); + wm.theme.traffic_light_buttons = true; + wm.theme.buttons_left = false; let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); - let separator_count = menu.items.iter().filter(|(_, a)| matches!(a, MenuAction::Separator)).count(); - assert_eq!(separator_count, 2, "one before the workspace section, one before Close"); + assert!(menu.items.iter().any(|(label, _)| *label == "Button Style: Traffic Lights")); + assert!(menu.items.iter().any(|(label, _)| *label == "Button Side: Right")); + + wm.theme.traffic_light_buttons = false; + wm.theme.buttons_left = true; + let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); + assert!(menu.items.iter().any(|(label, _)| *label == "Button Style: Traditional")); + assert!(menu.items.iter().any(|(label, _)| *label == "Button Side: Left")); } #[test] - fn row_at_maps_a_point_to_the_right_row() { + fn separators_and_headers_are_shorter_than_a_real_item_row() { + let (wm, id) = wm_with_window(); + let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); + for (i, (_, action)) in menu.items.iter().enumerate() { + let h = menu.row_height_for(i); + match action { + MenuAction::Separator => assert_eq!(h, SEPARATOR_HEIGHT), + MenuAction::Header => assert_eq!(h, HEADER_HEIGHT), + _ => assert_eq!(h, MENU_ROW_HEIGHT), + } + } + } + + #[test] + fn height_is_the_sum_of_every_rows_own_height() { + let (wm, id) = wm_with_window(); + let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); + let expected: i32 = (0..menu.items.len()).map(|i| menu.row_height_for(i) as i32).sum(); + assert_eq!(menu.height(), expected); + } + + #[test] + fn row_at_maps_a_point_to_the_right_row_with_variable_heights() { let (wm, id) = wm_with_window(); let menu = ContextMenu::open(&wm, id, (100, 100)).unwrap(); assert_eq!(menu.row_at(150, 100), Some(0), "top of the first row"); - assert_eq!(menu.row_at(150, 100 + TITLEBAR_HEIGHT as i32 - 1), Some(0), "bottom of the first row"); - assert_eq!(menu.row_at(150, 100 + TITLEBAR_HEIGHT as i32), Some(1), "top of the second row"); + assert_eq!(menu.row_at(150, 100 + menu.row_height as i32 - 1), Some(0), "bottom of the first row"); + assert_eq!(menu.row_at(150, 100 + menu.row_height as i32), Some(1), "top of the second row"); assert_eq!(menu.row_at(150, 100 + menu.height() - 1), Some(menu.items.len() - 1), "last row, last pixel"); + // Every row boundary in between must resolve to exactly one row -- + // the real regression this test guards against is a gap or an + // overlap between two rows of different heights. + for y in 0..menu.height() { + assert!(menu.row_at(150, 100 + y).is_some(), "no gap at offset {y}"); + } } #[test] diff --git a/crates/wayland/src/decoration.rs b/crates/wayland/src/decoration.rs index aa12a54..d24c035 100644 --- a/crates/wayland/src/decoration.rs +++ b/crates/wayland/src/decoration.rs @@ -87,20 +87,61 @@ pub(crate) const CORNER_RADIUS: u32 = 12; /// Still no submenus/icons - real gaps beyond this pass' own scope, not /// attempted blind. /// -/// A row whose label is *entirely* the box-drawing character `─` -/// (`\u{2500}`, one or more, no other content) renders as a real thin -/// divider line instead of text glyphs - a label that *mixes* `─` with -/// real text (`"─── Move to Workspace ───"`, a deliberate section-header -/// convention `core::ContextMenu` already uses) is untouched and still -/// renders as text, since that dual purpose (divider *and* caption) is -/// the actual design, not a plain separator. A pure divider is drawn as -/// pixels rather than characters because Unicode box-drawing glyphs -/// render inconsistently thin/dotted across fonts at small sizes - a -/// real 1px anti-aliased line, inset from both edges and blended low- -/// opacity against the panel (matching the AGS reference dropdown's own -/// `separator.menu-sep`: `color-mix(in srgb, var(--fg) 12%, transparent)`), -/// reads as an intentional divider rather than a run of stray dashes. -pub fn render_context_menu(width: u32, row_height: u32, items: &[(&str, bool)], bg: (u8, u8, u8), fg: (u8, u8, u8), highlight_bg: (u8, u8, u8), border: (u8, u8, u8)) -> Vec<u8> { +/// A row's shape, alongside its own label/height - `render_context_menu` +/// used to detect a divider by checking whether a label was made entirely +/// of the box-drawing character `─`, and had no representation for a +/// section-header row at all (`"─── Move to Workspace ───"` rendered, and +/// behaved, as an ordinary clickable-looking item that merely did +/// nothing). Reported live as looking wrong on both counts - a real, +/// distinct row kind is what `srdwm_core::context_menu::MenuAction`'s own +/// `Separator`/`Header` variants exist for; this mirrors that split here +/// without this crate needing to depend on that enum itself. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum MenuRowKind { + Item, + /// A thin divider line, no label. + Separator, + /// A non-interactive section caption - dimmer, smaller text, never + /// highlighted. + Header, +} + +/// Rounded floating panel with a per-row rounded hover highlight, matching +/// the reference this project's own AGS panel already settled on for its +/// global-menu dropdown (`widget/Bar/components/GlobalMenu/style.scss`'s +/// `popover box.menu-list`): flat rows with no border/outline at rest, a +/// soft tinted fill (not a frame) on the highlighted one, inset padding so +/// rows don't touch the panel's own edge, real gaps between rows. Reported +/// live as looking "squished, no spacing/padding/margining, not at all +/// polished" - the previous version drew edge-to-edge square rows with a +/// single hard 1px border around the whole menu, exactly what that +/// complaint (raised about the AGS dropdown, fixed there first) describes. +/// Still no submenus/icons - real gaps beyond this pass' own scope, not +/// attempted blind. +/// +/// `rows` carries each row's own height alongside its label/kind - a +/// [`MenuRowKind::Separator`]/[`MenuRowKind::Header`] row is much shorter +/// than a real item (see `srdwm_core::context_menu`'s own `SEPARATOR_ +/// HEIGHT`/`HEADER_HEIGHT`), reported live as looking wrong when every +/// row, divider included, took the same full item height: a hairline +/// sitting in the middle of a mostly-empty 32px slot. The caller is what +/// actually knows each row's real height (`ContextMenu::row_height_for`); +/// this function just draws whatever it's told, at the offsets implied by +/// summing those heights in order - the same summing `ContextMenu::row_ +/// at`/`row_y` do, so a click and a pixel can never disagree about where a +/// row actually is. +/// +/// A [`MenuRowKind::Separator`] is drawn as a real 1px anti-aliased line, +/// inset from both edges and blended low-opacity against the panel +/// (matching the AGS reference dropdown's own `separator.menu-sep`: +/// `color-mix(in srgb, var(--fg) 12%, transparent)`) - pixels, not +/// Unicode box-drawing characters, which render inconsistently thin/ +/// dotted across fonts at small sizes. A [`MenuRowKind::Header`] draws its +/// label dimmed (the same mix ratio a separator's own line uses) and +/// never highlighted, regardless of what the caller passes for that row's +/// `highlighted` field - section captions aren't clickable, so nothing +/// should ever visually suggest they are. +pub fn render_context_menu(width: u32, rows: &[(&str, bool, u32, MenuRowKind)], bg: (u8, u8, u8), fg: (u8, u8, u8), highlight_bg: (u8, u8, u8), border: (u8, u8, u8)) -> Vec<u8> { let _ = border; // No outline anywhere now - see this function's own doc comment. Kept as a parameter so callers/themes don't need updating for a look this function no longer draws. const PANEL_RADIUS: f32 = 10.0; const ROW_INSET: i32 = 4; @@ -114,17 +155,13 @@ pub fn render_context_menu(width: u32, row_height: u32, items: &[(&str, bool)], // accent to tint toward", not "the literal pixel colour". const HIGHLIGHT_MIX: f32 = 0.22; // Matches the reference's own `separator.menu-sep` background -- - // barely-there, a hairline rather than a visible bar. - const SEPARATOR_MIX: f32 = 0.12; + // barely-there, a hairline rather than a visible bar. Reused as the + // header caption's own dim ratio - both exist to read as "present, + // but deliberately not the main content" against the same panel. + const DIM_MIX: f32 = 0.12; - let (width, row_height) = (width.max(1) as usize, row_height.max(1) as usize); - // Exactly `row_height * items.len()`, same as before this pass -- - // `ContextMenu`/`DesktopMenu`'s own `height()` and `row_at()` (which - // this function has no access to and mustn't get out of sync with) - // assume row `i` starts at `i * row_height` with no extra top/bottom - // inset, so all of this rework happens *inside* that unchanged canvas - // rather than by growing it. - let height = (row_height * items.len().max(1)).max(1); + let width = width.max(1) as usize; + let height = rows.iter().map(|(_, _, h, _)| *h).sum::<u32>().max(1) as usize; let mut buf = vec![0u8; width * height * 4]; // The panel itself: one flat rounded-rect fill on an otherwise fully @@ -133,15 +170,19 @@ pub fn render_context_menu(width: u32, row_height: u32, items: &[(&str, bool)], fill_rounded_rect(&mut buf, width, height, 0, 0, width as i32, height as i32, PANEL_RADIUS, bg, bg); let highlight_fill = mix_rgb(bg, highlight_bg, HIGHLIGHT_MIX); - let separator_color = mix_rgb(bg, fg, SEPARATOR_MIX); + let dim_color = mix_rgb(bg, fg, DIM_MIX); let font = find_system_font(); - for (i, (label, highlighted)) in items.iter().enumerate() { - let row_top = (i * row_height) as i32; - if !label.is_empty() && label.chars().all(|c| c == '\u{2500}') { - let y = row_top + row_height as i32 / 2; - fill_rounded_rect_over(&mut buf, width, height, ROW_INSET * 2, y, width as i32 - ROW_INSET * 2, y + 1, 0.0, separator_color); + let mut row_top: i32 = 0; + for (label, highlighted, row_height, kind) in rows.iter().copied() { + let row_height = row_height as i32; + if kind == MenuRowKind::Separator { + let y = row_top + row_height / 2; + fill_rounded_rect_over(&mut buf, width, height, ROW_INSET * 2, y, width as i32 - ROW_INSET * 2, y + 1, 0.0, dim_color); + row_top += row_height; continue; } + let highlighted = highlighted && kind != MenuRowKind::Header; + let label_color = if kind == MenuRowKind::Header { dim_color } else { fg }; // The background text actually sits on, for `blit_glyph`'s own // blend-toward-a-known-solid-colour contract - the row's own // highlight fill (already baked into `buf` by this point, above) @@ -153,9 +194,9 @@ pub fn render_context_menu(width: u32, row_height: u32, items: &[(&str, bool)], // comment) - used correctly, it would leave a visible dark // fringe around every character's anti-aliased edge instead of a // clean blend into the row's real colour. - let row_bg = if *highlighted { highlight_fill } else { bg }; - if *highlighted { - fill_rounded_rect_over(&mut buf, width, height, ROW_INSET, row_top, width as i32 - ROW_INSET, row_top + row_height as i32, ROW_RADIUS, highlight_fill); + let row_bg = if highlighted { highlight_fill } else { bg }; + if highlighted { + fill_rounded_rect_over(&mut buf, width, height, ROW_INSET, row_top, width as i32 - ROW_INSET, row_top + row_height, ROW_RADIUS, highlight_fill); } if let Some(font) = &font { let baseline = row_top as f32 + row_height as f32 * 0.72; @@ -168,7 +209,7 @@ pub fn render_context_menu(width: u32, row_height: u32, items: &[(&str, bool)], if metrics.width > 0 && metrics.height > 0 { let glyph_x = pen_x + metrics.xmin as f32; let glyph_y = baseline - metrics.height as f32 - metrics.ymin as f32; - blit_glyph(&mut buf, width, height, glyph_x.round() as i32, glyph_y.round() as i32, &metrics, &coverage, row_bg, fg); + blit_glyph(&mut buf, width, height, glyph_x.round() as i32, glyph_y.round() as i32, &metrics, &coverage, row_bg, label_color); } pen_x += metrics.advance_width; if pen_x as usize >= width { @@ -176,6 +217,7 @@ pub fn render_context_menu(width: u32, row_height: u32, items: &[(&str, bool)], } } } + row_top += row_height; } buf } diff --git a/crates/wayland/src/decoration/tests.rs b/crates/wayland/src/decoration/tests.rs index b14cbca..f2a9ded 100644 --- a/crates/wayland/src/decoration/tests.rs +++ b/crates/wayland/src/decoration/tests.rs @@ -790,18 +790,18 @@ fn border_bottom_extra_rows_are_transparent_outside_the_corners() { } #[test] -fn context_menu_is_one_row_tall_per_item() { - let items = [("Minimize", false), ("Maximize", false), ("Always on Top", false), ("Close", false)]; - let buf = render_context_menu(160, 28, &items, (0x2e, 0x34, 0x40), (0xff, 0xff, 0xff), (0x4c, 0x56, 0x6a), (0x10, 0x10, 0x10)); +fn context_menu_is_the_sum_of_each_rows_own_height() { + let items = [("Minimize", false, 28, MenuRowKind::Item), ("Maximize", false, 28, MenuRowKind::Item), ("Always on Top", false, 28, MenuRowKind::Item), ("Close", false, 28, MenuRowKind::Item)]; + let buf = render_context_menu(160, &items, (0x2e, 0x34, 0x40), (0xff, 0xff, 0xff), (0x4c, 0x56, 0x6a), (0x10, 0x10, 0x10)); assert_eq!(buf.len(), 160 * (28 * 4) * 4); } #[test] fn context_menu_highlighted_row_has_a_different_background_than_the_rest() { - let items = [("Minimize", false), ("Close", true)]; + let items = [("Minimize", false, 28, MenuRowKind::Item), ("Close", true, 28, MenuRowKind::Item)]; let bg = (0x2e, 0x34, 0x40); let highlight = (0x4c, 0x56, 0x6a); - let buf = render_context_menu(160, 28, &items, bg, (0xff, 0xff, 0xff), highlight, (0x10, 0x10, 0x10)); + let buf = render_context_menu(160, &items, bg, (0xff, 0xff, 0xff), highlight, (0x10, 0x10, 0x10)); let width = 160usize; // Sample a background pixel from each row, away from the text/border. let px_at = |x: usize, y: usize| -> [u8; 3] { @@ -830,8 +830,8 @@ fn context_menu_panel_is_opaque_in_the_middle_but_rounded_at_the_corners() { // opaque - the opposite of what this test used to assert - while an // edge's midpoint (away from any corner's curve) and the panel's own // interior stay fully opaque either way. - let items = [("Close", false)]; - let buf = render_context_menu(100, 28, &items, (0, 0, 0), (0xff, 0xff, 0xff), (0, 0, 0), (0x99, 0x99, 0x99)); + let items = [("Close", false, 28, MenuRowKind::Item)]; + let buf = render_context_menu(100, &items, (0, 0, 0), (0xff, 0xff, 0xff), (0, 0, 0), (0x99, 0x99, 0x99)); let alpha_at = |x: usize, y: usize| buf[(y * 100 + x) * 4 + 3]; assert_eq!(alpha_at(0, 0), 0, "the exact corner pixel is now outside the rounded curve, not a hard square"); // 2px in from the flat top/bottom edges, at the midpoint (far enough @@ -844,15 +844,18 @@ fn context_menu_panel_is_opaque_in_the_middle_but_rounded_at_the_corners() { } #[test] -fn a_pure_separator_row_draws_a_narrow_line_not_a_full_text_row() { - // A label of only `\u{2500}` renders as a thin graphical line instead - // of text glyphs - see `render_context_menu`'s own doc comment for - // why (Unicode box-drawing glyphs render inconsistently at small +fn a_separator_row_draws_a_narrow_line_not_a_full_text_row() { + // `MenuRowKind::Separator` renders as a thin graphical line regardless + // of its (empty) label - see `render_context_menu`'s own doc comment + // for why (Unicode box-drawing glyphs render inconsistently at small // sizes; a real line reads as an intentional divider). Scans every // pixel in the row (not one fixed column, which could accidentally // land in a font glyph's own hollow spot) - a hairline should touch // only a couple of the row's own pixel rows, nowhere near a real text - // row's spread (see the sibling test just below). + // row's spread (see the sibling test just below). Given its own much + // smaller `SEPARATOR_HEIGHT`-sized slot (9px here), not the item rows' + // 28px, so this also exercises variable row heights, not just the + // divider's own look. // // Three items, separator in the middle: `fill_rounded_rect`'s own // distance field softens alpha within `PANEL_RADIUS` of *any* of the @@ -868,8 +871,9 @@ fn a_pure_separator_row_draws_a_narrow_line_not_a_full_text_row() { // so only the separator itself can account for a non-`bg` pixel here. let bg = (0x2e, 0x34, 0x40); let width = 160usize; - let items = [("Open", false), ("\u{2500}\u{2500}\u{2500}\u{2500}\u{2500}", false), ("Close", false)]; - let buf = render_context_menu(width as u32, 28, &items, bg, (0xff, 0xff, 0xff), (0x4c, 0x56, 0x6a), (0x10, 0x10, 0x10)); + const SEPARATOR_HEIGHT: u32 = 9; + let items = [("Open", false, 28, MenuRowKind::Item), ("", false, SEPARATOR_HEIGHT, MenuRowKind::Separator), ("Close", false, 28, MenuRowKind::Item)]; + let buf = render_context_menu(width as u32, &items, bg, (0xff, 0xff, 0xff), (0x4c, 0x56, 0x6a), (0x10, 0x10, 0x10)); let px_at = |x: usize, y: usize| -> [u8; 3] { let i = (y * width + x) * 4; [buf[i + 2], buf[i + 1], buf[i]] @@ -878,31 +882,43 @@ fn a_pure_separator_row_draws_a_narrow_line_not_a_full_text_row() { let safe_x = (PANEL_RADIUS + 2)..(width - PANEL_RADIUS - 2); let row_has_any_non_bg = |y: usize| safe_x.clone().any(|x| px_at(x, y) != [bg.0, bg.1, bg.2]); let separator_row_top = 28; - let non_bg_rows = (separator_row_top..separator_row_top + 28).filter(|&y| row_has_any_non_bg(y)).count(); + let non_bg_rows = (separator_row_top..separator_row_top + SEPARATOR_HEIGHT as usize).filter(|&y| row_has_any_non_bg(y)).count(); assert!(non_bg_rows <= 3, "a hairline separator should touch only a couple of the row's pixel rows, got {non_bg_rows}"); assert!(non_bg_rows >= 1, "the separator must actually draw something, not vanish entirely"); } #[test] -fn a_labeled_section_header_separator_still_renders_as_text() { - // `"─── Move to Workspace ───"` deliberately mixes the divider - // character with real text (`core::ContextMenu`'s own section-header - // convention) - it must keep rendering as text, not collapse into a - // plain hairline just because it contains `\u{2500}` characters too. - // Same whole-row scan and same "sandwich away from the panel's own - // rounded-edge antialiasing" shape as the sibling test above. +fn a_header_row_renders_dimmed_text_not_a_divider_and_ignores_highlight() { + // `MenuRowKind::Header` replaced an earlier hack that faked a section + // caption by embedding box-drawing characters directly in an ordinary + // item's label (`"─── Move to Workspace ───"`) - indistinguishable + // from a real, clickable row except for the dashes, and reported live + // as looking exactly like that: a menu item that does nothing. A real + // header row must still render as text (not collapse into a hairline + // the way `Separator` does), and must never show a hover highlight + // even if the caller passes `highlighted: true` for it by mistake -- + // a section caption isn't a target, so nothing should ever suggest it + // is one. let bg = (0x2e, 0x34, 0x40); let width = 200usize; - let items = [("Open", false), ("\u{2500}\u{2500}\u{2500} Move to Workspace \u{2500}\u{2500}\u{2500}", false), ("Close", false)]; - let buf = render_context_menu(width as u32, 28, &items, bg, (0xff, 0xff, 0xff), (0x4c, 0x56, 0x6a), (0x10, 0x10, 0x10)); + const HEADER_HEIGHT: u32 = 22; + let items = [("Open", false, 28, MenuRowKind::Item), ("Move to Workspace", true, HEADER_HEIGHT, MenuRowKind::Header), ("Close", false, 28, MenuRowKind::Item)]; + let buf = render_context_menu(width as u32, &items, bg, (0xff, 0xff, 0xff), (0x4c, 0x56, 0x6a), (0x10, 0x10, 0x10)); let px_at = |x: usize, y: usize| -> [u8; 3] { let i = (y * width + x) * 4; [buf[i + 2], buf[i + 1], buf[i]] }; let row_has_any_non_bg = |y: usize| (0..width).any(|x| px_at(x, y) != [bg.0, bg.1, bg.2]); let header_row_top = 28; - let non_bg_rows = (header_row_top..header_row_top + 28).filter(|&y| row_has_any_non_bg(y)).count(); - assert!(non_bg_rows > 3, "a text row (mixed divider+caption) should paint well more than a hairline's worth of rows, got {non_bg_rows}"); + let non_bg_rows = (header_row_top..header_row_top + HEADER_HEIGHT as usize).filter(|&y| row_has_any_non_bg(y)).count(); + assert!(non_bg_rows > 3, "a header's own caption text should paint well more than a hairline's worth of rows, got {non_bg_rows}"); + // No highlight fill anywhere in the header's own row band, even though + // `highlighted: true` was passed for it above - a flat highlight fill + // would show up as a large, uniform block of `highlight_bg`-tinted + // pixels well away from the text glyphs themselves; sampling a point + // near the row's own right edge (past where "Move to Workspace" 's + // text reaches) catches that without depending on exact glyph shapes. + assert_eq!(px_at(width - 10, header_row_top + 10), [bg.0, bg.1, bg.2], "a header row must never show the hover highlight fill"); } #[test] diff --git a/crates/wayland/src/input/pointer.rs b/crates/wayland/src/input/pointer.rs index 2ecc15c..c6ed5ba 100644 --- a/crates/wayland/src/input/pointer.rs +++ b/crates/wayland/src/input/pointer.rs @@ -514,14 +514,15 @@ pub(crate) fn handle_pointer_button(state: &mut CompState, pos: Point<f64, Logic if let Some(menu) = state.context_menu.take() { if let Some(row) = menu.row_at(pos.x as i32, pos.y as i32) { let (_, action) = menu.items[row]; - // A separator row occupies real space (`row_at` resolves a - // click on it same as any other) but isn't a real action -- - // same "click does nothing, menu stays open" convention any - // native menu's own divider follows, rather than either - // running a no-op action or dismissing the whole menu on - // what was very possibly a slightly-off click at a real - // item just above/below it. - if matches!(action, crate::context_menu::MenuAction::Separator) { + // A separator or section-header row occupies real space + // (`row_at` resolves a click on it same as any other) but + // isn't a real action - same "click does nothing, menu + // stays open" convention any native menu's own divider/ + // caption follows, rather than either running a no-op + // action or dismissing the whole menu on what was very + // possibly a slightly-off click at a real item just + // above/below it. + if !action.is_interactive() { state.context_menu = Some(menu); return; } diff --git a/crates/wayland/src/state/desktop_icons.rs b/crates/wayland/src/state/desktop_icons.rs index 8ed22a5..9d2dd91 100644 --- a/crates/wayland/src/state/desktop_icons.rs +++ b/crates/wayland/src/state/desktop_icons.rs @@ -593,8 +593,22 @@ impl CompState { fn build_desktop_menu_buffer(&mut self, menu: DesktopMenu) { let theme = self.wm.borrow().theme; - let items: Vec<(&str, bool)> = menu.items.iter().map(|&(label, _)| (label, false)).collect(); - let data = decoration::render_context_menu(menu.width, menu.row_height, &items, theme.titlebar_bg, theme.titlebar_fg_focused, theme.titlebar_fg_unfocused, theme.default_border_color); + // Not redesigned the way the titlebar menu was (`srdwm_core:: + // context_menu`'s own module doc comment) - this menu's rows are + // still all one uniform height, `Separator` included, matching + // its behaviour before `render_context_menu` grew a per-row + // height/kind. Every row here is real content or `DesktopMenuAction + // ::Separator`, never a non-interactive caption, so there is no + // `MenuRowKind::Header` case to map to. + let rows: Vec<(&str, bool, u32, decoration::MenuRowKind)> = menu + .items + .iter() + .map(|(label, action)| { + let kind = if matches!(action, crate::desktop_menu::DesktopMenuAction::Separator) { decoration::MenuRowKind::Separator } else { decoration::MenuRowKind::Item }; + (*label, false, menu.row_height, kind) + }) + .collect(); + let data = decoration::render_context_menu(menu.width, &rows, theme.titlebar_bg, theme.titlebar_fg_focused, theme.titlebar_fg_unfocused, theme.default_border_color); let buffer = MemoryRenderBuffer::from_slice(&data, Fourcc::Argb8888, (menu.width as i32, menu.height()), 1, Transform::Normal, None); self.desktop_menu_buffer = Some(buffer); self.desktop_menu = Some(menu); diff --git a/crates/wayland/src/state/menu.rs b/crates/wayland/src/state/menu.rs index b8cf094..b0b0a6e 100644 --- a/crates/wayland/src/state/menu.rs +++ b/crates/wayland/src/state/menu.rs @@ -14,8 +14,20 @@ impl CompState { return; }; let theme = self.wm.borrow().theme; - let items: Vec<(&str, bool)> = menu.items.iter().map(|&(label, _)| (label, false)).collect(); - let data = decoration::render_context_menu(menu.width, menu.row_height, &items, theme.titlebar_bg, theme.titlebar_fg_focused, theme.titlebar_fg_unfocused, theme.default_border_color); + let rows: Vec<(&str, bool, u32, decoration::MenuRowKind)> = menu + .items + .iter() + .enumerate() + .map(|(i, &(label, action))| { + let kind = match action { + crate::context_menu::MenuAction::Separator => decoration::MenuRowKind::Separator, + crate::context_menu::MenuAction::Header => decoration::MenuRowKind::Header, + _ => decoration::MenuRowKind::Item, + }; + (label, false, menu.row_height_for(i), kind) + }) + .collect(); + let data = decoration::render_context_menu(menu.width, &rows, theme.titlebar_bg, theme.titlebar_fg_focused, theme.titlebar_fg_unfocused, theme.default_border_color); let buffer = MemoryRenderBuffer::from_slice(&data, Fourcc::Argb8888, (menu.width as i32, menu.height()), 1, Transform::Normal, None); self.context_menu_buffer = Some(buffer); self.context_menu = Some(menu); @@ -60,17 +72,54 @@ impl CompState { MenuAction::MoveToWorkspace(workspace) => { self.wm.borrow_mut().move_window_to_workspace(window, workspace); } + MenuAction::CycleButtonStyle => { + let mut wm = self.wm.borrow_mut(); + wm.theme.traffic_light_buttons = !wm.theme.traffic_light_buttons; + drop(wm); + self.redraw_every_decoration(); + } + MenuAction::CycleButtonSide => { + let mut wm = self.wm.borrow_mut(); + wm.theme.buttons_left = !wm.theme.buttons_left; + drop(wm); + self.redraw_every_decoration(); + } MenuAction::Close => { if let Some(w) = self.id_to_window.get(&window) { crate::input::close_dwindow(w); } } // Never actually reached: the click-dispatch site - // (`input/pointer.rs`) intercepts `Separator` before calling - // this function at all. Handled here too so this match stays - // exhaustive without a catch-all that would silently swallow a - // real future variant added without updating this function. - MenuAction::Separator => {} + // (`input/pointer.rs`) intercepts `Separator`/`Header` before + // calling this function at all. Handled here too so this + // match stays exhaustive without a catch-all that would + // silently swallow a real future variant added without + // updating this function. + MenuAction::Separator | MenuAction::Header => {} + } + } + + /// Rebuilds every open window's titlebar/border bitmap against + /// whatever `wm.theme` currently holds - what `CycleButtonStyle`/ + /// `CycleButtonSide` need to actually show their effect immediately. + /// + /// `srd set button_style`/`button_side` (`crates/platform/src/ipc/ + /// dispatch.rs`) change the exact same `ThemeConfig` fields but are + /// deliberately scoped to "only affects windows created (or + /// redecorated) after this call" - that crate is backend-agnostic + /// and has no way to reach into a Wayland-specific redraw. A titlebar + /// menu action has no such excuse: it's *this* backend's own code, + /// already holding `&mut self`, and a "customize" action that doesn't + /// visibly change the very titlebar you clicked would be exactly the + /// kind of "doesn't make sense" this menu was reported for in the + /// first place. `redraw_decoration_buffer` already no-ops on any + /// window whose decoration signature didn't actually change, so + /// calling it for every window here costs nothing for the (common) + /// case where most of them don't use server-side decoration at all. + fn redraw_every_decoration(&mut self) { + let ids: Vec<WindowId> = self.wm.borrow().windows().map(|w| w.id).collect(); + for id in ids { + self.redraw_decoration_buffer(id); } } diff --git a/crates/x11/src/platform/actions.rs b/crates/x11/src/platform/actions.rs index 9d31d01..4847399 100644 --- a/crates/x11/src/platform/actions.rs +++ b/crates/x11/src/platform/actions.rs @@ -35,7 +35,7 @@ impl X11Platform { Ok(()) } - fn redraw_all_decorations(&mut self) -> PlatformResult<()> { + pub(super) fn redraw_all_decorations(&mut self) -> PlatformResult<()> { let focused = self.wm.borrow().focused_id(); let ids: Vec<WindowId> = self.frames.keys().copied().collect(); for id in ids { diff --git a/crates/x11/src/platform/context_menu.rs b/crates/x11/src/platform/context_menu.rs index 9238f6a..e6b4b7f 100644 --- a/crates/x11/src/platform/context_menu.rs +++ b/crates/x11/src/platform/context_menu.rs @@ -80,7 +80,7 @@ impl X11Platform { pub(super) fn redraw_context_menu(&mut self) -> PlatformResult<()> { let Some((menu, popup)) = &self.context_menu else { return Ok(()) }; let popup = *popup; - let (width, row_height, total_height) = (menu.width, menu.row_height, menu.height()); + let (width, total_height) = (menu.width, menu.height()); let theme = self.wm.borrow().theme; let bg = rgb_to_pixel(theme.titlebar_bg); let fg = rgb_to_pixel(theme.titlebar_fg_focused); @@ -89,14 +89,27 @@ impl X11Platform { self.conn.poly_fill_rectangle(popup, self.gc, &[Rectangle { x: 0, y: 0, width: width as u16, height: total_height as u16 }]).map_err(err)?; self.conn.change_gc(self.gc, &ChangeGCAux::new().foreground(fg).font(self.font)).map_err(err)?; + // Per-row heights, not a uniform `row_height * i` - a `Separator`/ + // `Header` row is shorter than a real item (`ContextMenu::row_ + // height_for`), same variable-height model the Wayland backend's + // own `render_context_menu` now uses; `menu.row_y(i)` is exactly + // how that side computes each row's own top too, so the two + // backends can't drift onto different geometry for the same menu + // data. No dimmed-text treatment for a `Header` row here (this + // backend draws everything through one single-colour `GC`, no + // per-pixel blending the way the Wayland renderer's own `mix_rgb` + // has) - feature parity (non-interactive, correctly sized) over + // pixel parity, matching this backend's existing bar elsewhere. for (i, (label, action)) in menu.items.iter().enumerate() { - let row_y = i as i32 * row_height as i32; + let row_y = menu.row_y(i); + let row_height = menu.row_height_for(i) as i32; if matches!(action, MenuAction::Separator) { - let mid = row_y + row_height as i32 / 2; + let mid = row_y + row_height / 2; self.conn.poly_line(CoordMode::ORIGIN, popup, self.gc, &[Point { x: 8, y: mid as i16 }, Point { x: width as i16 - 8, y: mid as i16 }]).map_err(err)?; continue; } - self.conn.image_text8(popup, self.gc, 10, (row_y + 20) as i16, label.as_bytes()).map_err(err)?; + let baseline = row_y + row_height * 3 / 4; + self.conn.image_text8(popup, self.gc, 10, baseline as i16, label.as_bytes()).map_err(err)?; } self.conn.flush().map_err(err)?; Ok(()) @@ -144,8 +157,28 @@ impl X11Platform { MenuAction::MoveToWorkspace(workspace) => { self.wm.borrow_mut().move_window_to_workspace(window, workspace); } + // Same reasoning as the Wayland backend's own `redraw_every_ + // decoration` (`crates/wayland/src/state/menu.rs`): `srd set + // button_style`/`button_side` are deliberately scoped to + // "takes effect on windows created after this call" because + // `crates/platform` is backend-agnostic and has no redraw hook + // to call - this menu action runs as this backend's own + // code, already holding `&mut self`, so it can and should + // repaint every open titlebar immediately instead. + MenuAction::CycleButtonStyle => { + let mut wm = self.wm.borrow_mut(); + wm.theme.traffic_light_buttons = !wm.theme.traffic_light_buttons; + drop(wm); + self.redraw_all_decorations()?; + } + MenuAction::CycleButtonSide => { + let mut wm = self.wm.borrow_mut(); + wm.theme.buttons_left = !wm.theme.buttons_left; + drop(wm); + self.redraw_all_decorations()?; + } MenuAction::Close => self.request_close(window)?, - MenuAction::Separator => {} + MenuAction::Separator | MenuAction::Header => {} } self.conn.flush().map_err(err)?; Ok(()) diff --git a/crates/x11/src/platform/events.rs b/crates/x11/src/platform/events.rs index 10f96a9..545e8f3 100644 --- a/crates/x11/src/platform/events.rs +++ b/crates/x11/src/platform/events.rs @@ -89,7 +89,7 @@ impl X11Platform { (menu.row_at(x, y).map(|r| menu.items[r].1), menu.window) }; match row_action { - Some(srdwm_core::context_menu::MenuAction::Separator) => {} + Some(action) if !action.is_interactive() => {} Some(action) => { self.close_context_menu()?; self.run_context_menu_action(menu_window, action)?; |