refactor: replace repeated glue with shared crates and derives - #205
Merged
Merged
Conversation
The app loop hand-rolled the same pieces in many places: mpsc receivers polled with try_recv for background results, Instant fields compared against fixed intervals, and retry deadlines. pollkit wraps them as Job, Every and Cooldown so each call site says what it wants instead of how to track it.
Every plugin host call returned PluginResultC directly, so each fallible step was spelled out as a match or if-let that converted the error by hand, plus a lock_runtime_or_return macro for the lock. c-result is an attribute macro that lets these functions return a Rust Result and use ?, and converts to the C type at the boundary. The conversion is a From impl on PluginResultC in the plugin API, so plugins can use it for their own callbacks as well.
The platform crate built NUL-terminated UTF-16 buffers by hand for every string argument, closed handles manually on each exit path, and drove the Run key through raw Reg* calls. The app user model ID lookup was also written twice, once for media activation and once for audio capture. windows-rs already ships these pieces: HSTRING and the w!/h! literals for string parameters, Owned<HANDLE> for handles, and the windows-registry crate, which was already in the lock file. Use them and move the shared process lookup into a process module. As a side effect, reveal_path now passes the path without a lossy UTF-8 round trip.
Every one of these std locks was unwrapped with PoisonError::into_inner, so poisoning was already ignored. parking_lot locks never poison, which removes that boilerplate and lets try_lock return an Option. Locks that do react to poisoning (plugin manager, logger, i18n) are left unchanged.
DockPosition and LyricTransitionMode each spelled out their config strings in several hand-written matches (as_str, FromStr and the String conversion). strum now derives them from one snake_case rule, so a new variant needs no extra match arms. Saved values stay byte-identical and unknown strings still fall back to the default.
Every switch, stepper and select on the general and music pages repeated one pattern: an action variant, a hand-built row, a click arm and a set_* or select_* helper. The settings-schema crates derive the field list, labels, limits and choices from #[setting(..)] on AppConfig, and one generic handler builds those rows and applies clicks, typed numbers and popup choices. Rows with side effects or runtime options (autostart, language, monitor, island style, font, lyrics folder, media apps) stay hand-written. Typed numbers are now rounded to the precision the row displays.
The Windows backend wrapped 42 foreign errors with the same closure, |error| PlatformError::Backend(error.to_string()). A From impl cannot cover them: winisland-platform takes no dependencies, the orphan rule keeps the impl out of winisland-platform-windows, and a blanket impl would overlap From<T> for T. A generic constructor lets every site use map_err(PlatformError::backend) instead.
The i18n catalogue, the file logger and pollkit's cancel slot still used std locks with poison handling. Release builds use panic = abort, so a poisoned lock is never observed there and those branches never ran. The i18n bundle functions keep returning Result so their callers compile unchanged. The v1 plugin manager keeps its std mutex because the plugin ABI v2 work replaces that file.
Every AppConfig default was written up to three times: in a default_* function, in a #[serde(default = ..)] attribute and in the hand-written Default impl. Each value is now declared once with #[educe(Default = ..)], and the struct-level #[serde(default)] fills missing keys from it. A config file that lacks a key now keeps its other values instead of being replaced by defaults. config_version keeps its field-level default of 0 so old files still migrate.
The notification feed tracked three WinRT async operations with a receiver, a hand-written cancel slot and a finish function that reset both on every branch. pollkit's Job already bundles the channel, the cancel action and the wake-up, so each operation is now one Job field and its finish function is a plain match on poll(). Finished jobs wake the loop through pollkit's hook, which the app sets to the same platform wake as the feed's waker.
The brightness probe and monitor connected to ROOT\WMI with the same eight-argument call and repeated both query strings, and the overlay and settings windows were created and registered the same way. Each now goes through one helper.
Both rect helpers summed the heights of the rows above the clicked item on their own. They now read it from one row_top helper.
draw_highlighted_lyric built the same text draw three times with only the colour changing, and the settings sidebar ran the same row hit test for clicks and for hover. Each now goes through one closure or helper.
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.
Pre-submission checklist
CONTRIBUTING.mdand completed the required local checksChange type (select one)
featNew featurefixBug fixdocsDocumentation or templatesstyleFormatting with no behavior changerefactorCode change that neither fixes a bug nor adds a featureperfPerformance improvementtestTest-related changechoreBuild, CI, dependency, or tooling changerevertRevertsecuritySecurity fixScope (select all that apply)
Related issues
Description
Summary
Replaces repeated settings, background-task, synchronization and Windows API glue with shared helpers and derives, integrated with the ABI v2 plugin host on
master.Motivation and context
Settings previously repeated row definitions, action variants and update handlers. Background tasks repeated receiver polling and cancellation state, while locks and Windows resources repeated recovery and cleanup code. Shared helpers keep these paths consistent.
Changes
pollkitfor polled jobs and throttles, andsettings-schema/settings-schema-derivefor typed settings metadata and updates.parking_lot,strum,educe, and existing windows-rs resource, string and registry helpers.pollkitjobs and its event-loop wake hook.c-resultcrate with its deleted ABI v1 manager; no plugin ABI change is introduced relative to currentmaster.The config format remains unchanged. Missing settings now use defaults instead of discarding the remaining config;
config_versionstill defaults to 0 for migration. Typed stepper inputs round to the displayed precision, position offsets accept and round decimals, and unsupported frame-rate values highlight the first popup option. Notification jobs wake the event loop through the existing platform wake function.Validation
cargo check,cargo clippy --workspace -- -D warnings,cargo fmt --all, andgit diff --checkpass.SKIA_BINARIES_URLbecause this machine's gnullvm target has no Skia prebuilt. They do not establish successful local linking.63c9350passes formatting, workspace Clippy,cargo build --releaseand the existingcargo testsuite.UI changes
No layout changes intended. Settings retain their existing rows; input rounding and popup fallback behavior are described above.