feat(osd) add mpris osd for track change#2999
Conversation
Tried to make it work with two rows but couldn't figure it out without breaking something. I've just hyphenated the artist and track. |
| return; | ||
| const auto activePlayer = activePlayerOpt.value(); | ||
| // first artist only for now | ||
| MprisOsdData osd_data = {.Title = activePlayer.title, .Artist = activePlayer.artists[0]}; |
There was a problem hiding this comment.
artists[0] will crash if nor artist available in the dataset. This should use joinedArtists() for safety (even tho it maybe bring more than one artist, I think it's fine)
Naming convention violation osd_data is snake_case, it should be osdData.
|
|
||
| private: | ||
| OsdOverlay* m_overlay = nullptr; | ||
| MprisOsdData lastData; |
There was a problem hiding this comment.
Naming convention violation lastData is a private member => m_lastData.
| class OsdOverlay; | ||
|
|
||
| struct MprisOsdData { | ||
| std::string Title; |
There was a problem hiding this comment.
Naming convention violation, should be title and artist (no uppercase)
| MprisOsdData osd_data = {.Title = activePlayer.title, .Artist = activePlayer.artists[0]}; | ||
| if (osd_data == lastData) | ||
| return; | ||
| m_overlay->show(makeMprisContent(osd_data)); |
There was a problem hiding this comment.
add aguard if (m_overlay == nullptr) return;
|
On a more general note, we dont want "mpris" to surface much in the front facing strings/transaltions. This is a very technical jargon that does not mean much to the regular user. I recommend to simply use "Media" or "Now playing" instead. So I would rename OsdKind::Mpris, the class name and also remove mpris from translations keys and strings. |
2 participants
Summary
Adds an osd notification for mpris, so that it is triggered when the currently playing track is changed
Motivation
Is a neat feature to have
Aformentioned
Type of Change
Related Issue
Testing
Not sure of what commands to run for tests, but it builds fine and does not break other osds.
Manual Coverage
Compositor agnostic
Screenshots / Videos
Checklist
CONTRIBUTING.md.just formatwith clang-format v22+ installed, or this PR has no code changes.assets/translations/en.json, or this PR adds no new user-facing strings.Additional Notes
Still draft because the current OSD architecture allows for one label, ideally i reckon we'd want two for the artist and track on seperate lines at least for horizontal layout. I'm still trying to understand that part of the code.