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

[2.2] Add track name and album art to panel #2662

Merged
merged 6 commits into from Apr 9, 2014

Conversation

Projects
None yet
4 participants
@Psykar
Contributor

Psykar commented Nov 26, 2013

Updates the sound@cinnamon.org applet to add track name and album art to the panel (configurable)

Applet is available on spices here:
http://cinnamon-spices.linuxmint.com/applets/view/164

@RavetcoFX

This comment has been minimized.

Show comment
Hide comment
@RavetcoFX

RavetcoFX Nov 27, 2013

Contributor

Hey @Psykar, your code is looking good, but is not functioning with some media players because of how they handle outputting to mpris (such as players that use mpris V1) would you mind testing and patching this to function with various media players such as Tomahawk, GMusic Browser and possibly others.

Cheers

Contributor

RavetcoFX commented Nov 27, 2013

Hey @Psykar, your code is looking good, but is not functioning with some media players because of how they handle outputting to mpris (such as players that use mpris V1) would you mind testing and patching this to function with various media players such as Tomahawk, GMusic Browser and possibly others.

Cheers

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Nov 27, 2013

Contributor

Cheers @RavetcoFX - I'm guessing that's mainly around the album icon - the track names should be the same at least?

Installing a couple of other players now...

Contributor

Psykar commented Nov 27, 2013

Cheers @RavetcoFX - I'm guessing that's mainly around the album icon - the track names should be the same at least?

Installing a couple of other players now...

@RavetcoFX

This comment has been minimized.

Show comment
Hide comment
@RavetcoFX

RavetcoFX Nov 27, 2013

Contributor

@Psykar In my testing gmusicbrowser with your patch does not display either. But tomahawk, VLC, Clementine do. Hmmm, this is interesting because gmusicbrowser works with the main applet. I will test some more

Contributor

RavetcoFX commented Nov 27, 2013

@Psykar In my testing gmusicbrowser with your patch does not display either. But tomahawk, VLC, Clementine do. Hmmm, this is interesting because gmusicbrowser works with the main applet. I will test some more

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Nov 28, 2013

Contributor

Cleaned up, fixed up play/pause, tested with tomahawk, gmusicbrowser, spotify, rhythmbox, vlc.
Seems gmusicbrowser doesn't send changed notifications properly, but I've worked around it.

gmusicbrowser also doesn't send it's album art at all, but the others work fine.

Also pushed the update to http://cinnamon-spices.linuxmint.com/applets/view/164 if people want to test it there (different namespace there though)

Contributor

Psykar commented Nov 28, 2013

Cleaned up, fixed up play/pause, tested with tomahawk, gmusicbrowser, spotify, rhythmbox, vlc.
Seems gmusicbrowser doesn't send changed notifications properly, but I've worked around it.

gmusicbrowser also doesn't send it's album art at all, but the others work fine.

Also pushed the update to http://cinnamon-spices.linuxmint.com/applets/view/164 if people want to test it there (different namespace there though)

@RavetcoFX

This comment has been minimized.

Show comment
Hide comment
@RavetcoFX

RavetcoFX Nov 28, 2013

Contributor

@Psykar Neat, perhaps it would be a good idea to limit the display information to 20-30 chars (?)

Contributor

RavetcoFX commented Nov 28, 2013

@Psykar Neat, perhaps it would be a good idea to limit the display information to 20-30 chars (?)

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Nov 29, 2013

Contributor

@RavetcoFX I'll get on the display length limit (probably make it a setting slider)
As for the centering the popup, in my mind that's an update that's required to the core ui/js code - it should probably be a default for Applet.TextIconApplet
At the moment, the left edge of the menu gets aligned with the center of the applet.

Contributor

Psykar commented Nov 29, 2013

@RavetcoFX I'll get on the display length limit (probably make it a setting slider)
As for the centering the popup, in my mind that's an update that's required to the core ui/js code - it should probably be a default for Applet.TextIconApplet
At the moment, the left edge of the menu gets aligned with the center of the applet.

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Dec 2, 2013

Contributor

@RavetcoFX ... and anyone else ;)

Pushed up a setting for truncation of long titles.

Contributor

Psykar commented Dec 2, 2013

@RavetcoFX ... and anyone else ;)

Pushed up a setting for truncation of long titles.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 14, 2013

is it possible to add a "slide" like animation when you hover with the mouse over the applet.To be able to see the rest of the text.

ghost commented Dec 14, 2013

is it possible to add a "slide" like animation when you hover with the mouse over the applet.To be able to see the rest of the text.

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Dec 18, 2013

Contributor

@zagortenay333 possible, yes. However, would require setting up a new loop and callback in the code, complicating the relative simplicity of this pull request at the moment.
I may have a look at it, just not in the next couple of weeks.

Contributor

Psykar commented Dec 18, 2013

@zagortenay333 possible, yes. However, would require setting up a new loop and callback in the code, complicating the relative simplicity of this pull request at the moment.
I may have a look at it, just not in the next couple of weeks.

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Jan 5, 2014

Contributor

I'm not convinced the defaults should be false, just so functionality doesn't change when a user upgrades.
If they are set to false, users are likely to not even know the options are there?

Happy to be overruled though.

Contributor

Psykar commented Jan 5, 2014

I'm not convinced the defaults should be false, just so functionality doesn't change when a user upgrades.
If they are set to false, users are likely to not even know the options are there?

Happy to be overruled though.

@RavetcoFX

This comment has been minimized.

Show comment
Hide comment
@RavetcoFX

RavetcoFX Jan 5, 2014

Contributor

@Psykar If this is merged, it is likely be mentioned in the release notes, so people would find out about it in that manner. And the album art icons can be laggy, just a thought :)

Contributor

RavetcoFX commented Jan 5, 2014

@Psykar If this is merged, it is likely be mentioned in the release notes, so people would find out about it in that manner. And the album art icons can be laggy, just a thought :)

@clefebvre

This comment has been minimized.

Show comment
Hide comment
@clefebvre

clefebvre Mar 18, 2014

Member

Hi,

It works well, but it needs two changes:

  • When no artwork is available a default icon should be used (right now I end up with an invisible icon in the panel so if I don't show the title as well I don't see my applet)
  • Both settings should be set to False by default.
Member

clefebvre commented Mar 18, 2014

Hi,

It works well, but it needs two changes:

  • When no artwork is available a default icon should be used (right now I end up with an invisible icon in the panel so if I don't show the title as well I don't see my applet)
  • Both settings should be set to False by default.
@dtrunk90

This comment has been minimized.

Show comment
Hide comment
@dtrunk90

dtrunk90 Mar 18, 2014

Contributor

@clefebvre There's a default icon in the popup when no artwork is available. Why not just use that? Here it is a simple disk:
bildschirmfoto vom 2014-03-18 20 15 41

Contributor

dtrunk90 commented Mar 18, 2014

@clefebvre There's a default icon in the popup when no artwork is available. Why not just use that? Here it is a simple disk:
bildschirmfoto vom 2014-03-18 20 15 41

@clefebvre

This comment has been minimized.

Show comment
Hide comment
@clefebvre

clefebvre Mar 23, 2014

Member

Yes, that would work nicely and it would be consistent with the artwork show[n|ed] in the applet itself.

Member

clefebvre commented Mar 23, 2014

Yes, that would work nicely and it would be consistent with the artwork show[n|ed] in the applet itself.

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Mar 24, 2014

Contributor

I had actually intended this to work with the music note icon - turns out I had an edge case that wasn't handling the no art correctly when moving from a track with art to one with no art.
Fixed up defaults too.

As an aside - should I be squashing these commits before it gets pushed?

Contributor

Psykar commented Mar 24, 2014

I had actually intended this to work with the music note icon - turns out I had an edge case that wasn't handling the no art correctly when moving from a track with art to one with no art.
Fixed up defaults too.

As an aside - should I be squashing these commits before it gets pushed?

Updates to sound applet.
Fix tracks with no album art.
Swap showing track name / art defaults to false.
@clefebvre

This comment has been minimized.

Show comment
Hide comment
@clefebvre

clefebvre Apr 1, 2014

Member

If you want.

I can't merge as it is though, there's a double try{} on line 818, and the fallback to the DVD icon doesn't work (I still get an empty icon in the panel when a DVD is shown on the art cover inside the applet).

Member

clefebvre commented Apr 1, 2014

If you want.

I can't merge as it is though, there's a double try{} on line 818, and the fallback to the DVD icon doesn't work (I still get an empty icon in the panel when a DVD is shown on the art cover inside the applet).

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Apr 2, 2014

Contributor

Fixed up the double try (due to the metadata being added in to the init since this pull was created)

What do you refer to by empty icon, just blank space?
It should be showing the 'audio-x-generic' icon, and all my testing has this icon when the track playing has no artwork. I tested with VLC and rhythmbox, by playing a track with no art, or a track with art followed by a track with no art. Both times it reverts to the correct icon.

What media player are you using? What steps reproduce it? (ie. playing a track with album art first, or just loading the media player for a track with no art?)

Contributor

Psykar commented Apr 2, 2014

Fixed up the double try (due to the metadata being added in to the init since this pull was created)

What do you refer to by empty icon, just blank space?
It should be showing the 'audio-x-generic' icon, and all my testing has this icon when the track playing has no artwork. I tested with VLC and rhythmbox, by playing a track with no art, or a track with art followed by a track with no art. Both times it reverts to the correct icon.

What media player are you using? What steps reproduce it? (ie. playing a track with album art first, or just loading the media player for a track with no art?)

@clefebvre

This comment has been minimized.

Show comment
Hide comment
@clefebvre

clefebvre Apr 2, 2014

Member

Hi,

I used Banshee for testing. I played a song with artwork first, then moved to next songs. Some songs with no artwork showed the symbolic music note icon, some songs with no artwork showed an empty icon (i.e. no icon at all). I'd like the artwork to be the same in the panel and the applet (for consistency), so if we show the album cover when there is one, we should show the DVD icon when there is none, just like we do inside the applet itself.

Member

clefebvre commented Apr 2, 2014

Hi,

I used Banshee for testing. I played a song with artwork first, then moved to next songs. Some songs with no artwork showed the symbolic music note icon, some songs with no artwork showed an empty icon (i.e. no icon at all). I'd like the artwork to be the same in the panel and the applet (for consistency), so if we show the album cover when there is one, we should show the DVD icon when there is none, just like we do inside the applet itself.

@clefebvre clefebvre changed the title from Add track name and album art to panel to [2.2] Add track name and album art to panel Apr 2, 2014

@Psykar

This comment has been minimized.

Show comment
Hide comment
@Psykar

Psykar Apr 3, 2014

Contributor

Seems Banshee sends a album art path, which may be an non existant file. Fixed.

Changed the icon to media-optical-cd-audio when we are showing album art. This is the same icon which is used in the applet itself, but not full colour (~ line 380)
If album art isn't being shown, then the existing 'audio-x-generic' is shown when a media player is open (as per existing code (~ line 770))

In the process, I've cleaned the code around setAppletIcon

Contributor

Psykar commented Apr 3, 2014

Seems Banshee sends a album art path, which may be an non existant file. Fixed.

Changed the icon to media-optical-cd-audio when we are showing album art. This is the same icon which is used in the applet itself, but not full colour (~ line 380)
If album art isn't being shown, then the existing 'audio-x-generic' is shown when a media player is open (as per existing code (~ line 770))

In the process, I've cleaned the code around setAppletIcon

@clefebvre

This comment has been minimized.

Show comment
Hide comment
@clefebvre

clefebvre Apr 9, 2014

Member

It's better. Be careful to test your code though, you're missing a closing bracket here.

There's also a tiny issue which sets the icon even when the setting is set to False.

Member

clefebvre commented Apr 9, 2014

It's better. Be careful to test your code though, you're missing a closing bracket here.

There's also a tiny issue which sets the icon even when the setting is set to False.

@clefebvre clefebvre merged commit 20939e6 into linuxmint:master Apr 9, 2014

@clefebvre

This comment has been minimized.

Show comment
Hide comment
@clefebvre

clefebvre Apr 9, 2014

Member

Fixed and merged.

Thanks for these new features.

Member

clefebvre commented Apr 9, 2014

Fixed and merged.

Thanks for these new features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment