Code review comment for lp:~mzanetti/unity-api/application-api

This is amazing, thanks Michael!! My comments:

3 nested namespaces, can't we reduce that?

+ ApplicationInfoInterface

First off I think we can be ruthless and get rid of deprecated stuff.

Launcher doesn't care if application is fullscreen, and technically a surface is fullscreen, not an application. Please leave that out until I decide what to do there. Also screenshots are made available by an image loader, with URI based on the appId - so that 'screenshot' API is going away

So I'd propose simplifying this interface to include only those things we both need to share: appId, name, comment, icon, stage, state, focused.

Next I think these properties should be read only. I expect to implement this interface by adding a constructor which takes app_id and then sets these properties from the correct desktop file. I don't see these needing to be set by shell. But if the desktop file changes, want to notify shell of those property changes.

Q_PROPERTY(QString icon READ icon WRITE setIcon NOTIFY iconChanged)
QUrl better? Will this will return things like file:///usr/share/icons/whatever.png or gicon:///network-enabled, if so I'd like to use QUrl.

States enum will also need "Suspended", "Stopped" for process management (Stopped means process killed, but its state was written to disk, so can be respawned and resume itself). Maybe also desktop-y things like "Wants Attention" ?

+ ApplicationListModelInterface

The only reason ApplicationListModelInterface was useful was for when we had 2 separate lists for main stage and side stage applications. But we agreed it is best to combine these into one master list. So is it worth having this class at all, we could instead making the ApplicationManager class an QAbstractListModel?

Pet peeve: QAbstractListModel doesn't have a "count" property, unlike javascript models. It would be nice to add it :)

+ ApplicationManagerInterface

+ Application* startProcess(QString desktopFile, ExecFlags flags, QStringList arguments)
your correct in your comment that this needs to change a lot. Yes it should use appId. Flags should go away too. Thus can remove that enum.

+ void unfocusCurrentApplication(StageHint stageHint);
I think this best replaced with unfocusCurrentApplication()

+Q_SIGNALS:
415 + // TODO: needed?
416 + void mainStageFocusedApplicationChanged();
417 + void sideStageFocusedApplicationChanged();

Can combine these into one focusedApplicationChanged() signal

418 + void focusRequested(FavoriteApplication favoriteApplication)
I believe upstart will handle this, so shell won't need to care. Can leave it out completely.