On Fri, Aug 9, 2013 at 2:18 PM, Tomasz M. <tmtimon gmail com> wrote:
On Fri, Aug 9, 2013 at 12:40 PM, Andrés G. Aragoneses <knocte gmail com> wrote:(Hey Tomasz, I'm CCing banshee-devel-mailing list this time, as this info may be appreciated by some audience; i.e.: we have extension developers that may be interested in the API improvements to make more things possible within an extension. Tomasz is our GSoC student this summer, we mainly communicate via IRC though) This is my first draft of review: - Patch 01: We shouldn't apply this because (yet) because if we do, it's not a guarantee that there will be other people using different MonoDevelop versions which would flush different changes. In general, it's very annoying that MonoDevelop keeps changing the projects even when the user didn't change them (there have been some improvements in this area in the last versions of MonoDevelop, but it's still not perfect; I myself even have a pull request open to improve this further, here https://github.com/mono/monodevelop/pull/199 , but if you read through it, you'll see that the solution is not as easy as it sounds, or the maintainer is not very willing to change the behaviour). Also, this patch includes a binary file by mistake.I sent it by mistake. But it's good to know why all those files were modified.- Patch 02: not committing this either and you know it :) but keep it applied in your working copy.:)- Patch 03: makes sense (in order to allow extension to plug a different ArtistListView -- notice: you've written the "what" in the commit message, but you're missing the "why"). - Patch 04: makes sense (in order to access the CompositeTrackSourceContents instance of the PlayerInterface instance from an extension --also missing the "why"). - Patch 05: mmm, good intentions, but there's no actually need for the variables, you can use just the fields. I've corrected this and pushed the change here: https://git.gnome.org/browse/banshee/commit/?id=a57e7145f8b71fd1d99e44e0e6c23cc4ae00e2a0 - Patch 06: this is partly a refactoring and seems to be partly needed for your FanArt extension, right? I noticed the latter, because your new method is public, but InitializeViews is protected. Can you confirm this?
Andrés suggested that we can apply that patch later (after API is changed) and I think we can do without patch 6 for now.
- Patch 07: as I changed your patch number 5 slightly, then this doesn't apply anymore. I guess the equivalent of this patch would be now to check each field for null before calling the ArtistListView constructor, right? Ok, but why is this needed? Are you calling InitializeViews () a second time from your extension?
Yes, checking each field for null before calling the ArtistListView constructor has equivalent behavior. However, InitializeViews method overload is called only by FilteredListSourceContents, so I think that we can skip that completely for now. On the other hand, this commit might be useful when we will try to solve problem that I mentioned in my previous email (see: the end of inline message).
My first idea (when I wrote that patch) was to make a method that would swap views, but later I decided that adding a CompositeTrackSourceContents.ArtistView property (in patch 10) would be better. So, when I extracted SetupArtistViews () method I made it public intentionally, but now I think it should be private. I think that that's the only non-private CompositeTrackSourceContents class member that is necessary if ArtistView property is displayed right away (otherwise, I will need another public method (e.g. UpdateViews () ).- Patch 08 & 10 & 11 & 12 & 13: these patches seem to be all following the same purpose (to swap the view), so I guess it doesn't make sense to separate them, right? Can you squash them in just one commit? (By the way, why the patch 13 has the "SongKick" word in it? Is it a typo?)Yes, "Songkick" in patch 13 is a obviously a typo. I will update (so that those commits work with corrected patch 5), squash and send those patches to banshee-devel-list soon. But event when I do that, it will not be ready to me merged with master branch, because there is an issue: When application uses ArtistView setter, ArtistView is displayed empty when "Library" is shown, but from the time you change Source to some other Source and change it back to "Library" everything works smoothly. I guess that CompositeTrackSourceContents.ArtistView's setter should be modified, but I don't know exactly how. Tomasz Maczyński- Patch 09: good, I've committed this here: https://git.gnome.org/browse/banshee/commit/?id=fb49f442f61430df96cc2e05ee8c850d4acc5342 PS: Patch 01 is too big for this mailing list maybe (1MB), so if anyone wants to view it, go here: http://monobin.com/1582/ On 9 August 2013 10:04, Andrés G. Aragoneses <knocte gmail com> wrote:On 9 August 2013 01:37, Tomasz M. <tmtimon gmail com> wrote:Hi, I'm trying to prepare Banshee classes to make it possible to change artist_view. I'm attaching patches from my most successful branch. I also pushed code to banshee-community-extension which is supposed to change artist_view (but not albumartist_view). To make it easy to notice if it works, it sets a view with duplicated artist column. Currently, view artists_view is empty when "Library" is shown, but from the time you change Source to some other Source and change it back to "Library", everything works smoothly. I guess that my 13th commit is not exactly what it should be. Could you have a look at that (the way all the things are "refreshed" and displayed is not obvious in this class and I think I've missed something small)? I also know that I need to in Banshee project: - add AlbumartistView property - add a way to set Views back to default ones (e.g. when plugin is turned off/unistalled). I think I should add SetDefaultArtistView method - delete redundant code introduced by 11th commit - check if all GUI calls are made from the main thread - tidy up a bit BTW: When I see a problem with some other plugin in banshee-community-extensions (like the recent one with changing "long" to "int") should I fix it (Makefile didn't work because of that problem)?In general, you should send me a patch to review (or open a bug and attach the patch there). Regarding this particular issue, take in account that I already committed a fix on behalf of Chow (hyperair on IRC). Did you pull?Or should I leave TODOs? And what about situations which are error-prone or may be misleading (e.g. TrackID is still an INTEGER in database schema in banshee-community-extensions/src/Lyrics/Banshee.Lyrics/Banshee.Lyrics/LyricsService.cs:73 - but I think it's not enough to change INTEGER to LONG because of backward compatibility).That is not an error. Sqlite's INTEGER simply maps to C# long type, it's not wrong.I will be on IRC tomorrow so we can discuss those topics, TomaszI will review your patches asap.
I will reply to Olivier's email soon. Note that my previous reply is inline (it wasn't send as it's "being held until the list moderator can review it for approval."). I'm attaching new patches (which work with current master) Tomasz Maczyński
Attachment:
0001-ThickClient-change-ArtistListView-to-TrackFilterList.patch
Description: Binary data
Attachment:
0002-Nereid-add-public-CompositeView-property-in-PlayerIn.patch
Description: Binary data
Attachment:
0003-ThickClient-add-public-CompositeTrackSourceContents..patch
Description: Binary data