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.

=====

 123 + ${CMAKE_CURRENT_BINARY_DIR}
 124 + ${LAUNCHER_API_INCLUDEDIR}

Is this required?

=====

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

=====

Please put Authors below copyright.

=====

 205 +LauncherBackend::~LauncherBackend()
 206 +{
 207 +
 208 +}

Is it just a template thing, or do we want empty destructors as a rule?

=====

 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.

=====

 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;

=====

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?

=====

 355 + QStringList storedApplications() const;

Shouldn't this be "pinnedApplications"?

=====

 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.

=====

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

=====

More to come.