Homerun and single-runner runners

[Aurélien Gâteau]

Le Wednesday 13 February 2013 15:02:33 Aaron J. Seigo a écrit :
> > 2. The way single-runners are queried is not very elegant. It looks like
> 
> > this:
> this probably should be provided by RunnerManager and RunnerSyntax. sth
> like:
> 
> QString RunnerSyntax::asQuery(const QString &userInput = QString()) const;
> bool RunnerSyntax::hasPlaceholder() const;

Makes sense.

> then there is the issue of AbstractRunners with more than one syntax ..
> which one to use? in your code the first one is used, and that will be
> "correct" in most cases .. but not when there are multiple search terms.

Right, I forgot to mention this. I was actually considering the possibility of 
letting the user choose the syntax if there is more than one, but that may be 
a bit over-the-top.

> looking at the current runners that provide multiple search terms, it is
> apparent that:
> 
> * having more than one syntax does not mean they are all useful in single
> mode * the first syntax is not always the most useful one
> * multiple syntaxes may in theory be useful to be run as default, though it
> seems no runner actually has such a set of syntaxes from what i can see
> 
> we could extend RunnerSyntax so that it has a SyntaxType flag, e.g.:
> 
> 	{ NoSyntaxType = 0, DefaultSyntax = 1, AutoQuerySyntax = 2}
> 
> setting DefaultSyntax would flag that syntax to be used in single runner
> mode, no query. AutoQuerySyntax would be used in single runner mode +
> query. that ought to cover all use cases?
> 
> and then RunnerManager::launchQuery could take all this into consideration,
> which would give this feature "for free" to all users of runners
> 
> thoughts?

Sounds good.
> 
> that's all libplasma2 stuff, though. for now, probably "rolling it yourself"
> in the model is the only way for 4.x.
> 
> > QString SingleRunnerModel::prepareSearchTerm(const QString &term)
> > {
> > 
> >     const char *placeHolder = ":q:";
> >     
> >     Q_ASSERT(m_manager);
> >     Plasma::RunnerSyntax *syntax =
> >     
> > 				m_manager->singleModeRunner()->defaultSyntax();
> 
> perhaps another assert here on m_manager->singleModeRunner()?

I do not check this because when I create the manager, if singleModeRunner() 
returns NULL I delete the manager and basically disable the model.

> >     QStringList queries = syntax->exampleQueries();
> >     Q_ASSERT(!queries.isEmpty());
> >     QString query = queries.first();
> 
> the assert will not stop a crash in production builds.

RunnerSyntax constructor takes an example query as parameter so there should 
always be at least one query, right?

> >     if (query.contains(placeHolder)) {
> >     
> >         return query.replace(placeHolder, term);
> >     
> >     } else {
> >     
> >         return query + ' ' + term;
> 
> this *should* fail in most cases as if the query is allowed to be followed
> by a term is *should* have a :q: there .. which means that if there is no
> :q: then there is not supposed to be a term :) the proper thing here is to
> just return the term.

OK, will fix. I think I bumped into runners which did not have the ":q:" 
placeholder but did support querying. I can't remember which though, but the 
proper fix is to add the placeholder to them.

I need to rework my code a bit then: If the Places single-runner is present in 
a Homerun tab, user will expect typing in the search field to filter the place 
list so that only places matching the typed criteria are listed.
Homerun provides a default filter for source models which do not implement 
filtering themselves: a source model announces it supports filtering by 
exposing the "query" property. The SingleRunner model exposes a "query" 
property so the search criteria can be passed to launchQuery (after going 
through the method I quoted). I would need to expose or not the "query" 
property depending on whether there is a placeholder in the runner syntax. 
Unfortunately, it is not possible (as far as I know) to disable a static Qt 
property at runtime :/

> for example: this won't work with the places runner as it would result in
> "places <term>" when it should just be "<term>"
> 
> >     }
> > 
> > }
> > 
> > 3. I noticed some runners advertise single-runner mode but do not define a
> > default syntax. Is it a bug or is there a reason for that?
> 
> depends on the runner. if there is a sensible set of results to return
> without query, then there should be a default syntax. if there is not, then
> there shouldn't be one.

I answered Marco regarding this.

Aurélien