GNOME.org
 

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

I think that those patches still have a bug.

I'm working on that.

Tomasz

On Fri, Aug 16, 2013 at 4:41 PM, Andrés G. Aragoneses <knocte gmail com> wrote:

Great Tomasz thanks for the update.

I think we're getting very very close to what the ideal API would be.

I'm going to review these patches this weekend or early next week. But, as
you say that you don't have any visualization bugs with these ones, let's do
this:

- Commit your Fanart patches right away.
- Keep working on Fanart extension, assuming that the patches to Banshee
that you attached have been already merged (commit them in your working
copy).

That way, you're not blocked, and you can keep working on your GSoC work in
the Fanart extension.

If I end up making any modification to the patches, I'll make sure to modify
Fanart extension to make it compile, and I'll make sure I don't break
anything.
Thanks again.



On 16 August 2013 16:23, Tomasz M. <tmtimon gmail com> wrote:


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
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]