After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 549524 - Show Love/Ban buttons in the toolbar in fullscreen mode
Show Love/Ban buttons in the toolbar in fullscreen mode
Status: RESOLVED WONTFIX
Product: banshee
Classification: Other
Component: Last.fm
git master
Other All
: Normal enhancement
: 1.x
Assigned To: Banshee Maintainers
Banshee Maintainers
gnome[unmaintained]
Depends on:
Blocks:
 
 
Reported: 2008-08-27 00:39 UTC by Levi Bard
Modified: 2020-03-17 08:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Windowed (232.57 KB, image/png)
2008-08-27 23:16 UTC, Levi Bard
  Details
Fullscreen (177.41 KB, image/png)
2008-08-27 23:16 UTC, Levi Bard
  Details
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar (2.44 KB, patch)
2008-08-30 01:50 UTC, Levi Bard
none Details | Review
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar (2.53 KB, patch)
2008-08-30 02:13 UTC, Levi Bard
needs-work Details | Review
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar (6.46 KB, patch)
2008-09-02 20:18 UTC, Levi Bard
needs-work Details | Review
Screenshot of the fullscreen controls (9.49 KB, image/png)
2008-09-02 21:54 UTC, Bertrand Lorentz
  Details
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar (7.25 KB, patch)
2008-09-03 02:00 UTC, Levi Bard
needs-work Details | Review
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar (6.12 KB, patch)
2008-09-10 12:11 UTC, Levi Bard
needs-work Details | Review
Screenshots of fullscreen toolbar w/ and w/o the patch (63.17 KB, image/png)
2008-11-09 21:36 UTC, Gabriel Burt
  Details
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar (6.75 KB, patch)
2008-11-15 17:43 UTC, Levi Bard
needs-work Details | Review

Description Levi Bard 2008-08-27 00:39:06 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?)
Comment 1 Bertrand Lorentz 2008-08-27 18:14:26 UTC
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.
Comment 2 Levi Bard 2008-08-27 23:15:43 UTC
Yes, the "Now Playing" fullscreen mode.  Attaching screenshots for further clarification.
Comment 3 Levi Bard 2008-08-27 23:16:22 UTC
Created attachment 117489 [details]
Windowed
Comment 4 Levi Bard 2008-08-27 23:16:47 UTC
Created attachment 117490 [details]
Fullscreen
Comment 5 Levi Bard 2008-08-30 01:50:02 UTC
Created attachment 117615 [details] [review]
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar
Comment 6 Levi Bard 2008-08-30 02:13:02 UTC
Created attachment 117616 [details] [review]
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar

Meh, tabs
Comment 7 Bertrand Lorentz 2008-08-30 13:47:23 UTC
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 ?
Comment 8 Gabriel Burt 2008-09-02 16:33:03 UTC
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.
Comment 9 Levi Bard 2008-09-02 20:18:47 UTC
Created attachment 117877 [details] [review]
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar

Reworked patch based on comment #8.
Comment 10 Bertrand Lorentz 2008-09-02 21:53:37 UTC
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.
Comment 11 Bertrand Lorentz 2008-09-02 21:54:49 UTC
Created attachment 117882 [details]
Screenshot of the fullscreen controls
Comment 12 Levi Bard 2008-09-03 02:00:55 UTC
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.
Comment 13 Bertrand Lorentz 2008-09-03 20:31:48 UTC
(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.
Comment 14 Levi Bard 2008-09-04 12:55:03 UTC
> > (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.

Comment 15 Bertrand Lorentz 2008-09-09 18:39:19 UTC
(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...
Comment 16 Levi Bard 2008-09-10 12:11:23 UTC
Created attachment 118419 [details] [review]
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar
Comment 17 Bertrand Lorentz 2008-09-14 19:31:46 UTC
My comments have been addressed.
Looks good to me, thanks !
Comment 18 Gabriel Burt 2008-11-09 21:34:52 UTC
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.
Comment 19 Gabriel Burt 2008-11-09 21:36:36 UTC
Created attachment 122287 [details]
Screenshots of fullscreen toolbar w/ and w/o the patch

As you can see, this needs a lot more work.
Comment 20 Levi Bard 2008-11-10 16:16:23 UTC
(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.
Comment 21 Levi Bard 2008-11-15 17:43:20 UTC
Created attachment 122740 [details] [review]
Patch to propagate main toolbar widgets to fullscreen "Now Playing" toolbar

Addresses comment #18.
Comment 22 Bertrand Lorentz 2008-12-07 10:27:15 UTC
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.
Comment 23 Gabriel Burt 2009-10-27 20:18:33 UTC
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.
Comment 24 Daniel Power 2010-08-19 11:55:43 UTC
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.
Comment 25 André Klapper 2020-03-17 08:24:53 UTC
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.