GSoC application review - reimplementing personal metadata importers

[Matěj Laitl]

On 21. 4. 2013 Konrad Zemek wrote:
> 2013/4/20 Matěj Laitl <matej at laitl.cz>:
> > On 20. 4. 2013 Konrad Zemek wrote:
> > I think there are no importer tests, please check it and then be more
> > certain in the proposal.
> 
> I did actually check that, but the lack of tests is a bit unusual for
> me, so I figured they may be just well-hidden somewhere. I've
> conducted more extensive research, and there are indeed none.

Yeah, we've been more serious about test-driven-development only very 
recently. :) One reason is that our "business logic" code is unfortunately 
often interwined with GUI code, which makes testing unpleasant.

> What I meant were integration tests with importer implementation as a
> black box. I'm sure they can be written so that they bypass any
> interactivity, and they would help me a lot with refactoring (as well
> as providing a "done" condition).

Okay, should be doable. Be warned that some of our code is not testing-
friendly - which is another opportunity for you to make it so and be of even 
greater benefit to Amarok. ;)

> > Nice. I should be able to review & merge your review request soon, but
> > please still make a distinction between already merged and yet-in-review
> > patches. Having submitted/merged one more little patch would be a small
> > bonus, too.
> 
> I understand "contribute" as "write and submit code, hoping it will be
> helpful", not necessarily that it's merged. Although again, I very
> well recognize how it can be understood the other way (and probably my
> understanding here is different than commonly accepted definition). I
> didn't change that in hopes that it will be true in both senses by the
> time of application; I will rewrite it if not.

Well, unfortunately we've already seen review requests with just random (and 
actually harmful) changes -- I don't think we should consider these 
"contributions". This is certainly not the case for your well-done and non-
trivial patch, which I should be able to merge soon.

> And as much as I'd love to submit a lot more patches right now, I
> can't really do that this week. I have an exam coming in the next few
> days, as well as some mid-term tests.

I see, good luck with your exams! The deadline for proposals is May 3, so 
maybe you'll manage to at least submit a small change by then.

> To alleviate this, I intend to add Rhythmbox as a new target for
> synchronization. It stores following personal track metadata compatible with
> Amarok:
>         * user rating
>         * playcount
>         * last played time
>         * time when the song was first added to collection

Good. Note that "added to collection" is currently not part of 
synchronization. (you could add it, but maybe it isn't worth the effort)

Also note that current iTunes import matches tracks by just filenames. I 
secretly hope that one can get metadata (at least: artist, album, title; 
better also: track number, disc number, album artist) out of iTunes db, but 
please check - inability to do so would be a show-stopper for iTunes provider.

> Implementation Details:
>   (...)
>         On the high level, the StatSyncing framework consists of the
>         following elements
>           (...)
>         * Track, which is an abstraction of a track and contains relevant
>           metadata used for trackmatching

...and methods to set the metadata.

>         * Process, which is responsible for matching the tracks between
>           collection and synchronizing them

"collections"

>         The project can be broken into three parts; reimplementation of
>         existing importers (read-only), introduction of new importers
>         (read-only) and finally an optional goal of making the
>         synchronization work in read-write mode (so that foreign statistics
>         can be updated as well as local ones). This last goal is optional
>         (soft goal), and will involve renaming importers to reflect their
>         new capabilities.

Sounds good, except for the renaming. StatSyncing::ITunesProvider sounds well 
for both cases.

> Tentative Timeline:
>         MIDTERM EVALUATION (deadline: August 02)
>                 The hard goal for midterm evaluation is to have both
> FastForward and iTunes importers reimplemented, and if possible (soft goal)
> unit-tested.

Good.

>         FINAL EVALUATION (deadline: September 27)
>                 The hard goal for final evaluation is to have all importers
>                 implemented and working. The slightly softer goal is to have
> them unit-tested. The soft goal is to implement two-way synchronization.

This looks well too.

>         June 17 - 23:
>                 Drafting up test cases for FastForward and iTunes importers
>         June 24 - 30:
>                 Implementing the initial test suite for importers (exam
> week) July 01 - 07:
>                 Reimplementing FastForward importer, cleaning up
> data-reading module, introducing StatSyncing framework

Ehm, I think I know what you mean, but "introducing StatSyncing framework" 
sounds like it didn't existed before.

>         July 08 - 14:
>                 Reimplementing FastForward importer, implementing unit tests
> July 15 - 21:
>                 Reimplementing iTunes importer, cleaning up data-reading
> module July 22 - 28:
>                 Reimplementing iTunes importer, implementing unit tests
>         July 29 - August 04: MIDTERM EVALUATION
>                 Ironing out last issues with reimplemented importers,
> submitting current work for midterm evaluation
>         August 05 - 11:
>                 In-depth research of Rhythmbox metadata format, implementing
> Rhythmbox importer
>         August 12 - 18:
>                 Implementing Rhythmbox importer, implementing unit tests
>         August 19 - 25:
>                 Implementing Amarok 2.x importer
>         August 26 - September 01:
>                 Implementing Amarok 2.x importer, unit tests
>         September 02 - 08:
>                 Investigating and implementing two-way synchronization
>         September 09 - 15:
>                 Implementing two-way synchronization
>         September 16 - 22:
>                 Ironing out bugs, cushion period in case of delays
>         September 23 - 29: FINAL EVALUATION
>                 Ironing out bugs, cushion period in case of delays,
> submitting work for final evaluation

Looks better than the original timeline. OTOH the general remark (not 
necessarily mine, I just predict what the reviewers could say) to me more 
detailed remains. Perhaps you can mention class names, what would be working 
at the time, what will be NOT working yet...

>         I'm a big fan of open source, and KDE in general. I've used Amarok
>         a lot and not only read the source,

This sounds like you don't use it anymore. perhaps "I've been using ..."?

>         I am also familiar with Qt framework and have used it to prototype
>         several small applications. I'm an early adopter, and my fiddling
>         with Qt 5 beta has led me to uncover an important bug [4].

Nice.

>         I'm very skilled in C++. Other than that I program mainly in Scala,
>         and Java when required (and my university requires that a lot).
>         I'm experienced with test driven development, continuous
>         integration, code reviews and other agile techniques.

Do you have some experience with git and/or other VCSes? (self-assessment: 
would you be able to explain difference between `git rebase` and `git merge`?) 
But don't actually explain it in the proposal. :-)

>         If my application is accepted, I am going to commit my full time to
>         complete the project, including putting my job on hold for the
>         duration of GSoC. I expect to be working 40 hours a week.

Nice. What are your plans after the GSoC has ended? Do they involve Amarok in 
any way?

[the questions above are rather hints for the proposal than something I'd like 
immediately answered]

This starts to look really promising,
		Matěj