Code review comment for lp:~albaguirre/unity8/add-screenshotter

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> * saveScreenshot
> make QImage and QStrings there const &
>
They need to be copies, because they will be run in a separate thread plus Qt has copy on writing semantics on those.

>
> * ScreenGrabber::captureAndSave
> Make QWindowList windows const so we get the "fast" operator[] when doing
> windows[0]
>
Agree.

>
> * onKeyReleased:
> bring the else up to the } line
>
Sure.

>
> * screenGrabber.enable(Powerd.status === Powerd.On):
> Looks like this could be a binding instead of a function call?
>
Ok will do.

> * There's a few ; missing in the two onStopped functions
>
Ok I'll fix it.

>
> * //TODO: This should be configurable (perhaps through gsettings?):
> What's the use case/benefit for the user of making this configurable?
>
I guess simply size/quality tradeoff.

« Back to merge proposal