GNOME Bugzilla – Bug 428849
clickable Album and Artist next to play controls
Last modified: 2020-03-17 08:35:15 UTC
I would really appreciate if the album and artist labels right of the play controls were clickable (on mouse over, too much color is bad) such that the text is copied to the search box in order to speed up viewing all tracks of a certain artist or on a certain album.
Created attachment 86214 [details] Mockup of the feature request.
Created attachment 86328 [details] [review] Patch making artist and album clickable Well, was not too easy. But works like a charm. Hope you like it. Couldn't live without it.
Thanks for your patch; it's certainly a nice feature. It doesn't work in SVN anymore though. (I'm sorry it wasn't reviewed sooner.) I tried porting it to old-code/Core/Banshee.Base/TrackInfoHeader.cs (where it is in the current codebase), but couldn't; I'm not much of a hacker. :( Would you be able to port it to SVN? If not, we could request some help on the mailing list.
*** Bug 543485 has been marked as a duplicate of this bug. ***
(Comment #0 on bug #543485) > Query "album==my awesome album name" is placed in the Search field so that the > library shows tracks from the album of the playing song. > > Awesome bonus feature: > > Right-click menu on album text with options like: > -Start playing album from first track > -Prepend play queue with the rest of the album after this track > -Show all tracks from this album > > Awesome bonus feature would sugged that a link button instead of a regular > button would be the best UI for this.
Thanks for finding the dupe...I looked hard for it but failed. :-) This will be a little harder to implement now, because the text is drawn with Cairo, does fancy fading etc. Still totally doable, but you can't just drop in some LinkButtons now. Hopefully I'll have a chance to do it, but don't count on it unless you see a patch. The class you're looking for is ClassicTrackInfoDisplay, btw, if anyone else wants to have a go at it. Marking old patch as obsolete, as that approach will unfortunately not work now.
Created attachment 115323 [details] [review] Initial work, provides basic functionality I started working on this a bit. I'm attaching this no-good patch in case I get distracted by some other shiny thing and somebody else wants something to start from. Provides: -Click on artist/album area of TrackInfoDisplay to perform an artist=="TrackInfo.ArtistName" search. -Mouse arrow turns to hand when hovering over that area. Still to do: -Style/format fixes...I was lazy and have MD set up with my own tab/space settings. -Differentiate between artist and album (will require changes to how the text is laid out). -Style text when hovering over that area (I personally like the way it looks in the mockup). -(Maybe) animation when hovering in/out and when clicking. There may also be regressions related to cover art popup behavior; I haven't really tested much yet.
I'm a bit further on this, but am experiencing somewhat random (but frequent) crashes when cursor modification is enabled. I have the text styled so that the artist and album look like hyperlinks, and clicking on artist does an artist== search, while clicking on album does an album== search. No animation yet. :-) I need to clean up some mess in it and figure out this crash, then I'll upload the updated patch.
Created attachment 116198 [details] [review] Updated patch, differentiates between artist/album and does some hyperlink styling Nevermind, I'm just attaching my crashy patch because I don't have any more time tonight. Will revisit later. Fairly certain that commenting out the contents of the new SetMouse method makes the crashing go away.
Nice work, Sandy! It seems to crash only after using it for a little while (at least, I can't reproduce it predictably). For what it's worth, it seems to crash more when clicking on albums with parentheses in their names, but I could be wrong. Either way, awesome stuff.
Thanks for trying it out, Andrew. If you have any suggestions I'd be happy to hear them, especially on these topics: 1. Should there be any difference between the appearance of album/artist information when the mouse is hovering over them vs when it is not? Right now it's hyperlink all the time...maybe it should look normal until you hover? 2a. If I'm playing a track from the music library, and I switch to a different source, then click on the artist, should I search the currently viewed source? Should I switch back to to the music library source and do the search there? 2b. If I'm playing a track from another source, and I click on the artist, should the search occur in the currently viewed source or always in the music library source? Perhaps this is source-dependent (maybe searching current source makes sense for podcasts but not for last.fm, etc). 2c. Should clickability even work when the music library is not the current source?
For a while now I've been slowly stewing the idea of first class album/artist/track (and other) objects that you can pull up contextual information for. Could be used to jump to various websites (ala the Last.fm source), pull concert info, get recommendations, rate/love/ban, etc etc. I'd love to see this be flexible enough to use such a system - eg if you right click to show a menu w/ the various options.
(In reply to comment #12) > love to see this be flexible enough to use such a system - eg if you right > click to show a menu w/ the various options. Totally doable; pretty sweet idea. Maybe I'll work on it this weekend. :-)
(In reply to comment #11) > 1. Should there be any difference between the appearance of album/artist > information when the mouse is hovering over them vs when it is not? Right now > it's hyperlink all the time...maybe it should look normal until you hover? I think it's more discoverable now; you know it's a link. It doesn't look quite as nice, but I'll take function over form. IANAD, yada yada. :) > 2a. If I'm playing a track from the music library, and I switch to a different > source, then click on the artist, should I search the currently viewed source? > Should I switch back to to the music library source and do the search there? Good question. I'd think the current source would be best, then you could click on another source and click it again if desired. That'd be the least surprising to me, anyway. > 2b. If I'm playing a track from another source, and I click on the artist, > should the search occur in the currently viewed source or always in the music > library source? Perhaps this is source-dependent (maybe searching current > source makes sense for podcasts but not for last.fm, etc). Same thing here, I think the current source would be the least surprising action. Last.fm's a good point though: what to do with an artist/album that doesn't exist in any source (other than Last.fm)? > 2c. Should clickability even work when the music library is not the current > source? Yeah, definitely. "I want to see what Abock and the Banshees I have in my 'Rock out with my pet rock out' playlist." And stuff.
Oh, before I forget...drag+drop to playlists and play queue would be nice, too.
(In reply to comment #10) > For what it's worth, it seems to > crash more when clicking on albums with parentheses in their names, but I could > be wrong. That's interesting. I just filed bug #547078, which is related to issues making queries with parentheses in the album name.
Unintended consequence of patch: text in notification area icon hover thingy is linkified. Of course, you can't click on it because when you move your mouse it goes away. :-P
Created attachment 116626 [details] [review] Quickie patch update to build with new LargeTrackInfoDisplay This is a really really ugly quick update to the patch so that it will build with the new LargeTrackInfoDisplay (cover art and track info in Now Playing). Makes bad assumptions about track info and probably has issues/crashes/etc with certain types of tracks. Will fix later.
Created attachment 117096 [details] [review] Updated patch. Removes build error, fixes height of by/from text.
Created attachment 117328 [details] [review] Fix crashes from setting mouse cursor Issues with LargeTrackInfoDisplay still hold, but I've fixed the crashes resulting from excessively dicking with the cursor.
Sandy, the indentation in your patch is done with tabs ! I guess somebody messed up your text editor's settings ;)
(In reply to comment #21) > Sandy, the indentation in your patch is done with tabs ! > I guess somebody messed up your text editor's settings ;) Oh, I forgot to set "needs-work", sorry about that. Yes, I violate as many rules of HACKING as possible. Promise I will fix when I consider the patch ready for real review. I'm sorry if this makes things hard in the meantime, but messing with text editor settings in MD gets in the way of my real work right now. On the bright side, Michael Hutchinson said that he'll be implementing per-project (or per-solution?) editor settings soonish in MonoDevelop. So to be clear, here's my MUST DO list: * Fix patch to comply with HACKING * Make sure everything is kosher in LargeTrackInfoDisplay * Probably fix the way ClassicTrackInfoDisplay appears in the notification area hover window, since the links are unreachable in that scenario. NICE TO HAVE list (can wait until post-1.4 imho): * Drag-and-drop * Sweet contextual menu
OK, no problem, I just wanted to make sure you were aware of that. > So to be clear, here's my MUST DO list: > * Fix patch to comply with HACKING > * Make sure everything is kosher in LargeTrackInfoDisplay > * Probably fix the way ClassicTrackInfoDisplay appears in the notification > area hover window, since the links are unreachable in that scenario. Depending on how you want to do it, this last item could have the same solution as bug #536534 : allow the user to interact with the content of the notification hover window.
(In reply to comment #13) > (In reply to comment #12) > > love to see this be flexible enough to use such a system - eg if you right > > click to show a menu w/ the various options. > > Totally doable; pretty sweet idea. Maybe I'll work on it this weekend. :-) > I've been experimenting with this. I've added a BansheeActionGroup to the ClassicTrackInfoDisplay for managing context menu items. As a proof of concept, I updated PlayQueueActions to add items to this menu (they don't do anything yet, though). Is this the right approach? I don't think it makes sense to have the same menu as when you right-click a track in the source view. I've still got to figure out how to query the source for the right tracks to add to the play queue.
Created attachment 118860 [details] [review] Updated patch, adds contextual menu The only update in this patch is the addition of a contextual menu to the ClassicTrackInfoDisplay, with items to add to play queue and to browse by artist/album (same as left-click). -Play queue items don't do anything -Still don't comply with HACKING -Should probably make album/artist text blue only when hovering
Created attachment 118881 [details] [review] Play queue menu items work Implemented "add to play queue" menu items.
Created attachment 118895 [details] [review] Fixed some bugs when changing sources
Created attachment 119035 [details] [review] Comply with HACKING, handle bug #552820 Okay, this is basically ready for review. But I think Aaron wanted the clickable stuff to only be blue on hover, so I'll do that when I get a chance. Please note that this patch *will* break translations.
Created attachment 119036 [details] [review] Added ChangeLog entry
I don't have time today to learn the gtk/cario fu necessary to make the clickable stuff only blue on hover. The current patch is the best I have to offer before the next 1.3.x release. Setting patch status to None so it can be properly reviewed.
Oh, and this patch results in some exceptions and crashes in the new Muinshee client when trying to interact with the clickable stuff. As I don't really "get" the Muinshee UI, I'll wait for guidance on the desired behavior (or should it be completely disabled in MuinsheeTrackInfoDisplay?).
Created attachment 119157 [details] [review] Muinshee and notification hover area fixes Now the Nereid PlayerInterface is responsible for adding its own actions to the new context menu. Also, text will only appear/behave like a hyperlink if there is a handler for the selection event. End result: * No hyperlink appearance/behavior in notification area hover thingy or Muinshee. * The only context menu item available in Muinshee is "Add to Play Queue", which seems applicable still. * No more crashes. ;-)
Created attachment 119161 [details] [review] Typo fix in Nereid PlayerInterface ActionGroup name
Created attachment 119244 [details] [review] Use string.format instead of concats and stringbuilder.append
Created attachment 119245 [details] [review] Fix minor spacing bug
Created attachment 119246 [details] [review] Fix from Brad Taylor to remove conflict marker Many thanks to Brad Taylor for catching this completely non-trivial error in my patch. I hope everybody made a backup before of their entire system before they applied the previous patch. Please use the BRAD VERSION of my patch from now on.
Created attachment 119265 [details] [review] Get link color from theme Now the link color comes from the gtkrc. Includes a fix to GtkUtilities to try/catch on StyleGetProperty, and return the default value in case of an exception. This happens when the property is not set in the gtkrc, which seems to be a common case.
Created attachment 119627 [details] [review] Typo fix in Album context menu
You install some actions with "f" as keybinding, but we already use "f" for fullscreen. Looks like you just forgot to remove the "f" from the two actions.
(In reply to comment #39) > You install some actions with "f" as keybinding, but we already use "f" for > fullscreen. Looks like you just forgot to remove the "f" from the two actions. Whoops! I suppose all of the actions I've added should have no accelerator key associated with them? Also, should I prefix "_" to appropriate letters of the context menu items?
How does this feature relate to the ability to select the song/artist/album (see bug 549548 for more information)?
I love the patch, running it on svn at the moment. I wish the hypertext was kept blue (international signal that this is clickable), but at the moment it appears much bigger than the "by" and "from" which are smaller and grey. Si, I think it would look better if the font was exactly the same size and type but blue as it is now. Also, it would be very nice if one could click to search (as it is now) but click again to remove the search. Thanks again for your work
(In reply to comment #41) > How does this feature relate to the ability to select the song/artist/album > (see bug 549548 for more information)? This feature could be expanded to add a "Copy" action to the contextual menu. I'm not sure how I feel about that. As is, by clicking the text and dumping a value in the search box like "artist=="Nirvana"", some more selectable text is created. After this patch gets in, adding context menu items will be very easy (I hope there will be more, like "search on last.fm", etc), and if "Copy" doesn't become a standard one it could be added with simple boo script. (In reply to comment #42) > I love the patch, running it on svn at the moment. I wish the hypertext was > kept blue (international signal that this is clickable), but at the moment it > appears much bigger than the "by" and "from" which are smaller and grey. Si, I > think it would look better if the font was exactly the same size and type but > blue as it is now. I'm really not sure I understand. You want "by" and "from" to also be clickable? They currently do not appear hyperlinked because the artist and the album text are separate links that perform separate actions. > Also, it would be very nice if one could click to search (as it is now) but > click again to remove the search. Oh I really like that idea!
The links are indeed quite ugly. (How do they contrast with dark themes, btw?) What about this UI: +-----+ |cover| [The song title] |image| |here | [by Artist] [from Album] +-----+ The buttons would have their relief style such that they only show their borders when hovering over them, i.e. the names appear as regular labels when not used. The words "by" and "from" would be a bit smaller and less contrasting, just like they are right now (without using the patch). When the buttons are clicked, a dropdown menu appears, very much like the "Next / Random" duo buttons Banshee already has. Perhaps the small down arrow on the button could be shown only on hover as well to avoid visual clutter.
(In reply to comment #44) > The buttons would have their relief style such that they only show their > borders when hovering over them, i.e. the names appear as regular labels when > not used. I like this idea a lot. Seems like it would balance the appearance preferences (people thinking links are bad/ugly, links aren't generally used for action items elsewhere in the UI) with the functionality. > Perhaps the small down arrow on the > button could be shown only on hover as well to avoid visual clutter. If we do buttons, I think we'd need something like this to show that these are clickable.
(In reply to comment #44) > The links are indeed quite ugly. (How do they contrast with dark themes, btw?) The link color is taken from the theme. Most themes leave it blue. > The buttons would have their relief style such that they only show their > borders when hovering over them, i.e. the names appear as regular labels when > not used. The words "by" and "from" would be a bit smaller and less > contrasting, just like they are right now (without using the patch). When the > buttons are clicked, a dropdown menu appears, very much like the "Next / > Random" duo buttons Banshee already has. Perhaps the small down arrow on the > button could be shown only on hover as well to avoid visual clutter. Neat idea, but hard to do (for me) because this is all Cairo drawing stuff.
Created attachment 119737 [details] [review] Update for latest svn
Created attachment 119817 [details] [review] Remove accelerators/keybindings, unmap temporary smart playlists, line length fixes Change to be made (probably by abock): * Fixes to translatable by/from strings (I made them too inflexible with my changes). Probably a nice little markup language. * Color/style/hover/something fixes to the album/artist text in the ClassicTrackInfoDisplay * Drag-and-drop
Created attachment 119818 [details] [review] Only add play queue actions if playing source is a library source
Created attachment 119820 [details] [review] Better behavior based on track's primary source Behavior fixes: * Undo last change. * Update play queue actions when track changes (not just when source changes). * If playing track is a DatabaseTrackInfo, use its PrimarySource when processing "add to play queue" actions. Irritating: * "add to play queue" actions aren't available on DAAP sources because those don't support playlists, and those actions work by creating temporary smart playlists and adding them to the queue.
Removing the check for primary_source.SupportsPlaylists in PlayQueueActions.UpdateActions makes DAAP work fine (as far as I can tell). I haven't really tested the repercussions of this action, though. The action ends up appearing for podcasts but not doing anything, and I don't have an iPod or DAP available to test the behavior there. Not updating the patch for it, yet.
Hi, thanks for the your hard work on this patch. However, I can't seem to be able to apply the patch on latest svn: patching file src/Extensions/Banshee.PlayQueue/Resources/GlobalUI.xml patching file src/Extensions/Banshee.PlayQueue/Banshee.PlayQueue.csproj patching file src/Extensions/Banshee.PlayQueue/Banshee.PlayQueue/PlayQueueActions.cs patching file src/Clients/Nereid/Nereid/PlayerInterface.cs patching file src/Core/Banshee.ThickClient/Banshee.Gui.Widgets/LargeTrackInfoDisplay.cs Hunk #1 succeeded at 186 with fuzz 2 (offset 23 lines). patching file src/Core/Banshee.ThickClient/Banshee.Gui.Widgets/TrackInfoDisplay.cs Hunk #1 succeeded at 425 with fuzz 2 (offset 11 lines). patching file src/Core/Banshee.ThickClient/Banshee.Gui.Widgets/ClassicTrackInfoDisplay.cs Hunk #2 FAILED at 48. Hunk #3 succeeded at 73 (offset 2 lines). Hunk #4 succeeded at 98 (offset 2 lines). Hunk #5 succeeded at 120 (offset 2 lines). Hunk #6 FAILED at 178. Hunk #7 FAILED at 250. Hunk #8 succeeded at 315 (offset 17 lines). Hunk #9 succeeded at 415 (offset 17 lines). 3 out of 9 hunks FAILED -- saving rejects to file src/Core/Banshee.ThickClient/Banshee.Gui.Widgets/ClassicTrackInfoDisplay.cs.rej patching file src/Libraries/Hyena.Gui/Hyena.Gui/GtkUtilities.cs I'm attaching the reject file just in case Also, there's been a call for reviewing the patches for inclusion, should the author push for this patch to be reviewed as well?
Created attachment 120558 [details] rejects for the latest patch
(In reply to comment #52) > Also, there's been a call for reviewing the patches for inclusion, should the > author push for this patch to be reviewed as well? No, it has been determined that this patch is too disruptive for Banshee 1.4. The patch has been reviewed by Aaron and the only work left to be done is to fix translatable strings (non-trivial) and to make final decisions about appearance and behavior. I'll update it to work with latest SVN shortly.
Created attachment 120560 [details] [review] Updated to build against SVN, but undoes some perf improvements Here's an updated patch that should work against latest SVN, but it undoes some of Aaron's Pango.Layout performance work from bug #555365, since I create a variable number of layouts on the second line instead of just one. It could be done more efficiently, but I just wanted to have an updated patch here for folks to work with. I wasn't planning on revisiting this patch until 1.4 is released, so feel free to update yourself! ;-)
*** Bug 560853 has been marked as a duplicate of this bug. ***
Created attachment 129022 [details] [review] Updated patch to clear search when clicked again, follow FDG Changes: * Implements suggestion from comment #42 (click once to populate search, click again to clear it) * Uses EventHandler<T> delegate instead of custom delegate for ArtistSelected/AlbumSelected events. Please let me know if TrackSelectedEventArgs should be in its own file, or somewhere else entirely. Problems (just to recap): * Undoes some of abock's performance work with pango layouts. * Needs i18n/l10n love.
Created attachment 129028 [details] [review] Restore pango layout optimizations Following the idea of abock's pango layout optimizations, this patch caches the list of second_line_layouts, only disposing/recreating when absolutely necessary. This could use some review, I'm sure. Plus I haven't done the i18n stuff yet.
I played around a bit with this, and I couldn't find any bad behavior. Nice job !
Sorry for all the nitpicks, but: We use String.Empty instead of string.Empty. We only use "string" for variable declarations. Please put {} around all control statements, too. Is there any advantage to using TrackSelectedEventArgs instead of Action<TrackInfo>? Not sure if it's worth it, but if you put the actions in a subclass of BansheeActionGroup, there is a protected method called ShowContextMenu. Some ideas that maybe are too much for now (brainstorming): Should probably abstract out the logic for calculating the extents for a particular property and for determining if a point is in such so it's not repeated once per property. It would be cool if this wasn't specific to artist/albums really at all. There could be a GUI-aware object for each target type that is aware of the extents, and maybe that has a Gtk.Action to trigger for its popup menu.
(In reply to comment #60) > Sorry for all the nitpicks, but: > > We use String.Empty instead of string.Empty. We only use "string" for variable > declarations. Please put {} around all control statements, too. Will do. > Is there any advantage to using TrackSelectedEventArgs instead of > Action<TrackInfo>? Well, you can add additional stuff to event args in the future without changing API. But I don't really care either way. Will update to use Action<TrackInfo> if that is available in the versions of Mono you support and if you prefer that. It is new to me, must have missed it in my FDG. :-) > Not sure if it's worth it, but if you put the actions in a subclass of > BansheeActionGroup, there is a protected method called ShowContextMenu. Will look into this. > Some ideas that maybe are too much for now (brainstorming): > > Should probably abstract out the logic for calculating the extents for a > particular property and for determining if a point is in such so it's not > repeated once per property. It would be cool if this wasn't specific to > artist/albums really at all. Yeah, that's a good point. > There could be a GUI-aware object for each target type that is aware of the > extents, and maybe that has a Gtk.Action to trigger for its popup menu. Neat, but probably out of scope for me. :-) Will hopefully have a new patch up soon.
*** Bug 582269 has been marked as a duplicate of this bug. ***
Created attachment 136042 [details] [review] Update for latest git Update diff to prevent conflicts caused by fixes to bug #560886 and bug #580237 (neither of which affect my patch). I'll put together a proper patch with a commit message and all that jazz soonish.
Created attachment 136043 [details] [review] [TrackInfoDisplay] Make artist and album text interactive (BGO#428849) In ClassicTrackInfoDisplay, left-click to browse, right-click for more options. TrackInfoDisplay.GetSecondLineText returns a list of phrases instead of a line of text, enabling derived classes to change display/interaction characteristics based on a phrase's properties. Added new actions for adding artist/album to playqueue, which appear in ClassicTrackInfoDisplay's context menu.
I vote too for drag'n'drop to playlists
(In reply to comment #65) > I vote too for drag'n'drop to playlists The current version of the patch does not add any drag'n'drop features. I agree it would be great, but somebody else needs to add that enhancement.
Created attachment 149738 [details] [review] [TrackInfoDisplay] Make artist and album text interactive (BGO#428849) In ClassicTrackInfoDisplay, left-click to browse, right-click for more options. TrackInfoDisplay.GetSecondLineText returns a list of phrases instead of a line of text, enabling derived classes to change display/interaction characteristics based on a phrase's properties. Added new actions for adding artist/album to playqueue, which appear in ClassicTrackInfoDisplay's context menu.
*** Bug 617890 has been marked as a duplicate of this bug. ***
This could be a gnome-love bug or?
(In reply to comment #69) > This could be a gnome-love bug or? I don't think so. I think the remaining work, though not too bad for an experienced GNOME developer, is a bit much for a n00b. It requires a pretty good understanding of the l10n/i18n issues involved.
Created attachment 160976 [details] [review] Latest diff against git master No functional changes, just fixing some merge conflicts.
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.