Introducing Homerun

[Aaron J. Seigo]

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.

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

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

there is zero reason why we could not formalize the QueryMatch::id() structure 
to do exactly the same thing.

in fact ... i think we already have with the QMimeData which gives you easy 
access to URLs. checking whether something is an executable or not is trivial 
to add to that. here is an example of a runner that supports mimedata:

QMimeData * ServiceRunner::mimeDataForMatch(const Plasma::QueryMatch *match)
{
    KService::Ptr service = KService::serviceByStorageId(match-
>data().toString());
    if (service) {
        QMimeData * result = new QMimeData();
        QList<QUrl> urls;
        urls << KUrl(service->entryPath());
        kDebug() << urls;
        result->setUrls(urls);
        return result;
    }

    return 0;
}

it is possible that it is not efficient enough to use mimeDataForMatch in the 
models (too much memory usage; too much CPU burn for something rarely used) 
and if that turns out to be the case (i don't think it would be, but who 
knows, right?) then we can address that through creative problem solving. 
together.

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.

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?

> Can you be more specific about which parts you are interested in?

whatever parts we talk about :) it's inneficient (and therefore annoying) to 
try and find the code you write about when you don't provide grep'able hints.

thanks for providing them in this email.

-- 
Aaron J. Seigo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20121114/ecd81434/attachment.sig>