Review Request: Plasma::AbstractRunner::Syntax

[Kevin Ottens]

> On 2009-03-24 06:24:42, Kevin Ottens wrote:
> > trunk/KDE/kdelibs/plasma/abstractrunner.h, line 83
> > <http://reviewboard.kde.org/r/372/diff/2/?file=3421#file3421line83>
> >
> >     Why not a default ctor instead? You've accessors anyway and it's mainly a "data store" class.
> 
>  wrote:
>     a Syntax without an example query and a description is meaningless; this prevents having meaningless items.

OK, I see what you're trying to enforce. Why not, I generally prefer allowing for a "null" or "invalid" value but that's prolly personal preference at this point.

Oh, and really no idea why, but it'd feel more natural to me to have the description coming first in the ctor. Can't find the reason,
but felt like pointing it out anyway.


> On 2009-03-24 06:24:42, Kevin Ottens wrote:
> > trunk/KDE/kdelibs/plasma/abstractrunner.h, line 87
> > <http://reviewboard.kde.org/r/372/diff/2/?file=3421#file3421line87>
> >
> >     I'd go for a setExampleQueries(const QStringList &queries) instead to make it symmetrical. Otherwise one would wonder how to reset the query list, etc. You'd end up having the most common QList services in the Syntax API.
> 
>  wrote:
>     which conflicts with the constructor that takes an example query. the common usage is "one search term, one meaning", but sometimes you want to add synonyms. it's called addExampleQuery, but maybe it should be addQuerySynonym? ;)

Nope, keeping addExampleQuery as name should be fine I think.


> On 2009-03-24 06:24:42, Kevin Ottens wrote:
> > trunk/KDE/kdelibs/plasma/abstractrunner.h, line 373
> > <http://reviewboard.kde.org/r/372/diff/2/?file=3421#file3421line373>
> >
> >     Again I'd go for a setter for QList<Syntax> instead of (add|clear)Syntax() methods.
> 
>  wrote:
>     clearSyntaxes() could be nicely replaced by a setSyntaxes(QList<Syntax>) ... however given that syntaxes may not be added all at once, and that without a simple addSyntax, we'll end up with more code of the form:
>     
>     QList<Syntax> syns = syntaxes();
>     syns << Syntax("foo", "bar");
>     ..
>     setSyntaxes(syns);
>     
>     instead of just:
>     
>     addSyntax(Syntax("foo", "bar"));
>     
>     so i'd like to keep addSyntax as that keeps the runners simpler, but add a setSyntaxes and removing clearSyntaxes does make sense ... thoughts?

Yeah you could keep addSyntax() as a convenience. But having a symmetrical pair of getter/setter is worth the removal of clearSyntaxes().

To also provide choice:
An alternative to addSyntax() would be a non const version of syntaxes(). addSyntaxes() calls would then become:
  syntaxes() << Syntax("foo", "bar");


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/372/#review585
-----------------------------------------------------------


On 2009-03-20 13:13:37, Aaron Seigo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/372/
> -----------------------------------------------------------
> 
> (Updated 2009-03-20 13:13:37)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Allows runners to register their syntax so that it can be displayed to the user, and perhaps even used in the future to automate the process of deciding whether a runner should actually be activated or not.
> 
> :q: is used to mean "the search term" in the texts.
> 
> A class for Syntax was added rather than a more simple addSyntax("example query", "description") API in AbstractRunner so that we can extend what is associated with Syntax in future versions if we wish. It also opens the door for runners to subclass Syntax and have the syntax objects update dynamically based on settings or environment.
> 
> apidox are missing.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/plasma/abstractrunner.h 938702 
>   trunk/KDE/kdelibs/plasma/abstractrunner.cpp 938702 
>   trunk/KDE/kdelibs/plasma/runnermanager.cpp 938756 
> 
> Diff: http://reviewboard.kde.org/r/372/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Aaron
> 
>