Skip to content

feat(gui): "Show in menu bar" setting + ⌘W to close windows#8

Merged
AprilNEA merged 3 commits into
masterfrom
feat/menubar-setting-and-close-window
May 31, 2026
Merged

feat(gui): "Show in menu bar" setting + ⌘W to close windows#8
AprilNEA merged 3 commits into
masterfrom
feat/menubar-setting-and-close-window

Conversation

@AprilNEA
Copy link
Copy Markdown
Owner

Two follow-ups to the tray work (#6, #7).

1. "Show in menu bar" setting (Settings ▸ General, macOS)

New AppSettings::show_in_menu_bar (defaults true, so existing configs keep the tray) with a live toggle:

  • On → OpenLogi lives in the menu bar with the dynamic Dock/menu-bar policy from feat(gui): dynamic Dock/menu-bar policy + start-minimized autostart #7.
  • Off → no status item; it stays an ordinary Dock app, and closing the last window keeps the Dock icon instead of dropping to accessory (the on_window_closed observer now checks the setting, so the user is never left with no window and no icon).

The status item is now always created; the setting just shows/hides it via tray::set_visible (NSStatusItem visible), so toggling applies live. --minimized autostart is gated on the setting (no point starting with neither a window nor a tray).

2. ⌘W closes the focused window

cmd-w was unbound. Added a CloseWindow action (+ Window-menu item) handled on the root view via window.remove_window(). It runs the normal window-closed path, so it composes with the tray policy (last window closed → back to the tray when enabled).

Also

  • Dedupes a duplicate Quit OpenLogi key in locales/app.yml that the check-yaml hook rejected once the file was touched. New strings (Close Window, Show in menu bar + description) get ja / zh-CN / zh-HK translations.

Verification

cargo fmt, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace (63 tests) pass on macOS. set_show_in_menu_bar is cfg(macos) (only the macOS toggle calls it) to avoid an unused-method warning on Linux; CI confirms the non-macOS build.

Two UX additions on top of the tray work:

- Add a macOS "Show in menu bar" toggle (Settings > General). On (default)
  OpenLogi lives in the menu bar with the dynamic Dock/menu-bar policy; off, it
  stays an ordinary Dock app with no status item, and closing the last window
  keeps the Dock icon instead of dropping to accessory. New
  `AppSettings::show_in_menu_bar` defaults to true so existing configs keep the
  tray; applied live via `tray::set_visible` (the status item is now always
  created, just shown/hidden).

- Bind Cmd-W to close the focused window (new `CloseWindow` action + Window-menu
  item); it was previously unbound. Closing this way runs the same
  window-closed path, so it composes with the tray policy.

Also dedupes a duplicate `Quit OpenLogi` key in locales/app.yml that the
check-yaml hook rejected once the file was touched.
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

cmd-w only closes the main window — Settings, About, AddDevice, and UpdateConsent windows don't respond to it, even though the PR title and Window-menu item read as window-agnostic.

Reviewed changes — initial review of the "Show in menu bar" preference and the new CloseWindow action introduced on top of the tray work from #6 / #7.

  • AppSettings::show_in_menu_bar — new bool field defaulting true; replaces the derived Default with a hand-written one and a default_true serde helper so configs predating the field keep the menu-bar behavior. clippy::struct_excessive_bools is allowed with a reason.
  • Live toggleAppState::set_show_in_menu_bar (cfg(macos)) persists, calls tray::set_visible, and pins Regular policy via show_in_dock when disabled so the user is never left without a window AND without an icon.
  • tray::set_visible — new on platform::tray::macos; stores the NSStatusItem in a new STATUS_ITEM: OnceLock<usize> so visibility can be toggled without tearing the item down.
  • Settings UIwindows/settings.rs factors the General GroupBox into a local binding and conditionally adds the macOS-only "Show in menu bar" row.
  • Main wiringmain.rs always installs the tray and follows the stored setting for initial visibility; gates --minimized autostart on show_in_menu_bar; the on_window_closed observer now consults AppState via try_global before dropping to accessory.
  • CloseWindow action — new entry in app_menu.rs actions, a cmd-w keybinding, and a Window-menu item; handler lives on AppView's root (app.rs:189) next to the existing Minimize / Zoom handlers.
  • Locales — removes a duplicate Quit OpenLogi key (the check-yaml hook trip) and adds Close Window, Show in menu bar, and the menu-bar description with ja / zh-CN / zh-HK translations.

⚠️ cmd-w does nothing in secondary windows

The CloseWindow action handler is registered on AppView's root only, so it fires for the main window's view tree. Settings, About, AddDevice, and UpdateConsent each have their own view roots with no on_action(CloseWindow), and there's no App-level fallback like the one used for Quit / Hide / OpenSettings. Hitting cmd-w (or picking Window ▸ Close Window from the menu) while one of those windows is focused is silently a no-op.

The PR description frames this as "closes the focused window" and the menu item carries the platform-standard cmd-w shortcut, so users will reasonably expect it to work in every window. Minimize / Zoom share the same limitation, but those are far less load-bearing than cmd-w.

Technical details
# `cmd-w` does nothing in secondary windows

## Affected sites
- `crates/openlogi-gui/src/app.rs:189` — only place `CloseWindow` is handled; bound to the `AppView` root.
- `crates/openlogi-gui/src/app_menu.rs:60``cmd-w` keybinding registered globally (`None` context), so the action *is* dispatched from every window — it just goes unhandled in non-`AppView` windows.
- `crates/openlogi-gui/src/windows/settings.rs`, `windows/about.rs`, `windows/add_device.rs`, `windows/update_consent.rs` — no `on_action(CloseWindow)` on any of these view roots.

## Required outcome
- `cmd-w` (and *Window ▸ Close Window*) closes whichever window is currently focused, not just the main one.

## Suggested approach
- Easiest fix: register a single App-level handler in `app_menu.rs::install` next to the existing `Quit` / `Hide` handlers, e.g.

    ```rust
    cx.on_action(|_: &CloseWindow, cx| {
        if let Some(w) = cx.active_window() {
            w.update(cx, |_, window, _| window.remove_window()).ok();
        }
    });
    ```
  …then drop the `on_action(CloseWindow)` on `AppView`. Same trick would let `Minimize` / `Zoom` work uniformly, but that's a separate cleanup.
- Alternative: add `on_action(|_: &CloseWindow, window, _| window.remove_window())` to each secondary window's root view. Repetitive but localized.

## Open questions for the human
- Is the App-level fallback acceptable, or do you want per-window opt-in (e.g. UpdateConsent intentionally non-closable by `cmd-w`)? The current update-consent flow does call `window.remove_window()` itself on accept/decline, so a `cmd-w` shortcut there would just dismiss the prompt without recording a choice.

ℹ️ No regression test pinning the show_in_menu_bar backward-compat default

default_true and the manual Default impl are what keep pre-PR config.toml files reading as show_in_menu_bar: true. A one-liner deserialization test (load a TOML body without the field, assert true) would lock that in cheaply alongside the existing config.rs tests — useful since the field is the one place in AppSettings where the field default disagrees with bool::default().

Technical details
# Lock in `show_in_menu_bar` backward-compat

## Affected sites
- `crates/openlogi-core/src/config.rs:88``#[serde(default = "default_true")]`
- `crates/openlogi-core/src/config.rs:107-117` — manual `Default for AppSettings`
- `crates/openlogi-core/src/config.rs:394+` — existing `mod tests`

## Required outcome
- A test that deserializes an `AppSettings` TOML body missing `show_in_menu_bar` and asserts the field comes back `true`. Optionally: a parallel test asserting `AppSettings::default().show_in_menu_bar == true`.

## Suggested approach
- Add to the existing `tests` module:

    ```rust
    #[test]
    fn show_in_menu_bar_defaults_to_true_for_old_configs() {
        let parsed: AppSettings = toml::from_str("").expect("deserialize");
        assert!(parsed.show_in_menu_bar);
        assert!(AppSettings::default().show_in_menu_bar);
    }
    ```

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment on lines +341 to +347
#[cfg(target_os = "macos")]
{
crate::platform::tray::set_visible(enabled);
if !enabled {
crate::platform::tray::show_in_dock();
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant #[cfg(target_os = "macos")] — the enclosing fn (line 332) is already macOS-gated, so this inner block-level cfg can't fire on its own. Drop the inner #[cfg(target_os = "macos")] and the surrounding { } and inline the two calls.

crate::platform::tray::show_in_dock();
}
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a short comment on the asymmetry: disabling restores show_in_dock so the user isn't stranded, but enabling deliberately does not immediately switch to accessory — the on_window_closed observer handles that the next time the last window closes. Without the note, a reader expects a hide_from_dock call on the enabled branch and wonders why it's missing.

Comment on lines +181 to +183
let tray_on = cx
.try_global::<AppState>()
.is_some_and(|s| s.app_settings().show_in_menu_bar);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try_global::<AppState>() here means: if AppState ever isn't installed when a window closes, tray_on silently becomes false and the app stays in Regular — the opposite of the default behavior. AppState is set before any window opens in this run, so it's safe today, but cx.global::<AppState>() would panic loudly on a regression instead of degrading silently. Consider switching unless you have a specific reason to expect it can be absent.

let general = {
let in_menu_bar = cx
.try_global::<AppState>()
.is_some_and(|s| s.app_settings().show_in_menu_bar);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this re-reads AppState after the earlier try_global at line 55. One read covering all four fields (launch, updates, language, show_in_menu_bar) would be a touch tidier and avoids a second try_global per render. Not a correctness issue.

AprilNEA added 2 commits May 31, 2026 18:03
The Nix apple-sdk setup hook sets DEVELOPER_DIR/SDKROOT to an SDK with no
Metal toolchain, so compiling the GUI (gpui shaders) failed everywhere
except openlogi:dmg (which goes through package-macos.sh). Force real Xcode
via a shared xcodeEnv snippet: in enterShell (fixes the interactive shell
and the prek cargo-clippy hook) and in the gui/bundle/check tasks. Non-GUI
crates are unaffected — the Nix clang wrapper keeps its own isysroot.
Per Pullfrog review on #8: the CloseWindow action was handled only on
AppView's root, so Cmd-W (and Window > Close Window) was a silent no-op in the
Settings / About / Add Device / Update Consent windows. Move it to an App-level
handler that closes cx.active_window(), so it works for whichever window is
focused.
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the two commits added since the prior pullfrog review address the [!IMPORTANT] body finding about cmd-w only firing in the main window, plus a devenv refactor for Xcode env handling.

  • CloseWindow lifted to App levelon_action(CloseWindow) now lives in app_menu::install and dispatches via cx.active_window().update(...), so cmd-w closes Settings, About, AddDevice, and UpdateConsent in addition to the main window. The per-view handler on AppView was removed and the CloseWindow import dropped from app.rs. A comment on the handler captures why it can't be view-level like Minimize / Zoom.
  • devenv.nix Xcode env refactor — introduces a let-bound xcodeEnv snippet that exports DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer and SDKROOT="$(/usr/bin/xcrun ...)". Applied in enterShell and prepended to the GUI / check / bundle task exec strings. The static env block no longer hard-codes DEVELOPER_DIR / SDKROOT; a comment notes the Nix apple-sdk setup hook would override the static values anyway.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@AprilNEA AprilNEA merged commit 3251bf8 into master May 31, 2026
6 checks passed
@AprilNEA AprilNEA deleted the feat/menubar-setting-and-close-window branch May 31, 2026 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant