mediacontrols@rbfraphael: add new features#8276
mediacontrols@rbfraphael: add new features#8276lumapu wants to merge 2 commits intolinuxmint:masterfrom
Conversation
* configuration window with a few settings * hover actions
There was a problem hiding this comment.
Pull request overview
This pull request adds new configuration options and UI enhancements to the mediacontrols applet. The changes introduce a settings schema to make the applet customizable, add hover effects for control buttons, and allow users to control what elements are displayed in the panel.
Changes:
- Added settings configuration with five customizable options (show/hide player icon, song title, player selector, and control buttons when inactive, plus hover color selection)
- Implemented hover style functionality with customizable background color for control buttons
- Changed CSS padding from right margin to horizontal padding for better spacing
- Version bumped to 1.1.0 with a last-edited timestamp
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| stylesheet.css | Changed margin-right to padding for more balanced spacing |
| settings-schema.json | New file defining configuration UI with appearance settings section |
| metadata.json | Version bump to 1.1.0 and added last-edited timestamp |
| applet.js | Added settings binding, UI rebuild logic, conditional element rendering, and hover style implementation |
| _onSettingsChanged() { | ||
| this._updateUi(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The _onSettingsChanged method is defined but never called. This appears to be dead code that should either be removed or connected to the settings if it serves a purpose.
| _onSettingsChanged() { | |
| this._updateUi(); | |
| } |
| this.mediaInfo.child.text = songDescription; | ||
| this.set_applet_tooltip(songDescription); | ||
|
|
||
| this.mediaInfo.child.text = songDescription.substr(0, 20); |
There was a problem hiding this comment.
The text is truncated to 20 characters using substr, which can cut in the middle of a multi-byte UTF-8 character, potentially causing display issues. Consider using slice() instead, or better yet, add an ellipsis (e.g., "...") to indicate truncation. Also, hardcoding the limit to 20 characters seems arbitrary and should ideally be configurable.
There was a problem hiding this comment.
slice() does the same as substr so that won't work. You can instead use splitByGlyph() in js/misc/utils.js
const Util = imports.misc.util;
...
this.mediaInfo.child.text = Util.splitByGlyph(songDescription).slice(0, 20).join("");[...songDescription].slice(0, 20).join("") should work in the future but not with the current version of cjs.
| [this.previousButton, this.playPauseButton, this.nextButton].forEach(button => { | ||
| if (this.configHideControlsInactivePlayer) { | ||
| button.hide(); | ||
| } else { | ||
| button.show(); | ||
| button.reactive = false; | ||
| button.opacity = 100; | ||
| } | ||
| }); |
There was a problem hiding this comment.
When a button is set to non-reactive (line 277), the hover style will still be applied by the event handlers in _setHoverStyle. This means inactive buttons will show hover effects even though they can't be clicked, which is confusing UX. Consider checking button.reactive in the hover event handlers, or removing hover handlers when buttons are made non-reactive.
| _setHoverStyle(button) { | ||
| // remove old event handlers, if there are any | ||
| if (button._enterEventId) { | ||
| button.disconnect(button._enterEventId); | ||
| } | ||
| if (button._leaveEventId) { | ||
| button.disconnect(button._leaveEventId); | ||
| } | ||
|
|
||
| // add new event handlers | ||
| button._enterEventId = button.connect('enter-event', () => { | ||
| button.set_style(`background-color: ${this.configBackgroundHover};`); | ||
| }); | ||
|
|
||
| button._leaveEventId = button.connect('leave-event', () => { | ||
| button.set_style(''); | ||
| }); | ||
| } |
There was a problem hiding this comment.
When _updateStyles is called after _buildUi during initialization, the hover event handlers are correctly set up. However, when _rebuildUi is called and new button instances are created, the old event handler IDs stored on the old button objects are lost. The new buttons will get new event handlers, which is correct, but the comment about "remove old event handlers" in _setHoverStyle is misleading in this context since the buttons are new objects. Consider clarifying this logic or ensuring old handlers are properly disconnected before button destruction.
| "type": "switch", | ||
| "default": true, | ||
| "description": "Show Artist and Song title", | ||
| "tooltip": "Shows Artist and Song tilte" |
There was a problem hiding this comment.
Spelling error in tooltip. "tilte" should be "title".
| "tooltip": "Shows Artist and Song tilte" | |
| "tooltip": "Shows Artist and Song title" |
@RBFraphael Thank you for this great plugin. It's exactly what I searched for. Modified it a bit and made it customizable.
new features
available settings