On Sun, Aug 11, 2013 at 6:29 PM, olivier dufour <olivier duff gmail com> wrote:Patches are in the attachment.
> Hello,
> Just my 2 cents if Andrés is ok with it:
>
> - Why not move the code of patch 12 and 13 inside the snap function.
> This function can become later protected or public, so it is is better to
> locate all code related to switch in the same function and avoid to have a
> bigger property setter? Don't you think?
> - Last point, can you create a common private function to avoid redundant
> code between snapview and OnArtistFilterChanged?
>
best regards,
Tomasz
> cheers,
> olivier dufour
>
> Olivier Dufour
>
>
> 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.
>> - 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?
>> - 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?
>> - 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?)
>> - 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,
>> >> Tomasz
>> >
>> >
>> > I will review your patches asap.
>>
>> _______________________________________________
>> banshee-devel-list mailing list
>> banshee-devel-list gnome org
>> https://mail.gnome.org/mailman/listinfo/banshee-devel-list
>>
>
>
> _______________________________________________
> banshee-devel-list mailing list
> banshee-devel-list gnome org
> https://mail.gnome.org/mailman/listinfo/banshee-devel-list
>
_______________________________________________
banshee-devel-list mailing list
banshee-devel-list gnome org
https://mail.gnome.org/mailman/listinfo/banshee-devel-list
Attachment:
proposedAPI.diff
Description: Binary data
Attachment:
trackApiChangesInFanArt.diff
Description: Binary data