GNOME Bugzilla – Bug 756491
Add media controls to the time and date drop down
Last modified: 2016-02-17 14:18:00 UTC
We lost controls for music apps with the notifications redesign. The new designs suggest showing media controls at the top of the notifications list: https://wiki.gnome.org/Design/OS/Notifications/#Time_and_Date_Drop-Down
Created attachment 321316 [details] [review] calendar: Split out message list base classes Currently both the base classes for messages/sections and the message list itself that instantiates the available sections are located in the same module. As a result, it isn't possible to define sections in a different module without introducing circular dependencies. The Calendar module is already unwieldily large, so split it up a bit to avoid it growing even bigger in the future.
Created attachment 321317 [details] [review] calendar: Add Media section We lost media controls outside of notification banners when implementing the new notification designs. Reimplement this functionality as a dedicated "Media" section in the message list based on MPRIS.
Review of attachment 321316 [details] [review]: Yes
Review of attachment 321317 [details] [review]: I will do another review this night if these questions I put need to be fixed in a new patch. ::: js/ui/mpris.js @@ +89,3 @@ + icon_size: 32 })); + } else { + this.setIcon(null); It's not there a missing album art icon or so? Non blocking for UI freeze @@ +152,3 @@ + + if (app) + app.activate(); why this difference between app/mpris.CanRaise? For music players that are playing as a daemon so you expect them to open a new window on activate? In case that's the reason, shouldn't the mpris raise remote handle this instead of us? Don't block here tho. @@ +183,3 @@ + let visible = this._playerProxy.CanPlay && + this._trackArtists.length > 0 && + this._trackTitle.length > 0; If I understood correctly, we hide the UI if the title or the artist is not provided? Could be do something like "unknown"? It would be odd to not be able to pause the current song/etc because of that. @@ +249,3 @@ + + if (newOwner && !oldOwner) + this._addPlayer(name); What about !newOwner && oldOwner, shouldn't we remove the player in that case?
Another small issue. All the headers do some actions, as they are buttons. But not the media one. Either we should provide an action there or disable the sensitive prop to avoid the highlight, right?
Playing with it a little more, indeed looks like we are not removing old players (probably when dbus notify of unwoning the dbus name, and player.closed doesn't get notified at all?) The result is the same player multiple times in the notification are, and this critical: (gnome-shell:8555): Gjs-WARNING **: JS ERROR: Exception in callback for signal: changed: TypeError: this.titleLabel.clutter_text is null Message<.setTitle@resource:///org/gnome/shell/ui/messageList.js:378 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 MediaMessage<._update@resource:///org/gnome/shell/ui/mpris.js:83 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 _emit@resource:///org/gnome/gjs/modules/signals.js:124 MprisPlayer<._updateState@resource:///org/gnome/shell/ui/mpris.js:181 wrapper@resource:///org/gnome/gjs/modules/lang.js:169
Ah ok, I think I found it, I think it's because in player.close handler we don't remove the message from the message list, so next time is shown that one would be still present.
(In reply to Carlos Soriano from comment #4) > ::: js/ui/mpris.js > @@ +89,3 @@ > + icon_size: 32 })); > + } else { > + this.setIcon(null); > > It's not there a missing album art icon or so? Non blocking for UI freeze Dunno, I'll try to get feedback from Allan/Jakub. > @@ +152,3 @@ > + > + if (app) > + app.activate(); > > why this difference between app/mpris.CanRaise? For music players that are > playing as a daemon so you expect them to open a new window on activate? In > case that's the reason, shouldn't the mpris raise remote handle this instead > of us? I just used RaiseRemote() initially, but the raised window ended up running into focus stealing prevention more often than not. Using the .desktop file instead doesn't have that issue - do you think there's a use case for an app to set the property but Raise() something different? I can only think of the case of multiple player windows, where app.activate() would raise all of them and Raise() just one ... > @@ +183,3 @@ > + let visible = this._playerProxy.CanPlay && > + this._trackArtists.length > 0 && > + this._trackTitle.length > 0; > > If I understood correctly, we hide the UI if the title or the artist is not > provided? Could be do something like "unknown"? It would be odd to not be > able to pause the current song/etc because of that. Mmmh, good point. I'm not sure not having that information for a playing song is very common, but possible. I added that code because Rhythmbox starts up with no current track, but still allows you to 'play' (in which case it plays the last played track, but only loads the metadata once it starts playing). Probably a better way to handle this is to use fallback values as you suggest for the case where a track without that metadata is played, and address the rhythmbox-startup case by checking for the 'Stopped' playback status ... > @@ +249,3 @@ > + > + if (newOwner && !oldOwner) > + this._addPlayer(name); > > What about !newOwner && oldOwner, shouldn't we remove the player in that > case? Mmh, that's supposed to be handled by the notify::g-name-owner handler in MprisPlayer. What player did you use for testing in comment #6? (In reply to Carlos Soriano from comment #5) > Another small issue. All the headers do some actions, as they are buttons. > But not the media one. Either we should provide an action there or disable > the sensitive prop to avoid the highlight, right? Yeah, the mockup at https://wiki.gnome.org/Design/OS/Notifications/#Active_Areas is a bit ... mysterious there ;-)
(In reply to Carlos Soriano from comment #7) > Ah ok, I think I found it, I think it's because in player.close handler we > don't remove the message from the message list, so next time is shown that > one would be still present. There's another player::closed handler in MediaMessage: this._player.connect('closed', Lang.bind(this, this.close)); So still no idea what's going on there :(
I'm testing with gnome-music, doing like: open gnome music. Play a song. Close gnome music. Open gnome music (two players now). And repeat.
(In reply to Florian Müllner from comment #8) > (In reply to Carlos Soriano from comment #4) > > ::: js/ui/mpris.js > > @@ +89,3 @@ > > + icon_size: 32 })); > > + } else { > > + this.setIcon(null); > > > > It's not there a missing album art icon or so? Non blocking for UI freeze > > Dunno, I'll try to get feedback from Allan/Jakub. > > > > > @@ +152,3 @@ > > + > > + if (app) > > + app.activate(); > > > > why this difference between app/mpris.CanRaise? For music players that are > > playing as a daemon so you expect them to open a new window on activate? In > > case that's the reason, shouldn't the mpris raise remote handle this instead > > of us? > > I just used RaiseRemote() initially, but the raised window ended up running > into focus stealing prevention more often than not. Using the .desktop file > instead doesn't have that issue - do you think there's a use case for an app > to set the property but Raise() something different? I can only think of the > case of multiple player windows, where app.activate() would raise all of > them and Raise() just one ... > Ah good point, probably worth a comment there in the code... > > > @@ +183,3 @@ > > + let visible = this._playerProxy.CanPlay && > > + this._trackArtists.length > 0 && > > + this._trackTitle.length > 0; > > > > If I understood correctly, we hide the UI if the title or the artist is not > > provided? Could be do something like "unknown"? It would be odd to not be > > able to pause the current song/etc because of that. > > Mmmh, good point. I'm not sure not having that information for a playing > song is very common, but possible. > I added that code because Rhythmbox starts up with no current track, but > still allows you to 'play' (in which case it plays the last played track, > but only loads the metadata once it starts playing). Probably a better way > to handle this is to use fallback values as you suggest for the case where a > track without that metadata is played, and address the rhythmbox-startup > case by checking for the 'Stopped' playback status ... > I think that's a better idea. > > > @@ +249,3 @@ > > + > > + if (newOwner && !oldOwner) > > + this._addPlayer(name); > > > > What about !newOwner && oldOwner, shouldn't we remove the player in that > > case? > > Mmh, that's supposed to be handled by the notify::g-name-owner handler in > MprisPlayer. What player did you use for testing in comment #6? hm right... gnome-music. > > > (In reply to Carlos Soriano from comment #5) > > Another small issue. All the headers do some actions, as they are buttons. > > But not the media one. Either we should provide an action there or disable > > the sensitive prop to avoid the highlight, right? > > Yeah, the mockup at > https://wiki.gnome.org/Design/OS/Notifications/#Active_Areas is a bit ... > mysterious there ;-) Oh :) Even with the points I raised, feel free to decide to merge if you need it to avoid the UI freeze.
(In reply to Carlos Soriano from comment #10) > I'm testing with gnome-music, doing like: > [...] > And repeat. Meh, found it - fixed locally.
Created attachment 321454 [details] [review] calendar: Add Media section (In reply to Carlos Soriano from comment #11) > (In reply to Florian Müllner from comment #8) > > > > > > It's not there a missing album art icon or so? Non blocking for UI freeze > > > > Dunno, I'll try to get feedback from Allan/Jakub. Added now, though Jakub will still have to do some tweaking. > > [...] Probably a better way > > to handle this is to use fallback values as you suggest for the case where a > > track without that metadata is played, and address the rhythmbox-startup > > case by checking for the 'Stopped' playback status ... > > > > I think that's a better idea. I ended up just using fallback artist/title labels - it's possible to be in 'Stopped' state and have correct track info, so we'd want to differentiate even further there, and I don't think it's worth the code. Plus it's really an issue with Rhythmbox, it has the same issue in its UI (the play button is sensitive and works, but there's no indication what track is going to play) ...
Review of attachment 321454 [details] [review]: Thanks for fixing the points I raised. The changes make sense to me, so go ahead :)
Created attachment 321487 [details] [review] calendar: Split out message list base classes Update attachment before pushing: - the new file contains translatable strings, so update POTFILES.in - telepathyClient was still looking for URLHighlighter in Calendar
Created attachment 321488 [details] [review] calendar: Add Media section Forgot to add mpris.js to POTFILES.in
Attachment 321487 [details] pushed as ee8fd1e - calendar: Split out message list base classes Attachment 321488 [details] pushed as 3ecdfaf - calendar: Add Media section