Merge lp:~gerboland/unity-mir/use-upstart-app-launch2 into lp:unity-mir
Status: | Merged | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Approved by: | Daniel d'Andrada on 2013-09-30 | ||||||||||||||||
Approved revision: | 103 | ||||||||||||||||
Merged at revision: | 94 | ||||||||||||||||
Proposed branch: | lp:~gerboland/unity-mir/use-upstart-app-launch2 | ||||||||||||||||
Merge into: | lp:unity-mir | ||||||||||||||||
Diff against target: |
866 lines (+299/-239) 10 files modified
debian/control (+2/-0) src/modules/Unity/Application/Application.pro (+1/-1) src/modules/Unity/Application/application.cpp (+28/-18) src/modules/Unity/Application/application.h (+3/-5) src/modules/Unity/Application/application_manager.cpp (+124/-176) src/modules/Unity/Application/application_manager.h (+8/-6) src/modules/Unity/Application/applicationscreenshotprovider.cpp (+7/-0) src/modules/Unity/Application/desktopfilereader.cpp (+2/-1) src/modules/Unity/Application/taskcontroller.cpp (+101/-27) src/modules/Unity/Application/taskcontroller.h (+23/-5) |
||||||||||||||||
To merge this branch: | bzr merge lp:~gerboland/unity-mir/use-upstart-app-launch2 | ||||||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Daniel d'Andrada (community) | 2013-09-26 | Approve on 2013-09-30 | |
PS Jenkins bot (community) | continuous-integration | Approve on 2013-09-27 | |
Michał Sawicz | code | Approve on 2013-09-27 | |
Review via email:
|
Commit message
Use upstart-app-launch for app start/stop
Description of the change
Use upstart-app-launch for app start/stop
- 97. By Gerry Boland on 2013-09-26
-
If process ends not by hand of shell, clean up after it properly
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:97
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Daniel d'Andrada (dandrader) wrote : | # |
+ DLOG("Applicati
s/appId=%p/appId=%s
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:97
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 98. By Gerry Boland on 2013-09-26
-
s/appId=%p/appId=%s - well spotted Daniel
Daniel d'Andrada (dandrader) wrote : | # |
"""
|@@ -354,12 +271,14 @@ void ApplicationMana
| authorized = false; //to be proven wrong
|
| DLOG("Applicati
|- Application* application = findApplication
|- if (application) {
""""
Speaking of log messages, I know this code is not touched by your patch but for extra bonus points it would be nice if
s/pid=%
Daniel d'Andrada (dandrader) wrote : | # |
"""
@@ -428,7 +347,10 @@ void ApplicationMana
QString argStr(
QStringList arguments(
- application = new Application(
+ application = new Application(
+ delete desktopData;
+ application-
+ application-
"""
It's rather wasteful to parse a desktop file twice in this situation. Can't we easily avoid it by having a second constructor that takes a DesktopFileReader instead of a appId as an argument?
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:98
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 99. By Gerry Boland on 2013-09-26
-
Support respawning apps again
Daniel d'Andrada (dandrader) wrote : | # |
I wonder if we can fix the mess that a pid is represented as a pid_t, a qint64 and a quint64.
Daniel d'Andrada (dandrader) wrote : | # |
Code-wise, my only complaint is the unnecessary recreation of the DesktopFileReader I explained above.
- 100. By Gerry Boland on 2013-09-26
-
Some handy extra debug info
- 101. By Gerry Boland on 2013-09-26
-
Fix for quick focus->
unfocus- >focus caused app to still suspend
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:99
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:101
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Gerry Boland (gerboland) wrote : | # |
> I wonder if we can fix the mess that a pid is represented as a pid_t, a qint64
> and a quint64.
Yep, it's a mess, and on my TODO list to fix in another MR.
Gerry Boland (gerboland) wrote : | # |
> It's rather wasteful to parse a desktop file twice in this situation. Can't we
> easily avoid it by having a second constructor that takes a DesktopFileReader
> instead of a appId as an argument?
You raise a fair point. I wanted to avoid having to parse desktop files in ApplicationManager at all, delegating it to Application alone, but the authorizer forced my hand.
I did the duplicate as I hope much of that authorizer code will go away soon. But I'll try to improve it now.
- 102. By Gerry Boland on 2013-09-27
-
Application constructor can take both appId or DesktopFileReader
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:102
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 103. By Gerry Boland on 2013-09-27
-
Add todo for screenshot
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:103
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PASSED: Continuous integration, rev:96 jenkins. qa.ubuntu. com/job/ unity-mir- ci/74/ jenkins. qa.ubuntu. com/job/ unity-mir- saucy-amd64- ci/16 jenkins. qa.ubuntu. com/job/ unity-mir- saucy-armhf- ci/74 jenkins. qa.ubuntu. com/job/ unity-mir- saucy-armhf- ci/74/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ unity-mir- saucy-i386- ci/74
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins: 8080/job/ unity-mir- ci/74/rebuild
http://