GNOME Bugzilla – Bug 549524
Show Love/Ban buttons in the toolbar in fullscreen mode
Last modified: 2020-03-17 08:24:53 UTC
It would be great if banshee would also show the last.fm love/ban buttons in the popup toolbar in fullscreen mode. (Is there some reason not to use exactly the toolbar from "normal" mode?)
I assume you're referring to the fullscreen mode that you get when you have the "Now Playing" selected ? Because if I switch to the fullscreen mode when I have a Last.fm source selected, I can see the love/ban button.
Yes, the "Now Playing" fullscreen mode. Attaching screenshots for further clarification.
Created attachment 117489 [details] Windowed
Created attachment 117490 [details] Fullscreen
Created attachment 117615 [details] [review] Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar
Created attachment 117616 [details] [review] Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar Meh, tabs
Thanks for the patch ! With the patch, when I'm in fullscreen mode, the "Leave fullscreen" button has no text and the icon is not the right one (it's the "enter fullscreen" icon). I also don't know if copying the widget from the toolbar is the best thing to do. What if something is added to it that we don't want in the fullscreen mode ? Would it be much more complicated to just add the Love/Ban buttons ?
Yeah, I think a better approach would be to add a new fullscreen toolbar in src/Core/Banshee.ThickClient/Resources/core-ui-actions-layout.xml and then modify the Lastfm UI code, src/Extensions/Banshee.Lastfm/Resources/GlobalUI.xml, to merge the love/hate actions in. Also, please follow the style guidelines set out in the HACKING file.
Created attachment 117877 [details] [review] Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar Reworked patch based on comment #8.
A few comments : 1. I think the changes you made in FullscreenWindow.cs are related to bug #549949. 2. The button to leave fullscreen should have the proper icon and text 3. The "Repeat" button is missing 4. The slider should expand to fill the available space 5. The "Leave fullscreen" button should be to the right. I guess the idea is to make it easy to find by putting it in the corner. I'll attach a screenshot, to make sure we're seeing the same things.
Created attachment 117882 [details] Screenshot of the fullscreen controls
Created attachment 117895 [details] [review] Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar (In reply to comment #10) > 1. I think the changes you made in FullscreenWindow.cs are related to bug > #549949. OK. Do you want me to leave them out of the patch, and make this bug depend on 549949? > 2. The button to leave fullscreen should have the proper icon and text Changed the text on toggle - I can't get the icon to change. > 3. The "Repeat" button is missing Fixed. > 4. The slider should expand to fill the available space Fixed. > 5. The "Leave fullscreen" button should be to the right. I guess the idea is to > make it easy to find by putting it in the corner. Done.
(In reply to comment #12) > Created an attachment (id=117895) [edit] > Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar > > (In reply to comment #10) > > 1. I think the changes you made in FullscreenWindow.cs are related to bug > > #549949. > > OK. Do you want me to leave them out of the patch, and make this bug depend on > 549949? Yes, the dependency is not really necessary, as your patch works without the fix. Banshee just crashes a lot while testing it ;) A couple of more issues, that were there in the previous version but I didn't see before (sorry !) : - The toolbar can be a local variable in FullscreenControls.BuildInterface(), like the HBox was. - Why is there a reference to "RefreshSmartPlaylist" in core-ui-actions-layout.xml ? As for the icon issue, somebody who knows more about GTK than me might help.
> > (In reply to comment #10) > > > 1. I think the changes you made in FullscreenWindow.cs are related to bug > > > #549949. > > > > OK. Do you want me to leave them out of the patch, and make this bug depend on > > 549949? > > Yes, the dependency is not really necessary, as your patch works without the > fix. Banshee just crashes a lot while testing it ;) OK, I'll omit those changes from the final patch. > - The toolbar can be a local variable in FullscreenControls.BuildInterface(), > like the HBox was. OK. > - Why is there a reference to "RefreshSmartPlaylist" in > core-ui-actions-layout.xml ? Because I cloned that section from the HeaderToolbar section. ;-) Does it need to be removed? > As for the icon issue, somebody who knows more about GTK than me might help. Honestly, I don't see it as that big a deal. The text changes, and the button is shown as toggled.
(In reply to comment #14) > > - Why is there a reference to "RefreshSmartPlaylist" in > > core-ui-actions-layout.xml ? > > Because I cloned that section from the HeaderToolbar section. ;-) > Does it need to be removed? Yes, let's keep things clean ! ;) > > As for the icon issue, somebody who knows more about GTK than me might help. > > Honestly, I don't see it as that big a deal. The text changes, and the button > is shown as toggled. It's not big, but it's a regression from what we have now. So having the right icon would be nice. Sorry I don't have an idea on how to do it...
Created attachment 118419 [details] [review] Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar
My comments have been addressed. Looks good to me, thanks !
I get these critical warnings with your patch applied: Domain: 'Gtk' Level: Critical Message: gtk_toolbar_get_item_index: assertion `GTK_WIDGET (item)->parent == GTK_WIDGET (toolbar)' failed Trace follows: at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message) at Gtk.Toolbar.gtk_toolbar_get_item_index(IntPtr , IntPtr ) at Gtk.Toolbar.GetItemIndex(Gtk.ToolItem item) at Banshee.Gui.InterfaceActionService.PopulateToolbarPlaceholder(Gtk.Toolbar toolbar, System.String path, Gtk.Widget item, Boolean expand) at Banshee.Gui.InterfaceActionService.PopulateToolbarPlaceholder(Gtk.Toolbar toolbar, System.String path, Gtk.Widget item) at Banshee.NowPlaying.FullscreenControls.BuildInterface() at Banshee.NowPlaying.FullscreenControls..ctor(Gtk.Window toplevel, Banshee.Gui.InterfaceActionService actionService) at Banshee.NowPlaying.FullscreenWindow.ShowControls() at Banshee.NowPlaying.FullscreenWindow.OnMotionNotifyEvent(Gdk.EventMotion evnt) at Gtk.Widget.motionnotifyevent_cb(IntPtr widget, IntPtr evnt) at Gtk.Application.gtk_main() at Gtk.Application.Run() at Banshee.Gui.GtkBaseClient.Run() at Banshee.Gui.GtkBaseClient.Startup() at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup) at Banshee.Gui.GtkBaseClient.Startup() at Banshee.Gui.GtkBaseClient.Startup(System.String[] args) at Nereid.Client.Main(System.String[] args) Domain: 'Gtk' Level: Critical Message: gtk_toolbar_remove: assertion `content_to_remove != NULL' failed Trace follows: at GLib.Log.PrintTraceLogFunction(System.String domain, LogLevelFlags level, System.String message) at Gtk.Container.gtk_container_remove(IntPtr , IntPtr ) at Gtk.Container.Remove(Gtk.Widget widget) at Banshee.Gui.InterfaceActionService.PopulateToolbarPlaceholder(Gtk.Toolbar toolbar, System.String path, Gtk.Widget item, Boolean expand) at Banshee.Gui.InterfaceActionService.PopulateToolbarPlaceholder(Gtk.Toolbar toolbar, System.String path, Gtk.Widget item) at Banshee.NowPlaying.FullscreenControls.BuildInterface() at Banshee.NowPlaying.FullscreenControls..ctor(Gtk.Window toplevel, Banshee.Gui.InterfaceActionService actionService) at Banshee.NowPlaying.FullscreenWindow.ShowControls() at Banshee.NowPlaying.FullscreenWindow.OnMotionNotifyEvent(Gdk.EventMotion evnt) at Gtk.Widget.motionnotifyevent_cb(IntPtr widget, IntPtr evnt) at Gtk.Application.gtk_main() at Gtk.Application.Run() at Banshee.Gui.GtkBaseClient.Run() at Banshee.Gui.GtkBaseClient.Startup() at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup) at Banshee.Gui.GtkBaseClient.Startup() at Banshee.Gui.GtkBaseClient.Startup(System.String[] args) at Nereid.Client.Main(System.String[] args) It also doesn't replicate the current toolbar look well, and it doesn't seem to actually work - the love/ban buttons don't show up for me.
Created attachment 122287 [details] Screenshots of fullscreen toolbar w/ and w/o the patch As you can see, this needs a lot more work.
(In reply to comment #18) > I get these critical warnings with your patch applied: > > [...] > > It also doesn't replicate the current toolbar look well, and it doesn't seem to > actually work - the love/ban buttons don't show up for me. Yeah, there have been a lot of codebase changes in the last two months, and the patch appears to have bit-rotted. I'll see if I can find some time to bring it up to date.
Created attachment 122740 [details] [review] Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar Addresses comment #18.
I get the following error when compiling with the patch : Compiling Banshee.NowPlaying.dll... ./Banshee.NowPlaying/FullscreenWindow.cs(213,22): error CS0169: The private method `Banshee.NowPlaying.FullscreenWindow.DestroyControls()' is never used Compilation failed: 1 error(s), 0 warnings I'm using mono 2.0.1.
Bulk changing the assignee to banshee-maint@gnome.bugs to make it easier for people to get updated on all banshee bugs by following that address. It's usually quite apparent who is working on a given bug by the comments and/or patches attached.
I'd like to bump this feature request with an idea on how to make it work. I'd love to have a Love/Ban button, because I'm terrible at remembering names of songs (I listen to a lot of songs with obscure titles), and being able to check my recently Loved songs on Last.fm would make my listening experience much better. The main issue mentioned here is, what do you do if the song isn't actually on last.fm, my suggestion is that you still show the Love/Ban buttons, and use a local cache. If the song is known on last.fm, you can have it detect the Love/Ban status from your last.fm account, and when you click love or ban, it will love or ban the song on your account. When a song is banned, it should also be skipped in the library, unless played specifically by double clicking said song. If the song is unknown on last.fm, since it can't detect from last.fm, it should use a local cache to save whether or not you Loved or Banned it. That way, even though the song isn't on last.fm, you can still use the functionality of skipping banned songs, and loving your favorite songs to make them easier to find.
Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived. Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Please feel free to reopen this ticket (or rather transfer the project to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the responsibility for active development again. See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.