Code review comment for lp:~gerboland/unity-api/appManVersion2

Revision history for this message
Gerry Boland (gerboland) wrote :

> Here's a couple of comments of things we should think of. Not necessarily
> disagreeing with them, just worth a though/discussion imo:
Sure. I'm just making up stuff as I go along

> 20 + Q_PROPERTY(Stages supportedStages READ supportedStages NOTIFY
> supportedStagesChanged)
> Do we assume that if an app claims support for both stages, all of its
> surfaces will support them? Probably yes.
Yes. This is property of an application. If it claims to support a stage, then its surface can be resized to suit that stage, and it can't stop that. One app can't have surfaces on both stage either. Note also that "stage" is a property shell will have to save when the app is closed, so user preference is retained.

> 31 + * Holds the current application focus state. True if a surface of
> this application is focused, false otherwise.
> Really needed? what would we gain with this information? Wouldn't it make more
> sense to obtain it the other was round? For instance:
I kept this as I thought it would make the job of the launcher easier. I can remove if you don't think it's worth it.

> 78 + Q_INVOKABLE virtual bool suspend() = 0;
> 89 + Q_INVOKABLE virtual bool resume() = 0;
>
> Where would you call this from? I'm not sure if we should expose this to unity
> but rather handle this stuff internally in the applicationmanager. We might
> add something like inhibitSuspension() at a later point if we want to give the
> user control over it.
>
> Also it doesn't really match with the existing API, given that we have
> startApplication() and stopApplication() in ApplicationManager (not
> ApplicationInfo) such things should be at the same place. If we decide to
> expose this to the shell, I'm not opposed to have them in both places
> (ApplicationManager::suspendApplication(appId) and
> ApplicationInfo::suspend()).

Not a bad point. I thought it a good idea to have shell decide when to suspend/resume apps, as only shell really knows what is visible or not, and shell implements the policy (on phone/tablet, all non-visible apps suspended, on desktop can be more flexible, maybe apps can opt-in to being suspended). I've been trying to make AppMan device independent, because I fear having some policy in AppMan, and some in shell, will end up with conflicts.

Another way this could be done by shell setting the surface visible flags, and if all an app's surfaces are hidden, it can be suspended. But then we tie rendering control to lifecycle control, which I don't like.

You're right, the API is not consistent. Will look more.

> ====
>
> 301 void setFocused(bool focused);
>
> What would this do?
That was in the Mock before. I dunno why it's there. I can remove

« Back to merge proposal