I'm attaching patches to Fanart extension. Thanks to them, Fanart
works with new API.
Tomasz Maczyński
On Fri, Aug 16, 2013 at 4:21 PM, Tomasz M. <tmtimon gmail com> wrote:
> I managed to fix a bug by using SetSource () method in setters. I
> think that both setters introduced in patch 003 work correctly.
>
> I'm attaching patches (the first one is Andrés' patch) which can be
> applied on top of master (7abadd673e28857fd337881c2a58700cf811f58a).
>
> My suggestions:
> - I think that we can skip patch 001 since there is no easy way to
> write Swap () method that works correctly (it is necessary to
> re-assign class fields and "refresh" view)
> - Maybe it would be better to add an interface with ArtistView and
> AlbumartistView properties (and implement it in CompositeTrackSource).
> Thanks to that, API users could use interface instead of
> CompositeTrackSource class. Should I do that?
>
> I'll sent next email with Fanart patches in a couple of minutes.
>
> best regards,
> Tomasz
>
> On Thu, Aug 15, 2013 at 2:52 AM, Tomasz M. <tmtimon gmail com> wrote:
>> On Wed, Aug 14, 2013 at 12:38 AM, Andrés G. Aragoneses <knocte gmail com> wrote:
>>> Tomasz, thanks for the new patches!
>>>
>>> I think you slightly misunderstood Olivier. If you end up with a patch X
>>> that adds some code, and then to address Olivier's suggestion you need to
>>> create a patch Y that removes some duplicated code, why write the duplicated
>>> code in the first place? (in patch X)
>>>
>>> To avoid this, I've already committed the suggestion from Olivier to
>>> separate a new "SwapView()" from OnArtistFilterChanged:
>>> https://git.gnome.org/browse/banshee/commit/?id=7abadd673e28857fd337881c2a58700cf811f58a
>>>
>>
>> Thanks!
>>
>>> Even though that commit doesn't expose (yet) SwapView as public, it will
>>> make the subsequent patches simpler, and it's even an improvement in itself
>>> (because thanks to it, a dynamic cast is removed -> see we removed the "as
>>> ArtistListView".
>>>
>>> After this, I've sat down to rework Tomasz patches (to update them and
>>> squash them even more), and I've realised that the FanArt extension was
>>> linking with Nereid (which is a bit ugly because it is a .exe instead of a
>>> .dll), so I've been re-thinking of how to expose all this so the extension
>>> can depend only on DLLs, and I've come up with the following solution, that
>>> I attach in 2 patches (one for FanArt and one for Banshee), based on
>>> previous Tomasz patches. Let me know what you think (and you Tomasz, if you
>>> could test these and let me know if with this new version, the "glitch"/bug
>>> you mention is still there? I don't have time to test it now).
>>
>> I agree that your design, which doesn't link exe files, is more
>> elegant, but those patches are not ready.
>>
>> The bug is even worse - after changing the artist_view content in not
>> shown at all (there is only a white box) and I didn't find a
>> workaround. I think it has something to do with the code that I put in
>> ArtistView setter. In proposed patches artist_view field is not
>> re-assigned appropriately. I'm not sure where equivalent code should
>> go in your design (should we add ArtistView and AlbumartistView
>> setters in IClientWindow?).
>>
>> What is more, it is hard to distinguish views of the same type (e.g.
>> CompositeTrackSourceContents.artist_view and
>> CompositeTrackSourceContents.albumartist_view), so I thnk we should
>> add some methods or setter to our API.
>>
>> I think that in your solution we should apply first section from patch 1
>> "ThickClient: change ArtistListView to TrackFilterListView<ArtistInfo>
>> where possible" so that we can swap on ArtistListView to any
>> TrackFilterListView<ArtistInfo> subclass.
>>
>> I think we should discuss it tomorrow on #banshee IRC channel.
>>
>> best regards,
>> Tomasz
>>
>>
>>> On 13 August 2013 18:44, Tomasz M. <tmtimon gmail com> wrote:
>>>>
>>>> On Sun, Aug 11, 2013 at 6:29 PM, olivier dufour <olivier duff gmail com>
>>>> wrote:
>>>> > 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?
>>>> >
>>>>
>>>> Patches are in the attachment.
>>>>
>>>> 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
>>>>
>>>