GSoC Application Review

[Matěj Laitl]

On 30. 4. 2013 Anmol Ahuja wrote:
> And for classes like CollectionLocation, should I create wrappers like
> CollectionLocationTypeExporter or simple modify the existing class,
> changing script evokable functions to public slots/ Q_INVOKABLE? I think
> the former would help keep the scripting logic in one place.

Yeah, the wrappers like the MetaTypeExporter (which should be renamed to 
TrackExporter by the way) should be preferred.

> Short description : My proposal aims at revamping the Amarok scripting
> interface, and adding an intuitive Script Creator graphical application
> allowing easy script creation by non-coders and coders alike.

Please update this to reflect our Saturday IRC discussion - i.e. removal of the 
Script Creator from the project.

> This involves
> getting rid of dangerous and obsolete methods, and adding accessors for all
> the new features added to Amarok over the years. Every major type will have
> a QtScript object representation, allowing unparalleled scripting
> flexibility. I also plan on writing unit tests to allow automated testing
> of the Amarok scripting API. The third part involves documentation of the
> entire scripting API in-source and automating the generation of Amarok
> Scripting API documentation from the in-source documentation. All of this
> will lead to a cleaner and more feature rich well-documented scripting API
> hopefully generating new interest in Amarok script.

Yeah, this all sounds nice. I'd say "scripting" in the last sentence. And you 
may want to work on documentation generation right from the start so that you 
can check generated documentation for classes you just create.

There may be some problems with the generation (because you are documenting in 
C++, while the target code is Javascript - but I believe that with some help 
from doxygen people, docs (and perhaps the guy who wrote qstestlib) and some 
work-arounds this will be possible. Feel free to rename class names to match 
their names in the scripting API, i.e. I don't understand why 
AmarokScript::AmarokEngineScript cannot be called just AmarokScript::Engine)

> Other features I plan
> to work upon, if time permits, are allowing users to terminate long running
> or stuck scripts instead of forcing a restart, adding GUI handling to the
> script creation utility and porting popular Amarok 1.4 scripts, if
> possible, to the newer QtScript based interface.

Right, although I'd remove the "porting popular A 1.4 scripts" - there are 
more useful things you'll be able to do, like promoting Amarok scripting, 
getting feedback on scripting interface from existing script authors etc.

> Freenode IRC Nick : darthcodus

"DarthCodus"? (Linux is case-sensitive, hehe ;) )

> IM Service and Username :  Gtalk, darthcodus

darthcodus at gmail.com?

> Motivation for Proposal:
> 
> Amarok has a thriving scripting community, but the existing scripting API
> is:
> <------- shortened ---------->

Yes, right, very nice research! Kudos for including bug links.

> Project Goals:
> 
> My proposal involves four main parts :
>    -    Part I : (...)
>       Remove AmarokCollectionScript and re-implement as QScriptObject

Hmm, that may end up being tricky. I propose you have this as a soft goal at 
the very end.

>       Remove direct SQL access

Please note that all the current scripts must still work, so you cannot simply 
remove any method/class (yet). What you should do is to deprecate them in a 
very visible way, my idea would be to introduce a helper function that would 
emit following message Amarok::Compontents::logger->longMessage():

The script <script name> (homepage: <clickable link>) uses deprecated method 
<method name>. Please try to update to newest version of the script in Amarok 
Config -> Scripts -> Manage Scripts. If you are already using the newest 
version please contact the author at <author addres>. The mentioned method may 
be removed in next Amarok release. See Amarok debugging log for complete list 
of used deprecated methods.

^^^ such message would appear only once per script, until Amarok is restarted.

>       - CollectionLocation
>       - Transcoding

Yeah, transcoding may end up just as some parameter of a method in 
CollectionLocation, although it may be useful

>       QueryMaker

Yeah, this is much needed, although the Javascript API may be simplified 
compared to the C++ API. I thought about using the textual query parser from 
the Collection Browser so that the script authors may use something they are 
familiar with and can test easily.

>       Implement Equalizer support in the AmarokEngineScript

See related https://git.reviewboard.kde.org/r/110036/ (on the other hand this 
may require some kind of EqualizerControlelr on the C++ side - maybe make this 
a soft goal)

>       - Synchronization Job

Do you mean statistics synchronization? Perhaps even more useful would be to 
expose "Track matching job" is some way.

>       Others
>             Add suspend inhibition to AmarokScript

Hmmm, I don't think we should support this - the inhibition is pretty much 
defined by the playback.

>    Add other requested features/ wishes and fix bugs

Nice research, although some users may request functionality that we simply 
don't want to expose.

>       Bug 213376 <https://bugs.kde.org/show_bug.cgi?id=213376> - Implement
>       setter for Amarok.Engine.currentTrack().imageURL

Isn't this already done by MetaTypeExporter::setImageUrl()?

>       - Bug 183497 <https://bugs.kde.org/show_bug.cgi?id=176663> -
>       Scripting API to add a context menu to everything representing a song
> or file
>       - Bug 187957 <https://bugs.kde.org/show_bug.cgi?id=187957> - Create
>       submenus from the Scripting interface
>       - Bug 176663 <https://bugs.kde.org/show_bug.cgi?id=176663> -
>       Installing a new script requires a restart of amarok
>       - Bug 23271 <https://bugs.kde.org/show_bug.cgi?id=232711>: - Provide
>       scripting interface to customize collection display (filter, sort,
> group, displayed tags, etc.)
>          - Bug 205509 <https://bugs.kde.org/show_bug.cgi?id=205509> : Add
>          dbus functions to update podcasts and download podcast tracks

These are for the D-Bus API instead, but yes, perhaps the reported would be 
happy with the Javascript API.

>    Add new scripts demonstrating the new scripting API ( and adding cool
>    new features )
>    -  A generic full-fledged synchronization framework like iTunes written
>       in Amarok Script for media devices
>       - Link to Google Music Search for currently playing artist (also Bug
>       136502 <https://bugs.kde.org/show_bug.cgi?id=136502>)
>       Automatic adjust equalizer according to genre (also Bug
> 129374<https://bugs.kde.org/show_bug.cgi?id=129374>
>       )

Please make this a soft-goal with rather low priority.

>    Part II : Design a graphical Script Creator utility
> 
> Create a user-friendly script creation GUI allowing everybody, coders and
> non-coders alike to write scripts for Amarok. The utility will be generic
> enough to allow easy porting to other KDE apps.

Please remove this from the project. You might want to make existing Amarok 
Scripting Console more useful and convenient, but no brand new GUI app please.

>    Part III : Automated Testing Scripts
> 
> Write scripts for unit-testing the scripting interface. Look into existing
> solutions like QSTestLib (
> http://blog.qt.digia.com/blog/2007/11/05/unit-testing-with-qt-script/ ) to
> see if they’re viable for

unfinished sentence?

>    Part IV: Automating Documentation
>    - Document any undocumented scripting API code

I don't think we have a documented scripting API class/method.

>    - Use Doxygen for generating the wiki Scripting API Documentation content
>    from in source documentation. [Or should I write my own scripts for
> that?]

No, Doxygen was the intent, altough you may have to fiddle with it a bit so 
that it produces desired results.


> Implementation Details //incomplete, is all this really needed?

Not really, this is too detailed and you already provided some details in the 
Project Goals section. Maybe mention just some interesting changes/removals, 
certainly not lists of all methods.

Instead of this, it may be really helpful if you provided a brief outline how 
general QtScript - Javascript - C++ interactions works, what QtScript classes 
are you likely to be using, what is the role of QObject, what are the memory 
management options etc. - so that we see that you understand the principles 
and can have confidence in you.

> *Enums will be handled as strings in Amarok Scipt, converted to and fro
> using static enum to string maps*

^^ Is this the best practice when creating QtScript bindings? How does 
qtscriptgenerator (you should check this project, it is used to generate 
bindings for actual Qt classes - I don't mean directly use it, but at least 
see how it converts C++ things to Javascript)

> 1. AmarokScript
> 
> public signals:
> configured  (I’m actually uncertain when it’s supposed to be emitted, when
> the Amarok configuration changes? )

Yes, but it isn't connected to anything recently. Perhaps remove it for now, a 
signal can be always added should the need arise.

> 2. AmarokEngineScript
> stopped(); paused(); //needed? can be inferred from playbackStateChanged();

I'd preserve these for convenience.

> 3. AmarokEqualizerScript

Needs some more thinking, no need to do it in the proposal.

> 5. AmarokScriptableServiceScript
> 
> StreamItem and ScriptableServiceScript in same header file, not in
> AmarokScript namespace where they should be

Right. (better wording would be to say how you'll fix it (move to bla bla bla) 
rather than what the current problem is)

> 7. Handling collections, copying between collections ,transcoding and
> querying
> 
> Implement in a more object oriented way.

Right.

> [ global CollectionManager object - Amarok.CollectionManager ]

Right.

>  public:
>         enum Type
>         {
>             UmsCollection,
>             IpodCollection,
>             (...)

No no no, this is artificial and no such enum actually exists in C++ code. 
Instead, there should be just a method to get a list of collections (please 
hide the distinction between viewable and whatever else, just return the 
"active" collections).


> b. class CollectionTypeExporter

Right.

> c. TranscodingConfigurationTypeExporter

Perhaps this won't be needed? I don't know, maybe postpone this until we get 
an idea whether yes or no.

>  d. CollectionLocationTypeExporter

Right.

> prepareCopy // accept TranscodingConfiguration Object
> prepareMove
> prepareRemove

I think only the three methods above should be exported by CollectionLocation. 
You may or may not support specifying transconding config - we already have GUI 
for user to select it and to remember it - I don't see now why a script would 
want to override it.

> Tentative Timeline:
> 
> May 27 - May 30 : Familiarize myself with members of the Amarok team.
> Discuss proposal with team-members and fine-tune implementation details
> based on the feedback.
> 
> May 31 - June 5 : Finalize all implementation details and the work plan,
> resolving any ambiguities.
> 
> June 6 - June 17 : Unavailable due to my exams
> 
> <-----Coding Period Starts----->
> 
> June 17 - June 23 : Work on cleaning up existing API,  implement
> CollectionManager and write QtScriptObject accessor for for Collections and
> CollectionLocations
> 
> June 24 - June 30 : Implement file copying and moving among
> CollectionLocations, with script configurable transcoding support
> 
> June 31  - July 6  :  Write QtScriptObject for Querymaker and Bookmarks.
> Add accessor for QueryMaker to Collections object
> 
> July 7  - July 13 : Podcast Management. Synchronization Jobs.
> 
> July 14  - July 20 :  Dynamic playlists and automatic playlist generators
> 
> July 21 - July 27 : Other requested features and bug-fixes
> 
> July 27 - July 31 : Other features, like suspend inhibition.
> 
> <--------August 2----------> Midterm Evaluations Deadline
> 
> August 3  - August 9 : Write the new scripts
> 
> August 10 - August 16 : Design a UI for Amarok Script Creator
> 
> August 17  - August 23 : Write code for Script Creator
> 
> August 24  - August 30 : Test script creator, add new features based on
> community feedback

Hmm, what I'd like most is to have the 4 weeks above completely removed - the 
other things will take more time, trust me. ;) Also see below.

> <----- All components of the scripting interface ready at this point----->
> 
> August 31  -  September 9 : Write unit tests for the Amarok scripting
> interface
> 
> September 10 - September 16 : Write missing documentation, set up automatic
> documentation generation

Hmm, I'd like to see setting up automatic doc generation as a very first task, 
not last. I'd also like to see that writing documentation for existing methods 
should be a part of the initial cleanup and I think you should devote more 
time for it (perhaps 2 weeks for the cleanup + documentation).

I also recommend writing the unit-tests right when some API class/method is 
finished, not in batch at the end. Writing documentation right with the 
implementation should be automatic. Because of that, I think it will take more 
time to get a new class API finished that you expect in above timeline.

> <-----September 16-----> Suggested Pencils Down
> 
> September 17   - September 23  : (Buffer Time)  Finish any pending tasks,
> implement miscellaneous features (GUI handling in Script Creator, abortion
> of long running scripts, porting Amarok 1.4 scripts)
> 
> <----- September 23----->Firm Pencils Down
> 
> September 23 and on - tweaking, bug fixing, polishing interface based on
> community feedback
> 
> About Me:
> 
> I’m Anmol, a 20 year old engineering student studying Computer Science and
> Engineering in New Delhi, India.
> 
> I  was introduced to KDE by my college senior Rishab (spacetime) as a
> solution to my session management problem, and I was hooked. Being an avid
> music fan, I was naturally drawn to Amarok as my music player of choice,
> and eventually the first  open source project I decided to contribute to.
> 
> The Amarok community has been very welcoming, especially Matěj
> (strohel)and Myriam (Mamarok),
> guiding me through my open source journey, and it’s been a lot of fun
>  working with them. Even more exciting has been adding features from my
> wishlist to Amarok myself and watching them being accepted upstream,
> knowing they’ll eventually reach the entire Amarok user-base [ waiting for
> 2.8 ;) ].
> 
> I am a rapid learner and a proficient C++ developer, it being the first
> language I started coding in four years ago in high school. I have worked
> in several other languages over the years, including Python, Java,
> Javascript and Dart . I have made some projects in college, and an Android
> app B’Wished
> <https://play.google.com/store/apps/details?id=com.anm.bwished>(now
> defunct), and have been an active member of the Amarok community.
> 
> After GSoC, I will continue contributing to Amarok as I have been, and
> hopefully contribute to some other KDE software too!

The whole About me section looks nice.

> My contributions to Amarok include the following patches:
>
> (...)

Impressive!

The proposal looks rather well, please take the GUI Scripting Creator out of 
the proposal (remember, you can work on anything you like after the GSoC ;) ), 
provide the requested overview of how QtScript generally works, dramatically 
shorten the Implementation Details section, resolve other minor remarks and 
this will be a really good one.

Regards,
		Matěj