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

Revision history for this message
Michael Zanetti (mzanetti) wrote :

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

« Back to merge proposal