GSoC Application Review

[Anmol Ahuja]

It wasn't supposed to be private, that was accidental.
Resending it to the amarok-devel mailing list.

Thanks for the thorough review :)


On Tue, Apr 30, 2013 at 3:56 AM, Matěj Laitl <matej at laitl.cz> wrote:

> 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.
>

Okay.


> > 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.
>

Okay.


> > 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)
>

Okay, starting with the documentation generation first.


> > 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,
>

I'm not certain about how I would promote Amarok scripting.

getting feedback on scripting interface from existing script authors etc.
>

I am interested in knowing what the Amarok scripting community wants. Can I
find Amarok scripters on #amarok? Can I set up an Amarok Script feature
request poll on the Amarok website?


> > Freenode IRC Nick : darthcodus
>
> "DarthCodus"? (Linux is case-sensitive, hehe ;) )
>

Silly me :p


> > 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.


I'm not exactly doing that anymore. I'm following the outline in the
implementation details below, so it's manageable.


> >       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.
>
>
I actually did intend deprecation, though it was poorly put in the proposal
:)


> ^^^ 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
>

I can see uses for a transcoding controls in Amarok Script. For instance,
you can have a script that uploads any song you love on Last.FM to be
uploaded to your Google Music account, transcoded to MP3 if, say, it is
FLAC or Apple Lossless, not otherwise. This sort of stuff is not
configurable from the main interface, not yet at least.


> >       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.
>

Okay. I'll include a textual query parser in the next version of the
proposal.


> >       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)
>

It would actually require modifications on the C++ side, but that's
manageable enough.


> >       - Synchronization Job
>
> Do you mean statistics synchronization? Perhaps even more useful would be
> to
> expose "Track matching job" is some way.
>
>
Okay, I'll include Track matching job in the next revision. The
SynchronizationJob is for playlist synchronization and stuff,
though admittedly I'm not too familiar with that part of Amarok.

>       Others
> >             Add suspend inhibition to AmarokScript
>
> Hmmm, I don't think we should support this - the inhibition is pretty much
> defined by the playback.
>
>
This is to allow scripts to inhibit suspend, for example, when they are
downloading something. Or is that actually bad? Maybe we should have  a
popup show up in case a script is inhibiting suspend so as not to perplex
users?

>    Add other requested features/ wishes and fix bugs
>
> Nice research, although some users may request functionality that we simply
> don't want to expose.
>

Okay.


> >       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()?
>

It's not yet implemented.


> >       - 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.
>

I was actually planning on adding those to the D-BUS too. Should I keep it
out of my proposal?


> >    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.
>

Okay.


> >    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?
>

I'm sorry, I should've had a closer look at it before mailing it.


> >    Part IV: Automating Documentation
> >    - Document any undocumented scripting API code
>
> I don't think we have a documented scripting API class/method.


A few of them do have in source documentation.


> >    - 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.
>

Okay.


>
> > 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.
>

Okay. I'll also add a class diagram or two concisely representing this
stuff.


> 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.
>

Okay. I should've realized it after reading you review of Konrad's
proposal.


> > *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)
>

I didn't know I could directly grant Enums access with the QMetaObject
system! Thanks.

> 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.
>

Okay.


> > 2. AmarokEngineScript
> > stopped(); paused(); //needed? can be inferred from
> playbackStateChanged();
>
> I'd preserve these for convenience.
>

They don't already exist, I was wondering whether I should add them, since
they can be inferred from engineState(), which already exists (not
playbackStateChanged() signal as mentioned above, that's erroneous). I'll
add them as properties. (The implementation details only list properties
and signals/ slots that don't already exist)


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


Is this unnecessary? I can't think of much use for the scriptable equalizer
either, except for things like mood scripts and switching equalizer preset
on inserting headphones (headphone detection isn't in the Amarok code yet,
either, and I'm not even sure if it's possible with Solid). But there
was a feature
request <https://bugs.kde.org/show_bug.cgi?id=279701> for this, and it
seems pretty reasonable to expose it.




> > 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)
>

Okay.


> > 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).
>

Okay. I'll just use a type field so script authors can identify the type of
collection.


>
> > 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.


Justified above.
Okay.


> > <----- 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.
>

Okay.
Can I move Script Creator to the bottom of the priority list? I won't waste
too many lines of the proposal on it either.


> 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
>

I will be submitting a new version of the proposal soon.
Thanks again :)

--
Anmol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130430/1d00e255/attachment-0001.html>