GNOME Bugzilla – Bug 544590
Notification bubble should show more info (rating, at least)
Last modified: 2020-03-17 08:28:41 UTC
The notification bubble should hold more info (I'd like it to be rating and year, but rating could be enough). Of course, being able to change the rating this way would be nice, too. Other information:
Created attachment 129824 [details] [review] show rating on tray icon tooltip Is this what you had in mind? This is my first crack at an enhancement for Banshee, so I'm not sure what I did is in line with how things "should" be done. There isn't much code involved here, so I just put it into the constructor of TrackInfoPopup. I didn't feel it would be appropriate in ClassicTrackInfoDisplay, as this change would then change the definition of a "ClassicTrackInfoDisplay." I tested updating the rating from the library listing, by right-clicking a track, and by right-clicking on the tray icon. Changes are always reflected in the tooltip. This patch could probably be tied in with http://bugzilla.gnome.org/show_bug.cgi?id=544592 (allow seeking on tooltip). If seeking is going to be allowed, then updating the rating should be too. As always, comments to improve upon this are welcome.
Yeah that's quite fine, it would require #544592 to be fixed to be usable, too.
Created attachment 132380 [details] [review] allow rating changes this patch is the whole sha-bang: * add rating entry widget to tooltip * delay hiding tooltip for 10 seconds after mouse pointer moves off tray icon * save rating changes * propagate rating changes from other sources to tooltip
After using this patch for sometime, it actually is kind of annoying. IMO, it needs the following tweaks: - instead of delaying hiding of tooltip for 10 seconds, delay it long enough (maybe 1 sec) so that the user can set the mouse pointer over the tooltip. - when tooltip has focus, keep it up for the duration that it is in focus (have to make sure it updates if track changes!!). - as soon as the tooltip no longer has focus, hide it. If this is still outstanding when I have more time, I'll take a look. Otherwise, anyone, feel free to use improve my patch.
Created attachment 136047 [details] [review] Add RatingEntry widget to TrackInfoPopup New, simpler patch. This excludes Alex's patch to src/Extensions/Banshee.NotificationArea/Banshee.NotificationArea/X11NotificationAreaBox.cs from bug #544592. So, apply that one as well to be able to use this patch as intended. Let me know if anything should be changed.
Thanks, the patch works fine. A couple of things: 1. The patch contains a few tabs here and there. Running git-apply with --whitespace=fix removes them. 2. The rating line looks a bit out of context if not ugly. I think it would look much nicer if the rating widget is incorporated into the ClassicTrackInfoDisplay and is shown in front of the track title. This would also fix bug 526994 (and would be a step forward to fixing bug 574153) Nevertheless, this patch is a major improvement and I wouldn't mind if it's committed as is.
Created attachment 136112 [details] [review] Close, but not quite Alex, you're right. That does look nicer. Now, let me explain what I've done here (bare with me, as I know nothing about GTK/Cairo and what I've done might be stupid :)). I couldn't add the RatingEntry widget to ClassicTrackInfoDisplay, even if the base class, TrackInfoDisplay, is changed to derive from Container instead of Widget. So, I added the RatingRenderer to ClassicTrackInfoDisplay. Still some troubles, though: * Positioning the Cairo.Context is a bit of a hack in the RenderTrackInfo method. Positioned purely by trial-and-error. * See the OnMotionNotifyEvent callback. I am throughly confused by what's happening there. I'm sure it's just due to my lack of experience and it's easy to fix. The rating_area intersection happens about an inch to the right of the TrackInfoDisplay TODO: * finish the rest of the event handling * Changing the rating from ColumnCellRating does not raise PlayerEvent.TrackInfoUpdated. Is it appropriate to add an event to that class? Or, is there a better way to notify ClassicTrackInfoDisplay of a rating change?
(In reply to comment #7) > * Positioning the Cairo.Context is a bit of a hack in the RenderTrackInfo > method. Positioned purely by trial-and-error. You should set the Xpad and Ypad to 0. Also, RatingRenderer.Width returns 6*Size but you want 5*Size, see for example ComputePosition(). This can be worked around by using MaxRating * Size instead of Width. > * See the OnMotionNotifyEvent callback. I am throughly confused by what's > happening there. I'm sure it's just due to my lack of experience and it's easy > to fix. The rating_area intersection happens about an inch to the right of the > TrackInfoDisplay The TrackInfoDisplay sets a NoWindow WidgetFlag. This means it draws on its parent GdkWindow using its relative coordinates. The coordinates received by notify events are relative to the TrackInfoDisplay and have to be translated. For example: int x = Allocation.X + (int)evnt.X; int y = Allocation.Y + (int)evnt.Y; > TODO: > * finish the rest of the event handling > * Changing the rating from ColumnCellRating does not raise > PlayerEvent.TrackInfoUpdated. Is it appropriate to add an event to that class? > Or, is there a better way to notify ClassicTrackInfoDisplay of a rating change? > This patch takes care of it: diff --git a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs index bec4165..6138363 100644 --- a/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs +++ b/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs @@ -153,6 +153,11 @@ namespace Banshee.Collection.Database PrimarySource.NotifyTracksChanged (fields_changed); } } + + if (TrackEqual (ServiceManager.PlayerEngine.CurrentTrack)) { + (ServiceManager.PlayerEngine.CurrentTrack as DatabaseTrackInfo).Refresh (); + ServiceManager.PlayerEngine.TrackInfoUpdated (); + } } protected virtual void ProviderSave ()
Created attachment 136127 [details] [review] Done? Awesome! Thanks for the help, Alex. I learned some things this weekend. :) I think this patch covers everything. Check it out and let me know what you think! It seems to be working well with your patch for keeping the TrackInfoPopup displayed too.
(In reply to comment #9) > Created an attachment (id=136127) [edit] > Done? > > Awesome! Thanks for the help, Alex. I learned some things this weekend. :) > > I think this patch covers everything. Check it out and let me know what you > think! It seems to be working well with your patch for keeping the > TrackInfoPopup displayed too. > Thanks Neil, the patch looks and works very well. I few minor nitpicks: * There are a few git white-space errors. * OnPlayerEvent() override is not really needed. You can remove it and revert TrackInfoDisplay.cs * Because the width of the rating_area is now exactly 5 stars, clearing the rating is rather unintuitive (the active area is about 2 pixels wide). You can fix this by reducing the X-coordinate of the rating_area before intersecting it with the mouse position in OnMotionNotifyEvent(). It's probably a good idea to perform the same test in OnButtonPressEvent(), currently the rating is changed if you click anywhere in the widget. * Is ButtonReleaseMask needed? * You can use the readonly modifier for the rating_renderer. * Why DatabaseTrackInfo.Refresh() call is needed in RenderRating()? * In the same method, if (track != null) is not needed because you have the same check in RenderTrackInfo()
Created attachment 136179 [details] [review] Applied maintainer comments (In reply to comment #10) > > Thanks Neil, the patch looks and works very well. I few minor nitpicks: > > * There are a few git white-space errors. should be clean now > > * OnPlayerEvent() override is not really needed. You can remove it and revert > TrackInfoDisplay.cs gonzo > > * Because the width of the rating_area is now exactly 5 stars, clearing the > rating is rather unintuitive (the active area is about 2 pixels wide). > > You can fix this by reducing the X-coordinate of the rating_area before > intersecting it with the mouse position in OnMotionNotifyEvent(). slightly different approach, but it works: x + 5 on IntersectsWith call > > It's probably a good idea to perform the same test in OnButtonPressEvent(), > currently the rating is changed if you click anywhere in the widget. > yup. Added > * Is ButtonReleaseMask needed? Nope > > * You can use the readonly modifier for the rating_renderer. done > > * Why DatabaseTrackInfo.Refresh() call is needed in RenderRating()? copied from old patch. It's removed. > > * In the same method, if (track != null) is not needed because you have the > same check in RenderTrackInfo() yup. Check is redundant. > Hope that covers everything. Neil
Actually, there is a display glitch when going from track to track. The second line showing the artist and album sometimes shifts a few pixels down, but not always. Not sure what it is yet.
(In reply to comment #11) > > * Because the width of the rating_area is now exactly 5 stars, clearing the > > rating is rather unintuitive (the active area is about 2 pixels wide). > > > > You can fix this by reducing the X-coordinate of the rating_area before > > intersecting it with the mouse position in OnMotionNotifyEvent(). > slightly different approach, but it works: x + 5 on IntersectsWith call With (x+5), the last 5 pixels of the rating_area are inactive. You really need to extend the area, for example: private bool IsPointOverRating (int x, int y) { var rect = new Gdk.Rectangle ( rating_area.X - rating_renderer.Size / 2, rating_area.Y, rating_area.Width + rating_renderer.Size / 2, rating_area.Height); return rect.IntersectsWith (new Gdk.Rectangle (x, y, 1, 1)); } ... then use it in both event handlers. (In reply to comment #12) > Actually, there is a display glitch when going from track to track. The second > line showing the artist and album sometimes shifts a few pixels down, but not > always. Not sure what it is yet. This happens because RatingRenderer.Render() modifies the context object. You can fix it by wrapping the lines from `cr.LineWidth = 1.0` into a catch/finally block, call cr.Save() in the beginning, and cr.Restore in finally. It looks like it's going to be the last iteration, thanks for all the fixes! Cheers, Alex
Created attachment 136181 [details] [review] Take #... lost count :) (In reply to comment #13) > With (x+5), the last 5 pixels of the rating_area are inactive. You really need > to extend the area, for example: > > private bool IsPointOverRating (int x, int y) > { > var rect = new Gdk.Rectangle ( > rating_area.X - rating_renderer.Size / 2, rating_area.Y, > rating_area.Width + rating_renderer.Size / 2, > rating_area.Height); > > return rect.IntersectsWith (new Gdk.Rectangle (x, y, 1, 1)); > } > > ... then use it in both event handlers. > Ahhh..Yes, that's much smarter than what I did. Extending both sides by half a star is definitely the way to go. > (In reply to comment #12) > > Actually, there is a display glitch when going from track to track. The second > > line showing the artist and album sometimes shifts a few pixels down, but not > > always. Not sure what it is yet. > > This happens because RatingRenderer.Render() modifies the context object. You > can fix it by wrapping the lines from `cr.LineWidth = 1.0` into a catch/finally > block, call cr.Save() in the beginning, and cr.Restore in finally. > > It looks like it's going to be the last iteration, thanks for all the fixes! That would have taken me forever to figure out :) Thanks! This patch should do it! Got rid of all the whitespace too. Neil
Looks good!
This is nice, good work guys. I feel like it would be more natural to have it on to the ride of the title. Do you think it's better where it is? Would it be possible to put on the right? (Would probably be harder, since it's position would change song to song etc)
I initially thought the same thing: that it would make more sense on the right. But, then I thought that it might look weird with the rating stars moving around from track to track. Therefore, I didn't even try to put it there to see what it would look like. Also, with the stars on the left side, they are always in one consistent place for the user to change ratings. However, if you'd like for me to try moving it to the right side, I can.
I agree with Neil, I wouldn't want my stars to jump around as songs change. One of the reasons we want them is to make it easier to rate the currently playing track. Placing the stars on a different location would make this harder. If you really want the stars on the right, I think they should be fixed to the far right location. Bug 526994 has a mock up of how this would look like.
*** Bug 602631 has been marked as a duplicate of this bug. ***
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.