Moon-0xff / gnome-mpris-label

A music related GNOME extension.
GNU General Public License v3.0
51 stars 10 forks source link

Todo List - Contributions needed #4

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

It's me again!

Features list for discussion (original list):

My thoughts:

  1. Interesting idea, I wonder if there is an automated way to locate/load these icons in gjs. It would be possible to include icon for the usual apps (rhythmbox, spotify, firefox, chromium,...) which would probably easier but I'm not sure if copyrights would allow it and there is a risk the icon shipped may not match the one used in the distro... there may be some content in the Vitals or runcat github repo which could be reused to load icons next to the text as they both have that.
  2. Could work. Alternatively, a drop down (or equivalent) with common apps might work too.
  3. In terms of implementation, there may be some code to be taken from the Vitals extension as it allows to chose content based on a menu activated when clicking the extension: https://github.com/corecoding/Vitals
  4. it might get confusing if clicking shows the menu in some cases but pauses in other cases? Including these controls within a menu might be better, something similar to this other discontinued mpris extension
  5. I agree that the current implementation which continuously updates is somewhat suboptimal but I don't know if this is even possibly. If it was, I wonder how much this would really save in terms of resource.

Features list for discussion (later additions):

Moon-0xff commented 1 year ago

I wanted to only discuss what should be added to the next release, but sure, let's discuss this list.

A general response to your thoughts

The objective of this extension (as the description says) is to work with any mpris compatible source. We don't want to restrict certain features to only a limited amount of players, and we don't have to! The freedesktop specifications (and others) are largely respected.

1. Icons alongside the label

sp-tray looks great with the spotify icon and I'm jealous!

How to achieve this? I don't know but GNOME does it already, so it's probably just a matter of copying the code. If that fails then I'm pretty sure there's a standard way of accessing icons, as icons rarely if ever goes missing, regardless of DE/WM or even container (flatpak,snap,appimages).

2. Filter list

We need this feature, specially with the auto-switch option as we discussed previously.

Implementing it isn't a problem. The challenge will be to accommodate it to the settings UI, probably will need a complete UI overhaul.

3. Change player with a dropdown menu

Cycling players by clicking the label it's a bit awkward. I wanted to do this in the first place.

The challenge here will be probably to maintain compatibility with GNOME 3.3x

4. Clicking the label to pause

Might get confusing? Probably, I was thinking of this as a optional feature (turned off by default). I find it useful!

Regarding the extension you linked, i don't want to replicate it. Other extensions do this already.

5. This extension as async

It's a matter of doing it right. The mpris specification and gjs.guide point to accessing DBus asynchronously.

The bonus

It's a good idea, the extension name and description it's far too technical, but i don't want to give the impression that this extension only works for music.

This extension originally was named mpris-music-label (4e6f3f45fe8dd09a95743d1e06f78e73132c82ad). I didn't like this name as really the media could be anything!

Batwam commented 1 year ago

Draft code for item 1 included in Pull Request #5 . Hopefully you won't be jealous for long!

Moon-0xff commented 1 year ago

5 was merged. Once again. Thanks @Batwam for all your contributions.

Moon-0xff commented 1 year ago

Last changes to this list:

Batwam commented 1 year ago

one thing to note also is that when available, mpris includes a path to the album icon as part of the metadata string under mpris:artUrl. This means that rather than loading the app icon, we could in theory load the album picture instead. For apps like shortwave with radio station logo instead of the app logo for instance, that could be quite neat. Same thing if we were to show the album picture rather than the Spotify icon also I guess, it kind of depends on the bar size/resolution as it could be quite small. Usually it's more something which gets loaded into a menu but I'm just putting this out there. image

When the icon is stored locally like with Rhythmbox, it will provide a local file:/// path.

Moon-0xff commented 1 year ago

Sounds real neat specially with shortwave, but we get less than 32px in a 1920x1080 screen, album covers are far too detailed to translate well into such a small amount of pixels.

As a "general" option i don't think is of too much use, as a "per-application" option sounds good, but we will need to implement "per-application options" first.

Batwam commented 1 year ago

for info, I have tested Opera/Brave/Vivaldi in a VM. As expected, they all report as chromnium and won't load the icons but the following (snipets of) code work. Brave had to be capitalised to work for some reason. The problem is obviously that since it's not searching through running apps, it will pick up the icon from the first app which is installed, regardless of whether it's the correct one or not... which is why I have loaded them in order of popularity: matchedEntries = Gio.DesktopAppInfo.search("Opera") matchedEntries = Gio.DesktopAppInfo.search("Brave") matchedEntries = Gio.DesktopAppInfo.search("Vivaldi")

if we could somehow find our the app hidden behind the mpris source or exclude non-running apps that would be a lot easier...

These were installed using debs (Opera/Vivaldi) when no flatpak was available. Brave was flatpak. I tried the snaps but for some reaons there is an issue with appArmor and they can't seem to be able to get access to mpris so the extension doesn't work for snap versions of these browsers (works for firefox though...).

Moon-0xff commented 1 year ago

As previously mentioned, there's a way of getting desktop entries from running apps. This doesn't solve our problem directly but surely a solution can be coded from it.

Regarding the problems with D-Bus access that's not so unexpected, I'm pretty sure a lot of packagers will perceive it as an unnecessary security risk.

Batwam commented 1 year ago

I'll move the comment above to the icon discussion for the record.

Batwam commented 1 year ago

By the way, I noticed that the color-picker extension has implemented different actions for right Vs left click on the extension icon. I thought that it's interesting as the other extensions I checked all handled left and right clicks the same way and I assumed that it was a limitation in gjs. Apparently not.

This opens up some additional opportunities for right click pop-up manually select player or select "Automatic Switch" Vs Left click track info, track controls or things like that: https://github.com/tuberry/color-picker

Moon-0xff commented 1 year ago

I thought the same. Then sure, we can use a button for the player selection and the other for "Additional actions". But how color-picker achieves that exactly?

Batwam commented 1 year ago

I haven't spent too much time on it but I suspect that it lies within this part of the extension.js code so it might be catching the click and run a different action if it's a left click: image

This second part seems to command differentiated action when picking colours with left=select, middle=menu, right=escape image

Moon-0xff commented 1 year ago

The recent changes I made to prefs.js can accommodate a filter list (we only need to add another page to prefsWidget)

However they are two problems:

  1. Under GNOME 4x the window looks wrong and is not correctly aligned at startup (in GNOME 3.3x looks and behaves very nice)
  2. I'm not sure how to implement the interface for the filter list in GTK.

edit: Added a workaround for the first problem

Batwam commented 1 year ago

Neat menu! yeah, I just noticed that you pushed a new commit for #1. It looks fine for me.

Wouldn't the interface for filter be a simple text field where the user would include his regex expressions, possibly delimited with commas?

Btw, will you be uploading v11 to EGO?

Moon-0xff commented 1 year ago

I was thinking in making some sort of dynamic list with widgets, easier to use but harder to implement.

Last update was less than a week ago so i don't think the changes justify an update yet.

Moon-0xff commented 1 year ago

I added the filters list page using your proposed interface. I don't think usability suffers at all.
The branch name is filter-list

I call it a day for today so if you can do the "backend" part of it and submit a pull request i would gladly accept it.

Batwam commented 1 year ago

I had a look at the white list implementation and still can't think of a case where it could prove useful. The only time it would do something would be if someone included the same source in blacklist and whitelist and even then, it would be questionable whether the blacklist or whitelist should take priority.

If we had a full on regex syntax with wildcards then there could have been a slim use case but since we don't (currently) and it would probably be over the top for this application, I still think that we are over-complicating the interface with a whitelist. Any thoughts?

Moon-0xff commented 1 year ago

As I said, the whitelist isn't very useful on it's own, so i added a new option to make it useful, the new option treats all non-whitelisted sources as blacklisted, this is useful if you want to use this extension with just one player.

Overall the whitelist and this new option just saves a few annoyances and a bit of user input, so if the whitelist proves to be problematic I'm not against removing it.

Batwam commented 1 year ago

Oh, I see. Would it be cleaner to instead have a drop down where the user selects blacklist or whitelist, with the text field underneath which will be treated accordingly depending on the drop down value?

I'm also thinking, we both know what each option does but some would probably be rather obscure to a new user. Is there a way to add some sort of ToolTip extended description which would appear when the user puts its mouse over the item?

Moon-0xff commented 1 year ago

Yes, i don't think using both whitelist and blacklist at the same time is useful at all, and this change could simplify the code a bit, although i don't see how we can present it in a clear way, with a drop-down one value (or both) will be obscured.

ToolTip extended description which would appear when the user puts its mouse over the item?

I searched for something like that in the documentation with no luck https://gjs-docs.gnome.org/gtk30~3.0/

edit: oh! it's the same name you mentioned: https://gjs-docs.gnome.org/gtk30~3.0/gtk.tooltip

Batwam commented 1 year ago

yeah, I was somewhat hopefuly reading this page but nothing seems to work. I had a look at some of the other extensions and couldn't find examples of tooltip.

When it comes to the filters themselves, if we are concerned that the terms blacklist/whitelist may no be self-explanatory, we could quite easily just add some text at the top of bottom to explain what each does.

Moon-0xff commented 1 year ago

I found Gtk.Widget.set_tooltip_text and does exactly what we want.

Add this line to the addLabel function in prefs.js: thisLabel.set_tooltip_text('Example Text'); and point the mouse over any label (any label constructed with addLabel)

Batwam commented 1 year ago

Nice! See #19 with tooltip implementation and some tooltips populated (rest left as undefined with no tooltip generated).

Moon-0xff commented 1 year ago

For the bonus, have you read the current description?

The day i rewrote the description the extension received two more comments on EGO so i think it works. Although the current description is a "condensed version" of the original rewrite.

Batwam commented 1 year ago

Nice once, I didn't notice actually. Do you want another screenshot to show it working with a different player in the bottom panel? Or maybe include "also compatible with Dash to panel for anyone wondering?

We can probably also add Shortwave to the list of compatible player considering how much work it created!

Moon-0xff commented 1 year ago

I've been considering the last one, but we can't be sure if shortwave will give us problems in the future or not. Same reason why I'm not sure if it's a good idea to somewhat promise compatibility with other extensions (dash to panel in this case).

Batwam commented 1 year ago

Ok, no worries. I marked the action as closed already anyway.

Moon-0xff commented 1 year ago

Submitted the new version to ego (hopefully isn't rejected twice again)

Batwam commented 1 year ago

Excellent. That's action 2&3 done then. Anything else to be done on those? I wasn't sure why the ChangeLog was changed from "v11" to "upcoming"

Moon-0xff commented 1 year ago

because the version is assigned by ego, and might be higher if the first upload is rejected

Batwam commented 1 year ago

Ah, that's why we jumped from v7 to v10, it all makes sense now! Did the resubmissions require code change? Do they tell you why they reject?

Moon-0xff commented 1 year ago

You can see the ego reviews for the extension in this link
Warning: some of them are really silly

For a quick rundown: v2: Forgot to delete the .git folder v5: Forgot to remove the install script, and global scope issues v6: The time i submitted this overlapped with the review time so i didn't read v5 review v8 and v9: global scope issues

Batwam commented 1 year ago

In a way, I'm positively impressed they are reviewing it thoroughly since most people download extensions blindly and they can potentially represent a pretty big security hole. They also provide feedback with links with references which is nice of them. Just learnt than the Lang module is depreciated which is good to know as I was about to use it...

edit by Moon-0xff: removed flashbang

Batwam commented 1 year ago

Thanks for updating the version to v12, otherwise the extension keeps getting updated automatically in the background by gnome and it overwrites the test versions if they have a lower rev.

Moon-0xff commented 1 year ago

I'm thinking of embedding the controls to the label this way:

This (maybe) could be implemented by just watching the click event and a timer.

I already added the control methods and started a branch: controls

Moon-0xff commented 1 year ago

I forgot about the Clutter button events, if we can make them work then we can do a better layout, I'm thinking of:

All of this, of course, customizable through settings

Batwam commented 1 year ago

Yeah, I'm just not convinced "double click" is an event type but I could be reading in the wrong place: https://lazka.github.io/pgi-docs/Clutter-0/enums.html#Clutter.EventType

I certainly haven't seen it implemented on any of the extensions I have installed. Do you have any early indication it might we possible?

Without it, the options are going to be limited to left/middle/right.

The alternative would to throw these controls into a menu like everyone else. I'm personally not bothered as I have keyboard shortcuts but if there is no other way and you are ok to include a section in the menu for these controls, we can always put the there and make that section optional.

PS: some further suggestions here to implement double click detection by measuring the time between first release and next click... https://www.mail-archive.com/gnome-shell-list@gnome.org/msg08777.html

Moon-0xff commented 1 year ago

I can definitely imagine how a double click could be detected:

Moon-0xff commented 1 year ago

As a user, I certainly would prefer the player controls as a mouse layout instead of a menu. Also, i haven't had any luck adding a menu to the extension, so i would prefer a solution that doesn't involve them.

Batwam commented 1 year ago

I did have a go at the menu thing and am making some progress although it still needs some of the building blocks (namely the bits that actually do things!).

Have a look at my menu_test branch. I haven't pushed it as it's not functional and it's probably not optimised but it does include a dynamic menu based on actual sources. Except the Settings button, it doesn't do anything currently as I don't have the function to activate the player selected but it shows what it could look like: image

The idea to build the menu would be to list the players in order and then select the active player purely on index. So selecting item 1 would select this.players.list[0]. This way, I don't even need to know what the player's name is, it's purely for display purposes (if that makes sense). Also, if there are multiple chromium sources... there would be 2 "Chromium" entries but they would each link to their own individual source in the players list so that still works.

I disabled the left click for now as otherwise, the left click will switch next but also open the menu. The menu would essentially replace the current left click source selection. Again, it's just a first pass proof of concept.

I don't plan to integrate controls into the menu but I personally think that having a menu in general looks quite neat to address item 4 (Player selection). I certainly appreciate having a shortcut to the Settings panel too! It would limit some possibilities for controls as it would remove the use of the right click which is why I wanted to bring this up. However, if we can differentiate left and right click, we could even in theory have a different menus for each, left click for controls and right click for player selection/Settings.

Let me know if this is something you'd like to explore as it could open up some possibilities.

PS: bonus thought, if we wanted to, we could have a little cross on the right of each item which would send the selected source into the newly created blacklist.

Moon-0xff commented 1 year ago

As you mentioned it, this could limit what we can do for the controls. I don't perceive this as a bad trade-off.

Embedding controls to the label is an afterthought, a neat and useful feature, but the idea of selecting the source with a menu is the original idea for the extension.

I share your ideas about the menu, however i think you are doing too much at the same time, maybe try to get the "bare minimum" working first, a working selection and the indicator for the current one. That alone would be release worthy (but we pushed an update very recently so that wouldn't be the case).

This is great! Keep up the good work! and submit a PR if you get the "bare minimum" working, i would add it to main right away.

edit: By the way, I tested it on GNOME 3.38, the menu works but the dot doesn't appear

HubKing commented 1 year ago

How about scrolling mouse over the song title controls the audio volume? I know that there already is an extension that controls volume by scrolling, so it must be technically possible. The problem of that existing extension is that the mouse scroll action applies to the entire Top Bar, which causes problems with other extensions that take scroll input.

Batwam commented 1 year ago

That sounds pretty odd if the scroll impacts other extensions. Do you have a link to that extension?

With the upcoming popmenu for player selection, right click will be taken but there could certainly be additional capabilities which could be considered if we are able to implement a method to distinguish between left/middle/scroll. We know it is possible on the color-picker extension but I haven't seen other extensions doing it and we haven't implemented this yet.

Once implemented, doing mpris calls like pre/next/play/pause/volume_up/volume_down should be relatively straightforward but that would be another thing to implement as we are currently only reading data.

HubKing commented 1 year ago

That sounds pretty odd if the scroll impacts other extensions. Do you have a link to that extension?

https://github.com/trflynn89/gnome-shell-volume-scroller

By "affecting", I meant that there are other extensions who do something else when the users scrolls the wheel above that extension's entry on the TopBar, and since this extension affects the whole TopBar, when I scrolled the wheel over the extension's entry, it did both (doing that extension's job and changing the volume).

Batwam commented 1 year ago

ok, I had a look and, if I understand correctly, the whole point of that extension is for the user to be able to change the volume by scrolling anywhere along the menu so the behaviour seems to make sense. Do you have a link to one of the extensions which takes user scroll as an input? that would be useful. Edit: nevermind, i found how to do it :-)

One thing we could potentially look into is, rather than changing the overall system volume like the extension you showed, it should be possible to only affect the volume of the individual mpris source selected. Somewhat similar in function to this extension, but without the menu. If people want to change the system volume, they can already scroll over the sound icon so no need to duplicate that.

Batwam commented 1 year ago

ok, I've managed to implement the volume control with wheel scroll by grabbing 'scroll-event'. It changes the volume of the active mpris source only, not the global system volume. See branch/pull request above, it's pretty neat actually. Any comment regarding the implementation should be made directly in #26.

@Moon-0xff I'll wait for your feedback before going further. We should finish implementing the filters we have been working on before considering this feature but it shows that it can be done. I know that you are busy so no rush ;-)

HubKing commented 1 year ago

If people want to change the system volume, they can already scroll over the sound icon so no need to duplicate that.

No, we can't, if by "sound icon" you mean the thing on the right-most of the TopBar where the Wi-Fi icon and battery icon is. In fact, I have been (internally) complaining about this. I have been thinking that I may need to create an extension that shows the system audio volume and the monitor brightness and I could change them (volume and brightness) )by scrolling over it.

PS: On second try, it seems I can change the volume by scrolling but only when the menu is not open. But I think the scrolling area is too narrow. I maybe create an extension with a larger area to scroll on, by having the numeric volume value.

Batwam commented 1 year ago

Yeah, it's arguably a bit small but you can definitely change the system volume by scrolling over the speaker icon.

Have you tried the controls_test branch? Does it do what you want? Is it ok to control the active music app only? I guess we could always have an option for the user to decide if the scroll changes the source or general volume if that helps.

Moon-0xff commented 1 year ago

Hello @Batwam I'm indeed quite busy to review the code.

The idea sounds good, and by the screenshot i judge it looks good too.

I think of two possible problems:

There's methods in mpris to know the players capabilities, that and by introspecting the proxy (with Object.keys) we can probably know if the player will react correctly (or not) to the signal.

Maybe i'm wrong but either way i would prefer to show a graphical indication that the volume control doesn't work with the player (if we can confidently know). I have no idea how that could be achieved.

Option wise, I'm thinking of a combobox with these options (if we can know if players react or not to the signal):

I can't help with the code or test it for now, but i will keep an eye on the comments and commit messages.

Let me know how it goes!

Moon-0xff commented 1 year ago

I trust your senses and if you think it's ready to merge i will do it.

Also, i tried to implement the popup menu as an option and failed.