12:51 p.m.
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.
7:17 p.m.
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.
12:49 p.m.
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(a)lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/inkscape-devel
4:41 a.m.
New subject: crash importing raster images
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).
7:54 p.m.
New subject: quality effort? Re: crash importing raster images
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.
4:38 a.m.
New subject: quality effort? Re: crash importing raster images
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 :)
6864
days inactive
6868
days old
inkscape-devel@lists.inkscape.org
5 comments
5 participants
participants (5)
-
unknown＠example.com -
Ben Crowell -
Bob Jamison -
bulia byak -
Peter Moulder