Merge lp://staging/~gerboland/unity-api/appManVersion2 into lp://staging/unity-api

Here's a couple of comments of things we should think of. Not necessarily disagreeing with them, just worth a though/discussion imo:

====

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.

====

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:

var focusedApp = SurfaceManager.appId(SurfaceManager.focusedSurfaceId)

====

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

====

301 void setFocused(bool focused);

What would this do?

On 01.05.2014 15:11, Michael Zanetti wrote:
> ====
>
> 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.

When thinking about stages, we're only considering single-surface apps.
So yes.

> 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

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

Right... Works for me. Thanks for caring about the Launcher :)

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

You're right... Makes more sense in the shell. I was still thinking too much like the old approach. But now that focused is something that's only available in QML this obviously doesn't work the old way any more.

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

Yeah... doesn't sound much better than the above.

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

Jup... Again, I'm fine with having them in both. We need to keep startApplication() in AppMan which makes it natural to have stopApplication() there too, and with it suspend/resume. But I can see how it would be convenient to have that as an ApplicationInfo object's property in QML.

So I'd probably go for
ApplicationManager:
startApplication()
stopApplication()
suspendApplication()
resumeApplication()

and ApplicationInfo:
stop()
Q_PROPERTY(suspended)

We can even implement and test those in the abstract interface so there's little risk an implementation would get them out of sync.

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

So my comment/question might be irrelevant. But I could foresee that trusted sessions might create a case where an app could be open on mainstage but invoked for a second surface within the trust session on side stage (or vice versa). Would your assumptions/implementation preclude this ?

On 01.05.2014 20:30, kevin gunn wrote:
> So my comment/question might be irrelevant. But I could foresee that trusted sessions might create a case where an app could be open on mainstage but invoked for a second surface within the trust session on side stage (or vice versa). Would your assumptions/implementation preclude this ?

The second surface will not be an app, it will be just a surface, from a
separate process, it will be part of the original "invoking" app.

So no, everything's good here.

> Jup... Again, I'm fine with having them in both. We need to keep
> startApplication() in AppMan which makes it natural to have stopApplication()
> there too, and with it suspend/resume. But I can see how it would be
> convenient to have that as an ApplicationInfo object's property in QML.
>
> So I'd probably go for
> ApplicationManager:
> startApplication()
> stopApplication()
> suspendApplication()
> resumeApplication()

Ok, I can do those

> and ApplicationInfo:
> Q_PROPERTY(suspended)

This one I'm not so sure about. We already use 'state' to get the process state, so adding the "suspended" property duplicates info. I also dislike it as some apps are excluded from lifecycle, so shell setting myApp.suspended=true is just incorrect.

I prefer just using the first part of your proposal - with {suspend,resume}Application() returning bool, so that shell knows if asking an app to suspend was actually acted upon or not.

I don't think shell really needs to know if an app is lifecycle-exempt, but it would care if suspendApplication returns false. Main use-case I can think of is there's no need for shell to save a screenshot to disk for a lifecycle-exempt app's surface, as there's no way that app will be killed.

> stop()
I might leave this out for the time being, just for ease of implementation, if you don't mind.

Also maybe just putting the process control methods (start/stop/suspend/resume) in AppManager is enough for a start.

ack. works for me

The stages and State enums are not tested. Can you please add a test that checks for all the values, but also that Stages are flags and can be used as such in QML?

===

Now that Stages are flags, we need to define their values explicitly in the enum definition. Now we have 0 and 1. ORing them loses information about 0.

===

Otherwise I think we're good now in terms of the API itself.

I guess we should bump the package version too