From 786756faf57e9ac58725bae0eb61ead1bf964352 Mon Sep 17 00:00:00 2001 From: "Youri T. K. K. Mattar" Date: Thu, 17 Sep 2026 16:18:03 -0300 Subject: [PATCH 1/4] feat(ui): the panes are divided where you drag them, not where I guessed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three numbers decided how the main window was divided: a 230px sidebar, a 6:4 graph against the detail pane, and a 280px file list inside the working directory. Each was a guess about a window nobody had seen, and no window could argue with it — a long commit message on a laptop screen had four tenths of the height and no way to ask for more. Every divider between those panes is draggable now, the file list included. ## Two units, and the reason for each The sidebar and the file list are pixels: what they need is set by the length of a branch name or a path, and that does not grow because the window did. The graph and the detail pane are a fraction of the height they share, because they are two halves of one view — a taller window should keep the graph the subject and the commit under it the footnote. Nothing can be dragged out of existence. The pixel splits stop at a minimum and at half the space they divide; the fraction keeps 15% for each side. A pane with no height has no divider left to drag it back with. ## How a drag arrives `mouse_area` reports that a press happened, not where, so the first pointer movement after a grab is the anchor and every size is a delta from it. That is what makes `layout.rs` testable without a window: nothing in it knows where anything is on screen. The pointer subscription exists only while a divider is held — following every mouse movement for a session to serve a drag nobody is doing is the most expensive subscription this could have had. `extent` — the size of the space being divided — is the one thing the arithmetic cannot derive, so the view measures it with `responsive` and the grab carries it. ## Where it is written `state.toml`, beside the window's own geometry, on release rather than per frame. It is state, not a setting: it changes by dragging, and nobody is going to open a file to choose a number of pixels. It is clamped again on the way in, so a hand-edited file cannot open hideGit with a sidebar wider than the window. A state file written before this exists loads unchanged. Double-clicking a divider restores it, which is why none of this needs a settings entry: the control that moved it is the control that undoes it. ## What it cost `staging::view` reached fourteen arguments — already flagged in #86 as wanting a struct, and still not this change. Its file list became its own function, because the row it sits in is built inside a `responsive` closure that may run more than once and therefore cannot consume anything. ## Tests Thirteen on the arithmetic, three on the divider widget — including that a press beside it grabs nothing — and five driving `update`, one of which holds that a divider dragged inside the working directory resizes the window's layout rather than the repository's. Two on the config file: a round trip, and a `state.toml` from before the dividers existed. --- crates/hidegit-ui/src/app.rs | 164 ++++++++- crates/hidegit-ui/src/layout.rs | 402 +++++++++++++++++++++ crates/hidegit-ui/src/lib.rs | 2 + crates/hidegit-ui/src/message.rs | 34 ++ crates/hidegit-ui/src/metrics.rs | 3 +- crates/hidegit-ui/src/screen/repository.rs | 45 ++- crates/hidegit-ui/src/state.rs | 7 + crates/hidegit-ui/src/widget/detail.rs | 2 + crates/hidegit-ui/src/widget/sidebar.rs | 4 +- crates/hidegit-ui/src/widget/split.rs | 174 +++++++++ crates/hidegit-ui/src/widget/staging.rs | 94 +++-- crates/hidegit/src/config.rs | 75 ++++ crates/hidegit/src/main.rs | 22 +- docs/ROADMAP.md | 7 + docs/UI_SPEC.md | 25 +- 15 files changed, 1011 insertions(+), 49 deletions(-) create mode 100644 crates/hidegit-ui/src/layout.rs create mode 100644 crates/hidegit-ui/src/widget/split.rs diff --git a/crates/hidegit-ui/src/app.rs b/crates/hidegit-ui/src/app.rs index 86674b3..cff771a 100644 --- a/crates/hidegit-ui/src/app.rs +++ b/crates/hidegit-ui/src/app.rs @@ -28,8 +28,8 @@ use iced::{Element as IcedElement, Subscription, Task, keyboard}; use hidegit_forge::{NewPullRequest, WebTarget}; use crate::message::{ - CommitLoad, Message, OpenedRepository, OperationOutcome, Page, PrsLoad, Refreshed, RepoMessage, - StatusLoad, UiError, + CommitLoad, LayoutMessage, Message, OpenedRepository, OperationOutcome, Page, PrsLoad, + Refreshed, RepoMessage, StatusLoad, UiError, }; use crate::state::{ ActionSheet, App, Confirmation, DetailPane, Draft, GraphView, OpenRepo, Operation, PAGE_SIZE, @@ -494,6 +494,8 @@ impl Hidegit { Task::none() } + Message::Layout(message) => self.update_layout(message), + Message::RememberGeometryToggled => { // Nothing in this crate acts on it; the shell reads it back out // and decides what to write. Kept here because the panel that @@ -1350,13 +1352,43 @@ impl Hidegit { Task::batch(indices.into_iter().map(|index| self.poll_task(index))) } + /// A drag on a pane divider, wherever it was raised from. + /// + /// The arithmetic is [`crate::layout::Layout`]'s; what happens here is the + /// mapping from four messages onto four of its methods. Nothing is loaded, + /// nothing is written, and nothing is redrawn that iced would not redraw + /// anyway — a divider drag is the cheapest message in the application, and + /// during a drag it arrives on every frame. + fn update_layout(&mut self, message: LayoutMessage) -> Task { + match message { + LayoutMessage::Grabbed(split, extent) => self.app.layout.grab(split, extent), + LayoutMessage::Dragged(x, y) => self.app.layout.drag_to(x, y), + LayoutMessage::Released => self.app.layout.release(), + LayoutMessage::Reset(split) => self.app.layout.reset(split), + } + + Task::none() + } + fn update_repo(&mut self, index: usize, message: RepoMessage) -> Task { + // Answered before the repository is even looked up: a divider inside + // the working directory addresses the window's layout, not this + // repository, and it only arrives here because the view it was dragged + // in speaks `RepoMessage`. + if let RepoMessage::Layout(message) = message { + return self.update_layout(message); + } + let Some(repo) = self.app.repos.get_mut(index) else { return Task::none(); }; let cache = self.caches.entry(index).or_default(); match message { + // Answered above, before `repo` was borrowed. The arm is here + // because the match has to be exhaustive, not because it can run. + RepoMessage::Layout(_) => Task::none(), + RepoMessage::Selected(selection) => { repo.selection = Some(selection.clone()); // A filter written against the last commit's files would hide @@ -3569,7 +3601,36 @@ impl Hidegit { _ => None, }); - Subscription::batch([keys, focus].into_iter().chain(watches).chain(polls)) + // Where the pointer is, but only while a divider is being dragged. + // + // A press on a divider says that it happened, not where: `mouse_area` + // reports the press and the window reports the position, and this is + // what joins them. It is switched off the rest of the time on purpose — + // a message per pointer movement for the whole session, to serve a drag + // nobody is doing, is the most expensive subscription in the + // application and the easiest one to avoid. + let pointer = if self.app.layout.is_dragging() { + iced::event::listen_with(|event, _, _| match event { + iced::Event::Mouse(iced::mouse::Event::CursorMoved { position }) => Some( + Message::Layout(LayoutMessage::Dragged(position.x, position.y)), + ), + // Released anywhere, including outside the window: a drag that + // only ended when the pointer came back would keep following it. + iced::Event::Mouse(iced::mouse::Event::ButtonReleased( + iced::mouse::Button::Left, + )) => Some(Message::Layout(LayoutMessage::Released)), + _ => None, + }) + } else { + Subscription::none() + }; + + Subscription::batch( + [keys, focus, pointer] + .into_iter() + .chain(watches) + .chain(polls), + ) } /// Which modal, if any, currently owns the keyboard. @@ -4721,6 +4782,103 @@ mod tests { .collect() } + // ---- dividers ---- + + /// Presses a divider, moves the pointer twice — the first movement is the + /// anchor — and lets go, the way the window delivers a drag. + fn drag_divider(app: &mut Hidegit, split: crate::layout::Split, extent: f32, by: (f32, f32)) { + let _ = app.update(Message::Layout(LayoutMessage::Grabbed(split, extent))); + let _ = app.update(Message::Layout(LayoutMessage::Dragged(500.0, 500.0))); + let _ = app.update(Message::Layout(LayoutMessage::Dragged( + 500.0 + by.0, + 500.0 + by.1, + ))); + let _ = app.update(Message::Layout(LayoutMessage::Released)); + } + + #[test] + fn dragging_the_sidebar_divider_resizes_the_sidebar() { + let mut app = app_with(3); + + drag_divider(&mut app, crate::layout::Split::Sidebar, 1440.0, (60.0, 0.0)); + + assert_eq!( + app.app.layout.sidebar(), + crate::layout::Layout::SIDEBAR + 60.0 + ); + } + + #[test] + fn a_divider_dragged_inside_the_working_directory_resizes_the_same_layout() { + // It arrives as a `RepoMessage` only because the staging view speaks + // one. If this ever stopped reaching the application's layout, the + // file list would resize and forget by the next render. + let mut app = app_with(3); + + let _ = app.update(Message::Repo( + 0, + RepoMessage::Layout(LayoutMessage::Grabbed(crate::layout::Split::Files, 1000.0)), + )); + let _ = app.update(Message::Repo( + 0, + RepoMessage::Layout(LayoutMessage::Dragged(400.0, 300.0)), + )); + let _ = app.update(Message::Repo( + 0, + RepoMessage::Layout(LayoutMessage::Dragged(440.0, 300.0)), + )); + + assert_eq!(app.app.layout.files(), crate::layout::Layout::FILES + 40.0); + } + + #[test] + fn a_divider_addresses_the_window_rather_than_the_repository_it_was_dragged_in() { + // Two tabs, a drag in the second: the sidebar is the window's, so + // switching back must not switch the width back with it. + let mut app = app_with_tabs(2); + + drag_divider(&mut app, crate::layout::Split::Sidebar, 1440.0, (40.0, 0.0)); + let dragged = app.app.layout.sidebar(); + let _ = app.update(Message::OpenRepository(PathBuf::from("/fake/repo-0"))); + + assert_eq!(app.app.layout.sidebar(), dragged); + } + + #[test] + fn the_pointer_is_only_followed_while_a_divider_is_held() { + // What decides whether the subscription that reports every pointer + // movement exists at all. + let mut app = app_with(3); + assert!(!app.app.layout.is_dragging()); + + let _ = app.update(Message::Layout(LayoutMessage::Grabbed( + crate::layout::Split::Detail, + 900.0, + ))); + assert!(app.app.layout.is_dragging()); + + let _ = app.update(Message::Layout(LayoutMessage::Released)); + assert!(!app.app.layout.is_dragging()); + } + + #[test] + fn double_clicking_a_divider_puts_it_back() { + let mut app = app_with(3); + drag_divider( + &mut app, + crate::layout::Split::Sidebar, + 1440.0, + (120.0, 0.0), + ); + assert_ne!(app.app.layout.sidebar(), crate::layout::Layout::SIDEBAR); + + let _ = app.update(Message::Layout(LayoutMessage::Reset( + crate::layout::Split::Sidebar, + ))); + + assert_eq!(app.app.layout.sidebar(), crate::layout::Layout::SIDEBAR); + } + #[test] fn dragging_a_row_across_the_list_moves_only_that_row() { // Remove-and-insert, not a swap. Swapping the ends would send the first diff --git a/crates/hidegit-ui/src/layout.rs b/crates/hidegit-ui/src/layout.rs new file mode 100644 index 0000000..91038dd --- /dev/null +++ b/crates/hidegit-ui/src/layout.rs @@ -0,0 +1,402 @@ +//! How the window divides itself, and what dragging a divider does to it. +//! +//! Until this module existed every division in the main window was a number +//! written into a `view`: the sidebar was `Fixed(230.0)`, the graph and the +//! detail pane below it were `FillPortion(6)` and `FillPortion(4)`, and the +//! file list inside the working directory was `Fixed(280.0)`. Each of those is +//! a reasonable guess about a window nobody had seen yet, and a guess is all it +//! can be — a 13" laptop reading a wide diff and a 32" monitor reading a branch +//! list want opposite things, and neither could ask for them. +//! +//! # Two units, on purpose +//! +//! The sidebar and the file list are held in **pixels**. What they need is set +//! by their contents — a branch name, a path — and that does not get longer +//! because the window did. A sidebar that grew with the window would take space +//! from the diff to show the same names in the same font. +//! +//! The graph and the detail pane are held as a **fraction** of the space they +//! share, because they are two halves of one view rather than a panel beside a +//! document: at 60/40 the graph is the subject and the commit under it is the +//! footnote, and that relationship is what a taller window should preserve. +//! +//! # Why a drag arrives as a delta +//! +//! A press on a divider says nothing about where the pointer is — `mouse_area` +//! reports that a press happened, not its position — so the first movement +//! after the grab is the anchor, and every size is computed from the pointer's +//! distance from it rather than from its absolute position. That is also what +//! makes the arithmetic here testable without a window: nothing in this module +//! knows where anything is on screen. +//! +//! `extent` is the one measurement the view has to supply, because the size of +//! the space being divided cannot be derived from a pointer delta. It is taken +//! at the moment of the grab, from the `responsive` wrapper in +//! [`screen::repository`](crate::screen::repository), and it is what turns a +//! pixel delta into a fraction and what bounds a pixel width. + +/// A divider that can be dragged, named for the pane that moves when it is. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum Split { + /// Between the sidebar and everything to its right. + Sidebar, + /// Between the graph and the detail pane under it. + Detail, + /// Between the file list and the diff, inside the working directory. + Files, +} + +/// A drag in progress. +#[derive(Debug, Clone, Copy, PartialEq)] +struct Drag { + split: Split, + /// The size of the space being divided, measured when the grab happened. + extent: f32, + /// What the split was when it was grabbed. Every position during the drag + /// is computed from this rather than from the previous position, so a drag + /// that leaves and re-enters the window does not accumulate error. + base: f32, + /// Where the pointer was when it first moved after the grab. + anchor: Option<(f32, f32)>, +} + +/// How the window's panes are divided, and the drag that is changing it. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct Layout { + sidebar: f32, + detail: f32, + files: f32, + drag: Option, +} + +impl Default for Layout { + fn default() -> Self { + Self { + sidebar: Self::SIDEBAR, + detail: Self::DETAIL, + files: Self::FILES, + drag: None, + } + } +} + +impl Layout { + /// The sidebar's width before anyone drags it. + pub const SIDEBAR: f32 = 230.0; + /// The detail pane's share of the height it splits with the graph. + pub const DETAIL: f32 = 0.4; + /// The file list's width inside the working directory. + pub const FILES: f32 = 280.0; + + /// Narrow enough to be worth doing, wide enough to still read a branch name. + const SIDEBAR_BOUNDS: (f32, f32) = (160.0, 520.0); + const FILES_BOUNDS: (f32, f32) = (180.0, 620.0); + /// Neither the graph nor the detail pane may be dragged out of existence: a + /// pane with no height is indistinguishable from a bug, and there is no + /// divider left to drag it back with. + const DETAIL_BOUNDS: (f32, f32) = (0.15, 0.85); + + /// The layout a config file asks for, made safe to use. + /// + /// A hand-edited file — or one written by a version that bounded these + /// differently — must not be able to produce a sidebar wider than the + /// window or a detail pane of zero height, so every value goes through the + /// same clamp a drag does. + pub fn restore(sidebar: f32, detail: f32, files: f32) -> Self { + Self { + sidebar: clamp(sidebar, Self::SIDEBAR_BOUNDS, Self::SIDEBAR), + detail: clamp(detail, Self::DETAIL_BOUNDS, Self::DETAIL), + files: clamp(files, Self::FILES_BOUNDS, Self::FILES), + drag: None, + } + } + + /// The sidebar's width, in pixels. + pub fn sidebar(&self) -> f32 { + self.sidebar + } + + /// The detail pane's share of the column it splits with the graph, as a + /// fraction between zero and one. + pub fn detail(&self) -> f32 { + self.detail + } + + /// The file list's width, in pixels. + pub fn files(&self) -> f32 { + self.files + } + + /// Is this divider the one being dragged right now? + /// + /// Only so the divider can say so: a hairline that does not change under + /// the pointer gives no sign that the drag was picked up at all. + pub fn dragging(&self, split: Split) -> bool { + self.drag.is_some_and(|drag| drag.split == split) + } + + /// Is any divider being dragged? + /// + /// The pointer subscription is only alive while this is true: listening to + /// every mouse movement in the session to serve a drag that is not + /// happening is a message per frame for nothing. + pub fn is_dragging(&self) -> bool { + self.drag.is_some() + } + + /// A divider was pressed. `extent` is the size of the space it divides. + pub fn grab(&mut self, split: Split, extent: f32) { + let base = match split { + Split::Sidebar => self.sidebar, + Split::Detail => self.detail, + Split::Files => self.files, + }; + + self.drag = Some(Drag { + split, + extent: if extent.is_finite() && extent > 0.0 { + extent + } else { + f32::INFINITY + }, + base, + anchor: None, + }); + } + + /// The pointer moved, in window coordinates. + pub fn drag_to(&mut self, x: f32, y: f32) { + if !x.is_finite() || !y.is_finite() { + return; + } + + let Some(drag) = &mut self.drag else { + return; + }; + + let Some((from_x, from_y)) = drag.anchor else { + drag.anchor = Some((x, y)); + return; + }; + + let drag = *drag; + match drag.split { + Split::Sidebar => { + self.sidebar = clamp( + drag.base + (x - from_x), + bounded_by(Self::SIDEBAR_BOUNDS, drag.extent), + Self::SIDEBAR, + ); + } + Split::Files => { + self.files = clamp( + drag.base + (x - from_x), + bounded_by(Self::FILES_BOUNDS, drag.extent), + Self::FILES, + ); + } + // The detail pane is *below* the divider, so dragging downwards + // makes it smaller. Getting this sign wrong produces a pane that + // runs away from the pointer, which is why it has its own test. + Split::Detail => { + self.detail = clamp( + drag.base - (y - from_y) / drag.extent, + Self::DETAIL_BOUNDS, + Self::DETAIL, + ); + } + } + } + + /// The pointer was released, wherever it was released. + pub fn release(&mut self) { + self.drag = None; + } + + /// Double-clicking a divider puts it back where it shipped. + /// + /// The way out of a layout somebody has dragged into uselessness, and the + /// reason this needs no settings entry: the control that broke it is the + /// control that fixes it. + pub fn reset(&mut self, split: Split) { + match split { + Split::Sidebar => self.sidebar = Self::SIDEBAR, + Split::Detail => self.detail = Self::DETAIL, + Split::Files => self.files = Self::FILES, + } + // A double click is a press as well, so a grab is in flight by the time + // this arrives. Leaving it would let the next pointer movement drag the + // divider away from the default it was just put back to. + self.drag = None; + } +} + +/// The bounds a pixel split gets inside a space of `extent` pixels. +/// +/// Half the space, at most: a sidebar that can eat the window is a sidebar +/// somebody has to drag back before they can read anything, and on a narrow +/// window the fixed maximum is more than the whole width. +fn bounded_by((low, high): (f32, f32), extent: f32) -> (f32, f32) { + (low, high.min(extent / 2.0).max(low)) +} + +/// Clamps, and refuses `NaN`. +/// +/// `f32::clamp` panics on a `NaN` bound and propagates a `NaN` value, and a +/// size of `NaN` reaches the layout engine as a pane that never draws. +fn clamp(value: f32, (low, high): (f32, f32), fallback: f32) -> f32 { + if value.is_finite() { + value.clamp(low, high) + } else { + fallback + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Grabs a divider and moves the pointer, which takes two movements: the + /// first is the anchor, the second is the drag. + fn dragged(split: Split, extent: f32, from: (f32, f32), to: (f32, f32)) -> Layout { + let mut layout = Layout::default(); + layout.grab(split, extent); + layout.drag_to(from.0, from.1); + layout.drag_to(to.0, to.1); + layout + } + + #[test] + fn dragging_the_sidebar_right_widens_it_by_what_the_pointer_travelled() { + let layout = dragged(Split::Sidebar, 1440.0, (230.0, 400.0), (310.0, 400.0)); + + assert_eq!(layout.sidebar(), Layout::SIDEBAR + 80.0); + } + + #[test] + fn the_first_movement_is_the_anchor_and_moves_nothing() { + // Otherwise the divider would jump to wherever the pointer happened to + // be when the press was reported, which is not where it was pressed. + let mut layout = Layout::default(); + layout.grab(Split::Sidebar, 1440.0); + layout.drag_to(900.0, 400.0); + + assert_eq!(layout.sidebar(), Layout::SIDEBAR); + } + + #[test] + fn the_sidebar_cannot_be_dragged_away_or_over_the_window() { + let shrunk = dragged(Split::Sidebar, 1440.0, (230.0, 400.0), (-900.0, 400.0)); + assert_eq!(shrunk.sidebar(), Layout::SIDEBAR_BOUNDS.0); + + let grown = dragged(Split::Sidebar, 1440.0, (230.0, 400.0), (9000.0, 400.0)); + assert_eq!(grown.sidebar(), Layout::SIDEBAR_BOUNDS.1); + } + + #[test] + fn a_narrow_window_bounds_the_sidebar_before_the_fixed_maximum_does() { + // 520 of a 700-pixel window leaves 180 for the graph, the diff and the + // commit message together. + let layout = dragged(Split::Sidebar, 700.0, (230.0, 400.0), (9000.0, 400.0)); + + assert_eq!(layout.sidebar(), 350.0, "half the window, not the maximum"); + } + + #[test] + fn dragging_the_detail_divider_down_makes_the_detail_pane_smaller() { + // 100 pixels down a 1000-pixel column is a tenth of it. + let layout = dragged(Split::Detail, 1000.0, (700.0, 500.0), (700.0, 600.0)); + + assert!( + (layout.detail() - (Layout::DETAIL - 0.1)).abs() < f32::EPSILON, + "the pane below the divider shrinks as the divider goes down, got {}", + layout.detail() + ); + } + + #[test] + fn neither_pane_can_be_dragged_out_of_existence() { + let flattened = dragged(Split::Detail, 1000.0, (700.0, 500.0), (700.0, 5000.0)); + assert_eq!(flattened.detail(), Layout::DETAIL_BOUNDS.0); + + let raised = dragged(Split::Detail, 1000.0, (700.0, 500.0), (700.0, -5000.0)); + assert_eq!(raised.detail(), Layout::DETAIL_BOUNDS.1); + } + + #[test] + fn a_release_ends_the_drag_and_later_movement_does_nothing() { + let mut layout = Layout::default(); + layout.grab(Split::Sidebar, 1440.0); + layout.drag_to(230.0, 400.0); + layout.drag_to(300.0, 400.0); + layout.release(); + layout.drag_to(600.0, 400.0); + + assert_eq!(layout.sidebar(), Layout::SIDEBAR + 70.0); + assert!(!layout.is_dragging()); + } + + #[test] + fn every_drag_is_measured_from_where_it_started() { + // Not from the previous position: a delta applied to the current size + // every time drifts, because the size is clamped and the pointer is not. + let mut layout = Layout::default(); + layout.grab(Split::Sidebar, 1440.0); + layout.drag_to(230.0, 400.0); + layout.drag_to(-900.0, 400.0); + layout.drag_to(280.0, 400.0); + + assert_eq!( + layout.sidebar(), + Layout::SIDEBAR + 50.0, + "the excursion into the clamp did not move the origin" + ); + } + + #[test] + fn resetting_puts_a_divider_back_and_drops_the_drag_that_did_it() { + let mut layout = dragged(Split::Files, 1200.0, (280.0, 400.0), (500.0, 400.0)); + assert_ne!(layout.files(), Layout::FILES); + + layout.grab(Split::Files, 1200.0); + layout.reset(Split::Files); + + assert_eq!(layout.files(), Layout::FILES); + assert!( + !layout.is_dragging(), + "a double click is a press too, and the drag it armed would undo the reset" + ); + } + + #[test] + fn only_the_divider_being_dragged_says_it_is() { + let mut layout = Layout::default(); + layout.grab(Split::Detail, 1000.0); + + assert!(layout.dragging(Split::Detail)); + assert!(!layout.dragging(Split::Sidebar)); + } + + #[test] + fn a_pointer_that_reports_nonsense_moves_nothing() { + let layout = dragged(Split::Sidebar, 1440.0, (230.0, 400.0), (f32::NAN, 400.0)); + + assert_eq!(layout.sidebar(), Layout::SIDEBAR); + } + + #[test] + fn a_config_file_cannot_ask_for_a_pane_that_does_not_fit() { + let restored = Layout::restore(9000.0, 2.0, -5.0); + + assert_eq!(restored.sidebar(), Layout::SIDEBAR_BOUNDS.1); + assert_eq!(restored.detail(), Layout::DETAIL_BOUNDS.1); + assert_eq!(restored.files(), Layout::FILES_BOUNDS.0); + } + + #[test] + fn a_config_file_full_of_nonsense_falls_back_to_the_defaults() { + let restored = Layout::restore(f32::NAN, f32::INFINITY, f32::NEG_INFINITY); + + assert_eq!(restored, Layout::default()); + } +} diff --git a/crates/hidegit-ui/src/lib.rs b/crates/hidegit-ui/src/lib.rs index 4b0a27f..76f2585 100644 --- a/crates/hidegit-ui/src/lib.rs +++ b/crates/hidegit-ui/src/lib.rs @@ -12,6 +12,7 @@ pub mod format; pub mod highlight; pub mod i18n; pub mod keymap; +pub mod layout; pub mod message; pub mod metrics; pub mod state; @@ -38,6 +39,7 @@ pub mod widget { pub mod settings; pub mod shortcuts; pub mod sidebar; + pub mod split; pub mod staging; pub mod tabs; } diff --git a/crates/hidegit-ui/src/message.rs b/crates/hidegit-ui/src/message.rs index 3b96c36..c378205 100644 --- a/crates/hidegit-ui/src/message.rs +++ b/crates/hidegit-ui/src/message.rs @@ -224,6 +224,25 @@ pub struct Page { pub more: bool, } +/// What a drag on a pane divider does, from press to release. +/// +/// Its own enum rather than four variants on [`Message`], because the same four +/// are needed on [`RepoMessage`] too and writing them twice is how the two +/// copies end up meaning different things. +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum LayoutMessage { + /// A divider was pressed, carrying the size of the space it divides — which + /// only the view that laid it out can know. + Grabbed(crate::layout::Split, f32), + /// The pointer moved, in window coordinates. Delivered by a subscription + /// that is only alive while a drag is in progress. + Dragged(f32, f32), + /// The pointer was released, wherever it happened to be. + Released, + /// A divider was double-clicked: put it back where it shipped. + Reset(crate::layout::Split), +} + #[derive(Debug, Clone)] pub enum Message { /// `Cmd+O`: ask for a repository with the platform's own picker. @@ -327,6 +346,15 @@ pub enum Message { RememberGeometryToggled, RepositoryMuteToggled(String), + /// A divider between two panes was grabbed, dragged or released. + /// + /// Reached from two places, because the dividers are in two places: the + /// main window emits this, and the one inside the working directory arrives + /// as [`RepoMessage::Layout`] and is forwarded here. The layout belongs to + /// the application rather than to a repository — the sidebar is the same + /// width whichever tab is in front — so both end up in the same handler. + Layout(LayoutMessage), + // ---- the forge ---- /// The client exists, and a stored session was restored if there was one. /// @@ -371,6 +399,12 @@ pub enum Message { #[derive(Debug, Clone)] pub enum RepoMessage { // ---- user intent ---- + /// A divider inside the working directory was dragged. + /// + /// Forwarded to [`Message::Layout`] unchanged. The staging view speaks + /// `RepoMessage` all the way down, so a divider placed in it cannot emit a + /// top-level message without every view above it changing its type. + Layout(LayoutMessage), Selected(Selection), /// A wheel or trackpad scroll, in pixels. Positive scrolls toward older /// history. diff --git a/crates/hidegit-ui/src/metrics.rs b/crates/hidegit-ui/src/metrics.rs index ba6608e..386094a 100644 --- a/crates/hidegit-ui/src/metrics.rs +++ b/crates/hidegit-ui/src/metrics.rs @@ -126,7 +126,7 @@ pub mod text { /// pretending otherwise would mean converting everything about a file at once /// or nothing at all. #[cfg(test)] -const ON_THE_LAYOUT_SCALE: &[&str] = &["widget/tabs.rs"]; +const ON_THE_LAYOUT_SCALE: &[&str] = &["widget/split.rs", "widget/tabs.rs"]; /// Files whose every type size comes from [`text`]. #[cfg(test)] @@ -139,6 +139,7 @@ const ON_THE_TYPE_SCALE: &[&str] = &[ "widget/resolver.rs", "widget/search.rs", "widget/sidebar.rs", + "widget/split.rs", "widget/staging.rs", "widget/tabs.rs", ]; diff --git a/crates/hidegit-ui/src/screen/repository.rs b/crates/hidegit-ui/src/screen/repository.rs index 570cff7..d0330ad 100644 --- a/crates/hidegit-ui/src/screen/repository.rs +++ b/crates/hidegit-ui/src/screen/repository.rs @@ -6,12 +6,13 @@ use iced::widget::{Space, button, column, container, responsive, row, text, tool use iced::{Center, Fill, Font, Length, Padding}; use crate::Element; +use crate::layout::Split; use crate::message::{Message, RepoMessage}; use crate::metrics; use crate::state::{App, OpenRepo, Pane, ROW_HEIGHT}; use crate::theme::Palette; use crate::widget::common; -use crate::widget::{detail, graph, sidebar}; +use crate::widget::{detail, graph, sidebar, split}; /// The main window. /// @@ -49,7 +50,7 @@ pub fn view<'a>( // Which pane the keyboard is pointed at, said out loud. `Tab` cycles all // three; before this only the graph showed it, through the tint on its // selected row, so two of the three positions looked identical. - let ring = |pane: Pane, element: Element<'a, Message>| -> Element<'a, Message> { + let ring = move |pane: Pane, element: Element<'a, Message>| -> Element<'a, Message> { let border = crate::theme::focus_ring(repo.focus == pane, palette); container(element) .style(move |_| container::Style { @@ -59,10 +60,27 @@ pub fn view<'a>( .into() }; - stack = stack.push( + // `responsive` because both dividers need a measurement no `view` is + // otherwise given: the height of the column decides what a pixel of + // pointer travel is worth as a fraction, and the width of the window + // decides how wide the sidebar is allowed to get. The layout pass knows + // both; nothing else does. + stack = stack.push(responsive(move |size| { + let layout = &app.layout; + // Portions rather than a fixed height, so the split survives a window + // resize as the proportion it was dragged to. A thousandth is finer + // than a pixel on any monitor this will run on. + let detail = (layout.detail() * 1000.0) as u16; + row![ ring(Pane::Sidebar, sidebar::view(app, repo, index, palette)), - common::vertical_rule(palette).map(repo_message), + split::vertical( + Split::Sidebar, + size.width, + layout.dragging(Split::Sidebar), + palette + ) + .map(Message::Layout), column![ // The portion goes on this container rather than inside // `graph_pane`, because `ring` wraps its child in a container @@ -75,21 +93,28 @@ pub fn view<'a>( Pane::Graph, graph_pane(repo, palette, cache).map(repo_message) )) - .height(Length::FillPortion(6)) + .height(Length::FillPortion(1000 - detail)) .width(Fill), - common::divider(palette).map(repo_message), + split::horizontal( + Split::Detail, + size.height, + layout.dragging(Split::Detail), + palette + ) + .map(Message::Layout), container(ring( Pane::Detail, - detail::view(repo, palette, &app.text).map(repo_message) + detail::view(repo, palette, &app.text, layout).map(repo_message) )) - .height(Length::FillPortion(4)) + .height(Length::FillPortion(detail)) .width(Fill), ] .height(Fill) .width(Fill), ] - .height(Fill), - ); + .height(Fill) + .into() + })); stack.into() } diff --git a/crates/hidegit-ui/src/state.rs b/crates/hidegit-ui/src/state.rs index a76a3f8..8b450bf 100644 --- a/crates/hidegit-ui/src/state.rs +++ b/crates/hidegit-ui/src/state.rs @@ -1332,6 +1332,12 @@ pub struct App { pub focused: bool, /// Which alerts to send, and when not to. pub alerts: AlertPrefs, + /// How the window's panes are divided, and the drag that is changing it. + /// + /// On `App` rather than on `OpenRepo` because it is a property of the + /// window: dragging the sidebar wider with one repository in front and + /// finding it narrow again in the next tab would read as a bug. + pub layout: crate::layout::Layout, /// Write a report to disk when hideGit panics. /// /// Held here for the same reason as the geometry switch: the panel shows it, @@ -1377,6 +1383,7 @@ impl Default for App { // than on demand. focused: true, alerts: AlertPrefs::default(), + layout: crate::layout::Layout::default(), remember_geometry: true, panic_reports: false, check_for_updates: true, diff --git a/crates/hidegit-ui/src/widget/detail.rs b/crates/hidegit-ui/src/widget/detail.rs index 11d5a32..37c0ce3 100644 --- a/crates/hidegit-ui/src/widget/detail.rs +++ b/crates/hidegit-ui/src/widget/detail.rs @@ -16,6 +16,7 @@ pub fn view<'a>( repo: &'a OpenRepo, palette: &'a Palette, text_catalogue: &'a crate::i18n::Catalogue, + layout: &'a crate::layout::Layout, ) -> Element<'a, RepoMessage> { let body: Element<'a, RepoMessage> = match &repo.detail { DetailPane::Empty => common::empty("Select a commit to read its message and diff", palette), @@ -60,6 +61,7 @@ pub fn view<'a>( palette, text_catalogue, repo.head.target(), + layout, ), // The one pane whose contents did not come from the repository. DetailPane::PullRequest(detail) => crate::widget::pr::detail(detail, palette), diff --git a/crates/hidegit-ui/src/widget/sidebar.rs b/crates/hidegit-ui/src/widget/sidebar.rs index 238b866..197aaf6 100644 --- a/crates/hidegit-ui/src/widget/sidebar.rs +++ b/crates/hidegit-ui/src/widget/sidebar.rs @@ -156,7 +156,9 @@ pub fn view<'a>( let palette = *palette; container(scrollable(sections).height(Fill)) - .width(Length::Fixed(230.0)) + // Whatever the divider beside it was last dragged to, rather than the + // 230 pixels this was born with. + .width(Length::Fixed(app.layout.sidebar())) .height(Fill) .style(move |_| container::Style { background: Some(palette.surface.into()), diff --git a/crates/hidegit-ui/src/widget/split.rs b/crates/hidegit-ui/src/widget/split.rs new file mode 100644 index 0000000..563356b --- /dev/null +++ b/crates/hidegit-ui/src/widget/split.rs @@ -0,0 +1,174 @@ +//! The dividers you can drag. +//! +//! A hairline is one pixel wide, and one pixel is not something a pointer can +//! be expected to hit. So a draggable divider is a strip +//! [`GRAB`](self::GRAB) pixels across with the hairline drawn down the middle +//! of it: what you see is the same rule [`common::divider`](crate::widget::common::divider) +//! draws everywhere else, and what you can grab is the part either side of it. +//! +//! It emits [`LayoutMessage`], and nothing else — the screen that places it +//! decides how that reaches `update`. That is what lets the same divider sit in +//! the main window, which speaks `Message`, and inside the working directory, +//! which speaks `RepoMessage`: both map it. + +use iced::widget::{Space, container, mouse_area}; +use iced::{Fill, Length, mouse}; + +use crate::Element; +use crate::layout::Split; +use crate::message::LayoutMessage; +use crate::metrics; +use crate::theme::Palette; + +/// How much of the divider is pointer target rather than hairline. +/// +/// Seven pixels: three either side of the rule. Small enough that the panes +/// keep the space they had to within a few pixels, large enough to hit without +/// aiming — the macOS window-resize border is about this wide, and it is the +/// closest thing to a platform answer. +pub const GRAB: f32 = 7.0; + +/// A divider between two columns: drag it left and right. +pub fn vertical<'a>( + split: Split, + extent: f32, + dragging: bool, + palette: &Palette, +) -> Element<'a, LayoutMessage> { + handle( + split, + extent, + dragging, + palette, + (Length::Fixed(GRAB), Fill), + (Length::Fixed(metrics::HAIR), Fill), + mouse::Interaction::ResizingColumn, + ) +} + +/// A divider between two rows: drag it up and down. +pub fn horizontal<'a>( + split: Split, + extent: f32, + dragging: bool, + palette: &Palette, +) -> Element<'a, LayoutMessage> { + handle( + split, + extent, + dragging, + palette, + (Fill, Length::Fixed(GRAB)), + (Fill, Length::Fixed(metrics::HAIR)), + mouse::Interaction::ResizingRow, + ) +} + +#[allow(clippy::too_many_arguments)] +fn handle<'a>( + split: Split, + extent: f32, + dragging: bool, + palette: &Palette, + (width, height): (Length, Length), + (rule_width, rule_height): (Length, Length), + interaction: mouse::Interaction, +) -> Element<'a, LayoutMessage> { + // The rule brightens while it is being dragged, and only then. Without it + // there is no way to tell a drag that was picked up from a press that + // missed — the panes either side move, but not until the pointer does. + let colour = if dragging { + palette.accent + } else { + palette.border + }; + + let rule = container(Space::new()) + .width(rule_width) + .height(rule_height) + .style(move |_| container::Style { + background: Some(colour.into()), + ..container::Style::default() + }); + + mouse_area( + container(rule) + .width(width) + .height(height) + .center_x(width) + .center_y(height), + ) + .interaction(interaction) + .on_press(LayoutMessage::Grabbed(split, extent)) + // The pointer usually leaves the divider during a drag, so the release that + // ends one arrives through the subscription rather than here. This is for + // the press that never became a drag at all. + .on_release(LayoutMessage::Released) + .on_double_click(LayoutMessage::Reset(split)) + .into() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::theme::Theme; + + /// A divider carries no text, so it cannot be found the way the rest of the + /// render tests find things. What can be asserted is what pressing it says, + /// which is the whole of its job. + fn pressed(element: Element<'_, LayoutMessage>, at: (f32, f32)) -> Vec { + let mut ui = iced_test::simulator(element); + ui.point_at(at); + let _ = ui.simulate(iced_test::simulator::click()); + + ui.into_messages().collect() + } + + #[test] + fn pressing_a_divider_grabs_it_and_carries_the_space_it_divides() { + let palette = Theme::default().palette; + + // Three pixels in: the hairline is one pixel of a seven-pixel strip, so + // this is a press that hits the target and misses the rule. + let messages = pressed( + vertical(Split::Sidebar, 1440.0, false, &palette), + (3.0, 100.0), + ); + + assert_eq!( + messages.first(), + Some(&LayoutMessage::Grabbed(Split::Sidebar, 1440.0)), + "the extent is measured by the view, so the grab has to carry it" + ); + } + + #[test] + fn a_press_beside_the_divider_grabs_nothing() { + let palette = Theme::default().palette; + + // Deliberately outside the strip: the grab area is generous, not + // unbounded, and a divider that swallowed presses meant for the pane + // beside it would be worse than a thin one. + let messages = pressed( + vertical(Split::Sidebar, 1440.0, false, &palette), + (40.0, 100.0), + ); + + assert!(messages.is_empty(), "got {messages:?}"); + } + + #[test] + fn a_horizontal_divider_is_grabbed_across_its_thickness_rather_than_its_length() { + let palette = Theme::default().palette; + + let messages = pressed( + horizontal(Split::Detail, 900.0, true, &palette), + (100.0, 3.0), + ); + + assert_eq!( + messages.first(), + Some(&LayoutMessage::Grabbed(Split::Detail, 900.0)) + ); + } +} diff --git a/crates/hidegit-ui/src/widget/staging.rs b/crates/hidegit-ui/src/widget/staging.rs index b961afe..e793e0a 100644 --- a/crates/hidegit-ui/src/widget/staging.rs +++ b/crates/hidegit-ui/src/widget/staging.rs @@ -10,7 +10,8 @@ //! behind it — the change it destroys was never committed anywhere. use iced::widget::{ - Space, button, column, container, mouse_area, row, scrollable, text, text_input, tooltip, + Space, button, column, container, mouse_area, responsive, row, scrollable, text, text_input, + tooltip, }; use iced::{Center, Fill, Font, Length, Padding}; @@ -25,8 +26,6 @@ use crate::widget::common; use crate::widget::diff; use crate::widget::sidebar::{heading, item_style}; -const LIST_WIDTH: f32 = 280.0; - #[allow(clippy::too_many_arguments)] pub fn view<'a>( status: &'a WorktreeStatus, @@ -42,6 +41,7 @@ pub fn view<'a>( palette: &'a Palette, text_catalogue: &'a crate::i18n::Catalogue, head: Option, + layout: &'a crate::layout::Layout, ) -> Element<'a, RepoMessage> { // Even a clean tree gets the composer: amending the last commit is a real // thing to want, and it needs nothing staged. @@ -49,11 +49,68 @@ pub fn view<'a>( return column![ container(clean(palette, text_catalogue, head)).height(Fill), common::divider(palette), - container(composer(status, draft, state, palette)).width(Length::Fixed(LIST_WIDTH)), + container(composer(status, draft, state, palette)).width(Length::Fixed(layout.files())), ] .into(); } + // Measured, because the divider between the list and the diff has to know + // how much space it is dividing before it can refuse to give away all of + // it. `responsive` is the only thing that knows; the cost is that the two + // sides are built inside the closure rather than before it, which is why + // the list is its own function. + responsive(move |size| { + row![ + column![ + mouse_area( + container(scrollable(files(status, selected, palette)).height(Fill)) + .width(Length::Fixed(layout.files())) + ) + .on_press(RepoMessage::EditingChanged(false)), + common::divider(palette), + composer(status, draft, state, palette), + ] + .width(Length::Fixed(layout.files())), + crate::widget::split::vertical( + crate::layout::Split::Files, + size.width, + layout.dragging(crate::layout::Split::Files), + palette + ) + .map(RepoMessage::Layout), + mouse_area( + container(pane( + staged, + unstaged, + selected, + lines, + focused_hunk, + mode, + state, + resolver, + status.conflicted.len(), + palette, + )) + .width(Fill), + ) + .on_press(RepoMessage::EditingChanged(false)), + ] + .height(Fill) + .into() + }) + .into() +} + +/// The four lists, in the order the working directory is read in. +/// +/// Its own function rather than a local, because the row it sits in is built +/// inside a `responsive` closure — which may run more than once per frame, and +/// therefore cannot consume anything. +fn files<'a>( + status: &'a WorktreeStatus, + selected: Option, + palette: &'a Palette, +) -> Element<'a, RepoMessage> { let mut list = column![].spacing(0); // Conflicts first: nothing else in the working directory matters until @@ -97,34 +154,7 @@ pub fn view<'a>( } } - row![ - column![ - mouse_area(container(scrollable(list).height(Fill)).width(Length::Fixed(LIST_WIDTH))) - .on_press(RepoMessage::EditingChanged(false)), - common::divider(palette), - composer(status, draft, state, palette), - ] - .width(Length::Fixed(LIST_WIDTH)), - common::vertical_rule(palette), - mouse_area( - container(pane( - staged, - unstaged, - selected, - lines, - focused_hunk, - mode, - state, - resolver, - status.conflicted.len(), - palette, - )) - .width(Fill), - ) - .on_press(RepoMessage::EditingChanged(false)), - ] - .height(Fill) - .into() + list.into() } /// The diff for whichever row is open. diff --git a/crates/hidegit/src/config.rs b/crates/hidegit/src/config.rs index eaba314..448172c 100644 --- a/crates/hidegit/src/config.rs +++ b/crates/hidegit/src/config.rs @@ -156,6 +156,56 @@ pub struct State { /// filename is enough: they are named by the moment they happened, so a /// newer one is a different one. pub announced_panic: Option, + /// Where the window's dividers were left. + pub panes: Panes, +} + +/// Where the dividers between the panes were left. +/// +/// State rather than settings: it is changed by dragging a divider, not by the +/// settings panel, and nobody is expected to open this file to choose a number +/// of pixels. It is written for the same reason the window's size is — having +/// to redo the same three drags on every start is the kind of small friction +/// that makes an application feel unfinished. +/// +/// The interface owns what these mean and what range each may take, so the +/// defaults are its defaults, and a value read from here goes back through its +/// clamp before it is used. +#[derive(Debug, Clone, Copy, Serialize, Deserialize)] +#[serde(default, deny_unknown_fields)] +pub struct Panes { + /// The sidebar's width, in pixels. + pub sidebar: f32, + /// The detail pane's share of the height it splits with the graph. + pub detail: f32, + /// The file list's width inside the working directory, in pixels. + pub files: f32, +} + +impl Default for Panes { + fn default() -> Self { + Self { + sidebar: hidegit_ui::layout::Layout::SIDEBAR, + detail: hidegit_ui::layout::Layout::DETAIL, + files: hidegit_ui::layout::Layout::FILES, + } + } +} + +impl From for Panes { + fn from(layout: hidegit_ui::layout::Layout) -> Self { + Self { + sidebar: layout.sidebar(), + detail: layout.detail(), + files: layout.files(), + } + } +} + +impl From for hidegit_ui::layout::Layout { + fn from(panes: Panes) -> Self { + Self::restore(panes.sidebar, panes.detail, panes.files) + } } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -406,6 +456,11 @@ mod tests { announced_panic: None, announced_update: None, last_update_check: None, + panes: Panes { + sidebar: 310.0, + detail: 0.55, + files: 240.0, + }, }; save(&path, &state); @@ -413,6 +468,26 @@ mod tests { assert_eq!(read.window.width, 1200.0); assert_eq!(read.recents.len(), 1); assert_eq!(read.recents[0].path, PathBuf::from("/src/hideGit")); + assert_eq!( + read.panes.sidebar, 310.0, + "a dragged divider survives a restart" + ); + assert_eq!(read.panes.detail, 0.55); + } + + #[test] + fn a_state_file_from_before_the_dividers_existed_still_loads() { + // `deny_unknown_fields` cuts both ways: a *missing* table has to be + // fine, or the first run after an upgrade loses the whole session — + // recents, window position and all — over a section nobody wrote yet. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state.toml"); + std::fs::write(&path, "[window]\nwidth = 1200.0\nheight = 800.0\n").unwrap(); + + let read: State = load(&path); + + assert_eq!(read.window.width, 1200.0); + assert_eq!(read.panes.sidebar, hidegit_ui::layout::Layout::SIDEBAR); } #[test] diff --git a/crates/hidegit/src/main.rs b/crates/hidegit/src/main.rs index fc543a4..aece97a 100644 --- a/crates/hidegit/src/main.rs +++ b/crates/hidegit/src/main.rs @@ -10,6 +10,7 @@ mod crash; use std::path::PathBuf; use hidegit_core::{GitError, MINIMUM_GIT_VERSION, git_preflight}; +use hidegit_ui::message::{LayoutMessage, RepoMessage}; use hidegit_ui::{Hidegit, Message}; use iced::{Size, Subscription, Task, window}; @@ -153,6 +154,7 @@ fn main() -> iced::Result { unannounced_panic: unannounced, announced_update: state.announced_update, last_update_check: state.last_update_check, + panes: state.panes, }; // `mut` is only used on Linux; see the cfg block below. @@ -255,6 +257,8 @@ struct Session { unannounced_panic: Option, announced_update: Option, last_update_check: Option, + /// Where the dividers between the panes were left. + panes: config::Panes, } fn boot( @@ -270,6 +274,7 @@ fn boot( unannounced_panic, announced_update, last_update_check, + panes, } = session; // Off disk once, here, rather than every time the panel is opened: a theme @@ -320,6 +325,9 @@ fn boot( // Set here rather than passed through `Hidegit::new`: the interface only // shows this one, and the shell is what acts on it. ui.app.remember_geometry = config.window.remember_geometry; + // Clamped on the way in by `Layout::restore`, so a hand-edited file cannot + // open hideGit with a sidebar wider than the window. + ui.app.layout = panes.into(); ui.app.panic_reports = config.diagnostics.panic_reports; ui.app.check_for_updates = config.diagnostics.check_for_updates; @@ -407,6 +415,17 @@ fn update(shell: &mut Shell, message: ShellMessage) -> Task { // handler — Cmd+Q on macOS closes the window without ever emitting // `CloseRequested`, and a kill or a panic reaches nothing at all. let touched_recents = matches!(message, Message::RepositoryOpened(_)); + // Written when a drag ends rather than while it runs: the same + // reason the window's own geometry is not written per frame, and + // here it is one file write instead of one per pointer movement. + let touched_panes = matches!( + message, + Message::Layout(LayoutMessage::Released | LayoutMessage::Reset(_)) + | Message::Repo( + _, + RepoMessage::Layout(LayoutMessage::Released | LayoutMessage::Reset(_)) + ) + ); let task = shell.ui.update(message).map(ShellMessage::Ui); if touched_settings { shell.config.theme.name = shell.ui.app.theme.name.clone(); @@ -440,7 +459,7 @@ fn update(shell: &mut Shell, message: ShellMessage) -> Task { }; } - if touched_recents { + if touched_recents || touched_panes { persist(shell); } task @@ -531,6 +550,7 @@ fn persist(shell: &mut Shell) { .map(|path| RecentRepository { path: path.clone() }) .collect(), announced_panic: shell.announced_panic.clone(), + panes: shell.ui.app.layout.into(), announced_update: shell.announced_update.clone(), last_update_check: shell.last_update_check, }; diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 20e3655..b4d84cc 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -382,6 +382,13 @@ Everything between "works" and "someone who does not write Rust can install it". can already see. Opening a repository that is already open switches to its tab rather than opening a second copy — two tabs on one repository would each hold their own idea of its state. Closing lands on the neighbour rather than the last tab +- **Resizable panes.** **Landed.** Every divider in the main window is draggable: the sidebar, the + graph against the detail pane, and the file list against the diff. Two of the three are pixels and + one is a fraction, because a list of names does not need more room on a wider window while the + graph and the commit under it are two halves of one view and should keep their ratio. Where they + are left is written to `state.toml` with the window's own geometry, and clamped on the way back in. + Double-clicking a divider restores it, which is why this needs no settings entry — before it, three + numbers chosen against a window nobody had seen decided how much of a commit message you could read - Settings UI covering everything currently in TOML. **Partly landed**: theme and every alert switch are on a `Cmd+,` panel, applied as they are changed and written back to `config.toml` **in place** — the file keeps its comments, its key order and any key hideGit does not own, because it is diff --git a/docs/UI_SPEC.md b/docs/UI_SPEC.md index 2863bc9..d40f510 100644 --- a/docs/UI_SPEC.md +++ b/docs/UI_SPEC.md @@ -194,7 +194,8 @@ worktree cannot be checked out in another, so the row is the answer to a checkou refused. The right-hand column carries that branch — or Git's own `detached at ` — except when the worktree is locked or its directory is gone, which are states to act on and take the column instead. Both reasons are in the tooltip along with the full path; the row shows the directory name, -because a path does not fit in 230px and its useful end is the last component. +because a path does not fit in a sidebar that is 230px by default and its useful end is +the last component. A worktree whose directory is gone is **listed, not hidden**. It still holds its branch until somebody runs `git worktree prune`, so hiding it would leave the refusal it causes with no visible @@ -279,6 +280,28 @@ submodule it left exactly as it found it, so "the operation succeeded" is not th submodule is now current". The second one is what the user is owed, and a toast says so by name when it is not true. +**Every divider between the panes is draggable.** Three of them: the sidebar against everything +right of it, the graph against the detail pane below it, and — inside the working directory — the +file list against the diff. A divider is a one-pixel rule with a seven-pixel grab area around it, it +shows the platform's resize cursor, and it turns the accent colour while it is being held. +Double-clicking one puts it back where it shipped, which is why resizing needs no settings entry: +the control that moved it is the control that undoes it. + +**Two of them are pixels and one is a fraction, on purpose.** The sidebar and the file list hold +names, and a name does not get longer because the window did, so they keep the width they were +given. The graph and the detail pane are two halves of one view, so they hold a *ratio* — a taller +window keeps the graph the subject and the commit under it the footnote, at whatever proportion the +last drag chose. Defaults are 230px, 60/40, and 280px. + +Nothing can be dragged out of existence: the sidebar and the file list stop at a minimum and at half +the space they divide, and the graph and detail pane each keep at least 15% of the column. A pane +with no height has no divider left to drag it back with. + +**Where they end up is written to `state.toml`, not to `config.toml`.** It is state, like the +window's own size — changed by dragging rather than by the settings panel, written when the drag +ends rather than during it, and clamped again on the way back in so a hand-edited file cannot open +hideGit with a sidebar wider than the window. + **Graph** — the centre. Virtualised: only visible rows are laid out and drawn. Refs are rendered as badges on their commits. Selecting a row updates the detail pane. Full rendering rules in [COMMIT_GRAPH.md](./COMMIT_GRAPH.md). From 687cb5b42a19fbcaef5e05b75f16e9220d06471c Mon Sep 17 00:00:00 2001 From: "Youri T. K. K. Mattar" Date: Thu, 17 Sep 2026 16:28:41 -0300 Subject: [PATCH 2/4] fix(ui): text nobody chose the length of scrolls instead of being cut off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three places drew text of unbounded length at whatever height it asked for and let the window clip the rest: no scrollbar, no wheel, and no way to read the end of it short of making the window bigger. **A commit message.** The detail pane's header — title, author, body, stats — grew with the body. A release note of four paragraphs pushed the changed files and the diff off the bottom of the window, and the end of the message was not reachable either way. It scrolls now, and is capped at 240px so that reading a long message does not cost the diff underneath it. **Git's stderr in a failed pane.** A rejected push prints a paragraph and a merge prints the conflicted paths. The pane showed the lines that fitted and dropped the rest, which for an error message is the half that says what to do about it. **Git's stderr in a toast.** A toast is bottom-aligned, so its details grew *upwards*: thirty lines carried the summary and both controls — dismiss and Copy details — off the top of the window, on exactly the failures somebody needs to read and report. Both tests assert what is *visible* rather than what is in the tree. A widget pushed out of the window is still in the widget tree, so the usual `shows` helper passes against the bug, and the toast one lays out at 1000x500 rather than the harness's roomy default — the bug is about a toast outgrowing its window, so the window has to be one a toast can outgrow. Both were confirmed to fail with the cap removed. --- crates/hidegit-ui/src/app/tests/render.rs | 65 +++++++++++++++++++++++ crates/hidegit-ui/src/widget/detail.rs | 37 ++++++++++--- crates/hidegit-ui/src/widget/overlay.rs | 26 +++++++-- docs/UI_SPEC.md | 9 ++++ 4 files changed, 125 insertions(+), 12 deletions(-) diff --git a/crates/hidegit-ui/src/app/tests/render.rs b/crates/hidegit-ui/src/app/tests/render.rs index 647bd3b..c166460 100644 --- a/crates/hidegit-ui/src/app/tests/render.rs +++ b/crates/hidegit-ui/src/app/tests/render.rs @@ -294,6 +294,71 @@ fn app_showing_a_diff(name: &str) -> Hidegit { app } +#[test] +fn a_commit_message_of_any_length_leaves_the_diff_on_screen() { + // The message is the one part of this pane whose length nobody chose. It + // is scrolled and capped: before the cap, a body like this one was drawn + // at whatever height it wanted, and the files and the diff underneath it + // went off the bottom of the window. + let mut app = app_showing_a_diff("src/main.rs"); + { + let repo = app.app.repos.get_mut(0).unwrap(); + if let crate::state::DetailPane::Commit { detail, .. } = &mut repo.detail { + detail.commit.body = Some( + (1..=60) + .map(|n| format!("paragraph {n} of a release note nobody will read")) + .collect::>() + .join("\n\n"), + ); + } + } + + // Visible, not merely present: a widget pushed off the bottom of the + // window is still in the tree, so `shows` would pass against exactly the + // bug this is about. The hunk header rather than a line of code — a + // highlighted line reaches the tree as rich text, which this harness + // cannot see inside of. + let mut ui = render(&app); + let diff = ui.find("@@ -1 +1 @@").expect("the diff is in the tree"); + + assert!( + diff.visible_bounds().is_some(), + "a long commit message pushed the diff out of the window" + ); +} + +#[test] +fn a_toast_with_a_long_stderr_keeps_its_summary_on_screen() { + // A toast is bottom-aligned, so its details grow upwards: before they were + // capped, a `git` stderr of thirty lines carried the summary — and the + // dismiss button — off the top of the window, on exactly the failures + // somebody needs to read. + let mut app = app_with(2); + app.app.toast(&crate::message::UiError { + summary: "The push was rejected".to_owned(), + details: (1..=40) + .map(|n| format!(" ! [rejected] branch-{n} -> branch-{n} (fetch first)")) + .collect::>() + .join("\n"), + }); + + // Laid out in a window somebody might actually have, rather than the + // harness's roomy default: the bug is about a toast outgrowing its window, + // so the window has to be a size a toast can outgrow. + let view = app.view(); + let mut ui: iced_test::Simulator<'_, Message> = iced_test::Simulator::with_size( + iced_test::core::Settings::default(), + iced::Size::new(1000.0, 500.0), + view, + ); + let dismiss = ui.find("Copy details").expect("the toast is in the tree"); + + assert!( + dismiss.visible_bounds().is_some(), + "the details pushed the toast's own controls out of the window" + ); +} + #[test] fn an_open_in_flight_says_which_repository_and_which_step() { // Two of them open at once from the command line, so the name is on the diff --git a/crates/hidegit-ui/src/widget/detail.rs b/crates/hidegit-ui/src/widget/detail.rs index 37c0ce3..a78835b 100644 --- a/crates/hidegit-ui/src/widget/detail.rs +++ b/crates/hidegit-ui/src/widget/detail.rs @@ -12,6 +12,13 @@ use crate::state::{DetailPane, DiffMode, FILE_FILTER_ID, OpenRepo, Selection}; use crate::theme::Palette; use crate::widget::common; +/// How much of the detail pane a commit message may take before it scrolls. +/// +/// A number rather than a share of the pane: a one-line commit should not +/// reserve a third of the screen on the chance that the next one is long, and +/// what is under this is where the reading actually happens. +const MESSAGE_HEIGHT: f32 = 240.0; + pub fn view<'a>( repo: &'a OpenRepo, palette: &'a Palette, @@ -388,9 +395,18 @@ fn commit<'a>( .color(palette.muted); column![ - container(column![header, Space::new().height(6), stats].spacing(2)) - .width(Fill) - .padding(Padding::from([10, 14])), + // Scrolled, and capped, because the commit message is the one part of + // this pane whose length nobody chose. A body of four paragraphs used + // to be drawn to whatever height it wanted and then clipped — no + // scrollbar, no wheel, and no way to read the end of it except to make + // the window bigger. Capped as well as scrolled, so a long message + // cannot push the diff off the bottom on its way to being readable. + container(scrollable( + column![header, Space::new().height(6), stats].spacing(2) + )) + .width(Fill) + .max_height(MESSAGE_HEIGHT) + .padding(Padding::from([10, 14])), common::divider(palette), row![ file_list, @@ -430,10 +446,17 @@ fn failure<'a>(error: &UiError, palette: &Palette) -> Element<'a, RepoMessage> { text(error.summary.clone()) .size(metrics::text::BODY) .color(palette.danger), - text(error.details.clone()) - .size(metrics::text::LABEL) - .font(Font::MONOSPACE) - .color(palette.muted), + // Git's own words, at Git's own length: a failed merge prints the + // conflicted paths, and a rejected push prints a paragraph. The + // pane showed as many lines as fitted and silently dropped the + // rest, which for an error message is the half that says what to + // do about it. + scrollable( + text(error.details.clone()) + .size(metrics::text::LABEL) + .font(Font::MONOSPACE) + .color(palette.muted) + ), ] .spacing(8) .max_width(600), diff --git a/crates/hidegit-ui/src/widget/overlay.rs b/crates/hidegit-ui/src/widget/overlay.rs index 49f9220..82d32d9 100644 --- a/crates/hidegit-ui/src/widget/overlay.rs +++ b/crates/hidegit-ui/src/widget/overlay.rs @@ -11,7 +11,7 @@ //! done to one item, and a **prompt** is a modal that collects text before //! acting. Both are centred cards over the same scrim as the confirmation. -use iced::widget::{Space, button, column, container, row, stack, text, text_input}; +use iced::widget::{Space, button, column, container, row, scrollable, stack, text, text_input}; use iced::{Center, Fill, Font, Length, Padding}; use crate::Element; @@ -457,6 +457,12 @@ fn toast_layer<'a>(toasts: &'a [Toast], palette: &'a Palette) -> Element<'a, Mes .into() } +/// How tall a toast's details may grow before they scroll instead. +/// +/// Twelve lines or so: enough for a push refusal in full, short of a toast that +/// covers the window it is reporting on. +const DETAILS_HEIGHT: f32 = 180.0; + fn one_toast<'a>(toast: &'a Toast, palette: Palette) -> Element<'a, Message> { let mut body = column![ row![ @@ -478,11 +484,21 @@ fn one_toast<'a>(toast: &'a Toast, palette: Palette) -> Element<'a, Message> { .spacing(6); if !toast.details.is_empty() { + // Capped and scrolled. A toast is bottom-aligned, so a `git` stderr of + // thirty lines grew *upwards* — past the top of the window, taking the + // summary and the dismiss button with it, on exactly the failures + // worth reading. body = body.push( - text(toast.details.as_str()) - .size(metrics::text::LABEL) - .font(Font::MONOSPACE) - .color(palette.muted), + container( + scrollable( + text(toast.details.as_str()) + .size(metrics::text::LABEL) + .font(Font::MONOSPACE) + .color(palette.muted), + ) + .height(Length::Shrink), + ) + .max_height(DETAILS_HEIGHT), ); } diff --git a/docs/UI_SPEC.md b/docs/UI_SPEC.md index d40f510..a740d54 100644 --- a/docs/UI_SPEC.md +++ b/docs/UI_SPEC.md @@ -944,6 +944,15 @@ honestly if the repository was left mid-operation — including a stale `index.l reported rather than silently removed. See [ADR-0005](./adr/0005-progress-and-cancellation.md). +**Anything whose length nobody chose scrolls, and is capped before it does.** Three things vary +without limit: a commit message, Git's stderr in a failed pane, and Git's stderr in a toast. Each +used to be drawn at whatever height it asked for and then clipped — no scrollbar, no wheel, and no +way to read the end of it short of resizing the window. A commit message takes at most 240px of the +detail pane before it scrolls, so a long one cannot push the diff off the bottom on its way to being +readable; a toast's details take at most 180px, because a toast is bottom-aligned and thirty lines of +stderr grew *upwards*, carrying its own dismiss and copy controls out of the window on exactly the +failures worth reading. + **Errors.** Recoverable errors appear inline where the action was attempted, with the action that fixes them. Unexpected errors become a toast with a **Copy details** action containing the argument vector and Git's own stderr — or, for a forge failure, the provider's own message. Git's error From c4cba381f83695e0e29568fb1e503ffbc294f2a7 Mon Sep 17 00:00:00 2001 From: "Youri T. K. K. Mattar" Date: Thu, 17 Sep 2026 16:48:33 -0300 Subject: [PATCH 3/4] feat(app): a menu bar built from the commands hideGit already has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit macOS puts a menu bar on screen whether an application asks for one or not. hideGit never asked, so what was up there was winit's fallback: one menu holding About, Services, Hide and Quit, every item naming the executable rather than the application — "About hidegit", lowercase — and nothing else at all. No File, no Edit, therefore no Copy or Paste in a menu, and no way to discover that Cmd+O opens a repository except by already knowing it. ## It is the command palette's table, in a different shape `widget::palette::COMMANDS` already holds every named action: an id, a title, the chord that runs it and the message it sends. The palette reads it, the shortcut reference is tested against it, and `[shortcuts]` in `config.toml` remaps by its ids. The menu is now assembled from it too, which is why this is a short file and why nothing in it can drift: a menu item whose title, action or shortcut disagreed with the palette's would have to disagree with itself. Three things fall out of that: - **An entry is greyed out exactly when its command has nothing to do.** The rule is the same one the palette already used — Push with no repository open produces no message — so an item cannot be enabled and inert. - **The shortcut shown is the user's own**, from the keymap first and the built-in chord second. A menu accelerator is taken by the operating system before the window sees the key, so advertising the default chord of a moved command would resurrect the binding they replaced. - **Cut, Copy, Paste, Undo, Minimise and Quit are the platform's own items**, not hideGit messages wearing their names. ## Where it is installed, and why there From `update`, in answer to `window::open_events()`. Both halves matter: iced runs `update` on the main thread, and the window having opened is what proves `NSApp` exists to hang a menu from. A failure is logged and otherwise ignored — losing a menu bar costs discoverability, refusing to start costs everything. Clicks arrive on muda's own channel, which is not a future, so one thread parks on it and forwards what it gets. The alternative is a timer waking thirty times a second to find nothing. ## What it does not do **Windows and Linux keep no menu bar.** Attaching one needs a native window handle iced 0.14 does not expose, and on Linux a GTK window hideGit does not have. Said in the module, in UI_SPEC and in the roadmap rather than left to be found. Nothing lives only in the menu — every command in it is still reachable by chord and from the palette — which is what makes its absence survivable. `muda` is a macOS-only dependency with `default-features = false`: the defaults are gtk3 and libxdo, forty packages in the lockfile for a platform this does not run on. ADR-0009 records the decision, including why the `objc2` already under winit is not an option: the workspace forbids `unsafe`. --- Cargo.lock | 48 +++- crates/hidegit/Cargo.toml | 7 + crates/hidegit/src/main.rs | 84 ++++++ crates/hidegit/src/menu.rs | 465 +++++++++++++++++++++++++++++++ docs/ARCHITECTURE.md | 3 +- docs/ROADMAP.md | 9 + docs/UI_SPEC.md | 35 +++ docs/adr/0009-native-menu-bar.md | 116 ++++++++ docs/adr/README.md | 1 + 9 files changed, 758 insertions(+), 10 deletions(-) create mode 100644 crates/hidegit/src/menu.rs create mode 100644 docs/adr/0009-native-menu-bar.md diff --git a/Cargo.lock b/Cargo.lock index 5ebe8b0..ed1f0e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1201,7 +1201,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -1403,7 +1403,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.52.0", ] [[package]] @@ -2220,7 +2220,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1a791e6620676a875f362f3156ed213e73ca099a09bf992c18812abe65cc37b1" dependencies = [ "bstr", - "hashbrown 0.17.1", + "hashbrown 0.15.5", ] [[package]] @@ -2975,6 +2975,7 @@ dependencies = [ "hidegit-forge", "hidegit-ui", "iced", + "muda", "png 0.18.1", "rfd", "serde", @@ -3816,6 +3817,16 @@ dependencies = [ "simple_asn1", ] +[[package]] +name = "keyboard-types" +version = "0.8.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0fbe853b403ae61a04233030ae8a79d94975281ed9770a1f9e246732b534b28d" +dependencies = [ + "bitflags 2.13.1", + "serde", +] + [[package]] name = "keyring" version = "4.1.5" @@ -4148,6 +4159,25 @@ dependencies = [ "pxfm", ] +[[package]] +name = "muda" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cca139e57da4383727f189e43e66e84fa372347027fc996f9f1a007ec0a37996" +dependencies = [ + "crossbeam-channel", + "dpi", + "keyboard-types", + "objc2 0.6.4", + "objc2-app-kit 0.3.2", + "objc2-core-foundation", + "objc2-foundation 0.3.2", + "once_cell", + "png 0.18.1", + "thiserror 2.0.19", + "windows-sys 0.60.2", +] + [[package]] name = "mundy" version = "0.2.3" @@ -4304,7 +4334,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -5598,7 +5628,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.61.2", + "windows-sys 0.52.0", ] [[package]] @@ -6128,7 +6158,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c3d1e2c7f27f8d4cb10542a02c49005dbd6e93095799d6f3be745fae9f8fedd4" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -6318,7 +6348,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix 1.1.4", - "windows-sys 0.61.2", + "windows-sys 0.52.0", ] [[package]] @@ -6760,7 +6790,7 @@ checksum = "f2f6fb2847f6742cd76af783a2a2c49e9375d0a111c7bef6f71cd9e738c72d6e" dependencies = [ "memoffset", "tempfile", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -7330,7 +7360,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.52.0", ] [[package]] diff --git a/crates/hidegit/Cargo.toml b/crates/hidegit/Cargo.toml index b369697..5df5234 100644 --- a/crates/hidegit/Cargo.toml +++ b/crates/hidegit/Cargo.toml @@ -42,3 +42,10 @@ tempfile = "3" [lints] workspace = true + +# The native menu bar. macOS only for now — see ADR-0009. `default-features` +# off because the defaults are gtk3 and libxdo, which are how muda draws a menu +# on Linux and are forty packages hideGit would carry in its lockfile for a +# platform it does not use them on. +[target.'cfg(target_os = "macos")'.dependencies] +muda = { version = "0.20", default-features = false } diff --git a/crates/hidegit/src/main.rs b/crates/hidegit/src/main.rs index aece97a..0605ba7 100644 --- a/crates/hidegit/src/main.rs +++ b/crates/hidegit/src/main.rs @@ -6,6 +6,7 @@ mod config; mod crash; +mod menu; use std::path::PathBuf; @@ -76,6 +77,12 @@ struct Shell { announced_update: Option, /// When the update check last ran. last_update_check: Option, + /// The native menu bar, once the window exists to hang it from. + /// + /// Held because dropping it takes the menu off the screen, and because its + /// items are what get greyed out as what hideGit can do changes. + #[cfg(target_os = "macos")] + menu: Option, } #[derive(Debug, Clone)] @@ -87,6 +94,12 @@ enum ShellMessage { Moved(iced::Point), /// The periodic write of anything that has been collected since the last. Flush, + /// The window exists, which on macOS is what makes it safe to build a menu: + /// a menu bar is attached to `NSApp`, and `NSApp` is the event loop's. + WindowOpened, + /// A menu item was chosen, by the id it carries — which is the id the + /// command palette and `[shortcuts]` already use. + MenuChosen(String), CloseRequested, } @@ -386,6 +399,8 @@ fn boot( // The one just announced, or the one from before if there was // nothing new to say. announced_panic: unannounced_panic.or(announced_panic), + #[cfg(target_os = "macos")] + menu: None, }, Task::batch([task.map(ShellMessage::Ui), check]), ) @@ -462,6 +477,16 @@ fn update(shell: &mut Shell, message: ShellMessage) -> Task { if touched_recents || touched_panes { persist(shell); } + + // After every message rather than after a list of the ones that + // could matter: it is a handful of boolean writes against items + // already in memory, and a list of "what might have changed this" + // goes stale the first time a command is added. + #[cfg(target_os = "macos")] + if let Some(menu) = &shell.menu { + menu.sync(shell.ui.app.active); + } + task } @@ -512,6 +537,34 @@ fn update(shell: &mut Shell, message: ShellMessage) -> Task { Task::none() } + ShellMessage::WindowOpened => { + // Built here rather than at boot: a menu bar is attached to + // `NSApp`, which the event loop owns and which does not exist until + // it has opened something. `update` runs on the main thread, which + // is the other half of what attaching one requires. + #[cfg(target_os = "macos")] + { + let keymap = std::sync::Arc::clone(&shell.ui.app.keymap); + match menu::Bar::install(&keymap, shell.ui.app.active) { + Ok(bar) => shell.menu = Some(bar), + // Never fatal. Losing the menu bar costs discoverability; + // refusing to run costs everything. + Err(error) => tracing::warn!(%error, "the menu bar was not built"), + } + } + Task::none() + } + + ShellMessage::MenuChosen(id) => { + // The menu carries command ids, not actions, so what a menu item + // does is whatever the command palette would have done — including + // nothing, for a repository command with no repository open. + match menu::message(&id, shell.ui.app.active) { + Some(message) => update(shell, ShellMessage::Ui(message)), + None => Task::none(), + } + } + ShellMessage::CloseRequested => { persist(shell); iced::exit() @@ -580,9 +633,38 @@ fn subscription(shell: &Shell) -> Subscription { // difference between one file write and one per frame of a drag. iced::time::every(FLUSH_INTERVAL).map(|_| ShellMessage::Flush), window::close_requests().map(|_| ShellMessage::CloseRequested), + window::open_events().map(|_| ShellMessage::WindowOpened), + #[cfg(target_os = "macos")] + menu_events(), ]) } +/// Menu clicks, forwarded from muda's own channel. +/// +/// That channel is not a future, so one thread parks on it for the life of the +/// process and forwards what arrives. A timer polling it would be the +/// alternative, and a menu that answers in under a frame is worth more than a +/// wake-up every thirty milliseconds is worth avoiding — but neither is needed: +/// this wakes only when somebody chooses something. +#[cfg(target_os = "macos")] +fn menu_events() -> Subscription { + use iced::futures::StreamExt; + + Subscription::run(|| { + let (sender, receiver) = iced::futures::channel::mpsc::unbounded(); + + std::thread::spawn(move || { + while let Ok(event) = muda::MenuEvent::receiver().recv() { + if sender.unbounded_send(event.id.0).is_err() { + break; + } + } + }); + + receiver.map(ShellMessage::MenuChosen) + }) +} + enum Arguments { /// The repositories to open, in the order given — one tab each. Run(Vec), @@ -669,6 +751,8 @@ mod tests { announced_panic: None, announced_update: None, last_update_check: None, + #[cfg(target_os = "macos")] + menu: None, } } diff --git a/crates/hidegit/src/menu.rs b/crates/hidegit/src/menu.rs new file mode 100644 index 0000000..4a616f5 --- /dev/null +++ b/crates/hidegit/src/menu.rs @@ -0,0 +1,465 @@ +//! The native menu bar. +//! +//! macOS puts a menu bar on screen whether an application asks for one or not. +//! hideGit never asked, so what was up there was winit's fallback: an +//! application menu with About, Services, Hide and Quit, every one of them +//! naming the *executable* — "About hidegit", lowercase — and nothing else at +//! all. No File, no Edit, so no Copy or Paste in a menu, and no way to discover +//! that `Cmd+O` opens a repository except by already knowing. +//! +//! # The menu is built from the command palette's table +//! +//! [`COMMANDS`] already holds every action with a name: its id, its title, the +//! chord that runs it, and the message it sends. The palette reads it, the +//! shortcut reference is checked against it, `[shortcuts]` in `config.toml` +//! remaps by its ids — and now the menu bar is assembled from it too. +//! +//! That is the whole reason this file is as short as it is, and the reason +//! nothing here can drift: a menu entry is an `id` from that table, so a menu +//! item whose action, title or shortcut disagreed with the palette's would have +//! to disagree with itself. A test holds that every id named below exists. +//! +//! # What is not here +//! +//! **Windows and Linux.** `muda` draws menus on both, but attaching one needs a +//! native window handle that iced 0.14 does not hand out, and on Linux it needs +//! a GTK window hideGit does not have — it is winit and wgpu all the way down. +//! Neither platform *expects* an application to own the screen's menu bar the +//! way macOS does, so the cost of going without is much lower there. See +//! [ADR-0009](../../../docs/adr/0009-native-menu-bar.md). + +use hidegit_ui::Message; +use hidegit_ui::widget::palette::COMMANDS; + +/// One line of a menu. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Entry { + /// A command from the palette's table, named by the id it answers to in + /// `config.toml`. + Command(&'static str), + /// A label and the page it opens in the browser. + Url(&'static str, &'static str), + /// A rule between two groups of entries. + Separator, + /// Something the platform draws and handles itself — Copy, Quit, Minimise. + /// + /// These are deliberately *not* wired to hideGit messages. Cut and Paste + /// belong to the operating system's editing machinery, and reimplementing + /// them to route through `update` would get their behaviour subtly wrong in + /// every text field at once. + Standard(Standard), +} + +/// A menu item the platform owns. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[allow(clippy::enum_variant_names)] +pub enum Standard { + About, + Services, + Hide, + HideOthers, + ShowAll, + Quit, + CloseWindow, + Undo, + Redo, + Cut, + Copy, + Paste, + SelectAll, + Minimize, + Zoom, + BringAllToFront, +} + +/// A top-level menu. +#[derive(Debug, Clone, Copy)] +pub struct Menu { + /// What the menu bar shows. The first menu's title is replaced by the + /// application's own name on macOS, which is why this one says `hideGit` + /// and is not expected to be read from here. + pub title: &'static str, + pub entries: &'static [Entry], +} + +/// The menu bar, as data. +/// +/// Ordered the way macOS orders a menu bar, because a menu that puts its own +/// ideas where Edit belongs is worse than no menu: the first submenu is the +/// application menu, Edit is second-from-left after File, and Window and Help +/// are last. +pub const BAR: &[Menu] = &[ + Menu { + title: "hideGit", + entries: &[ + Entry::Standard(Standard::About), + Entry::Separator, + Entry::Command("settings"), + Entry::Separator, + Entry::Standard(Standard::Services), + Entry::Separator, + Entry::Standard(Standard::Hide), + Entry::Standard(Standard::HideOthers), + Entry::Standard(Standard::ShowAll), + Entry::Separator, + Entry::Standard(Standard::Quit), + ], + }, + Menu { + title: "File", + entries: &[ + Entry::Command("open"), + Entry::Command("clone"), + Entry::Separator, + Entry::Standard(Standard::CloseWindow), + ], + }, + Menu { + title: "Edit", + entries: &[ + Entry::Standard(Standard::Undo), + Entry::Standard(Standard::Redo), + Entry::Separator, + Entry::Standard(Standard::Cut), + Entry::Standard(Standard::Copy), + Entry::Standard(Standard::Paste), + Entry::Standard(Standard::SelectAll), + ], + }, + Menu { + title: "View", + entries: &[ + Entry::Command("search"), + Entry::Command("diff-mode"), + Entry::Separator, + Entry::Command("shortcuts"), + ], + }, + Menu { + title: "Repository", + entries: &[ + Entry::Command("fetch"), + Entry::Command("pull"), + Entry::Command("push"), + Entry::Separator, + Entry::Command("commit"), + Entry::Command("commit-and-push"), + Entry::Command("discard"), + Entry::Separator, + Entry::Command("continue"), + ], + }, + Menu { + title: "Pull Requests", + entries: &[ + Entry::Command("connect"), + Entry::Command("refresh-pull-requests"), + ], + }, + Menu { + title: "Window", + entries: &[ + Entry::Standard(Standard::Minimize), + Entry::Standard(Standard::Zoom), + Entry::Separator, + Entry::Standard(Standard::BringAllToFront), + ], + }, + Menu { + title: "Help", + entries: &[Entry::Url( + "hideGit on GitHub", + "https://github.com/youhide/hideGit", + )], + }, +]; + +/// What choosing this entry does, given which repository is in front. +/// +/// `None` means the entry has nothing to do right now — every repository +/// command is `None` with no repository open — and that is also what decides +/// whether it is greyed out. One rule, applied to both, so an item can never be +/// enabled and inert. +pub fn message(id: &str, active: Option) -> Option { + if let Some(url) = id.strip_prefix("url:") { + return Some(Message::OpenUrl(url.to_owned())); + } + + COMMANDS + .iter() + .find(|command| command.id == id) + .and_then(|command| (command.message)(active)) +} + +/// Whether the entry can do anything at all right now. +pub fn enabled(id: &str, active: Option) -> bool { + message(id, active).is_some() +} + +/// The title and shortcut an entry shows, with the user's own remapping in +/// front of the built-in chord — the same order the shortcut reference uses. +/// +/// A menu accelerator is taken by the operating system *before* the window sees +/// the key, so a menu that advertised the default chord of a command somebody +/// had moved would not merely be wrong on screen: it would shadow their +/// remapping with the binding they had replaced. +pub fn label( + id: &str, + keymap: &hidegit_ui::keymap::Keymap, +) -> Option<(&'static str, Option)> { + let command = COMMANDS.iter().find(|command| command.id == id)?; + let chord = keymap + .chord_for(command) + .map(str::to_owned) + .or_else(|| command.chord.map(str::to_owned)); + + Some((command.title, chord)) +} + +#[cfg(target_os = "macos")] +pub use platform::Bar; + +#[cfg(target_os = "macos")] +mod platform { + use muda::{AboutMetadata, MenuItem, PredefinedMenuItem, Submenu, accelerator::Accelerator}; + + use super::{BAR, Entry, Standard}; + + /// The menu bar, once it is on screen. + /// + /// Holds its own items so their enabled state can be kept honest: what a + /// command can do changes as repositories open and close, and a menu that + /// learned the answer once would offer Push with nothing open. + pub struct Bar { + /// Kept because dropping a `muda::Menu` takes the menu off the screen + /// with it. + _menu: muda::Menu, + items: Vec<(&'static str, MenuItem)>, + } + + /// Hand-written because none of muda's types implement `Debug`, and the + /// workspace warns on a public type that does not. + impl std::fmt::Debug for Bar { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Bar") + .field( + "items", + &self.items.iter().map(|(id, _)| id).collect::>(), + ) + .finish() + } + } + + impl Bar { + /// Builds the bar and hands it to the application. + /// + /// **Must run on the main thread, after the window exists.** A menu is + /// attached to `NSApp`, and `NSApp` is created by the event loop — this + /// is called from `update`, which iced runs on the main thread, in + /// answer to the window having opened. + pub fn install( + keymap: &hidegit_ui::keymap::Keymap, + active: Option, + ) -> Result { + let menu = muda::Menu::new(); + let mut items = Vec::new(); + + for section in BAR { + let submenu = Submenu::new(section.title, true); + + for entry in section.entries { + match entry { + Entry::Separator => { + submenu.append(&PredefinedMenuItem::separator())?; + } + Entry::Standard(standard) => { + submenu.append(&predefined(*standard))?; + } + Entry::Url(label, url) => { + let item = MenuItem::with_id(format!("url:{url}"), *label, true, None); + submenu.append(&item)?; + items.push((*label, item)); + } + Entry::Command(id) => { + let Some((title, chord)) = super::label(id, keymap) else { + continue; + }; + // A chord that will not parse costs the shortcut, + // not the item: the command is still reachable from + // the menu, and the log says which one was dropped. + let accelerator = chord.and_then(|chord| { + chord.parse::().ok().or_else(|| { + tracing::warn!(%chord, id, "no menu accelerator for this chord"); + None + }) + }); + + let item = MenuItem::with_id( + *id, + title, + super::enabled(id, active), + accelerator, + ); + submenu.append(&item)?; + items.push((id, item)); + } + } + } + + menu.append(&submenu)?; + } + + menu.init_for_nsapp(); + + Ok(Self { _menu: menu, items }) + } + + /// Greys out what cannot be done right now. + /// + /// Called after every message rather than when something specific + /// changes: it is a handful of boolean writes against items already in + /// memory, and the alternative is a list of "what might have changed + /// this" that goes stale the first time a command is added. + pub fn sync(&self, active: Option) { + for (id, item) in &self.items { + let enabled = super::enabled(id, active); + if item.is_enabled() != enabled { + item.set_enabled(enabled); + } + } + } + } + + fn predefined(standard: Standard) -> PredefinedMenuItem { + match standard { + Standard::About => PredefinedMenuItem::about( + Some("About hideGit"), + Some(AboutMetadata { + name: Some("hideGit".to_owned()), + version: Some(env!("CARGO_PKG_VERSION").to_owned()), + copyright: Some("GPL-3.0-only © hideGit contributors".to_owned()), + website: Some("https://github.com/youhide/hideGit".to_owned()), + ..AboutMetadata::default() + }), + ), + // `None` takes the platform's own wording, which is already + // translated and already says the right thing. + Standard::Services => PredefinedMenuItem::services(None), + Standard::Hide => PredefinedMenuItem::hide(Some("Hide hideGit")), + Standard::HideOthers => PredefinedMenuItem::hide_others(None), + Standard::ShowAll => PredefinedMenuItem::show_all(None), + Standard::Quit => PredefinedMenuItem::quit(Some("Quit hideGit")), + Standard::CloseWindow => PredefinedMenuItem::close_window(None), + Standard::Undo => PredefinedMenuItem::undo(None), + Standard::Redo => PredefinedMenuItem::redo(None), + Standard::Cut => PredefinedMenuItem::cut(None), + Standard::Copy => PredefinedMenuItem::copy(None), + Standard::Paste => PredefinedMenuItem::paste(None), + Standard::SelectAll => PredefinedMenuItem::select_all(None), + Standard::Minimize => PredefinedMenuItem::minimize(None), + Standard::Zoom => PredefinedMenuItem::zoom(None), + Standard::BringAllToFront => PredefinedMenuItem::bring_all_to_front(None), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn command_ids() -> Vec<&'static str> { + BAR.iter() + .flat_map(|menu| menu.entries) + .filter_map(|entry| match entry { + Entry::Command(id) => Some(*id), + _ => None, + }) + .collect() + } + + #[test] + fn every_menu_entry_names_a_command_that_exists() { + // The menu holds ids rather than actions, so a command renamed in the + // palette would leave a menu item that silently does nothing. + for id in command_ids() { + assert!( + COMMANDS.iter().any(|command| command.id == id), + "the menu names “{id}”, which is not a command" + ); + } + } + + #[test] + fn no_command_is_in_two_menus() { + // Two items for one action is two places to keep right, and the second + // is the one nobody updates. + let ids = command_ids(); + for id in &ids { + assert_eq!( + ids.iter().filter(|other| *other == id).count(), + 1, + "“{id}” appears in the menu bar twice" + ); + } + } + + #[test] + fn a_repository_command_is_dead_until_a_repository_is_open() { + // What greys the item out. `open` needs nothing; `push` needs somewhere + // to push from. + assert!(enabled("open", None)); + assert!(!enabled("push", None)); + assert!(enabled("push", Some(0))); + } + + #[test] + fn the_help_entry_opens_the_page_it_names() { + let Some(Message::OpenUrl(url)) = message("url:https://github.com/youhide/hideGit", None) + else { + panic!("the help entry did not open a page"); + }; + + assert_eq!(url, "https://github.com/youhide/hideGit"); + } + + #[test] + fn an_entry_shows_the_chord_the_user_remapped_it_to() { + // A menu accelerator is taken before the window sees the key, so + // advertising the default chord of a moved command would shadow the + // remapping with the binding it replaced. + let keymap = hidegit_ui::keymap::Keymap::parse([("push", "Cmd+U")]); + + let (title, chord) = label("push", &keymap).expect("push is a command"); + + assert_eq!(title, "Push"); + assert_eq!(chord.as_deref(), Some("Cmd+U")); + } + + #[test] + fn an_entry_falls_back_to_the_chord_it_ships_with() { + let (_, chord) = label("push", &hidegit_ui::keymap::Keymap::default()).unwrap(); + + assert_eq!(chord.as_deref(), Some("Cmd+Shift+U")); + } + + #[cfg(target_os = "macos")] + #[test] + fn every_chord_in_the_menu_is_one_the_platform_can_take() { + // `Cmd+,`, `Cmd+/`, `Cmd+Enter` and `Cmd+Backspace` are all in the + // table, and all four are places a parser gives up. A chord that does + // not parse costs the shortcut and keeps the item, which is the right + // failure — but it should not be happening to anything that ships. + let keymap = hidegit_ui::keymap::Keymap::default(); + + for id in command_ids() { + let Some((_, Some(chord))) = label(id, &keymap) else { + continue; + }; + + assert!( + chord.parse::().is_ok(), + "“{chord}”, the shortcut for “{id}”, is not one a menu can show" + ); + } + } +} diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 208bee5..ceaa102 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -65,7 +65,7 @@ Dependencies point downward only. `hidegit-core` depends on neither `iced` nor ` | `hidegit-core` | Git domain types, `GitBackend` and its hybrid implementation, commit graph layout, diff model | `iced`, `hidegit-forge`, any HTTP client | | `hidegit-forge` | `Forge` trait, GitHub implementation, OAuth device flow, token storage, PR polling | `iced` | | `hidegit-ui` | Screens, widgets, theme, the commit-graph canvas, keyboard handling | — | -| `hidegit` | Binary: CLI arguments, config loading, logging/tracing setup, window bootstrap | — | +| `hidegit` | Binary: CLI arguments, config loading, logging/tracing setup, window bootstrap, the native menu bar | — | The rule about `hidegit-core` is the one that will be under pressure. When a graph layout function "just needs a colour" or a diff "just needs to know the viewport width", the answer is to return @@ -92,6 +92,7 @@ data and let `hidegit-ui` decide, not to reach upward. | `notify-rust` | 4 | Native desktop notifications | M4 | | `open` | 5 | Handing a URL to the platform's browser | M4 | | `syntect` | via `iced/highlighter` | Syntax highlighting in the diff | M6 | +| `muda` | 0.20 | The native menu bar — **macOS only**, and with `default-features = false`, whose defaults are gtk3 and libxdo | M6 | Versions are pinned in the workspace `Cargo.toml` and inherited by every crate, so a bump happens in one place. Crates whose milestone has not arrived carry no version here — recording a number diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index b4d84cc..044e602 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -389,6 +389,15 @@ Everything between "works" and "someone who does not write Rust can install it". are left is written to `state.toml` with the window's own geometry, and clamped on the way back in. Double-clicking a divider restores it, which is why this needs no settings entry — before it, three numbers chosen against a window nobody had seen decided how much of a commit message you could read +- **A native menu bar.** **Landed, on macOS.** What was up there before was winit's fallback — one + menu with About, Services, Hide and Quit, every item naming the executable rather than the + application, and no File or Edit at all, so no Copy or Paste in a menu and no way to discover + `Cmd+O` except by already knowing it. It is built from the command palette's own table, so an + item's title, action and shortcut cannot disagree with the palette's, and the shortcut it shows is + the one the user remapped it to rather than the one it ships with. **Windows and Linux keep no + menu bar**: attaching one needs a native window handle iced 0.14 does not expose, and on Linux a + GTK window hideGit does not have. Nothing lives only in the menu, which is what makes its absence + survivable — see [ADR-0009](./adr/0009-native-menu-bar.md) - Settings UI covering everything currently in TOML. **Partly landed**: theme and every alert switch are on a `Cmd+,` panel, applied as they are changed and written back to `config.toml` **in place** — the file keeps its comments, its key order and any key hideGit does not own, because it is diff --git a/docs/UI_SPEC.md b/docs/UI_SPEC.md index a740d54..4e3a7f1 100644 --- a/docs/UI_SPEC.md +++ b/docs/UI_SPEC.md @@ -14,6 +14,7 @@ Layout sketches are structural, not visual design. - [Diff view](#diff-view) - [Conflict resolver](#conflict-resolver) - [PR panel](#pr-panel) +- [Menu bar](#menu-bar) - [Keyboard shortcuts](#keyboard-shortcuts) - [Layout scale](#layout-scale) - [Theming](#theming) @@ -630,6 +631,40 @@ Catching those would mean reading every thread on every pull request on every po N+1 that [ADR-0006](./adr/0006-poll-pull-requests-over-graphql.md) exists to avoid. A new comment and a new review thread both do notify. +## Menu bar + +**macOS only, and said plainly rather than left to be discovered.** Windows and Linux keep no menu +bar: attaching one needs a native window handle iced 0.14 does not hand out, and neither platform +expects an application to own the screen's menu bar the way macOS does. See +[ADR-0009](./adr/0009-native-menu-bar.md). + +``` +hideGit File Edit View Repository Pull Requests Window Help +``` + +**Every entry is a command id from the same table the command palette reads.** An item's title, the +message it sends and the shortcut beside it come from `widget::palette::COMMANDS`, so a menu item +that disagreed with the palette would have to disagree with itself. Adding a command to that table +does not add it to a menu — that is a deliberate choice about where it belongs, and a test holds +that every id a menu names exists. + +**The shortcut shown is the user's own.** `[shortcuts]` in `config.toml` moves a command, and a menu +accelerator is taken by the operating system before the window ever sees the key — so a menu showing +the default chord of a moved command would resurrect the binding they replaced, not merely describe +it wrongly. + +**An entry is greyed out exactly when its command has nothing to do.** Push with no repository open +is the case that matters, and it is the same rule that decides whether the palette's own entry does +anything: the item cannot be enabled and inert. + +**Cut, Copy, Paste, Undo, Minimise and Quit are the platform's own items**, not hideGit messages +wearing their names. Editing belongs to the operating system's machinery, and reimplementing it +would get its behaviour subtly wrong in every text field at once. + +**Nothing lives only in the menu.** Every command in the bar is still reachable by chord, from the +command palette, or from a control on screen. That is what makes the menu safe to have on one +platform and absent on two. + ## Keyboard shortcuts `Cmd` on macOS, `Ctrl` elsewhere. diff --git a/docs/adr/0009-native-menu-bar.md b/docs/adr/0009-native-menu-bar.md new file mode 100644 index 0000000..a3307a4 --- /dev/null +++ b/docs/adr/0009-native-menu-bar.md @@ -0,0 +1,116 @@ +# 0009 — A native menu bar on macOS, built from the command palette's table + +- **Status:** Accepted +- **Date:** 2026-09-17 + +## Context + +macOS puts a menu bar on screen for every application, asked for or not. hideGit +never asked, so what was up there was winit's fallback: one menu, containing +About, Services, Hide, Hide Others, Show All and Quit — and nothing else. No +File, no Edit, therefore no Copy and no Paste in a menu, and no way to find out +that `Cmd+O` opens a repository except by already knowing it or by opening the +command palette, which is itself only reachable by a shortcut. + +Every item in that fallback also named the *executable* rather than the +application: "About hidegit", lowercase, in an application whose own name has +been a deliberate decision since M1. + +iced 0.14 has no menu API at all. It draws its own widgets into a wgpu surface, +and a menu bar is not a widget — on macOS it belongs to `NSApp`, which the event +loop owns. + +Three constraints shaped the answer. + +- **The workspace forbids `unsafe`.** `unsafe_code = "forbid"` is set at the + workspace root, and `forbid` cannot be lifted for a module. Writing the + `NSMenu` directly through `objc2` — which is already in the dependency tree + underneath winit, so it would cost nothing to add — is therefore not available + without weakening a lint that protects everything else. +- **Shortcuts are remappable.** `[shortcuts]` in `config.toml` moves a command to + a different chord, and moving it makes the default stop working. A menu + accelerator is taken by the operating system *before* the window sees the key, + so a menu advertising a default chord for a command somebody had moved would + not merely be wrong on screen: it would resurrect the binding they replaced. +- **There is already a table of every named action.** `widget::palette::COMMANDS` + holds an id, a title, a chord and a message for each. The command palette reads + it, the shortcut reference is tested against it, and `[shortcuts]` remaps by its + ids. + +## Decision + +**The menu bar is built from `COMMANDS`, by id, through `muda`, on macOS only.** + +A menu entry is one of four things: an id from that table, a URL, a separator, or +an item the platform owns (Copy, Quit, Minimise). The bar itself is a `const` +in `crates/hidegit/src/menu.rs`, which is the whole of the layout decision. + +Three consequences follow from building it that way, and they are the reason for +it: + +- **An item's title, action and shortcut cannot disagree with the palette's**, + because there is only one of each. A test asserts every id in the bar exists. +- **Enabled state has one rule.** An entry is enabled exactly when its command + yields a message for the repository in front — which is what "Push with nothing + open" already meant. The item cannot be enabled and inert. +- **The accelerator is the user's.** It comes from `Keymap::chord_for` first and + the built-in chord second, the same order the shortcut reference shows. + +`muda` is a macOS-only dependency with `default-features = false`: its defaults +are gtk3 and libxdo, which are how it draws menus on Linux and which would put +forty packages into the lockfile for a platform this does not run on. + +The menu is installed from `update`, in answer to `window::open_events()`. Both +halves of that matter: iced runs `update` on the main thread, and the window +having opened is what proves `NSApp` exists. A failure to build it is logged and +otherwise ignored — losing a menu bar costs discoverability, and refusing to +start costs everything. + +Menu clicks arrive on muda's own channel, which is not a future. One thread +parks on it for the life of the process and forwards what it gets; the +alternative is a timer, and a timer that wakes thirty times a second to find +nothing is exactly what [ADR-0005](./0005-progress-and-cancellation.md) avoids +elsewhere. + +## What this does not do + +**Windows and Linux keep no menu bar.** muda draws one on both, but attaching it +needs a native window handle iced 0.14 does not expose, and on Linux it needs a +GTK window hideGit does not have — it is winit and wgpu the whole way down. +Neither platform expects an application to own a screen-wide menu bar the way +macOS does, so going without costs less there. This is stated in +`menu.rs`, in `UI_SPEC.md` and in the roadmap rather than left to be discovered. + +**Nothing moves out of the interface and into the menu.** Every command in the +bar is reachable exactly as it was — by chord, by the command palette, or by a +control on screen. The menu is a third way to the same messages, which is what +makes it safe to have and what makes it useless to depend on. + +## Alternatives considered + +**Write the `NSMenu` with `objc2` directly.** No new dependency, since winit +already pulls it in, and complete control. Rejected: it needs `unsafe`, which the +workspace forbids outright, and the blast radius of that lint reaching across +four crates is worth more than a menu. + +**Draw a menu bar as iced widgets, inside the window.** Portable, themeable, and +tested by the same render suite as everything else. Rejected for macOS: a +menu bar inside the window is not where a macOS user looks, and the screen's real +menu bar would still say "About hidegit" beside it. It remains the obvious answer +for Windows and Linux if either turns out to want one. + +**Wait for iced to grow a menu API.** It may; 0.14 is pre-alpha's toolkit and +breaking releases are expected ([ADR-0001](./0001-gui-toolkit-iced.md)). Rejected +as a plan: the table this is built from is iced-independent, so whatever iced +ships later replaces `platform::Bar` — about a hundred lines — and nothing else. + +## Consequences + +- One new dependency, on one platform, behind one module. +- The menu, the palette and the shortcut reference are three views of one table, + and a command added to that table appears in the palette and the reference + automatically — the menu still has to be told, which is the seam a test covers. +- `Cmd+O`, `Cmd+F` and the rest are now taken by the menu on macOS before the + window sees them. They dispatch the same messages the keyboard bindings did, so + the behaviour is unchanged; a chord the parser cannot read loses its + accelerator and keeps its menu item, with a warning naming it. diff --git a/docs/adr/README.md b/docs/adr/README.md index 4c37486..93420e2 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -17,6 +17,7 @@ and re-litigated every six months. | [0006](./0006-poll-pull-requests-over-graphql.md) | Poll pull request state over GraphQL, not conditional REST | Accepted | | [0007](./0007-rebase-plan-through-the-environment.md) | Drive interactive rebase by handing the plan through the environment | Accepted | | [0008](./0008-translations-as-toml-catalogues.md) | Translate through TOML catalogues, with no i18n runtime | Accepted | +| [0009](./0009-native-menu-bar.md) | A native menu bar on macOS, built from the command palette's table | Accepted | ## Conventions From 90047baab9c38484e66d7762982295b342f88658 Mon Sep 17 00:00:00 2001 From: "Youri T. K. K. Mattar" Date: Thu, 17 Sep 2026 18:27:45 -0300 Subject: [PATCH 4/4] fix(app): the menu's wiring exists only where there is a menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Linux and Windows failed to compile: `-D warnings` makes dead code an error, and with no `Bar` to build, every one of `Entry`, `Standard`, `Menu`, `BAR`, `enabled`, `label` and `ShellMessage::MenuChosen` was unreachable. macOS passed, which is exactly the shape of failure a platform-gated feature has. The message that reports a menu click, the arms that handle it, and the two subscriptions that raise them are now gated with the rest of the menu. There are no menu clicks without a menu, and nothing to wait for a window to be ready for either. The table and the three functions that read it stay compiled everywhere, behind an `allow` that says why: they are portable data, and their tests — that every id still names a command, that a remap wins over the built-in chord — are worth running on all three runners. On two of them the code is dead by construction rather than by mistake, which is the distinction the lint cannot make and the comment can. Checked by compiling the crate with the gate flipped to a target this machine is not, so the non-macOS path is built rather than assumed. --- crates/hidegit/src/main.rs | 13 +++++++++++-- crates/hidegit/src/menu.rs | 7 +++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/crates/hidegit/src/main.rs b/crates/hidegit/src/main.rs index 0605ba7..b6b66ce 100644 --- a/crates/hidegit/src/main.rs +++ b/crates/hidegit/src/main.rs @@ -94,11 +94,17 @@ enum ShellMessage { Moved(iced::Point), /// The periodic write of anything that has been collected since the last. Flush, - /// The window exists, which on macOS is what makes it safe to build a menu: - /// a menu bar is attached to `NSApp`, and `NSApp` is the event loop's. + /// The window exists, which is what makes it safe to build a menu: a menu + /// bar is attached to `NSApp`, and `NSApp` is the event loop's. + /// + /// macOS only, along with everything else about the menu — there is nothing + /// to build on the platforms that do not get one, so there is nothing to + /// wait for either. + #[cfg(target_os = "macos")] WindowOpened, /// A menu item was chosen, by the id it carries — which is the id the /// command palette and `[shortcuts]` already use. + #[cfg(target_os = "macos")] MenuChosen(String), CloseRequested, } @@ -537,6 +543,7 @@ fn update(shell: &mut Shell, message: ShellMessage) -> Task { Task::none() } + #[cfg(target_os = "macos")] ShellMessage::WindowOpened => { // Built here rather than at boot: a menu bar is attached to // `NSApp`, which the event loop owns and which does not exist until @@ -555,6 +562,7 @@ fn update(shell: &mut Shell, message: ShellMessage) -> Task { Task::none() } + #[cfg(target_os = "macos")] ShellMessage::MenuChosen(id) => { // The menu carries command ids, not actions, so what a menu item // does is whatever the command palette would have done — including @@ -633,6 +641,7 @@ fn subscription(shell: &Shell) -> Subscription { // difference between one file write and one per frame of a drag. iced::time::every(FLUSH_INTERVAL).map(|_| ShellMessage::Flush), window::close_requests().map(|_| ShellMessage::CloseRequested), + #[cfg(target_os = "macos")] window::open_events().map(|_| ShellMessage::WindowOpened), #[cfg(target_os = "macos")] menu_events(), diff --git a/crates/hidegit/src/menu.rs b/crates/hidegit/src/menu.rs index 4a616f5..cec6ad7 100644 --- a/crates/hidegit/src/menu.rs +++ b/crates/hidegit/src/menu.rs @@ -27,6 +27,13 @@ //! Neither platform *expects* an application to own the screen's menu bar the //! way macOS does, so the cost of going without is much lower there. See //! [ADR-0009](../../../docs/adr/0009-native-menu-bar.md). +//! +//! The table below and the three functions that read it are portable, and their +//! tests run on every platform CI covers — an id that stopped naming a command +//! is worth catching on all three. Only macOS builds a menu out of them, so +//! there they are dead code by construction rather than by mistake, which is +//! what the `allow` says. +#![cfg_attr(not(target_os = "macos"), allow(dead_code))] use hidegit_ui::Message; use hidegit_ui::widget::palette::COMMANDS;