Code review comment for lp:~submarine/ubuntu-scopes/colourlovers-set-as-wallpaper

Revision history for this message
James Henstridge (jamesh) wrote :

A few issues:

[1]
Do you actually need the Preview.__init__() method? All it seems to be doing is chaining to the parent implementation without adding anything.

[2]
In your Preview.do_run() method, you start by assigning to a number of local variables that only get used once. It doesn't look like there is any real reason for this (e.g. code readability, or shortening long lines of code).

The preview variable is set to None, only to be assigned some other value before it is used further. Perhaps the code would be easier to read without these extra assignments?

[3]
For the path manipulation in Scope.wallpaper_action(), might it be simpler to use the Python standard library to check for the file contents and make the cache directory (i.e. os.path.isfile() and os.makedirs(..., exist_ok=True)).

[4]
In Scope.set_as_wallpaper(), you have more single-use locals. And creating a list of key strings that are then only accessed by index just makes the code more difficult to read compared to using those strings directly.

I'm also not sure what the purpose of this line is:

    gsettings.get_string(KEY[0])

At first I thought that it might be there to trigger an exception if the 'picture-uri' key didn't exist, but it seems to crash the process with unknown keys (probably a bug in the GIR data). Perhaps remove it?

[5]
Can you test this new preview code? Ideally we'd want to know that the preview is generated as expected, and that activating "wallpaper" action correctly downloads the content. You've already split the gsettings code out into a separate method, so it should be easy to stub that out for the test so you don't mess with the developer's settings.

review: Needs Fixing

« Back to merge proposal