Re: [Banshee-devel-list] work on patches to Banshee



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

Attachment: 0001-ThickClient-extract-CompositeTrackSourceContents.Set.patch
Description: Binary data

Attachment: 0002-ThickClient-delete-duplicated-code-in-CompositeTrack.patch
Description: Binary data



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]