script for embedding raster images
The following patch to sp-image.cpp allows Inkscape to read Adobe Illustrator files that have the "data:;base64" syntax for embedded images. The embedded images are displayed correctly on the screen. However, if you save the file, the resulting svg output still has "data:;base64", which is obviously a Bad Thing. I started looking around in the code for where the output happens, but C++ isn't a language I'm very comfortable with, and I rapidly got lost. I'd be grateful for any insight from anyone who understands the code and the language better. I'd prefer not to commit the patch for the input without making the output standards-compliant.
===================== patch to sp-image.cpp =======================
902a903,913
// bcrowell, 2004 dec 13: // Adobe Illustrator writes output like this: // data:;base64, // This is illegal according to the SVG standard, but there seems to be a consensus // on inkscape-devel that Inkscape should allow input like this anyway. It would // be nice to do a better syntax check here, but Lauris Kaplinski's old-school C parser // is already pretty complicated; probably we should be using a C++ regular expression // library instead. The following line allows input of lame Illustrator SVG: data_is_image = 1; // However, the output isn't any more correct than the input.
Quoting Ben Crowell <inkscapecrowell04@...603...>:
The following patch to sp-image.cpp allows Inkscape to read Adobe Illustrator files that have the "data:;base64" syntax for embedded images. The embedded images are displayed correctly on the screen. However, if you save the file, the resulting svg output still has "data:;base64", which is obviously a Bad Thing.
I can take a look at things later tonight.
I've also been in that code recently, so I have some ideas on how it should get hooked in. Hopefully what I remember still applies. :-)
BTW, personally I think that even if a patch is tiny, you can go ahead and use Sourceforge's interface. If nothing else, it allows a nice way of tracking what goes in when and how.
This is probably unrelated, but loading images normally crashes now. dir-utils.cpp:30 is dying from a NULL value for 'base.'
Bob
jon@...18... wrote:
Quoting Ben Crowell <inkscapecrowell04@...603...>:
The following patch to sp-image.cpp allows Inkscape to read Adobe Illustrator files that have the "data:;base64" syntax for embedded images. The embedded images are displayed correctly on the screen. However, if you save the file, the resulting svg output still has "data:;base64", which is obviously a Bad Thing.
I can take a look at things later tonight.
I've also been in that code recently, so I have some ideas on how it should get hooked in. Hopefully what I remember still applies. :-)
BTW, personally I think that even if a patch is tiny, you can go ahead and use Sourceforge's interface. If nothing else, it allows a nice way of tracking what goes in when and how.
SF email is sponsored by - The IT Product Guide Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now. http://productguide.itmanagersjournal.com/ _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On Wed, 15 Dec 2004 11:49:09 -0600, Bob Jamison <rjamison@...357...> wrote:
This is probably unrelated, but loading images normally crashes now. dir-utils.cpp:30 is dying from a NULL value for 'base.'
Latest CVS still craches on bitmap import. Looks like the person who touched this code recently was pjrm. Peter, could you please look into this? And please test your changes thoroughly before committing. (Of course errors happen, but this is a very obvious bug which should have been caught before committing).
On Thu, Dec 16, 2004 at 05:41:32AM -0400, bulia byak wrote:
crashes now. dir-utils.cpp:30 is dying from a NULL value for 'base.'
Latest CVS still craches on bitmap import. Looks like the person who touched this code recently was pjrm. Peter, could you please look into this? And please test your changes thoroughly before committing. (Of course errors happen, but this is a very obvious bug which should have been caught before committing).
I did indeed test before committing, both by using inkscape and ensuring that the changed code was executed, and even by creating a new unit test program (dir-util-test.cpp). To detect this bug, it isn't sufficient to test the changed code or even to test every caller of the changed code. I suggest that this bug is not caused by inappropriately little testing by the developer.
What then can we learn from this bug in terms of how to change our development practice?
One response would be: "The bug has been found in the normal course of use, and is believed now fixed. We needn't change our development practice."
Another response would be to encourage greater peer review. For this bug, a reviewer of the diff might have noticed the change in behaviour for base==NULL, and might question whether this change affected any callers.
The cost would be more time spent reviewing instead of developing, and more time spent responding to reviewer suggestions. In one project I know of (developing a compiler, so they have higher quality obligations than inkscape does), they require all non-trivial diffs to be posted to a mailing list and reviewed & OK'd by someone before committing. (Some simple/important changes can be committed before being OK'd, but still require posting to the mailing list.) If we do go with the `review more' approach, then I suggest that we start much lower key, merely encouraging ppl to do `cvs -q diff -dpU5 -r BASE -r HEAD' and read the diffs.
Incidentally, please compile your changes before committing, and look for any introduced warnings. E.g. this would have alerted Bulia to a bug he introduced a few days ago which I've now fixed.
Btw, I won't be around for the next week or two. Visiting my parents in the country.
pjrm.
On Fri, 17 Dec 2004 11:54:38 +1100, Peter Moulder <Peter.Moulder@...38...> wrote:
Latest CVS still craches on bitmap import. Looks like the person who touched this code recently was pjrm. Peter, could you please look into this? And please test your changes thoroughly before committing. (Of course errors happen, but this is a very obvious bug which should have been caught before committing).
I did indeed test before committing, both by using inkscape and ensuring that the changed code was executed, and even by creating a new unit test program (dir-util-test.cpp). To detect this bug, it isn't sufficient to test the changed code or even to test every caller of the changed code. I suggest that this bug is not caused by inappropriately little testing by the developer.
I did not mean to offend. It just seemed to me that both calling a function with NULL (for automated testing) and importing bitmaps (for workflow testing) are very obvious things to check. Maybe I was wrong. I'm not advocating any change in our process, just being more careful.
Incidentally, please compile your changes before committing, and look for any introduced warnings. E.g. this would have alerted Bulia to a bug he introduced a few days ago which I've now fixed.
Yeah, but to be fair, that bug was in debug code that does not normally run, and it would not crash anyway :)
participants (5)
-
unknown＠example.com -
Ben Crowell -
Bob Jamison -
bulia byak -
Peter Moulder