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

> 78 + unity8-private | unity8-launcher-impl,
> 79 + unity8-launcher-impl-0.2,
>
> 87 +Provides: unity-launcher-impl,
> 88 + unity-launcher-impl-0.2
>
>
> As mentioned on IRC: unity-launcher-impl, not unity8, and -0.2 should be what
> you set to VERSION in the unity-api MP.

Done

> =====
>
> 123 + ${CMAKE_CURRENT_BINARY_DIR}
> 124 + ${LAUNCHER_API_INCLUDEDIR}
>
> Is this required?

Not any more as the header files need to be added to sources anyways... => removed

> =====
>
> # copy qmldir file into build directory for shadow builds
> file(COPY "${CMAKE_CURRENT_SOURCE_DIR}/qmldir"
> DESTINATION ${CMAKE_CURRENT_BINARY_DIR}
> )
>
> See in Ubuntu.Gestures for a better approach to this (using a custom target
> depending on the files to update when changed - I'll later go through our
> current codebase and fix it across the board; we should also use
> ${CMAKE_COMMAND} -E copy... etc.)
>

Nice one, done.

> =====
>
> Please put Authors below copyright.

Hopefully now all the wrong ones are gone so I won't copy paste a wrong one again.

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

> =====
>
> 227 +QString LauncherBackend::displayName(const QString &appId) const
> 228 +{
> 229 + // TODO: get stuff from desktop files instead this hardcoded map
> 230 + QHash<QString, QString> map;
> 231 + map.insert("phone-app.desktop", "Phone");
> 232 + map.insert("camera-app.desktop", "Camera");
> 233 + map.insert("gallery-app.desktop", "Gallery");
> 234 + map.insert("facebook-webapp.desktop", "Facebook");
> 235 + map.insert("webbrowser-app.desktop", "Browser");
> 236 + map.insert("twitter-webapp.desktop", "Twitter");
> 237 + map.insert("gmail-webapp.desktop", "GMail");
> 238 + map.insert("ubuntu-weather-app.desktop", "Weather");
> 239 + map.insert("notes-app.desktop", "Notes");
> 240 + map.insert("ubuntu-calendar-app.desktop", "Calendar");
> 241 + return map.value(appId);
> 242 +}
> 243 +
> 244 +QString LauncherBackend::icon(const QString &appId) const
> 245 +{
> 246 + // TODO: get stuff from desktop files instead this hardcoded map
> 247 + QHash<QString, QString> map;
> 248 + map.insert("phone-app.desktop", "phone-app");
> 249 + map.insert("camera-app.desktop", "camera");
> 250 + map.insert("gallery-app.desktop", "gallery");
> 251 + map.insert("facebook-webapp.desktop", "facebook");
> 252 + map.insert("webbrowser-app.desktop", "browser");
> 253 + map.insert("twitter-webapp.desktop", "twitter");
> 254 + map.insert("gmail-webapp.desktop", "gmail");
> 255 + map.insert("ubuntu-weather-app.desktop", "weather");
> 256 + map.insert("notes-app.desktop", "notepad");
> 257 + map.insert("ubuntu-calendar-app.desktop", "calendar");
> 258 + return map.value(appId);
>
> I know that's temporary code, but making those static and only filling it once
> would save us some CPU cycles.

Done.

> =====
>
> 301 +void LauncherBackend::triggerQuickListAction(const QString &appId, const
> QString &quickListId)
>
> s/quickListId/actionId/ maybe?
>
> Hmm, from https://code.launchpad.net/~mzanetti/unity-api/launcher-api-
> pinning/+merge/173064
>
> 201 + Q_INVOKABLE virtual void quickListActionInvoked(const QString
> &appId, int quickListIndex) = 0;

Changed in both.

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

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

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

> =====
>
> One think we're missing is the Gettext domain to use to translate the strings.
> It could be considered a backend task, though.

yeah. I expect the backend to deliver translated app names so far. It would be quite easy to change it turns out to be a bad idea... But thats it for now and both, me and Antti where happy with it.