QtMir

Code review comment for lp:~dandrader/qtmir/appRestart-lp1527737

appRestart-lp1527737
Merge into trunk

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-01-07:

On 07/01/2016 14:39, Gerry Boland wrote:
> Review: Needs Fixing
>
> === modified file 'src/modules/Unity/Application/application_manager.cpp'
> + // We interpret this as a focus request for a suspended app.
> + // Shell will have this app resumed if it complies with the focus request
> QObject::connect(controller, &TaskController::resumeRequested,
> - manager, &ApplicationManager::onResumeRequested);
> + manager, &ApplicationManager::onFocusRequested);
>
>
> The original purpose of the resumeRequested signal from UAL was to tell AppMan to un-suspend a suspended app, since AppMan used to manage that. But since UAL does suspend/resume now itself, this signal is worthless. Suspect we should remove completely.
>
> Does not the focusRequested signal always get fired if url-dispatcher wants to focus an app? In which case, is this interpretation even necessary? If we can, I think we should remove it.

When you tap on a suspended app in the dash you immediately get a
appStarting followed by a resumeRequest. If you don't respond to it (ie,
resume&focus the app), ages later you get a focusRequested.

So if I remove resumeRequested I would have to interpret appStarting for
a already running application as a focus request, which actually
wouldn't be wrong.

Btw, I find it odd that ubuntu_app_launch work that way, always sending
an appStarting signal even though the app is already running, but that's
the way it is...

> + << "ApplicationManager::onProcessStarting(appId="<<appId<<") - User wants to start"
> + " a new instance of an application that is still closing. Queuing its start.";
> we should investigate how to launch new instance immediately, instead of waiting for the closing one to quit. I'd be worried if the closing app crashes during close, as it won't be considered closed until apport has completed collecting the crash dump, causing the new instance a long delay. Not for this MP, but perhaps a TODO?

Agreed. The delay is noticeable. Added the TODO comment.

>
> Also nitpick: could you line up the qCDebug strings, in both spots in this method?

Wrapped the qCDebug boilerplate in a macro.

>
>
> + // TaskController doesn't give us any PID so we have no way of telling apart two instances
> + // of an application.
> we should get that fixed. UAL must have way of dealing with multiple instances of the same App, and distinguishing them. If not, we'll need to add it.

As far as I understand it ubuntu-app-launch (and the upstart service?)
have the concept that you can have only one running instance of an
application (like a service, that you start/stop using upstart).

This sounds like a bigger architectural change, out of scope for this MP.

> + // DeferredDelete is special: likes to be called out specifically or it won't come out
> weird?? Event loop blocked somehow?
>

QObject::deleteLater() generates a DeferredDelete event which is handled
in a special way in QCoreApplication etc, unlike any other QEvent.
There's code in there specifically for leaving it out of forced event
processing (like sendPostedEvents) unless you explicitly ask for that
particular event type.

On 07/01/2016 14:39, Gerry Boland wrote:
> Review: Needs Fixing
>
> === modified file 'src/modules/Unity/Application/application_manager.cpp'
> +    // We interpret this as a focus request for a suspended app.
> +    // Shell will have this app resumed if it complies with the focus request
>       QObject::connect(controller, &TaskController::resumeRequested,
> -                     manager, &ApplicationManager::onResumeRequested);
> + manager, &ApplicationManager::onFocusRequested);
>
>
> The original purpose of the resumeRequested signal from UAL was to tell AppMan to un-suspend a suspended app, since AppMan used to manage that. But since UAL does suspend/resume now itself, this signal is worthless. Suspect we should remove completely.
>
> Does not the focusRequested signal always get fired if url-dispatcher wants to focus an app? In which case, is this interpretation even necessary? If we can, I think we should remove it.

When you tap on a suspended app in the dash you immediately get a 
appStarting followed by a resumeRequest. If you don't respond to it (ie, 
resume&focus the app), ages later you get a focusRequested.

So if I remove resumeRequested I would have to interpret appStarting for 
a already running application as a focus request, which actually 
wouldn't be wrong.

Btw, I find it odd that ubuntu_app_launch work that way, always sending 
an appStarting signal even though the app is already running, but that's 
the way it is...

> +                << "ApplicationManager::onProcessStarting(appId="<<appId<<") - User wants to start"
> + " a new instance of an application that is still closing. Queuing its start.";
> we should investigate how to launch new instance immediately, instead of waiting for the closing one to quit. I'd be worried if the closing app crashes during close, as it won't be considered closed until apport has completed collecting the crash dump, causing the new instance a long delay. Not for this MP, but perhaps a TODO?

Agreed. The delay is noticeable. Added the TODO comment.

>
> Also nitpick: could you line up the qCDebug strings, in both spots in this method?

Wrapped the qCDebug boilerplate in a macro.

>
>
> +    // TaskController doesn't give us any PID so we have no way of telling apart two instances
> + // of an application.
> we should get that fixed. UAL must have way of dealing with multiple instances of the same App, and distinguishing them. If not, we'll need to add it.

As far as I understand it ubuntu-app-launch (and the upstart service?) 
have the concept that you can have only one running instance of an 
application (like a service, that you start/stop using upstart).

This sounds like a bigger architectural change, out of scope for this MP.

> + // DeferredDelete is special: likes to be called out specifically or it won't come out
> weird?? Event loop blocked somehow?
>

QObject::deleteLater() generates a DeferredDelete event which is handled 
in a special way in QCoreApplication etc, unlike any other QEvent. 
There's code in there specifically for leaving it out of forced event 
processing (like sendPostedEvents) unless you explicitly ask for that 
particular event type.

« Back to merge proposal