-
Notifications
You must be signed in to change notification settings - Fork 88
feat: add marquee effect to volume popup's audio inputs #1250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: add marquee effect to volume popup's audio inputs #1250
Conversation
- Wraps the label in a ScrolledWindow with hidden scrollbars - Configurable max length before marquee starts - Configurable scrolling speed - Configurable inter-loop pause duration - Configurable separator - Optional pause on hover or play on hover
5e806f4 to
5283596
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I've raised a few bits in the first pass - there's quite a lot going on here, so I imagine this'll need a few runs through before I catch everything.
Agreed this will be a good thing to be able to build into the label module and custom widget, as well as introduce into other modules where it makes sense.
RE the styling concern, I think it's a non-issue. I've addressed it in a comment against the documentation.
…over enum Replaces two boolean fields (play_on_hover, pause_on_hover) with a single on_hover enum field accepting "none", "pause", or "play" values. This simplifies configuration and makes the behavior more explicit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Document that truncate takes precedence over marquee when both are configured, matching the actual implementation behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove redundant examples from separator field description as the string type and multi-character default make it self-explanatory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove the tip about internal widget tree changes when marquee is enabled. The documented class names remain stable regardless of internal structure, so descendant selectors work consistently. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Change marquee field from Option<MarqueeMode> to MarqueeMode, allowing direct checks on marquee.enable instead of unwrapping an Option. - Add struct-level #[serde(default)] to MarqueeMode - Remove field-level #[serde(default)] attributes (handled by Default trait) - Update volume module to check marquee.enable directly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Convert function comments to rustdoc format (///) and use 3rd person singular form as per Rust conventions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove Option wrappers from scroll_speed, pause_duration, and separator fields, providing defaults through the Default trait instead of runtime unwrap_or calls. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace RefCell with Cell for bool and Option types that implement Copy. Cell is more efficient as it doesn't require runtime borrow checking for simple value types. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace unwrap with expect, explaining the invariant that reset_at is always set before is_scrolling becomes true. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace _clone suffixed variables with scoped blocks that clone values, allowing the cloned variables to use the same names as their outer scope. This improves readability and avoids ambiguous naming. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Reword separator description to focus on user-visible behavior (gap when text loops) rather than implementation details. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Move marquee widget recreation logic from client to module's UpdateInput handler. This maintains proper separation of concerns - the client reports state changes while the module handles rendering decisions. When an input's name changes, the title widget is now recreated in the module to properly reset marquee scrolling state. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Set pause_started_at when setting up scrolling to add an initial pause before the text starts moving, matching the pause behavior at loop points. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Track the input name separately to properly detect changes. Previously, comparing against ui.label.label() didn't work because marquee duplicates the text with separator, causing recreation on every update. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
JakeStanger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, cheers
Part of #1011. Based on #1066 - GTK4 actually fixes that scrollbars bug, so pretty much no changes aside from updating to GTK4 and expanding what's configurable.
Overview
Known issues
None so far.
Limitations and future considerations
It's opt-in so no breaking changes, but anyone wishing to use might need to update their CSS (implementation details leak a bit):
For future extensions of other modules, even aside from just music (and its popup) I could see a lot of use-cases for making every Label scrollable like this. But this will necessarily mean replicating the same less-than-perfect UX pattern with regards to user CSS - wanted to hear your thoughts @JakeStanger on this.