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

Screenshot path is not calculated correctly for absolute paths #181

Open
ysb33r opened this issue May 12, 2019 · 3 comments
Open

Screenshot path is not calculated correctly for absolute paths #181

ysb33r opened this issue May 12, 2019 · 3 comments
Labels

Comments

@ysb33r
Copy link

ysb33r commented May 12, 2019

If I run something like

$ cd /working/dir
$  decktape --screenshots --screenshot-format jpg --screenshots-directory /path1 reveal \
   file:///path/to/source/index.html /path2/index.pdf

then the screenshot directory is calculated as /path1/path2/. I would not expect it to append the parent path from index.pdf to the screenshot directory.

The only way to get it work correctly atm, is to calculate the relative path from /working/dir back up to the filesystem root and then pass that as the screenshot directory and output to the same folder as where the PDF will be.

$  decktape --screenshots --screenshot-format jpg --screenshots-directory ../.. reveal \
   file:///path/to/source/index.html /path2/index.pdf
@ysb33r
Copy link
Author

ysb33r commented May 20, 2019

There is an additional issue to this. When using something like ../.. on Windows, the path to the screenshot file will be illegal. For example for C:\path2\index.pdf it will resolve the JPG file to C:\C:\path2\index.pdf. (Yes, the drive letter is repeated).

This will cause an issue for us in the v3.0 release of the Asciidoctor Gradle plugin (asciidoctor/asciidoctor-gradle-plugin#381)

@gitpitch
Copy link

I have a local patched decktape to workaround this problem. My solution was to treat any path passed on the --screenshots-directory flag as an absolute path. In decktape.js, I simply replaced the current implementation with this:

path           : path.join(options.screenshotDirectory,
          `_${context.currentSlide}_${resolution.width}x${resolution.height}.${options.screenshotFormat}`),

This approach would seem to solve the immediate issue, ie. current path.join logic is broken. And it would also seem to address any issues related to platform specific paths. I think. So perhaps it might make sense to adopt this approach here, as a general fix for the handling of this flag. Just food for thought.

@cavo789
Copy link

cavo789 commented May 5, 2020

Thanks @gitpitch , your workaround is working; had the same problem.

I'm running the latest version of decktape i.e. 2.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants