QtMir

Merge lp://staging/~dandrader/qtmir/multiSessionApp into lp://staging/qtmir

multiSessionApp
Merge into trunk

Proposed by Daniel d'Andrada on 2017-03-31

Status:

Merged

Approved by:

Gerry Boland on 2017-04-05

Approved revision:

639

Merged at revision:

638

Proposed branch:

lp://staging/~dandrader/qtmir/multiSessionApp

Merge into:

lp://staging/qtmir

Diff against target:

1193 lines (+292/-239)

9 files modified

src/modules/Unity/Application/application.cpp (+146/-90)
src/modules/Unity/Application/application.h (+12/-13)
src/modules/Unity/Application/application_manager.cpp (+29/-58)
src/modules/Unity/Application/application_manager.h (+3/-7)
src/modules/Unity/Application/dbusfocusinfo.cpp (+28/-25)
src/modules/Unity/Application/session.cpp (+1/-1)
src/modules/Unity/Application/session_interface.h (+6/-5)
tests/modules/Application/application_test.cpp (+15/-15)
tests/modules/ApplicationManager/application_manager_test.cpp (+52/-25)

To merge this branch:

bzr merge lp://staging/~dandrader/qtmir/multiSessionApp

High

Invalid

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michał Sawicz			Approve on 2017-04-05
Gerry Boland (community)		2017-03-31	Approve on 2017-04-05
Unity8 CI Bot (community)	continuous-integration		Approve on 2017-03-31
Review via email: mp+321581@code.staging.launchpad.net

Commit message

Support multiple sessions per Application

Description of the change

* Are there any related MPs required for this MP to build/function as expected? Please list.
No

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-31:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/652/rebuild

review: Approve (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-03-31:

+ if (session->state() > combinedState) {
+ combinedState = session->state();
+ }
I guess this is as good a "combined" state as any.

The code, you're using the convenience to the order of the states in its enum? Strikes me as something easily broken in future - but I'm glad to see you've added a comment to make that less likely.

+void Application::die()
terminate() would match the "kill" term

Going from memory, the setApplicationPid stuff was a workaround for supporting apps that are not launched via UAL - we had to save the pid we got from Mir. Are apps launched with desktop_file_hint still working?

How are you testing this? I made this simple client which might be handy: git clone -b multiple-connections https://git.launchpad.net/~gerboland/+git/basic_mir_client

Is moving the InitialSurfaceSize calls into the Application required for this MP?

review: Needs Information

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-31:

On 31/03/2017 13:26, Gerry Boland wrote:
> +void Application::die()
> terminate() would match the "kill" term

Ok. Renamed.

> Going from memory, the setApplicationPid stuff was a workaround for supporting apps that are not launched via UAL - we had to save the pid we got from Mir. Are apps launched with desktop_file_hint still working?

Ooops! You're right. Being able to remove this bookkeeping of pids from
ApplicationManager was too good to be true.
Fixed.

> How are you testing this? I made this simple client which might be handy: git clone -b multiple-connections https://git.launchpad.net/~gerboland/+git/basic_mir_client

That's not handy. Launching some random app from the application drawer
is handy. I tried with nautilus and mahjongg.
As a general rule: try to launch some apps from the app drawer without
this patch. You should get endless splash screens for them. Then install
this patch and try the same ones again. They should now work.

You can also add debug-level logging to surface and application
categories and check in unity8.log that such applications are indeed
getting *3* separate mir sessions each (at least the gtk ones I checked).

> Is moving the InitialSurfaceSize calls into the Application required for this MP?

That was how things were being done before the latest landing (had the
patch ready) and that sure is simpler as well as the application has
many sessions and thus entries in that InitialSurfaceSize singleton.

Application knows his own pids and his own initialSurfaceSize. So it can
update InitialSurfaceSize on his own. No need to involve
ApplicationManager in that. Don't know what you gain from it.
Furthermore, ApplicationManager no longer keeps tracks of all pids going
around. It just momentarily holds them between the moment a session is
authorized and the moment it shows up to be assigned to an Application
object.

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-31:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/655/rebuild

review: Approve (continuous-integration)

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-31:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/656/rebuild

review: Approve (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-04-03:

Ok, this does improve the situation a lot.

review: Approve

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-04-05:

<bregma> Saviq, the first way to test the multiple-connection situation would probably be to create a custom .desktop file that removes GDK_BACKEND from the app's environment, which will force it to autoprobe and use Mir instead

Saviq tried this and got http://pastebin.ubuntu.com/24319827/ where the second mir connection from the process is rejected. I suspect things changed under our feet, can you re-test

review: Needs Fixing

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-04-05:

Still fixes the main thing, will keep

review: Approve

Revision history for this message

Michał Sawicz (saviq) wrote on 2017-04-05:

Sorry, apparently GTK_BACKEND="" is incorrect, ="*" or unsetting it altogether showed GTK apps launch fine.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Daniel d'Andrada

Mir development team