refactor(gui): split start_tray_handler into focused helpers#48
Merged
Conversation
Extract platform bring-up into spawn_tray_platform (cfg-gated for Linux vs. others) and the event dispatch loop into spawn_tray_event_loop, leaving start_tray_handler as a four-line orchestrator. Drop the unused bg_executor binding, the redundant crate::gui::tray:: prefix, and inferable type annotations on the async closure parameters. No behavior change; public API of start_tray_handler is preserved. Refs #47
spawn_tray_icon_event_forwarder, tray_event_from_icon_event, and the test that exercises the latter are only reachable on non-Linux targets — the call site in start_tray_handler_inner is already #[cfg(not(target_os = "linux"))]. Without the same gate on the definitions, Linux builds emit dead_code warnings that fail precheck.sh's clippy -D warnings step. Refs #47
The CI Precheck job only installs nightly rustfmt explicitly. The pinned stable channel from rust-toolchain.toml is auto-installed by rustup on first cargo invocation, but rustup only fetches cargo and rustc — clippy is left out, so scripts/precheck.sh's `cargo clippy --all-targets --all-features -- -D warnings` step fails with 'cargo-clippy is not installed for the toolchain 1.95.0' on every fresh runner. This is a pre-existing infrastructure bug, not introduced by this PR, but folded in here because the refactor cannot land while CI is red. Refs #47
Two follow-up fixes after the previous attempt: 1. src/gui/tray.rs: TrayIconEvent is only used by the now cfg-gated spawn_tray_icon_event_forwarder / tray_event_from_icon_event helpers, so the import itself must also be #[cfg(not(target_os = "linux"))], otherwise Linux builds emit an unused_imports warning that fails precheck's clippy -D warnings step. 2. .github/workflows/ci.yml: the previous attempt used `rustup show active-toolchain || rustup toolchain install ... --component clippy`, but `rustup show active-toolchain` succeeds (it auto-installs the pinned toolchain), so the install with --component clippy never ran. Split into two unconditional commands: trigger the toolchain auto-install via `rustup show`, then `rustup component add clippy` on the active toolchain. Refs #47
The previous commit fixed CI to actually run clippy, which surfaced
five clippy errors that pre-existed in the codebase but were never
caught because clippy had silently been a no-op on every fresh runner.
None of these are introduced by this PR; folding them in here so the
refactor can land.
- src/app.rs: replace X11::new().expect(...) with a fallible match;
on failure log an error and skip the X11 init instead of crashing
the app on startup.
- src/gui/tray.rs: replace gtk::init().expect(...) with a fallible
if-let; on failure log an error and bail out of the spawned task
instead of panicking inside background_spawn.
- src/gui/tray.rs: in start_tray_handler_inner, swap the call order
so the cfg-gated spawn_tray_icon_event_forwarder takes the cloned
sender first and spawn_tray_menu_event_forwarder consumes the
original. On Linux the icon forwarder is compiled out, so the menu
forwarder consumes tx directly with no clone (silencing both
needless_pass_by_value and redundant_clone); on other platforms
exactly one clone is needed and is now where it belongs.
- src/gui/x11.rs: replace `if enable { 1 } else { 0 }` with
`u32::from(enable)`.
Refs #47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactor
start_tray_handlerinsrc/gui/tray.rsso the function body stops being sliced by#[cfg]blocks. Platform bring-up and event dispatch each move into their own helpers, leaving the entry point as a four-line orchestrator.Linked Issue
Closes #47
Changes
spawn_tray_platform(i18n, cx, tx) -> Option<TrayIcon>with two cfg-gated implementations (Linux spawns on the GTK thread and leaks the icon; non-Linux returns it directly).spawn_tray_event_loop(cx, rx, window_handle)to own theTrayEventdispatch future.bg_executorbinding on Linux.crate::gui::tray::send_active_action(...)call with the baresend_active_action(...).cx.spawnasync closure parameters.No behavior change. The public signature of
start_tray_handleris preserved, so callers insrc/app.rsand theguire-export are untouched.Testing
scripts/precheck.shpasses locally (cargo check,clippy --all-targets --all-features -D warnings,cargo test -- --test-threads=1→ 414 passed, i18n / icons / themes all OK).src/gui/tray.rs::testsalready exercise the tray event mapping helpers, which were not modified. The platform bring-up path is not unit-testable because it requires a liveApp.Self-Check
gpui-componentthiserror(no manualimpl Display/Error)