search

faaxm / spix

UI test automation library for QtQuick/QML Apps
MIT License
188 stars 49 forks source link

Screenshot warning #61

Closed ctmbl closed 1 year ago

ctmbl commented 2 years ago

This PR aims to help the client know if takeScreenshot method has managed to produce and save an image by returning a boolean value through xmlrpc and adding some logs. The need of such logs appeared to me in the case you don't specify an image format, for example if you type s.takeScreenshot("mainWindow","image") no image will be produce because Qt doesn't recognize a valid file extension (because the optional parameter const char *format = nullptr of QImage::save isn't set) and the client currently hasn't a clue of why it didn't work. However, I'm not quite sure if I've done it right, following your conventions, so please tell me if anything has been done wrong.

faaxm commented 1 year ago

Hi @ctmbl, very sorry for not responding earlier. I had lots of other things to do and couldn't focus on spix, which I am only maintaining in my spare time..

Are you still interested in this PR? In terms of the changes, I would prefer if it would only use the env.state().reportError function for now and not report back a bool value indicating the success. This would also make the needed changes much smaller (only in Screenshot::execute I think), as no promise has to be used to report the value back.

While I am also not too happy anymore with the reportError mechanism, it is the mechanism used by all other parts of spix to report errors and I would like to keep the code consistent. Eventually it would be great to move away from the error log generated by reportError, but then it would be important to do that change for all commands in spix, not just for one. E.g. the AnyRPC library supports reporting exceptions back to python and it would be great to make use of that. There could also be a way to do it by using the existing reportError and changing how it works internally, which would mean much less refactoring in the various commands... I will look into writing a separate issue to track this refactoring.

For this PR, it would be cool to keep it as minimal as possible using the existing error reporting style, to not add to the changes that will be required in the error reporting refactoring.