Skip to content
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

Support screenshot image filenames containing parentheses #250

Merged
merged 1 commit into from
Jun 12, 2015

Conversation

peruukki
Copy link
Contributor

@peruukki peruukki commented Apr 9, 2015

I have a website that has a page with parentheses in its URL. When running e.g. wraith capture, cropping the page’s screenshots and generating thumbnails fails with errors like these:

cropping images
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `convert shots/__programme__tutt(i)uus/1024__auraco.png -background none -extent 1024x3801 shots/__programme__tutt(i)uus/1024__auraco.png'
cropping images
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `convert shots/__programme__tutt(i)uus/320__auraco.png -background none -extent 320x4202 shots/__programme__tutt(i)uus/320__auraco.png'
...
Generating thumbnails
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `convert shots/__programme__tutt(i)uus/1024__auraco.png -thumbnail 200 -crop 200x200+0+0 shots/thumbnails/__programme__tutt(i)uus/1024__auraco.png'
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `convert shots/__programme__tutt(i)uus/320__auraco.png -thumbnail 200 -crop 200x200+0+0 shots/thumbnails/__programme__tutt(i)uus/320__auraco.png'

I solved this by escaping the screenshot filenames before passing them to shell execution. I started writing separate tests for this case too, but in the end I felt it resulted in too much (almost duplicate) test code. So I just test this using the existing tests with an adjusted filename.

@peruukki
Copy link
Contributor Author

peruukki commented Apr 9, 2015

This also seems to fix #202.

@@ -27,7 +28,8 @@ def percentage(img_size, px_value, info)
end

def compare_task(base, compare, output, info)
cmdline = "compare -dissimilarity-threshold 1 -fuzz #{wraith.fuzz} -metric AE -highlight-color blue #{base} #{compare} #{output}"
compare_escaped = Shellwords.escape(compare)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just do compare.shellescape

@bootstraponline
Copy link

shellwords needs to be added to the gemspec as a runtime dependency.

@bootstraponline
Copy link

This pull request contains merge conflicts that must be resolved.
Only those with write access to this repository can merge pull requests.

and the pull request needs to be rebased

Escape filenames before passing them to shell execution. The fix is tested
most easily with the existing tests with an adjusted filename.
@peruukki
Copy link
Contributor Author

@bootstraponline, thanks for the tip about calling shellescape directly on the string, it makes the changes much nicer one-liners! Shellwords is part of the Ruby standard library, so apparently it shouldn't be added to runtime depedencies; I tried adding it, but then the gem installation failed to this:

ERROR:  While executing gem ... (Gem::UnsatisfiableDependencyError)
    Unable to resolve dependency: 'wraith (= 2.4.1)' requires 'shellwords (>= 0)'

I changed the commit to call shellescape everywhere and rebased it.

@bootstraponline
Copy link

Shellwords is part of the Ruby standard library, so apparently it shouldn't be added to runtime depedencies;

oh, sorry didn't know that. The PR looks good to me. 👍

@bootstraponline
Copy link

/cc @DaveBlooman

dblooman added a commit that referenced this pull request Jun 12, 2015
Support screenshot image filenames containing parentheses
@dblooman dblooman merged commit a5adefb into bbc:master Jun 12, 2015
@peruukki peruukki deleted the escape-image-filenames branch August 13, 2015 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants