-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[nemo-qml-plugin-utilities] Add Screenshots component #1
base: master
Are you sure you want to change the base?
Conversation
} else { | ||
// If there's no target item, then take an actual screenshot of the | ||
// desktop root window. | ||
snapshot = QPixmap::grabWindow(QApplication::desktop()->winId(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the issues with winId() in client applications, can this cause any issues? I'd assume not, since the desktop() should always have a native rendering window, but thought I'd ask, since I don't know much about that stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what are these issues? The only one I know of is that only top-level widgets have windows, but that should be ok here as you say. Are there other known issues with winId()?
About the WId type itself, the 4.8 docs say "Portable in principle, but if you use it you are probably about to do something non-portable. Be careful."
I think it's ok here because all I do is pass it right back to a Qt function that knows what to do with it. In fact I was quite happy to not need any X11 specific code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue is that of performance - and as the top level widget has one anyway, this shouldn't be a problem. I just thought I'd mention it in case it hadn't been considered. (eg, I've seen the use of winId() cause framerates in client applications to drop from 60 fps to 12 fps, after a call to winId() to get an id to use in a call to XSetTransientFor()).
I'm not sure I follow the purpose of this. Do you expect tests to try take screenshots themselves every time a condition fails? That seems like it could get messy/complicated. I spent a while last week prototyping adding the capability for qtest-qml to grab screendumps of windows when something explodes: http://qtl.me/0001-WIP-take-a-screenshot-whenever-a-test-fails.patch It's not really finished (which is why I hadn't come to talk to you yet :)), but it's completely automated (requiring no changes to unit tests at all), which I think is one very compelling change to what this looks like. As a simple example I was toying with:
the last compare will fail (and thus, screenshot, and you'll notice the window is the wrong color) |
We also need the ability to "trigger" automated taking of screenshots from applications without test failures. For example think about l10n cases , documentation, general QA etc .. |
Robin, that would indeed be great :) Often when tests fail in automated VM tests it's hard to figure out what really went wrong and a screenshot of the failure would go a long way. There are two use-cases I had in mind (and one of them is specifically waiting for this pull actually)
|
@iamer: yes, I think you covered cases like that back in December -- when I added the capability to the display manager to take screenshots. Why can it not be done using that? I'd rather fix it if it doesn't do what you need than introduce more code doing similar/overlapping things. It seems to me that test framework hacking can more elegantly cover issue 1, and issue 2 is already possible (unless I'm missing something?) |
I think what you're missing is that the display manager's screenshots can't be triggered from QML :) It needs a plugin... like this one. And there are also other problems with the mcompositor approach:
All in all, the plugin would have to work around everything mcompositor does except one line, the grabWindow call itself. I don't see that one line as code doing similar/overlapping things, it's just using Qt as a library as intended. The bulk of qdeclarativescreenshots.cpp is just implementing an API to control the screenshots from QML, which is something that would be needed anyway. Which leads me to ask... why does your quicktestlib patch not rely on mcompositor's screenshot ability? :-) It's in C++ so you don't even have the QML problem. |
Screenshots can be taken of the root window or of specific components. | ||
*/ | ||
|
||
class DeclarativeScreenshots : public QDeclarativeItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSameer told me that I can just use a QObject here because it's a non-graphical component, and that might even work for both QtQuick1 and QtQuick2 (whereas QDeclarativeItem usage would definitely need porting)
The screenshots have already been useful in 2 cases where things weren't working in QA VM even though they worked on device: showing that the virtual keyboard didn't open, and showing that another window was covering the application. Both of those were only visible because the screenshot took a desktop snapshot. I see that your qtest-qml patch takes a rendering of the application windows, which wouldn't have shown that. I think, if it has to be one or the other, I would prefer desktop snapshots for debugging. Do you agree? |
It doesn't need to inherit QDeclarativeItem because it's not a visual item.
This helps when the application is not in the device's default orientation. Implementing this also involved changing the snapshot to be a QImage instead of a QPixmap. This is ok since Qt used to do that conversion internally to save the pixmap anyway.
This patch provides the ability to trigger screenshots from QML and save them to a specified directory. Being able to do this from QML is useful for test scripts.
There are two modes of use: by default it takes screenshots from the desktop root window, so that composed elements such as the virtual keyboard show up.
If you set the "target" property, then it takes screenshots of the target qml item by asking the graphics scene to render it to a pixmap. This renders the item as seen in the scene, so with transformations applied and with other items possibly obscuring it.