Issue 6615073: Add utility to convert skp files to pdf files. (Closed)
|
Created:
11 years, 8 months ago by edisonn Modified:
11 years, 8 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdd utility to convert skp files to pdf files. Code compatible with render_pictures_main.cpp. I could have refactored the code to avoid code duplication, but it does not seem right, and it will overcomplicate the code.
Committed: https://code.google.com/p/skia/source/detail?r=6253
Patch Set 1 #Patch Set 2 : #
Total comments: 24
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #MessagesTotal messages: 12
I forgot to include this file in the previous cl.
Sign in to reply to this message.
https://codereview.appspot.com/6615073/diff/3001/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.appspot.com/6615073/diff/3001/gyp/tools.gyp#newcode123 gyp/tools.gyp:123: 'target_name': 'render_pdfs', Other developers who look here will probably be confused as to the difference between "render_pdfs" and "pdf_renderer"... "which target do I need to build?" My thoughts: 1. If there are no other targets that depend on "pdf_renderer", maybe remove that target and just roll the source files into the "render_pdfs" target. 2. It there ARE other targets that depend on "pdf_renderer", maybe the "tools" subdir isn't the right place for it anyway? Maybe PdfRenderer.cpp should go in src/pdf (perhaps renamed something like SkPDFRenderPicture.cpp) and be part of pdf.gyp? 3. If the above don't seem right to you, maybe just add a brief comment in here explaining the difference between the two targets. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:17: Please include a description of what this file is for... "command-line wrapper for PdfRenderer", maybe? https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:33: static void make_output_filepath(SkString* path, const SkString& dir, please add a brief description of the parameters for this and all other functions. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:37: path->remove(path->size() - 3, 3); Rather than just blindly removing the last 3 characters, I think it would be a good idea to make sure you are removing input_file_extension (which the caller should set to "skp"), whatever its length is. As the code stands now, what happens if you run this tool against a single SkPicture file named "testfile"? I think it will write out the result as "testf.pdf", which I doubt is the desired behavior. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:45: make_output_filepath(&outputPath, outputDir, inputFilename, "pdf"); rather than defining magic strings like "pdf" inline, please use constants that are defined at the top of the file https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:67: SkAutoTUnref<SkPicture> aur(picture); Could you replace these two lines with: SkAutoTUnref<SkPicture> picture(SkNEW_ARGS(SkPicture, (&inputStream, &success))); There are two advantages: 1. the AutoUnref object has a more descriptive name ("picture" instead of "aur") 2. there is no chance that any new code that comes along later will try to use the SkPicture* object directly OR, if you prefer, just rename "aur" to something more descriptive (like "autoUnrefPicture" or "pictureAutoUnref") https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:88: SkOSFile::Iter iter(input.c_str(), "skp"); again, no magic strings inline please https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:91: if (iter.next(&inputFilename)) { I guess this branch is used to check whether "inputFilename" refers to a directory or a file? That seems fragile to me. What if it's a directory that doesn't contain any .skp files? I think it would be less fragile, and easier to understand, if you just checked directly: does this path refer to a file or a directory? https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:95: if (!render_pdf(inputPath, outputDir, renderer)) please use curly braces for the "if" code block https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:101: ++failures; curly braces please https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:114: SkDELETE(renderer); Why is parse_commandline() in charge of deleting renderer if it exits? Wouldn't it be cleaner/easier to parse the commandline BEFORE allocating any objects? https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:148: SkDELETE(renderer); Seems like another good use for AutoUnref. Why make extra work for ourselves?
Sign in to reply to this message.
That what I deserve for not using svn cp. This is a modified version of render_pictures_main.cpp Looking at the bigger picture, do you want me to also fix render_pictures_main.cpp? They have the same architecture. But if I go to the trouble of modifying render_pictures_main I could as well redesign them. Any thoughts? On 2012/10/25 15:39:55, epoger wrote: > https://codereview.appspot.com/6615073/diff/3001/gyp/tools.gyp > File gyp/tools.gyp (right): > > https://codereview.appspot.com/6615073/diff/3001/gyp/tools.gyp#newcode123 > gyp/tools.gyp:123: 'target_name': 'render_pdfs', > Other developers who look here will probably be confused as to the difference > between "render_pdfs" and "pdf_renderer"... "which target do I need to build?" > > My thoughts: > > 1. If there are no other targets that depend on "pdf_renderer", maybe remove > that target and just roll the source files into the "render_pdfs" target. > > 2. It there ARE other targets that depend on "pdf_renderer", maybe the "tools" > subdir isn't the right place for it anyway? Maybe PdfRenderer.cpp should go in > src/pdf (perhaps renamed something like SkPDFRenderPicture.cpp) and be part of > pdf.gyp? > > 3. If the above don't seem right to you, maybe just add a brief comment in here > explaining the difference between the two targets. > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp > File tools/render_pdfs_main.cpp (right): > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:17: > Please include a description of what this file is for... "command-line wrapper > for PdfRenderer", maybe? > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:33: static void make_output_filepath(SkString* path, > const SkString& dir, > please add a brief description of the parameters for this and all other > functions. > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:37: path->remove(path->size() - 3, 3); > Rather than just blindly removing the last 3 characters, I think it would be a > good idea to make sure you are removing input_file_extension (which the caller > should set to "skp"), whatever its length is. > > As the code stands now, what happens if you run this tool against a single > SkPicture file named "testfile"? I think it will write out the result as > "testf.pdf", which I doubt is the desired behavior. > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:45: make_output_filepath(&outputPath, outputDir, > inputFilename, "pdf"); > rather than defining magic strings like "pdf" inline, please use constants that > are defined at the top of the file > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:67: SkAutoTUnref<SkPicture> aur(picture); > Could you replace these two lines with: > > SkAutoTUnref<SkPicture> picture(SkNEW_ARGS(SkPicture, (&inputStream, > &success))); > > There are two advantages: > 1. the AutoUnref object has a more descriptive name ("picture" instead of "aur") > 2. there is no chance that any new code that comes along later will try to use > the SkPicture* object directly > > > OR, if you prefer, just rename "aur" to something more descriptive (like > "autoUnrefPicture" or "pictureAutoUnref") > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:88: SkOSFile::Iter iter(input.c_str(), "skp"); > again, no magic strings inline please > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:91: if (iter.next(&inputFilename)) { > I guess this branch is used to check whether "inputFilename" refers to a > directory or a file? That seems fragile to me. What if it's a directory that > doesn't contain any .skp files? > > I think it would be less fragile, and easier to understand, if you just checked > directly: does this path refer to a file or a directory? > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:95: if (!render_pdf(inputPath, outputDir, renderer)) > please use curly braces for the "if" code block > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:101: ++failures; > curly braces please > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:114: SkDELETE(renderer); > Why is parse_commandline() in charge of deleting renderer if it exits? Wouldn't > it be cleaner/easier to parse the commandline BEFORE allocating any objects? > > https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... > tools/render_pdfs_main.cpp:148: SkDELETE(renderer); > Seems like another good use for AutoUnref. Why make extra work for ourselves?
Sign in to reply to this message.
On 2012/10/26 02:24:33, edisonn wrote: > That what I deserve for not using svn cp. > > This is a modified version of render_pictures_main.cpp > > Looking at the bigger picture, do you want me to also fix > render_pictures_main.cpp? They have the same architecture. > > But if I go to the trouble of modifying render_pictures_main I could as well > redesign them. > > Any thoughts? Yikes! I wish render_pictures_main.cpp had been more strictly reviewed. (Or maybe my idea of "clean enough" is an outlier.) If there is substantial sharing between this and render_pictures_main.cpp, I think the cleanest/best approach might be: CL 1: refactor render_pictures_main.cpp to allow access to the parts you need CL 2 (or maybe within CL 1): fix above-noted problems in refactored render_pictures_main.cpp CL 3: add render_pdfs_main.cpp which leverages some of the "guts" of render_pictures_main.cpp I know that "starting over" with a multiple-CL approach is time consuming; feel free to get a "second opinion" from somebody else! :-)
Sign in to reply to this message.
On 2012/10/29 09:47:19, epoger wrote: > On 2012/10/26 02:24:33, edisonn wrote: > > That what I deserve for not using svn cp. > > > > This is a modified version of render_pictures_main.cpp > > > > Looking at the bigger picture, do you want me to also fix > > render_pictures_main.cpp? They have the same architecture. > > > > But if I go to the trouble of modifying render_pictures_main I could as well > > redesign them. > > > > Any thoughts? > > Yikes! I wish render_pictures_main.cpp had been more strictly reviewed. (Or > maybe my idea of "clean enough" is an outlier.) > > If there is substantial sharing between this and render_pictures_main.cpp, I > think the cleanest/best approach might be: > > CL 1: refactor render_pictures_main.cpp to allow access to the parts you need > CL 2 (or maybe within CL 1): fix above-noted problems in refactored > render_pictures_main.cpp > CL 3: add render_pdfs_main.cpp which leverages some of the "guts" of > render_pictures_main.cpp > > I know that "starting over" with a multiple-CL approach is time consuming; feel > free to get a "second opinion" from somebody else! :-) P.S. As part of a different CL, I might be adding sk_isdir() and related functions to SkOSFile.h, which may help... ping me later if you want to check on progress on those.
Sign in to reply to this message.
Even if the structure of the render_* files is similar, it does not make sense to reactor much of it, it would really be overengineering. I will fix the new file now, and will add a comment in render_picture_main.cpp that it needs cleanup too, referring to this cl On 2012/10/29 09:58:04, epoger wrote: > On 2012/10/29 09:47:19, epoger wrote: > > On 2012/10/26 02:24:33, edisonn wrote: > > > That what I deserve for not using svn cp. > > > > > > This is a modified version of render_pictures_main.cpp > > > > > > Looking at the bigger picture, do you want me to also fix > > > render_pictures_main.cpp? They have the same architecture. > > > > > > But if I go to the trouble of modifying render_pictures_main I could as well > > > redesign them. > > > > > > Any thoughts? > > > > Yikes! I wish render_pictures_main.cpp had been more strictly reviewed. (Or > > maybe my idea of "clean enough" is an outlier.) > > > > If there is substantial sharing between this and render_pictures_main.cpp, I > > think the cleanest/best approach might be: > > > > CL 1: refactor render_pictures_main.cpp to allow access to the parts you need > > CL 2 (or maybe within CL 1): fix above-noted problems in refactored > > render_pictures_main.cpp > > CL 3: add render_pdfs_main.cpp which leverages some of the "guts" of > > render_pictures_main.cpp > > > > I know that "starting over" with a multiple-CL approach is time consuming; > feel > > free to get a "second opinion" from somebody else! :-) > > P.S. As part of a different CL, I might be adding sk_isdir() and related > functions to SkOSFile.h, which may help... ping me later if you want to check on > progress on those.
Sign in to reply to this message.
https://codereview.appspot.com/6615073/diff/3001/gyp/tools.gyp File gyp/tools.gyp (right): https://codereview.appspot.com/6615073/diff/3001/gyp/tools.gyp#newcode123 gyp/tools.gyp:123: 'target_name': 'render_pdfs', Done 1) On 2012/10/25 15:39:56, epoger wrote: > Other developers who look here will probably be confused as to the difference > between "render_pdfs" and "pdf_renderer"... "which target do I need to build?" > > My thoughts: > > 1. If there are no other targets that depend on "pdf_renderer", maybe remove > that target and just roll the source files into the "render_pdfs" target. > > 2. It there ARE other targets that depend on "pdf_renderer", maybe the "tools" > subdir isn't the right place for it anyway? Maybe PdfRenderer.cpp should go in > src/pdf (perhaps renamed something like SkPDFRenderPicture.cpp) and be part of > pdf.gyp? > > 3. If the above don't seem right to you, maybe just add a brief comment in here > explaining the difference between the two targets. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:17: On 2012/10/25 15:39:56, epoger wrote: > Please include a description of what this file is for... "command-line wrapper > for PdfRenderer", maybe? Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:33: static void make_output_filepath(SkString* path, const SkString& dir, On 2012/10/25 15:39:56, epoger wrote: > please add a brief description of the parameters for this and all other > functions. Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:37: path->remove(path->size() - 3, 3); On 2012/10/25 15:39:56, epoger wrote: > Rather than just blindly removing the last 3 characters, I think it would be a > good idea to make sure you are removing input_file_extension (which the caller > should set to "skp"), whatever its length is. > > As the code stands now, what happens if you run this tool against a single > SkPicture file named "testfile"? I think it will write out the result as > "testf.pdf", which I doubt is the desired behavior. Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:45: make_output_filepath(&outputPath, outputDir, inputFilename, "pdf"); On 2012/10/25 15:39:56, epoger wrote: > rather than defining magic strings like "pdf" inline, please use constants that > are defined at the top of the file Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:67: SkAutoTUnref<SkPicture> aur(picture); On 2012/10/25 15:39:56, epoger wrote: > Could you replace these two lines with: > > SkAutoTUnref<SkPicture> picture(SkNEW_ARGS(SkPicture, (&inputStream, > &success))); > > There are two advantages: > 1. the AutoUnref object has a more descriptive name ("picture" instead of "aur") > 2. there is no chance that any new code that comes along later will try to use > the SkPicture* object directly > > > OR, if you prefer, just rename "aur" to something more descriptive (like > "autoUnrefPicture" or "pictureAutoUnref") Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:88: SkOSFile::Iter iter(input.c_str(), "skp"); On 2012/10/25 15:39:56, epoger wrote: > again, no magic strings inline please Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:91: if (iter.next(&inputFilename)) { On 2012/10/25 15:39:56, epoger wrote: > I guess this branch is used to check whether "inputFilename" refers to a > directory or a file? That seems fragile to me. What if it's a directory that > doesn't contain any .skp files? > > I think it would be less fragile, and easier to understand, if you just checked > directly: does this path refer to a file or a directory? Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:95: if (!render_pdf(inputPath, outputDir, renderer)) On 2012/10/25 15:39:56, epoger wrote: > please use curly braces for the "if" code block Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:101: ++failures; On 2012/10/25 15:39:56, epoger wrote: > curly braces please Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:114: SkDELETE(renderer); On 2012/10/25 15:39:56, epoger wrote: > Why is parse_commandline() in charge of deleting renderer if it exits? Wouldn't > it be cleaner/easier to parse the commandline BEFORE allocating any objects? Done. https://codereview.appspot.com/6615073/diff/3001/tools/render_pdfs_main.cpp#n... tools/render_pdfs_main.cpp:148: SkDELETE(renderer); On 2012/10/25 15:39:56, epoger wrote: > Seems like another good use for AutoUnref. Why make extra work for ourselves? Done.
Sign in to reply to this message.
https://codereview.appspot.com/6615073/diff/12001/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.appspot.com/6615073/diff/12001/tools/render_pdfs_main.cpp#... tools/render_pdfs_main.cpp:29: static const char PDF_FILE_EXTENTION[] = "pdf"; spelling throughout: extention -> extension https://codereview.appspot.com/6615073/diff/12001/tools/render_pdfs_main.cpp#... tools/render_pdfs_main.cpp:56: if (path->endsWith(SKP_FILE_EXTENTION)) { It seems strange to me that the OLD extension is hard-coded to SKP, but the NEW extension is passed in as a parameter. I think this would be cleaner: static bool replace_filename_extension(SkString* path, const char old_extension[], const char new_extension[]) { if (path->endsWith(old_extension)) { ... etc } static bool make_output_filepath(SkString* path, const SkString& dir, const SkString& name) { sk_tools::make_filepath(path, dir, name); return replace_filename_extension(path, SKP_FILE_EXTENSION, PDF_FILE_EXTENSION); }
Sign in to reply to this message.
https://codereview.appspot.com/6615073/diff/12001/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.appspot.com/6615073/diff/12001/tools/render_pdfs_main.cpp#... tools/render_pdfs_main.cpp:29: static const char PDF_FILE_EXTENTION[] = "pdf"; On 2012/11/01 15:58:40, epoger wrote: > spelling throughout: extention -> extension Done. https://codereview.appspot.com/6615073/diff/12001/tools/render_pdfs_main.cpp#... tools/render_pdfs_main.cpp:56: if (path->endsWith(SKP_FILE_EXTENTION)) { On 2012/11/01 15:58:40, epoger wrote: > It seems strange to me that the OLD extension is hard-coded to SKP, but the NEW > extension is passed in as a parameter. I think this would be cleaner: > > static bool replace_filename_extension(SkString* path, > const char old_extension[], > const char new_extension[]) { > if (path->endsWith(old_extension)) { > ... etc > } > > static bool make_output_filepath(SkString* path, const SkString& dir, > const SkString& name) { > sk_tools::make_filepath(path, dir, name); > return replace_filename_extension(path, SKP_FILE_EXTENSION, > PDF_FILE_EXTENSION); > } Done.
Sign in to reply to this message.
LGTM One optional thing... https://codereview.appspot.com/6615073/diff/18001/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.appspot.com/6615073/diff/18001/tools/render_pdfs_main.cpp#... tools/render_pdfs_main.cpp:60: return false; Optional: It's a little strange that the function modifies the path even in the failure case. ("filename.skp" -> "filename.pdf", but "filenameskp" -> "filename") I think it would be a good idea to either document that ("if false is returned, contents of path are undefined") or change it (only modify path if successful)
Sign in to reply to this message.
https://codereview.appspot.com/6615073/diff/18001/tools/render_pdfs_main.cpp File tools/render_pdfs_main.cpp (right): https://codereview.appspot.com/6615073/diff/18001/tools/render_pdfs_main.cpp#... tools/render_pdfs_main.cpp:60: return false; On 2012/11/01 19:46:44, epoger wrote: > Optional: It's a little strange that the function modifies the path even in the > failure case. ("filename.skp" -> "filename.pdf", but "filenameskp" -> > "filename") > > I think it would be a good idea to either document that ("if false is returned, > contents of path are undefined") or change it (only modify path if successful) Done.
Sign in to reply to this message.
|
