Review Request: Plasma::AbstractRunner::Syntax

[Aaron Seigo]

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

a Syntax without an example query and a description is meaningless; this prevents having meaningless items.


> On 2009-03-24 06:24:42, Kevin Ottens wrote:
> > trunk/KDE/kdelibs/plasma/abstractrunner.h, line 242
> > <http://reviewboard.kde.org/r/372/diff/2/?file=3421#file3421line242>
> >
> >     Probably requires a plural?

yes, already done this locally.


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

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


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

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?


- Aaron


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