Introducing Homerun

[Aurélien Gâteau]

Le mercredi 14 novembre 2012 16:31:19 Aaron J. Seigo a écrit :
> On Wednesday, November 14, 2012 15:05:09 Aurélien Gâteau wrote:
> > Le mercredi 14 novembre 2012 12:33:55 Aaron J. Seigo a écrit :
> > > On Wednesday, November 14, 2012 11:30:58 Aurélien Gâteau wrote:
> > > > Le mardi 13 novembre 2012 11:23:00 Aaron J. Seigo a écrit :
> > > > > krunnermodel, a Qt model that comes with a QML component plugin (so
> > > > > you
> > > > > can
> > > > > just import and use it directly) is in
> > > > > kde-runtime/plasma/declarativeimports
> > > > 
> > > > I know about this model. We used it in the beginning, I even
> > > > contributed
> > > > a
> > > > bit to it. Unfortunately it was not adapted for us because it made it
> > > > difficult to group results by runners.
> > > > Also, while it maps the QList<Plasma::QueryMatch> to a model, it does
> > > > not
> > > > bring full benefit of models from this: KRunnerModel has no way to
> > > > know
> > > > if
> > > > a runner added/modified or removed an item so it always has to refresh
> > > > the whole model (minus the hack I contributed to improve this in a
> > > > certain case).
> > > 
> > > if you need to be able to group by runners, add that to the existing
> > > implementation. how does the model in homerun facilitate grouping by
> > > runner?
> > 
> > In a scary way... you know how QML is not really handy to deal with tree
> > models: Homerun RunnerModel groups results by runner by exposing each
> > group
> > of runners as a submodel. Probably not the most elegant approach,
> > definitely not something I would like to publish in a stable API like
> > libplasma.
> 
> * RunnerModel is not in libplasma.

True, it is in kde-runtime, but that does not change anything: either it is a 
public API and you should not break it or it is a private API and Homerun 
should not use it.

> * this can probably be done with a filter model (filtering on the runner for
> a given match, e.g.)
> 
> * if this is not the case, it is quite likely that the useful bits of
> homerun's RunnerModel could be put into kde-runtime with a specialization or
> additional models that use that RunnerModel placed in homerun. that is
> almost certainly the worst case scenario.
> 
> * what is and is not suitable for inclusion as publicly usable API is
> usually a decision we make together as a team, not isolated individuals.

Here is the confusion. Homerun is a third-party application, build using 
Plasma components. It is not a part of the upcoming KDE SC 4.10 release.

You have to take into account that when Homerun was started, KDE SC 4.9 was 
being developed and the target platform for Homerun was kdelibs 4.8. Today, 
Homerun 0.1.x does work with kdelibs 4.8.

When you develop against a fixed platform, the only way to work-around problems 
with components in the platform is to fork the components, hoping to get your 
fixes integrated back into the next version of your target platform.

I dislike forking, and I do whatever is possible to ensure our fixes go back 
in. Homerun ships with copies of a few components and I made my best to 
document when and how to create a copy, document why the existing copies are 
there and track the integration of our fixes so that we know with which version 
of the platform we can drop them. See:

https://projects.kde.org/projects/playground/base/homerun/repository/entry/fixes/README.md?rev=homerun%2F0.1

On the topic of RunnerModel: Originally, Homerun did not group results by 
runner, so the Plasma RunnerModel fitted the bill. Homerun needs evolved and 
the Plasma RunnerModel was not adapted anymore. I could have tried to put 
another layer on top of it, but decided it was simpler to use KRunnerManager 
directly. I could have rewritten a class using KRunnerManager from scratch, 
but it felt more efficient to base Homerun RunnerModel on existing code.

> > > > The problem with QueryMatch::id() is that its format is
> > > > runner-specific.
> > > > This makes sense for KRunner, but not for Homerun because we want to
> > > > identify whether the match is an application or a place so that it can
> > > > be
> > > > stored in the appropriate favorite collection.
> > > 
> > > how is this accomplished in homerun?
> > 
> > For most Homerun sources: by exposing the favoriteId role, which follows
> > the format described in libhomerun doc (which you can build it with "make
> > dox"). For the Runner source: we have code for the "services" and
> > "locations" runners which extract information from QueryMatch::data() and
> > returns a favoriteId role.
> 
> here's some of the key snippets that deal with favoriteId:
> 
> static QString serviceIdFromFavoriteId(const QString &favoriteId)
> {
>     if (!favoriteId.startsWith("app:")) {
>         kWarning() << "Wrong favoriteId" << favoriteId;
>         return QString();
>     }
>     return favoriteId.mid(4);
> }
> 
> QString AppNode::favoriteId() const
> {
>     return QString("app:") + m_service->storageId();
> }
> 
>     function favoriteModelForFavoriteId(favoriteId) {
>         if (favoriteId === undefined || favoriteId === "") {
>             return null;
>         }
>         var lst = favoriteId.split(":");
>         if (lst.length === 0) {
>             return null;
>         }
>         var model = favoriteModels[lst[0]];
>         if (model === undefined) {
>             console.log("favoriteModelForFavoriteId(): No favorite model for
> favoriteId '" + favoriteId + "'");
>             return null;
>         } else {
>             return model;
>         }
>     }
> 
>     } else if (role == FavoriteIdRole) {
>         QString runnerId = match.runner()->id();
>         if (runnerId == "services") {
>             return QVariant("app:" + match.data().toString());
>         } else if (runnerId == "locations") {
>             KUrl url(match.data().toString());
>             return QVariant("place:" + url.url());
>         } else {
>             return QString();
>         }
> 
> so this is just as ad-hoc as with runners. what is different is that you've
> defined some prefixes, such as "app:". (i noticed that this prefix is
> hardcoded all throughout the code base ... i would recommend putting these
> magic strings behind API somewhere.)

Actually that last snippet is from the code dealing with runners, so it is 
indeed ad-hoc. What is different is libhomerun defines a format for such ids. It 
can (and should) be done for runner ids as well, but again this was not ready 
in kdelibs 4.8.

(oh, and I agree it makes more sense to have the magic strings moved to 
libhomerun)

> what pains me is that you've spent a lot of effort on this (that much is
> obvious from looking at the code), probably without reason to. if you had
> simply come by and said: "i need something that i can easily parse into an
> address for favorites" we could have made this happen together quickly.

This would have tied Homerun to kdelibs 4.10, which is not the target platform 
of Homerun 0.1.x.

> maybe it is just me, but inneficiency due to people not communicating is
> extremely frustrating. we don't have 100s of developers whose time we can
> waste without consequence. we also won't be able to build a coherent user
> experience if everyone develops in their own quiet little cave forking
> whatever doesn't work (to their knowledge) in quite the way they want. we
> also won't succeed if instead of asking questions of each other we just
> quietly assume things such as "RunnerModel is public API and in libplasma"
> :(
> 
> would it be possible to agree to fix this situation by increasing our
> communication with each other?

I am quite reluctent to bring topics more complex than simple bug fixes for 
discussion with your community. I had bad experiences trying to influence 
Plasma development in the past. At that point I told myself there were other 
areas within KDE where my contributions would be more welcomed and I have 
since been keeping myself busy as far away from Plasma as possible.

Unexpectedly, a combination of work-related projects (LightDM KDE some time 
ago, now Homerun) brought me back with you folks, time will tell how this 
will work out.

Aurélien