Code review comment for lp:~unity-team/unity8/launcher-backend

Michael Zanetti (mzanetti) wrote :

On Monday 08 July 2013 17:02:59 you wrote:
> W dniu 08.07.2013 16:11, Michael Zanetti pisze:
> >> 205 +LauncherBackend::~LauncherBackend()
> >> 206 +{
> >> 207 +
> >> 208 +}
> >>
> >> Is it just a template thing, or do we want empty destructors as a rule?
> >
> > It's mostly just a template thing... Can remove it if you want, but I'm
> > quite sure Antti will need it again.
> OK.
> >> Hmm hmm... Why doesn't LauncherBackend have an API definition in
> >> lp:unity-api? Or is it just a stop-gap? Same for QuikcListEntry? Or is
> >> it even just a thing that the shell won't ever see?
> >
> > Exactly... Thats the idea. Both are an implementation detail of me and
> > Antti but never show up in the public QML api.
> Yeah, but then... If we wanted to mock one or the other... Will all of
> that later get away into a separate "unity-launcher" project for the
> backend?

yes. Thats how I understand it... basically the complete folder
plugins/Unity/Launcher moves away to a different projects.

In there we can mock it for unit tests, but so far I'm following the idea of
pointing the backend to a directory with some desktopFile entries (instead of
/usr/share/application/) so we would test the whole thing at once. Anyways,
thats not totally planned through yet.

From a shell POV there's no need to mock the backend only because we will mock
the laucher plugin at the API. Because the backend is not exposed through the
api we wouldn't mock it.

> >> 355 + QStringList storedApplications() const;
> >>
> >> Shouldn't this be "pinnedApplications"?
> >
> > No, because this does not only contain pinned apps. It also contains
> > recent ones. So if you look at it from the backend perspective, this is
> > whant needs to be stored into the settings. The decision if its pinned,
> > running, recent, whatever happens one layer above in LauncherModel.
> Why should recent be stored? Info about recent apps should come from
> Zeitgeist, and be volatile, no?

Ah... hmm... didn't know the zeitgeist thing. In that case I would rename it
to pinnedApps indeed and have a second method for recent ones. However, this
is really stuff where Antti needs to jump in and it will evolve as he starts
working on it.

> >> 401 + //TODO: Fill in getters for all the other things needed from
> >> the
> >>
> >> .desktop file.
> >>
> >> I'm not sure that's needed, we should have a generic "AppData" component
> >> or
> >> similar that would take an appId and expose all the data read from
> >> .desktop
> >> and potentially other places.
> >
> > Yeah... Not really sure how the desktop file parser will work yet. In
> > general I would leave that to Antti to finish up the backend API as he
> > sees fit.
> Let's just make sure we don't need to learn different APIs wherever
> we're looking at an app.

Note that the Backend is an internal class and will not be exposed to anybody
else in form of an API. The only reason I commented it like an API is because
its the "internal interface" between me and Antti and I wanted to make it as
easy as possible for him to get started.

« Back to merge proposal