Merge lp:~aacid/unity8/suspend_screenshoting into lp:unity8
| Status: | Merged |
|---|---|
| Approved by: | Michael Zanetti on 2015-06-30 |
| Approved revision: | 1755 |
| Merged at revision: | 1841 |
| Proposed branch: | lp:~aacid/unity8/suspend_screenshoting |
| Merge into: | lp:unity8 |
| Diff against target: |
735 lines (+524/-22) 17 files modified
debian/unity8-private.install (+1/-0) plugins/CMakeLists.txt (+1/-0) plugins/SessionGrabber/CMakeLists.txt (+8/-0) plugins/SessionGrabber/SessionGrabber.qmltypes (+23/-0) plugins/SessionGrabber/plugin.cpp (+27/-0) plugins/SessionGrabber/plugin.h (+32/-0) plugins/SessionGrabber/qmldir (+3/-0) plugins/SessionGrabber/sessiongrabber.cpp (+135/-0) plugins/SessionGrabber/sessiongrabber.h (+85/-0) qml/Stages/ApplicationWindow.qml (+46/-22) qml/Stages/Splash.qml (+2/-0) qml/Stages/SpreadDelegate.qml (+1/-0) tests/plugins/CMakeLists.txt (+1/-0) tests/plugins/SessionGrabber/CMakeLists.txt (+14/-0) tests/plugins/SessionGrabber/sessiongrabbertest.cpp (+79/-0) tests/plugins/SessionGrabber/sessiongrabbertest.qml (+22/-0) tests/qmltests/Stages/tst_ApplicationWindow.qml (+44/-0) |
| To merge this branch: | bzr merge lp:~aacid/unity8/suspend_screenshoting |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| PS Jenkins bot | continuous-integration | Needs Fixing on 2015-06-30 | |
| Michael Zanetti (community) | Abstain on 2015-06-30 | ||
| Andrea Cimitan (community) | Abstain on 2015-04-30 | ||
| Michał Sawicz | 2015-04-22 | Approve on 2015-04-30 | |
| Daniel d'Andrada (community) | Needs Information on 2015-04-30 | ||
|
Review via email:
|
|||
Commit Message
Save screenshots of suspended apps to disk
Description of the Change
* Are there any related MPs required for this MP to build/function as expected?
No
* Did you perform an exploratory manual test run of your code change and any related functionality?
Not yet :D
* Did you make sure that your branch does not contain spurious tags?
Yes
* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A
* If you changed the UI, has there been a design review?
N/A
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1743
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Albert Astals Cid (aacid) wrote : | # |
> Is the screenshot unloaded if app has a drawn-to surface?
Yes, see
}
}
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1744
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Albert Astals Cid (aacid) wrote : | # |
> Generate qmltypes please.
Done
| Albert Astals Cid (aacid) wrote : | # |
> Some clarification about the activity indicator - we do want it to run over
> either screenshot or splash screen *if* the app is active/focused, just not if
> it's in the background.
This should be there now.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1747
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Albert Astals Cid (aacid) wrote : | # |
> I think we're unnecessarily complicating this by using the intermediate SOURCES variable, not sure why we started it ;)
Fixed
> Why not QStandardPaths:
Utils/windowsta
> We tend to put the comma after newline.
Fixed, We need a bot to do this :D
> Any reason why not if (image.save(path))?
I think at some point i was returning b, changed it
> If you s/Screenshoter/
Done
> Maybe ::drop() or ::clear()... or just ::remove()? It feels weird to have a nice and short ::take() and then the verbose removeScreenshot().
Honestly i'd prefer not to, the thing is that we have "SessionGrabber", so it's quite clear what "grab" does, but "remove/drop/clear" seems like it may clear the grabber (i.e. for example reset parameters), not actually remove the screenshot.
> Third time, please factor out, maybe even cachePath(appId)?
Sure
> /me not totally familiar with QtConcurrent, can you explain why we need the QSharedPointer here?
You mean for the watcher or for the grab result?
> Typo "AppdId"
Fixed
> Typotoo.
Fixed
> As you rename... grabbed()?
Done
> If you rename ::removeScreenshot, likely rename this too, clearSessionGrab or so?
Not done since i didn't change the other one
> I'd probably use screenshoter.path || d.defaultScreen
Ok, changed, not that it makes a difference
> Maybe add a comment why that?
Done
> Spinner active if running?
If the splash is still around (it'll go away as soon as the first frame is drawn), yes
> No spinner if stopped? Should be if stopped and !active?
How would one be stopped and active at the same time?
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1748
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Michał Sawicz (saviq) wrote : | # |
>> Why not QStandardPaths:
> Utils/windowsta
Yeah, but at least it writes in ~/.cache/*unity8*/, not in ~/.cache/ directly ;)
Also, there's no transition when screenshot is loaded over splashscreen.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1750
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1752
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel d'Andrada (dandrader) wrote : | # |
"""
added file 'plugins/
"""
Anything against using CamelCase for the file names? eg: SessionGrabber.cpp
What do we gain from making it all lower-case?
| Michał Sawicz (saviq) wrote : | # |
* Did you perform an exploratory manual test run of the code change and any related functionality?
Y
* Did CI run pass? If not, please explain why.
Some flakiness, will run again before top-approving.
* Did you make sure that the branch does not contain spurious tags?
Y
| Albert Astals Cid (aacid) wrote : | # |
> """
> added file 'plugins/
> """
>
> Anything against using CamelCase for the file names? eg: SessionGrabber.cpp
> What do we gain from making it all lower-case?
Having to press the shift key fewer times :D
Honestly we have half the plugins that use lowercase and half that use CamelCase, at some points we should decide what we want to use, but not sure this time should be now. Maybe on Dallas?
| Andrea Cimitan (cimi) wrote : | # |
After discussing with Gerry, eventually we might decide to have full resolution screenshots (maybe in black and white to indicate that is suspended), but we will fix it in a different branch
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1752
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Albert Astals Cid (aacid) wrote : | # |
> After discussing with Gerry, eventually we might decide to have full
> resolution screenshots (maybe in black and white to indicate that is
> suspended), but we will fix it in a different branch
FWIW the screenshot in disk is full resolution, it's the showing only that is not full resolution so won't lose resolution or have to re-take screenshots when changing it later
| Albert Astals Cid (aacid) wrote : | # |
Merged
| Michael Zanetti (mzanetti) wrote : | # |
Doesn't compile any more after merging with trunk: https:/
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1753
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 1754. By Albert Astals Cid on 2015-06-30
-
Merge unity8
- 1755. By Albert Astals Cid on 2015-06-30
-
Build++
| Albert Astals Cid (aacid) wrote : | # |
> Doesn't compile any more after merging with trunk:
> https:/
> wily-i386.
Fixed.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1755
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

As mentioned, SessionScreenshoter could use a better name... For one, it could be SessionShotter (note double-t), as it's not shooting the screen, just the session ;). SessionGrabber? We already have a ScreenGrabber!
Generate qmltypes please.
Is the screenshot unloaded if app has a drawn-to surface?
Some clarification about the activity indicator - we do want it to run over either screenshot or splash screen *if* the app is active/focused, just not if it's in the background.