Skip to content

Cleanup passes (consistency/perf/docs/robustness) — proposed PR split & sign-off on behavior-affecting fixes #964

Description

@dacrab

Title: Cleanup passes (consistency/perf/docs/robustness) — proposed PR split & sign-off on behavior-affecting fixes

Body:

Hi @MalpenZibo — I ran a series of audits over the whole src/ (consistency, design review, comments/docs, plus perf and duplication passes) and have a batch of improvements ready to contribute. Per the contributing policy I'm opening this issue first to agree on scope and the PR split before sending anything.

What I have ready

1. Bug fixes (small, high-confidence)

  • Workspaces scroll used substring monitor matching (eDP-1 bar scrolling matched a DP-1 workspace as current) — the view filter already uses equality for exactly this reason; the scroll handler didn't. One-line fix.
  • Niri CustomDispatch("spawn", …) passed the whole args string as one argv element, so any command with flags (kitty --hold sh) never worked. Now split on whitespace.
  • Hyprland Lua dispatch interpolated special-workspace names into a Lua string unescaped — a name containing " or \ breaks toggling on Lua-config systems.
  • Tray config was silently skipped on hot-reload (only module without ConfigReloaded) — [tray] blocklist/right_click changes needed a restart.

2. Robustness (small)

  • MPRIS cover fetch: no timeout / no size cap on an attacker- or bug-controlled artUrl — now 20s timeout + 10 MiB cap, mirroring the weather client pattern.
  • Notification daemon revisions map grew unboundedly for apps that never send CloseNotification — now capped (same spirit as the history cap).
  • image-data hints with spec-compliant rowstride padding were rejected (icon silently dropped) — now handled row-by-row.
  • A pipewire thread unwrap on a oneshot send was process-fatal under panic = "abort" on a subscription-cancellation race.
  • Behavior question: on State::Error, audio + network retry after 5s but the other 9 services park pending() forever — so a D-Bus daemon starting after ashell (bluetoothd, upower) permanently disables its module. I unified everything to the retry behavior. Confirm that's the intent (or should some stay one-shot)?

3. Config validation (small)

  • appearance.menu.backdrop wasn't range-checked (unlike opacity) — backdrop = 50 produced garbage rendering; now rejected like opacity.
  • max_workspaces unclamped — 100000 materializes 100k workspace buttons (freeze/OOM); now clamped to 100 with a warning.
  • SystemInfoTemperature warn<alert invariant only checked when both set — now checked against resolved values.
  • A TOML syntax error at startup silently yielded a fully default bar with no terminal feedback — now eprintln!s like the unknown-fields path already does (still non-fatal).

4. Performance (medium) — view-path work moved to update(): tempo clock timezone parse + chrono format was every frame → cached display_text; tray blocklist regexes per item per frame → HashSet recomputed on service updates; custom-module icon/alert regexes per frame → resolved per data update; notification timestamps formatted once at insertion; notification history capped at 500 (was unbounded in a daemon); assorted per-frame String/Vec clones removed.

5. Duplication consolidation (large but mechanical, ~300 lines): ServiceEvent::apply() helper collapsing 10 modules' Init/Update/Error boilerplate; impl_service_subscription! macro for 9 identical subscribe() bodies; per-module dispatch() matches → one-liners; shared with_more_button() component; audio device_submenu(kind); upower battery_events(); theme.bar_geometry().

6. Comments/docs (zero-risk): module-level docs for the architectural core files (services/mod.rs, config.rs, outputs.rs, modules/mod.rs, theme.rs, app.rs, osd.rs); doc comments on the service traits / ServiceEvent / Menu / Remote::value() contracts; fixed ~6 stale/wrong comments (e.g. the ConnectivityState "worst wins" comment that actually selects best); deleted ~20 obvious restate-comments.

7. Naming consistency (large, most contentious — happy to drop): bare getters per Rust API guidelines (get_x()x(), ~30 defs); XxxModuleConfig infix everywhere; ServiceEventEvent message variant (7 modules already used Event, 5 shadowed the type name); boxed CompositorService events in the two keyboard modules (matches workspaces/window_title); TrayModuleTray struct name; NearByNearby; one hardcoded English string → Fluent key (en/fr/de).

Proposed split

  1. fix — item 1 (bug fixes)
  2. fix — item 2 (robustness) + item 3 (validation) — or separate, your call
  3. perf — item 4
  4. refactor — item 5 (consolidation)
  5. docs — item 6
  6. refactor — item 7 (only if wanted)

Each PR is individually green (fmt/clippy/tests/build). I'd send them one at a time or stacked, whatever you prefer.

Questions for you

  1. The error-retry unification (item 2, last bullet) — intended behavior or should the pending-forever services stay as-is?
  2. Do you want the naming sweep (item 7) at all, or is the churn not worth it to you?
  3. Anything above you'd rather not receive? Happy to carve it out.

Everything is on my fork (dacrab/ashell, commits after 93a39a3) if you want to eyeball the full diff first.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions