GNOME Bugzilla – Bug 646765
Artist Browser is just text and hard to locate an artist easily
Last modified: 2014-07-21 20:58:46 UTC
Created attachment 185160 [details] Some ideas I had to fix this issue This is more of an issue than a bug. The artist browser is currently just plain text, making it harder to find an artist fast/easily. Though this isn't a problem for some, when one has thousands of artists with similar names and is trying to find one artist in particular, text can be tedious or straining on the eyes (not to mention, not very aesthetically appealing). Perhaps adding a more visual or album art oriented artist browser (optional prehaps?) would help, and of course make banshee look a little more appealing to the eye. Due to my inability to code, I attached a picture of some ideas I had in mind, in attempt to help.
I also posted these ideas in the the mailing list: http://banshee-media-player.2283330.n4.nabble.com/Idea-for-improving-Banshee-td3424654.html
The stacking idea is nice and would help to navigate through the artist, imho. My problem with this layout is that you will have a massively smaller scrollbar because the images need more space.
Perhaps that could be resolved by adding an option of choosing large (48 pixels, like in the picture) or small (24 pixels, half the size) album art? When I have some time, I'll make a mock-up of what I mean.
Created attachment 185195 [details] A small image version of the idea
This solution would be suitable, I think.
Created attachment 185277 [details] [review] add optional cover art in two sizes to the artist list view Adds a custom cell renderer that is able to render covers into the artist list view in two different sizes (selectable in Preferences). The normal text based list view can be chosen for better performance from the Preferences.
Created attachment 185278 [details] Artist List with normal image sizes screenshot of above patch with normal image size
Created attachment 185279 [details] Artist List with small image sizes screenshot of above patch in "space saving mode", i.e. small image size and different stacking
I have attached the above patch, but could not (due to limited music in my development machine) test for performance. Would be nice if someone with a large collection could go ahead and do that.
Created attachment 185282 [details] [review] add optional cover art in two sizes to the artist list view small improvement to the last patch: covers are sorted so that empty images go to the back and actual covers come to the front.
Created attachment 185283 [details] [review] add optional cover art in two sizes to the artist list view discovered and fixed a problem with queuedraw when albums were merged
Created attachment 185286 [details] [review] add optional cover art in two sizes to the artist list view another minor improvement in stacking: tighter y-stacking and better x-spacing. Also everything ends up nicely in the selection now.
Created attachment 185287 [details] Artist List with normal image sizes after updated patch, an updated screenshot
Created attachment 185288 [details] Artist List with small image sizes and Preferences after updated patch an updated screenshot
Wow, that was fast. Looks, good. A pity that's not in 2.0 but next release. :) Good, that you display also empty covers, to get the feeling that there are more then one album of this artist.
I have just tested on my machine with about 9000 songs and 1500 artists. It's a Core2 Duo P8400@2.26GHz with 2GB of memory with Ubuntu 10.10. The scrolling is less smooth in the artist browser that without that patch. However it is only noticeable then scrolling fast, for example with the scrollbar, opposite using the scroll wheel. I you want a more scientific measurement of the performance, please tell me how to do that :-)
Wow, it's amazing to see my ideas already implemented in a patch haha :)... but it doesn't seem to want to patch for me. I downloaded Banshee's 2.0 source tarball and ran "patch < /pathtopatch/patch", it attempts to patch but throws me a few errors (like "patching file ArtistListView.cs, Hunk #1 FAILED at 3, Hunk #2 FAILED at 34"). I take I'm doing something wrong?
(In reply to comment #17) > Wow, it's amazing to see my ideas already implemented in a patch haha :)... but > it doesn't seem to want to patch for me. I downloaded Banshee's 2.0 source > tarball and ran "patch < /pathtopatch/patch", it attempts to patch but throws > me a few errors (like "patching file ArtistListView.cs, Hunk #1 FAILED at 3, > Hunk #2 FAILED at 34"). I take I'm doing something wrong? You might get it to work this way, just wanted to let you know the way I got it to work. So first get the latest code from Git, see how here: http://banshee.fm/download/development/ Then patch in the src directory: $ git am /pathtopatch/patch If you don't use Monodevelop you can build from command line: $ ./autogen.sh $ make $ make run (this is assuming you are on Linux or a Unix-like-something-OS) Hope this helps.
Created attachment 185327 [details] Terminal output when Banshee Crashed while viewing unknown artist/album Thanks the patch worked! I found two bugs though: 1.Artist's without album in the "small variant" have 180 rotated icon (the b looks like a q) 2.Banshee crashes when I try to look at the "Unknown Artist" with either small or large variants on, but not with the "Artist Grid" turned off. If it helps, those Unknown Artists all have Unknown Albums. The error.out attachment is what I got in the terminal when it crashed. Tested about 5 times, the problem seems very consistent.
Created attachment 185336 [details] Screenshot of a GUI bug I just found another bug, with the preferences GUI. The options to turn off and on the artist grid, the small and the album grid disappear from the preferences after banshee is open for a while (see attachment). I think it maybe another bug entirely because I think I've seen this happen in 1.8, but obviously it only consisted of the disappearance of the album grid option. I'm not sure though.
The bug with the GUI I just posted in the last comment seems to be hard to reproduce, because it seems to be absolute random. The two times it's happened to me so far, I was playing music and changing the settings, but I still can't reproduce it intentionally. Once it does happens, the only way to make the options reappear is by restarting banshee. I can't tell if its a regression of Banshee 2.0, an already reoccurring bug or caused by this patch; I don't know if I should report another bug report.
Banshee also crashes when I open a smart playlist for me :(
Created attachment 185386 [details] [review] add optional cover art in two sizes to the artist list view fixed above issues: 1. Unknown Artist is now properly displayed 2. Smart Playlists: filtering of how many albums are in the playlist seems very difficult, so the covers are shown, but the Album count is not shown anymore. Same behaviour is for playlists unresolved issues: 1. Preferences Dialog: I use the same code as the album grid for the preferences. Does that option disappear as well? Then it is another bug. 2. unknown album cover upside down: that is an issue with the artwork-manager delivering/rendering the thumbnail, so also another bug.
Created attachment 185387 [details] [review] add optional cover art in two sizes to the artist list view Found and fixed the cause for the Preferences bug: it happens when a source (i tried miro podcast) is removed. It's the same bug for album view for which I will open another bug and attach a separate patch there.
@Jonas: So as I understand, the machine doesn't slow down significantly and it is still a better user experience than before and very well acceptable performance from your subjective point of view? Don't know how to be more scientific as well ;)
Created attachment 185391 [details] [review] add optional cover art in two sizes to the artist list view Just had the idea of how to filter in playlists and smart playlists as well. That adds a bit of code, but just looks nicer to the user.
(In reply to comment #25) > @Jonas: So as I understand, the machine doesn't slow down significantly and it > is still a better user experience than before and very well acceptable > performance from your subjective point of view? Don't know how to be more > scientific as well ;) I would call It acceptable performance, and I like the idea. To give you some pointer about the scrolling speed, it is sightly worse the scrolling in the album browser. I actually have an idea about how to implement the idea in a different way, I have posted it on the mailing list, but post it here as well: I kind of think Banshee have enough album art in different places already. If you have “Show Cover Art” enabled you could potential see the same album cover tree places (the album browser, next to the player progress bar and the “Show Cover Art” widget). With this proposal this could turn into four. How about adding the functionality to the album browser, so it has two modes, the first one being the current one. The second one could be this proposal, where the albums are stacked, and the sorting is “by artist” instead of “by album”. What is basically suggest is that a “sort by” function is added to the album browser, where the album covers are stacked when “sort by artist” is selected. Hope this makes sense, I could make a muck-up if you are interested in that.
How do you select an album then though?
There should be a potion(In reply to comment #28) > How do you select an album then though? Have not quite decided yep, I see two options: When you are in "browse by artist" mode you can't, you get all the tracks by that artist in the track browser. You have to switch to "browse by album" mode to select a specific album. or When you click on the artist the mode switches to “browse by album”, then you can select a specific album. There might be other possibilities, I haven't thought about.
(In reply to comment #29) > There might be other possibilities, I haven't thought about. Sounds like a much bigger and not yet completely thought through change. Also that includes a major change in how you'd use Banhee and affects artist as well as album view. If you have a solution well thought through you should open another bug for it.
(In reply to comment #30) > (In reply to comment #29) > > There might be other possibilities, I haven't thought about. > > Sounds like a much bigger and not yet completely thought through change. Also > that includes a major change in how you'd use Banhee and affects artist as well > as album view. If you have a solution well thought through you should open > another bug for it. Yep, this could turn into a big change. This bug just triggerd it I guess, and a saw some similarities. I will think a bit more about it and might come up with a mock-up, and open a bug if I like to share it. Thx :-)
Created attachment 185418 [details] [review] add optional cover art in two sizes to the artist list view just realized I uploaded an old file the last time. this is the version with playlists correctly working and displaying album counts
Works beautifully! No bugs to be seen. Thanks a lot for your help! The only other suggestion I would make is to make the albums stack up to the right rather than down to the right. I'm not sure, but it seems more appealing for some reason. Feel to shoot down my idea if you don't like it though, I just wanted to throw it out there. I modified two lines in ColumnCellArtist.cs to make it look like what I mean. I'm not the greatest at programming, so it took me a few hours of trial and error, but it works to accomplish what I'm purposing: About line 270: int y_offset = (images.Count > 1) ? 0 : y_image_spacing; Changed to: int y_offset = (images.Count > 1) ? 2 : 2 - y_image_spacing; A few lines down: int move_y = y + ((3 - i) * y_image_spacing); Changed to: int move_y = y + ((i - 1) * y_image_spacing); Let me know what you think, it's a subtle change but it seems more natural and aesthetically appealing somehow.
Created attachment 185440 [details] [review] add optional cover art in two sizes to the artist list view It wasn't as appealing to me until I also reversed the left-right order. It makes more sense as it gives the impression the stack front is looking at you with the artist view being on the left side. otherwise the covers were sort of looking out of the "side window"...
Yeah I definitely agree, this much better than the other two. I hope this gets into banshee soon, it looks fantastic. Thanks again for your help!
I think I'd prefer the prefs/options go only in the View menu, not in Preferences. (I'd be in favor of moving the 'sort by year' option there too).
Created attachment 185484 [details] [review] add optional cover art in two sizes to the artist list view Moved Disable and SmallImage options from Preferences to View Menu. Actually cleans up the code a bit.
The counterpart for album grid and sortbyyear is here: https://bugzilla.gnome.org/show_bug.cgi?id=647129
Review of attachment 185484 [details] [review]: Works well, i see no formatting errors
What speak against using this patch?
Created attachment 200952 [details] [review] add optional cover art in two sizes to the artist list view rebase against current git master and remove of specific package references in csproj-file
*** Bug 666543 has been marked as a duplicate of this bug. ***
Suggestion from the duplicate: Some artists have a logo that identifies them (Rammstein, U2, etc.), so it would be interesting to be able to set it and display it.
How do I find the logo? It'd be just a minor code change.
I do not find any webservice on last.fm to get band logo. Same for amazon. I do nt know for rhapsody but it will be difficult (I guess) to get the band logo. I am not sure that we have a such api on internet. Last.fm as images but no logo bank. Anyway, why not get this patch in and improve it later if a such logo bank exist somewhere...
(In reply to comment #44) > How do I find the logo? It'd be just a minor code change. As Olivier says, it is not easy, but I had an idea: This MusicBrainz API returns an artist ID: http://musicbrainz.org/ws/2/artist?limit=1&query=michael%20jackson Which is: f27ec8db-af05-4f36-916e-3d57f91ecf5e With that we can query this page: http://musicbrainz.org/artist/f27ec8db-af05-4f36-916e-3d57f91ecf5e And the twitter link: http://twitter.com/michaeljackson Twitter has the advantage that it has a square image icon for all profiles. We could just retrieve the URL of the image by looking for the <img> tag which has the class "avatar" and is inside the <div> with class "profile-header-inner-overlay", et voila: https://twimg0-a.akamaihd.net/profile_images/2238087069/MJ_Image_bigger.jpg The way to do lookups inside webpages (instead of APIs) should be maybe stored in the server side or something, to be able to update it without a release of Banshee, whenever the pages change.
(In reply to comment #46) > And the twitter link: ... I meant that we can extract the twitter link from the MB page. I.e.: get the <ul> element with class "external_links", and then find any <a> element with an href that starts with "http://twitter.com".
The only problem with approaches like the one mentioned in comment#46 and comment#47 is that the images retrieved may be just "artist images" rather than "artist logos", which may be hard to distinguish if they are just a photo of the people from the band. To address that I've been googling specifically for the term "artist logo", and I found an API that could fit: http://fanart.tv/api-docs/music-api/ As an example of a response for getting image artists from that API, we get: { "Evanescence": { "mbid_id": "f4a31f0a-51dd-4fa7-986d-3095c40c5ed9", "artistbackground": [ { "id": "9", "url": "http://fanart.tv/fanart/music/f4a31f0a-51dd-4fa7-986d-3095c40c5ed9/artistbackground/9/evanescence-4dc719c5cc68f.jpg", "likes": "1" }, { "id": "5", "url": "http://fanart.tv/fanart/music/f4a31f0a-51dd-4fa7-986d-3095c40c5ed9/artistbackground/5/evanescence-4dc7197019e85.jpg", "likes": "0" }, { "id": "5474", "url": "http://fanart.tv/fanart/music/f4a31f0a-51dd-4fa7-986d-3095c40c5ed9/musiclogo/5474/evanescence-4df95bceb4b1c.png", "likes": "1" }, { "id": "27702", "url": "http://fanart.tv/fanart/music/f4a31f0a-51dd-4fa7-986d-3095c40c5ed9/musiclogo/27702/evanescence-4f8acb2ae6002.png", "likes": "1" } ], "albums": { "2187d248-1a3b-35d0-a4ec-bead586ff547": { "albumcover": [ { "id": "43", "url": "http://fanart.tv/fanart/music/f4a31f0a-51dd-4fa7-986d-3095c40c5ed9/albumcover/43/fallen-4dc8683fa58fe.jpg", "likes": "0" } ], "cdart": [ { "id": "17739", "url": "http://fanart.tv/fanart/music/f4a31f0a-51dd-4fa7-986d-3095c40c5ed9/cdart/17739/fallen-4f133f8a16d25.png", "likes": "0", "disc": "1", "size": "1000" } ] ... Which then means we would need to filter the URLs to only retrieve the ones inside the subfolder "musiclogo".
Created attachment 256343 [details] [review] updated patch to latest code I've updated the old patch to the latest banshee master code and moved the preferences into the preferences tab.
(In reply to comment #49) > I've updated the old patch to the latest banshee master code and moved the > preferences into the preferences tab. Hey Frank, thanks for your hard work! Sorry that we didn't merge this yet. These months, what basically happened is, that I added in Gnome's wiki what I suggested in comment#48 as a project idea for GSoC, and a student applied for it, and implemented it! He actually used quite a bit of the code in this patch (he credited the files properly) to create a new extension that changes the appearance of the ArtistView. Given this, I think the best approach for this patch is to also end up as an extension. Do you mind spending some time to convert it? If you're too bothered, I'm actually willing to help doing it or do it myself at some point, because I think the feature is a very interesting alternative to image logos.
typo: s/image logos/artist logos/
Hi Andres, I assume you're talking about the FanArt extension? For the moment, I was interested in getting the code working on the latest Banshee as I use that ArtistListView myself and have really gotten accustomed to it. Is the lastest code good to go for the extension? If so, it shouldn't be hard to convert and I'd be happy to have a look at it. I actually took the liberty of disabling the FanArt extension in BCE as default behaviour after properly integrating it into the build system, because it failed to compile and couldn't be disabled. Cheers
(In reply to comment #52) > Hi Andres, > > I assume you're talking about the FanArt extension? Yes. (He also did a SongKick extension for the first part of the GSoC program BTW.) I'm adding him to the CC list of the bug now. > For the moment, I was interested in getting the code working on the latest > Banshee as I use that ArtistListView myself and have really gotten accustomed > to it. Is the lastest code good to go for the extension? Pretty much. > If so, it shouldn't be > hard to convert and I'd be happy to have a look at it. I actually took the > liberty of disabling the FanArt extension in BCE as default behaviour after > properly integrating it into the build system, because it failed to compile and > couldn't be disabled. Thanks for doing that. Sorry that it doesn't compile yet, it's actually my fault because I haven't merged the patch in main Banshee required to make FanArt build. (I'm going to attach the patch here, for the record.) The reason why I haven't merged it yet is actually because I don't know what would happen if, given the API proposed in the patch, one would enable both the FanArt extension *and* the future extension that would implement the feature you add in your patch (you need to find a good name for it, so we can refer to it easily :) ). That's why I was thinking that maybe the API proposed in this patch is not the ideal one. Maybe what should happen is that we add an extension point, to which the normal ArtistView hooks up as "default", and then if some extensions implement that extension point, they would appear as context menu options of the ArtistView, to be able to switch to each different UI of each extension easily. Do you like the idea? If yes, then you're more than welcome in helping me to define this extension point. BTW, we haven't migrated FanArt or any b-c-e extensions to GTK3 yet, so, for now, let's make them all still target GTK2 (for this purpose, banshee repository now has a "gtk2" branch). The purpose of this would be to do a release of b-c-e that is still compatible with Banshee 2.6.x. And after we release Banshee 2.9.0 we could start porting all b-c-e extension to Gtk3, with the aim of finishing the easy ones for a 2.9.1 release. Ah, one last thing: Tomasz (the GSoC student) got your feature working for his FanArt extension before working on the FanArt specific view. Not sure if that code still leaves in the extension as an optional UI that can be enabled, but if that is the case, I guess the work that Frank needs to do is much easier (just extract the code into a different extension).
Created attachment 256478 [details] [review] Banshee patch to make FanArt extension compile
Created attachment 258209 [details] [review] Artist List as extension point to add custom renderers I've just had a look on how to rework the artist list to have an extension point to add new ColumnCell renderers or even ColumnControllers (to support FanArt). They will be automatically loaded and remembered in the context menu as well as in the view menu when the artist list is in focus. the interface is simple and an extension needs to provide only a few public methods (IArtistListRenderer). Have a look and let me know what you think.
Awesome Frank! Did you also adapt your code to make it be an extension that hooks into this extension point? If yes, I would love to see the code, and test it.
Hi Andres. I didn't make it into a separate extension just yet. I've added ALL renderers in extension form though via <Extension path="/Banshee/Gui/ArtistListView"> <ArtistListRenderer class="Banshee.Collection.Gui.ColumnCellArtistText"/> <ArtistListRenderer class="Banshee.Collection.Gui.ColumnCellArtistCoverSmall"/> <ArtistListRenderer class="Banshee.Collection.Gui.ColumnCellArtistCoverLarge"/> </Extension> in Banshee.ThickClient.addin.xml. So basically this addin-xml and three classes Banshee.Collection.Gui.ColumnCellArtistCover (base class for the next two) Banshee.Collection.Gui.ColumnCellArtistCoverSmall Banshee.Collection.Gui.ColumnCellArtistCoverLarge Could be extracted to make it an actual extension. I haven't set up BCE to compile on the git version of banshee at the moment, so I couldn't test it that way, yet, but it shouldn't be hard to do. I could produce two patches instead of one. The first to patch banshee to accept renderer extensions and the other to patch BCE with the new extension. FanArt would need changes btw. It would need to provide two classes instead of a modified FanArtistList class which has UI to switch modes. But that seems very straight-forward and I did the same with the above classes, just sub-classing the renderer.
Hi, it is nice to see that people are interested in this topic. I'm really sorry that I didn't write earlier – I've been really busy. You are right about the fact that multiple extensions that use old implementation of API can cause bugs and that new API is needed. I looked at Frank's new API and I like his idea (but I made no tests though). Andrés was right that I extracted Frank's old patch into Fanart extension at some point. My modifications in Frank's old code were very limited (mostly renaming) and I think that this code is not useful in current circumstances. Here are some links (just for reference): * https://gitorious.org/banshee-community-extensions/banshee-community-extensions/source/05b5cf63ff2d411d6394fc5448b337f4300789b2:src/Fanart/Banshee.Fanart.UI/CoverArtArtistListView.cs * https://gitorious.org/banshee-community-extensions/banshee-community-extensions/source/05b5cf63ff2d411d6394fc5448b337f4300789b2:src/Fanart/Banshee.Fanart.UI/CoverArtArtistColumnCell.cs That code is not used anymore, so I removed it from master branch.
Created attachment 259007 [details] [review] Artist List as extension point to add custom renderers without adding new lists I've removed the new custom album cover artist lists and made the patch just into adding the new extension point with the text list as default extension. I will attach a new patch for BCE to add the custom album cover artist lists. I've also fixed some little glitches that happened when loading new extensions (restart was required). One thing to know: when turning off an extension which has its artist list enabled, it'll stay enabled until the next restart.
Created attachment 259008 [details] [review] ArtistList extension for BCE to add new album covers artist lists Attached the new BCE extension to build on top of the new patch which adds the new extension point to add custom artist lists. It'll add the two album cover artist lists that were originally part of the first patch. The extension compiles only with GTK3 and the patched Banshee.ThickClient (patch 259007), so I had to add a custom Makefile.am
Awesome, thanks Frank and Tomasz! I'll probably commit this soon. I'm guessing we need to create a 'gtk2' branch in b-c-e, and make master default to only build the GTK3-compatible extensions. Tomasz (or Frank), would you have time to port Fanart extension to Frank's extension point?
Created attachment 259138 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point I've added a new patch for Fanart to use the same new extension point. It actually simplifies the code, so I assume that's a good thing. I've also converted it to GTK3. It works a charm also with both extensions enabled! I would have created a branch and converted the whole BCE to GTK3, but I really don't get how BANSHEE_LIBS gets set during the configure process. Can you help with that?
Created attachment 259139 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point Reattached with a small bug fix
(In reply to comment #62) > I would have created a branch and converted the whole BCE to GTK3, but I really > don't get how BANSHEE_LIBS gets set during the configure process. Can you help > with that? BANSHEE_LIBS is set by pkg-config, through the PKG_CHECK_MODULES call at this line: https://gitorious.org/banshee-community-extensions/banshee-community-extensions/source/configure.ac#L31 It is set to the same value as the output of "pkg-config --libs banshee-thickclient" The value comes from the .pc file with the corresponding name, which is installed by Banshee, usually at /usr/lib/pkgconfig/banshee-thickclient.pc To point to another set of Banshee assemblies, you can override this by setting the BANSHEE_LIBS environment variable, for example: export BANSHEE_LIBS="-r:/path/to/banshee_sources/bin/Banshee.Widgets.dll -r:/..." I hope this helps.
Thanks Bertrand. I'll look into that a bit further then. I've rethought the patches after I applied the changes to the Fanart extension and made some slight modifications to the whole system to make it easier, simpler, less code and more consistent. I've modified the IArtistListRenderer interface to only provide one method (besides the helper methods for name etc.) for the modification of the column controller. I'll attach the three modified patches.
Created attachment 259140 [details] [review] Artist List as extension point to add custom renderers without adding new lists New and simpler patch for Banhsee ThickClient
Created attachment 259141 [details] [review] BCE: ArtistList extension for BCE to add new album covers artist lists Modified version of the new BCE ArtistListCovers extension
Created attachment 259142 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point Modified version of the Fanart extension
(In reply to comment #64) > It is set to the same value as the output of "pkg-config --libs > banshee-thickclient" > > The value comes from the .pc file with the corresponding name, which is > installed by Banshee, usually at /usr/lib/pkgconfig/banshee-thickclient.pc Those libs are still set to all the 2.0 libs instead of the 3.0 libs when compiling and installing the latest banshee master.
Created attachment 261284 [details] [review] Artist List as extension point to add custom renderers without adding new lists updated to apply on latest git master
Created attachment 262821 [details] [review] BCE: ArtistList extension for BCE to add new album covers artist lists updated to apply to git master
Created attachment 262822 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point updated to apply to latest git master
I've seen there is now again some work going on on the repository. Any chance to get this committed? I can do the BCE commit if the banshee one is in.
Sorry that I've neglected looking at this patches lately, Frank, but there has been a lot of work in the GStreamer side of things since the Gnome .NET hackfest (http://nullreference.me/blog/2013/10/12/net-gnome-hackfest-2013-everybody-jump-on-the-application-porting-omnibus-day-5) and I've been working on that since then, and the next release of banshee is mainly going to have that big change (defaulting to gstreamer-sharp-based media-engine). So 2.9.0's big change was the migration to GTK3, and 2.9.1 will be this one. And 2.9.1 will happen in about a week or less. So I think we shouldn't rush merging this yet. Do you mind if we merge this post-2.9.1 to decrease the possibility of needing last-minute fixes? (I would hate to have to release a 2.9.1.1! you know)
No worries Andrés. That makes perfect sense.
Created attachment 267266 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point I've extracted the GTK3 code and will put that into a separate patch. This new patch is the conversion of FanArt to the new ArtistList extension point only.
Created attachment 267267 [details] [review] BCE: Migrate FanArt extension to GTK3 This is the new patch to migrate FanArt to compile on GTK3.
Comment on attachment 261284 [details] [review] Artist List as extension point to add custom renderers without adding new lists The patch seems very high quality. Just found some nits that would be nice to fix: - You're mixing tabs with spaces in some files. - You could use more the "var" keyword especially when the type is repeated twice in the same line, i.e.: ComplexMenuItem item = new ComplexMenuItem (); - Braces in 'if' conditions are in the same line. Found this case but there could be more: + if (last_item != null) + { + actions.AttachSubmenu (last_item); + } - Please order this a bit better in groups (the hyena usings together, the MonoBCL usings together, etc.): +using System; +using Mono.Unix; +using Gtk; + +using Banshee.MediaEngine; +using Banshee.ServiceStack; +using Banshee.Equalizer.Gui; +using Hyena.Data.Gui; +using System.Collections.Generic; +using Mono.Addins; +using Hyena; +using Banshee.Collection.Gui; +using System.Collections; +using Banshee.Widgets; +using Hyena.Widgets; +using Banshee.Configuration; - If you don't plan to check for null after this cast, please use a static cast instead of a dynamic one: + MenuItem parent = item as MenuItem; + parent.Submenu = CreateMenu (); - Please use EventHandler<T> instead of old fashioned delegate here: + public event ArtistListModeChangedHandler ArtistListModeChanged; - In some cases you have forgotten a space before parenthesis. This is one of them: + if(handler != null) { - Please use property initializiation here: + ArtistListModeChangedArgs evargs = new ArtistListModeChangedArgs (); + evargs.Renderer = ArtistListActions.renderer; that is, simply: var args = new ArtistListModeChangedArgs { Renderer = renderer }; - Public fields are evil, please use properties: + public class ArtistListModeChangedArgs : EventArgs + { + public IArtistListRenderer Renderer; + } - Copy/paste typo: +++ b/src/Core/Banshee.ThickClient/Banshee.Gui/ArtistListActions.cs @@ -0,0 +1,292 @@ +// +// ViewActions.cs +// - Use lambda syntax instead of delegate(Foo): + actions.ArtistListModeChanged += delegate(ArtistListModeChangedArgs args) { + if (last_item != null) + { + actions.AttachSubmenu (last_item); + } + }; - I'm guessing you meant to mark this method as private, not public: + public void SetRenderer (IArtistListRenderer renderer) + { - Why not use a setter here? + public void SetArtistListActions (ArtistListActions actions) + { Sorry it took so long to review! While you address these changes, I'll test the patch in the following days.
Created attachment 275101 [details] [review] Artist List as extension point to add custom rederers without adding new lists I've worked through the review and reformatted the files, made the recommended code improvements (added "null" check, use EventHandler, use properties, corrected copy&paste file headers, use lambda syntax, changed method visibility, and used a setter instead of a method). I also fixed a little visual bug, when switching to a different view, by adding a QueueResize which enables column controllers with differently sized columns to be rendered properly immediately after the switch.
Thanks for the quick reply! But I still see tabs+spaces in that patch Frank :) (we use spaces in Banshee).
Created attachment 275257 [details] [review] Artist List as extension point to add custom rederers without adding new lists Should have been only tabs, as I chose the wrong format. Corrected and converted the indents to spaces.
Created attachment 275843 [details] [review] Artist List as extension point to add custom rederers without adding new lists I've removed some whitespace changes and irrelevant changes, and added a commit message. I'll probably commit this early next week. Sorry for the delay again, bear with me!
Created attachment 275846 [details] [review] Artist List as extension point to add custom rederers without adding new lists I've improved more things: - Converted GetName() to be a property instead. - Changed "String" to "string". - Removed many unnecessary "using" statements (recommendation to Frank, use SourceAnalysis feature of MonoDevelop to detect these!). - Added a "using" pattern for a Pango.Layout variable instead of calling Dispose(). - Usage of more "var" keywords to avoid redundant types. - Added "private" to some method that was missing it.
Review of attachment 275846 [details] [review]: Thanks for the changes. I agree, they are good improvements.
Created attachment 276958 [details] [review] Artist List as extension point to add custom rederers without adding new lists Improved one more bit as discussed with Andres: removed static variable and made into class variable (ArtistListActions.renderer).
Review of attachment 276958 [details] [review]: Thanks for your patience Frank! Another (but hopefully last) thing: The method SetColumnController() seems a bit weird because it's not really "setting" anything in the ColumnCellArtistText. So I'm wondering, isn't it better that, instead of having this: + public class ColumnCellArtistText : ColumnCellText, IArtistListRenderer + { + public ColumnCellArtistText () : base ("DisplayName", true) + { + } ... + public void SetColumnController (ColumnController column_controller) + { + var current_layout = new Column ("Artist", this, 1.0); + column_controller.Add (current_layout); + } ... + private void UpdateRenderer () + { + column_controller.Clear (); + renderer.SetColumnController (column_controller); ColumnController = column_controller; + QueueResize (); + } We have something like this: + public class ColumnCellArtistText : ColumnCellText, IArtistListRenderer + { + public ColumnCellArtistText () : base ("DisplayName", true) + { + CurrentLayout = new Column ("Artist", this, 1.0); + } ... + public Column CurrentLayout { get; private set; } ... + private void UpdateRenderer () + { + column_controller.Clear (); + column_controller.Add (renderer.CurrentLayout); ColumnController = column_controller; + QueueResize (); + }
I've caught another little thing, I think this ArtistListActions' method should be private, not public: + public Menu CreateMenu () + { And you still haven't told me if this change is intended? @@ -52,8 +122,7 @@ namespace Banshee.Collection.Gui // set on the sources themselves that give us access to the IListView<T>. /*protected override bool OnPopupMenu () { - ServiceManager.Get<InterfaceActionService> ().TrackActions["TrackContextMenuAction"].Activate (); - return true; + return base.OnPopupMenu (); }*/ } } Also, you can simplify this: protected override bool OnFocusInEvent (Gdk.EventFocus evnt) { action_service.ArtistListActions ["ArtistListMenuAction"].Visible = action_service.ArtistListActions.ListActions ().Length > 2 ? true : false; with this: protected override bool OnFocusInEvent (Gdk.EventFocus evnt) { action_service.ArtistListActions ["ArtistListMenuAction"].Visible = action_service.ArtistListActions.ListActions ().Length > 2;
Created attachment 277075 [details] [review] Artist List as extension point to add custom rederers without adding new lists I've made some additional changes close to the suggestions. Because there could be multiple columns (e.g. FanArt has two) I've chosed a ColumnController property. I've simplified the code as suggested. The popup menu changes are intentional. I'll also attach updated patches for the extensions.
Created attachment 277077 [details] [review] BCE: ArtistList extension for BCE to add new album covers artist list
Created attachment 277078 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point
Created attachment 277079 [details] [review] Artist List as extension point to add custom rederers without adding new lists Had missed the public CreateMenu method. Updated to private
(In reply to comment #89) > The popup menu changes are intentional. Not sure you understood me, I mean the change inside a TODO comment. The TODO comment is this: // TODO add context menu for artists/albums...probably need a Banshee.Gui/ArtistActions.cs file. Should // make TrackActions.cs more generic with regards to the TrackSelection stuff, using the new properties // set on the sources themselves that give us access to the IListView<T>. /*protected override bool OnPopupMenu () { ServiceManager.Get<InterfaceActionService> ().TrackActions["TrackContextMenuAction"].Activate (); return true; }*/ And you're converting it to: // TODO add context menu for artists/albums...probably need a Banshee.Gui/ArtistActions.cs file. Should // make TrackActions.cs more generic with regards to the TrackSelection stuff, using the new properties // set on the sources themselves that give us access to the IListView<T>. /*protected override bool OnPopupMenu () { return base.OnPopupMenu (); }*/ Which essentially means a NO-OP (because an overriden method that calls its base is like there was no method). So, why are you making this change and not changing the TODO comment?
Hmm.. I'm just seeing in both cases the whole method is commented out, so they are both NO-OPs, and a change wouldn't be necessary. I know I changed something there previously, but now I doubt the reasons. We could just revert to what is was. The actual reason I remember was though, that the previous version doesn't make sense, as it merely copies the code from the super class...
(In reply to comment #94) > Hmm.. I'm just seeing in both cases the whole method is commented out, so they > are both NO-OPs, and a change wouldn't be necessary. I know I changed something > there previously, but now I doubt the reasons. We could just revert to what is > was. > > The actual reason I remember was though, that the previous version doesn't make > sense, as it merely copies the code from the super class... But, is your patch adding context menus? If not, then I don't think your patch should touch that comment at all (the commented code is related to the TODO, and even if it does exactly the same as the base class, I guess it's there to guide the next developer to an example of what it can do before the "interesting thing" that the developer codes there in that method. Anyway, let's move on. Now I'm having trouble because this bit from one of your patches doesn't work for me: +BANSHEE_LIBS = -r:$(BANSHEE_LIBDIR)/Banshee.Widgets.dll -r:$(BANSHEE_LIBDIR)/Banshee.ThickClient.dll -r:$(BANSHEE_LIBDIR)/Banshee.Services.dll \ + -r:$(BANSHEE_LIBDIR)/Hyena.Gui.dll -r:$(BANSHEE_LIBDIR)/Banshee.Core.dll -r:System -r:$(BANSHEE_LIBDIR)/Mono.Media.dll -r:Mono.Posix \ + -r:$(BANSHEE_LIBDIR)/Hyena.dll -r:$(BANSHEE_LIBDIR)/Hyena.Data.Sqlite.dll -r:${expanded_libdir}/pkgconfig/../../lib/cli/cairo-sharp-1.10/cairo-sharp.dll \ + -r:${expanded_libdir}/pkgconfig/../../lib/cli/dbus-sharp-1.0/dbus-sharp.dll -r:${expanded_libdir}/pkgconfig/../../lib/cli/dbus-sharp-glib-1.0/dbus-sharp-glib.dll \ + -r:${expanded_libdir}/pkgconfig/../../lib/cli/glib-sharp-3.0/glib-sharp.dll -r:${expanded_libdir}/pkgconfig/../../lib/cli/Mono.Addins-0.2/Mono.Addins.dll \ + -r:${expanded_libdir}/pkgconfig/../../lib/cli/pango-sharp-3.0/pango-sharp.dll -r:${expanded_libdir}/pkgconfig/../../lib/cli/atk-sharp-3.0/atk-sharp.dll \ + -r:${expanded_libdir}/pkgconfig/../../lib/cli/gdk-sharp-3.0/gdk-sharp.dll -r:${expanded_libdir}/pkgconfig/../../lib/cli/gtk-sharp-3.0/gtk-sharp.dll \ + -r:${expanded_libdir}/cli/taglib-sharp-2.1/taglib-sharp.dll -r:$(BANSHEE_LIBDIR)/MusicBrainz.dll I guess it fails because you're assuming so many things putting those paths there (i.e. the cli/ thing is a debianism, and cannot work in other distros; what you need to do is use real paths obtained from pkgconfig). And the thing is, you're redefining there a variable that is set by the macro PKG_CHECK_MODULES(BANSHEE, ...) (take in account a call to PKG_CHECK_MODULES(FOO, ...) will set the value of the var FOO_LIBS). So I removed that bit from your patch, and now I get an error which I guess was the one you were trying to fix: ./Banshee.Fanart.UI/FanartArtistColumnCell.cs(127,49): error CS1503: Argument `#1' cannot convert `Cairo.Color [Mono.Cairo, Version=4.0.0.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756]' expression to type `Cairo.Color [cairo-sharp, Version=1.10.0.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756]' This is because we're passing to the compiler both a reference to Mono.Cairo and cairo-sharp. The latter is what the gtk3 bindings bring, so we need to reference that, and stop referencing Mono.Cairo.
I don't know how that is puzzled together. The Mono.Cairo reference seems to come from banshee-hyena-gui as far as I can tell. How could that be changed?
Created attachment 278433 [details] [review] Removing old Mono.Cairo references for Hyena.Gui to enable BCE patches to compile I've figured it out. Hyena.Gui was still introducing the old Mono.Cairo as a requirement whereas a the new cairo-sharp is also reference. I've created a patch to correct the pkg-config. This will enable the BCE extensions to build correctly after removing the Makefile changes.
Comment on attachment 278433 [details] [review] Removing old Mono.Cairo references for Hyena.Gui to enable BCE patches to compile Thanks for the patch ! BCE is probably the only user of that .pc file, so we didn't notice the problem earlier. I've committed it to git master: https://git.gnome.org/browse/banshee/commit/?id=7b4ca0a82
Thanks for committing that Bertrand! Alright, I think we need to improve a bit the context menu, and then we would be done and ready to commit. Right now it is like this: Play ----- Add to Play Queue Play After > Add To Playlist > ArtistList > Text Artist List ------ Fanart Single Column Fanart Double Column Write CD... ------ Remove From Library Delete From Drive ------ Open Containing Folder ------ Properties Edit Track Information But I think it should be like this: Play ----- Add to Play Queue Play After > Add To Playlist > ------ View > FanArt Single Column FanArt Double Column ----- Default Plain Artist Text List ------ Write CD... ------ Remove From Library Delete From Drive ------ Open Containing Folder ------ Properties Edit Track Information The above also means that, if there is any hook into the extension point (i.e.: FanArt extension has been enabled), then the the new view should be enabled by default without the need of forcing the user to use the context menu, for easier discoverability.
I also found another issue: when switching back from FanArt visualization to default artist list, the font size is not corrected (it kept the size used for FanArt visualization).
Hi Andrés, I've had a look at the menu changes you suggested. They are quite complex to implement. As the "view" menu (easy to change the name) is added dynamically (because the context menu is shared among all the listviews), adding and removing the separators is really hard (as compared to the single menu item). Also, selecting a newly added extension as default adds a number of complex scenarios (even though I understand the intention). There is a property saved with the previously selected view and in case of no extension, that is the default text view (which is now an extension as well). So we'd need to detect that an extension was not enabled/active before and that could be tricky. For the visualisation issues, I cannot see them. I do see a lot of other issues with the ListViews now though, since I have rebased to the latest master and the updated Hyena. Before everything seemed fine.
Hey Frank, (In reply to comment #101) > Hi Andrés, > > I've had a look at the menu changes you suggested. They are quite complex to > implement. As the "view" menu (easy to change the name) is added dynamically > (because the context menu is shared among all the listviews), adding and > removing the separators is really hard (as compared to the single menu item). Understandable. I'll have a look at it (a gsoc student I have this summer may help me too because he has dealt with this menus recently). > Also, selecting a newly added extension as default adds a number of complex > scenarios (even though I understand the intention). There is a property saved > with the previously selected view and in case of no extension, that is the > default text view (which is now an extension as well). So we'd need to detect > that an extension was not enabled/active before and that could be tricky. Good to know. The GSoC student that we have this year working on SongKick and FanArt may look at this too. > I do see a lot of other issues > with the ListViews now though, since I have rebased to the latest master and > the updated Hyena. Before everything seemed fine. Welcome to the world of GTK3! I guess Bertrand has been fixing visualization glitches in Gnome3(Adwaita theme?) and he has accidentally broken visualization on Ubuntu(Ambiance theme), is this what you're using? Because it is what I'm using and the artist listview's text is certainly disappearing when banshee is focused (not when banshee is unfocused, in the background). It also can be that Bertrand didn't break anything, but that he fixed some CSS handling bug, which has exposed in banshee a bug in the Ambiance theme (I've had to fix this theme some months ago, submitted my patches to launchpad, etc). These are, unfortunately, the typical issues that we're dealing with these days more often in the master branch (as opposed to the stable-2.6 branch which still uses GTK2), and which obviously require more attention than b-c-e stuff, so I guess you can understand now how busy we are and how it takes so long for us to find time to review non-Core patches. You're more than welcome to help us if you find the motivation. Fortunately, at least, these issues are not appearing in versions that are normally shipped with distros, because we've buried the GTK3 work under a big 2.9.x unstable-development-series schedule (and distros still normally use the stable versions). If we had done the switch to GTK3 much earlier, I fear we would have been impacted by many issues, example: http://blogs.gnome.org/mortenw/2014/06/23/how-does-one-create-a-gtk-application/ (Sorry for hijacking this bug with this rant!)
Created attachment 280826 [details] [review] BCE: ArtistList extension for BCE to add new album covers artist list I've fixed the rendering problems. Bertrand had changed the "Render" methods and they had to be adapted to the new parameters.
Created attachment 280827 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point Fixed the rendering in FanArt as well.
Created attachment 280828 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point Corrected problem when switching back from FanArt visualization to default artist list (the font size was not corrected) I was not able to reproduce this problem, as my theme does not seem to switch font sizes at all, but I could see the problem in the code. If I am correct, with the old/previous patch, the font size would keep increasing when switching forth and back between text view and fanart view.
Created attachment 280835 [details] [review] Artist List as extension point to add custom rederers without adding new lists I've addressed the restructure of the menu and added separators. To avoid ambiguity I've renamed it to "ArtistList View" instead of "View" (as that is already in the "View" menu). Last item to potentially address would be automatic activation of new extensions.
(In reply to comment #106) > Created an attachment (id=280835) [details] [review] > Artist List as extension point to add custom rederers without adding new lists > > I've addressed the restructure of the menu and added separators. Nice one! > To avoid > ambiguity I've renamed it to "ArtistList View" instead of "View" (as that is > already in the "View" menu). I think using PascalNotation words in a user-facing string is an anti-pattern, we should use "Artist List View". Small review: + ColumnController column_controller; Add "readonly" please. + get { return "Text Artist List"; } You should use Catalog.GetString() here. And I would rather prefer to use "Default Text Artist List", don't you think? + public ColumnController ColumnController + { This brace should go in the same line (it's a property). + SeparatorMenuItem separator1 = new SeparatorMenuItem (); + SeparatorMenuItem separator2 = new SeparatorMenuItem (); Use "var" here in both lines. + if (!alreadyHasSeparator2) + separator2.Show (); Missing braces. + var label = new Label ("ArtistList View"); Missing Catalog.GetString() + RadioAction radio_action = this [ArtistListMode.Get ()] as RadioAction; Let's use "var" here too. +// Copyright (C) 2007-2008 Novell, Inc. You're missing adding copyright to yourself. -// Author: +// Authors: // Aaron Bockover <abockover@novell.com> +// Frank Ziegler <funtastix@googlemail.com> // // Copyright (C) 2006-2007 Novell, Inc. Same here. + private ArtistListActions artist_list_actions; Missing 'readonly'. You don't need to fix this, I can do it myself before I commit it, but noting them here so I don't forget.
Created attachment 281001 [details] [review] Artist List as extension point to add custom rederers without adding new lists Added all the above review improvements except for the last readonly which is not possible because all interface actions are initialized outside of the constructor. I also added the ability to automatically select a new renderer when it is added for the very first time (saving all ever registered renderers in the configuration).
(In reply to comment #108) > Created an attachment (id=281001) [details] [review] > Artist List as extension point to add custom rederers without adding new lists > > Added all the above review improvements except for the last readonly which is > not possible because all interface actions are initialized outside of the > constructor. Nice thanks! > I also added the ability to automatically select a new renderer when it is > added for the very first time (saving all ever registered renderers in the > configuration). Why are you exactly saving the previous renderers in a new PreviousArtistListModes schemaEntry? I think we only need one SchemaEntry here (for the current renderer), but I might be wrong. Let me state the reasoning: a) Case 1: There are no extensions, then we simply use the default renderer (and is saved in the ArtistListMode schema config). b) Case 2: We're using the default renderer, and someone enables the FanArt plugin -> renderer switches to FanArt automatically (and is saved in the ArtistListMode schema config).. c) Case 3: We're using the FanArt renderer, and the user enables the ArtistListCovers -> renderer switches to ArtistListCover automatically (and is saved in the ArtistListMode schema config). d) Case 4: We're using the ArtistListCover renderer (FanArt is installed too). User disables the ArtistListCover -> renderer switches to FanArt (because there's an existent renderer which is not the default). e) Case 5: We're using the FanArt renderer (there's only this one), and user disables the FanArt extension -> renderer switches to default renderer automatically (and is saved in the ArtistListMode schema config). Am I missing any of the cases? Last nitpick: you added calls for Catalog.GetString() in the SchemaEntry declarations, but all banshee codebase doesn't does this (it probably should, though, but that should be a separate patch, let's be consistent for now).
(In reply to comment #109) > d) Case 4: We're using the ArtistListCover renderer (FanArt is installed too). > User disables the ArtistListCover -> renderer switches to FanArt (because > there's an existent renderer which is not the default). To simplify things, you can also make it so banshee always switches to the default renderer when the extension of the current renderer is disabled.
Created attachment 281074 [details] [review] Artist List as extension point to add custom rederers without adding new lists I've added the functionality to automatically enable a renderer when an extension is added without the need for a schema entry (achieved this by not doing this during startup). The renderer falls back to default text when the currently active extension is disabled. I've added the Catalog calls in the schema because it will otherwise just not work if the string is translated (because the renderer name comes from the Catalog as well and may be in a different language).
Created attachment 281075 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point I also fixed two bugs in the Fanart extension: 1. There was a null pointer exception in the cleanup code. I have moved that code to dispose instead of disable, as otherwise the tables may still be needed by the async job 2. This fixes also a problem where the tables are already dropped but still used by the image job
Comment on attachment 281074 [details] [review] Artist List as extension point to add custom rederers without adding new lists (In reply to comment #111) > Created an attachment (id=281074) [details] [review] > Artist List as extension point to add custom rederers without adding new lists > > I've added the functionality to automatically enable a renderer when an > extension is added without the need for a schema entry (achieved this by not > doing this during startup). The renderer falls back to default text when the > currently active extension is disabled. Thanks, although you forgot to remove the SchemaEntry field. I've added that change and committed this patch. > > I've added the Catalog calls in the schema because it will otherwise just not > work if the string is translated (because the renderer name comes from the > Catalog as well and may be in a different language). Banshee's configuration values shouldn't depend on the system's language, so I've changed the approach to use renderer.GetType().Fullname instead of renderer.Name.
Comment on attachment 281075 [details] [review] BCE: Converted Fanart extension to new ArtistList extension point (In reply to comment #112) > Created an attachment (id=281075) [details] [review] > BCE: Converted Fanart extension to new ArtistList extension point > > I also fixed two bugs in the Fanart extension: > 1. There was a null pointer exception in the cleanup code. I have moved that > code to dispose instead of disable, as otherwise the tables may still be needed > by the async job > 2. This fixes also a problem where the tables are already dropped but still > used by the image job These two bugs are not related to this bug (and I think they need a bit of discussion), so I've removed these 2 bugfixes from your patch and committed it. Please file a new bug for these problems. I will commit the ArtistListCovers extension later in the day. Thanks, almost ready to mark as FIXED!
Comment on attachment 267267 [details] [review] BCE: Migrate FanArt extension to GTK3 This has been already committed by Dmitrii I believe.
Comment on attachment 280826 [details] [review] BCE: ArtistList extension for BCE to add new album covers artist list I fixed many details in this patch (i.e. fixed tabsVSspaces issues, added Catalog.GetString() calls, used proper namespaces, removed redefining of BANSHEE_LIBS), and then I committed it to b-c-e. Thanks for your persistence Frank!
I'm marking this then as FIXED. But I have one more question for Frank. I spotted this in the patch for Banshee core: Catalog.GetString ("_ArtistList") I guess this should be change to be "_Artist List", however I don't find this string anywhere in the UI. Is it really a string that is user facing? If not, then I should remove the Catalog.GetString (and the underscore) usage. Thanks This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Created attachment 281298 [details] [review] Remove obsolete label code The _ArtistList label was indeed overwritten in the custom proxy. Actually very nasty, as it is not very transparent. I've corrected this to use the actual action label and changed it from _ArtistList to the correct "Artist List View"