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

> 3 nested namespaces, can't we reduce that?

It's not my personal preference either, but more important is to keep consistency between the apis. As lp:unity-api has been defined with this namespace structure, we need to adopt it. Note that you don't need to use them in the actual implementation. Just "using namespace unity::shell:application" once in your header should be enough to not have to care about it all any more. Check out the launcher implementation in lp:unity8 plugins/Unity/Launcher/.

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

Done.

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

Done.

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

Done.

>
> 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" ?

Added Suspended and Stopped. I don't think "Wants Attention" or the like belongs in here. Just because an app wants attention it is still one of the other states (e.g. running). I'd probably even create a separate boolean property for "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?

Done. This matches the Launcher api now very closely :)

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

added.

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

done

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

done

>
>
> +Q_SIGNALS:
> 415 + // TODO: needed?
> 416 + void mainStageFocusedApplicationChanged();
> 417 + void sideStageFocusedApplicationChanged();
>
> Can combine these into one focusedApplicationChanged() signal

done.

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

done.