You are viewing a plain text version of this content. The canonical link for it is here.
Posted to batik-dev@xmlgraphics.apache.org by Keven Ring <ke...@mitre.org> on 2003/02/03 20:19:30 UTC
[Fwd: RE: Problem with Batik source code]
-------- Original Message --------
Subject: RE: Problem with Batik source code
Date: Mon, 3 Feb 2003 13:27:00 -0500
From: Thomas E Deweese <th...@kodak.com>
To: Keven Ring <ke...@mitre.org>
CC: deweese@apache.org
References: <3E...@mitre.org>
Hi Keven,
I would rather discuss this on batik-dev than off list, if
possible.
>>>>> "KR" == Keven Ring writes:
KR> We are using batik to render a large (>2000) SVG image set. These
KR> images consist of references to other SVGs, for compositional
KR> purposes. We plan on distributing the SVGs in a jar file, due
KR> both to the large number of images, and because we plan on
KR> utilizing JNLP for distribution purposes. The images are grouped
KR> into categories and subcategories appropriately, with each
KR> category represented by a directory.
Sounds cool.
KR> When we originally developed the image set, we were rendering
KR> the icons as they resided on the filesystem, ie, not in a JAR
KR> file. As a result, the icons had relative path locations to the
KR> other SVG components. These relative paths often went up one or
KR> more directories, via the standard '..' notation.
KR> When we put the icons in a JAR file, the previous directory
KR> notation ['..'] no longer applied.
Why? I'm under the impression that relative references should
work within a JAR URL...
KR> We converted all of the references to use absolute paths from the
KR> context of the jar file itself. Thus, all xlink targets start
KR> with a 'root directory' notation, or '/'.
KR> None of the icons rendered appropriately in batik when we did
KR> this. We tracked it down to the way that the
KR> ParsedURLDefaultProtocolHandler constructed 'relative' URLs,
KR> specifically relating to jar files. Because it appeared that only
KR> jar files were affected, we made a change to the
KR> ParsedURLJarProtocolHandler. The patch for this file is attached.
Thanks, I'm more or less happy to accept a change (I have some
comments on it below). I knew that I wasn't really handling the
absolute case.
KR> Specifically, the default protocol handler removed the jar file
KR> reference (ie, the in the syntax: jar: jar
file> !) when a relative path was provided. We
file> believe
KR> this to be incorrect, and instead, the should
KR> be replaced or appended to (depending on the absolute or relative
KR> nature of the path provided).
Well I'm 99% certain that relative paths work just as you
describe. There are a bunch of existing tests for this in the
ParsedURL testSuite (see
'xml-batik/test-resources/org/apache/batik/util/unitTesting.xml' tests
'ParsedURL.15' -> 'ParsedURL.21').
Could you please provide additional tests for absolute JAR sub
URL's? This will help ensure that you have covered all cases and that
we don't break anything you are depending on in the future.
In order to run regard (our test tool) you will need to do
one of two things.
1) comment out the line:
In 'xml-batik/test-resources/org/apache/batik/test/regard.xml'.
2) Get a copy of the SVG Working Groups BE test suite. and unpack
it in the same directory as xml-batik (so beSuite is a sibling
of xml-batik).
Then to run the ParsedURL tests simply use:
% build.sh regard ParsedURL
---
Comments on the code:
I'm a little disappointed with the use of java.net.URL here (as
opposed to directly handling the absolute and relative cases),
although I've most convinced myself that this doesn't introduce a
significant compatability issue (given the data protocol URL
requiremenst of SVG, java.net.URL makes me twichy :).
As you seem to suspect I might, I really don't like the openStream
call. Do you have a case that you expect to encounter here? If not I
would rather drop it.
Finally, you are better off using 'constructParsedURLData(url)'
rather than calling 'parseURL(url.toExternalForm())'.
+ // It's relative so base it off baseURL
+ try {
+ java.net.URL context = new java.net.URL(baseURL.toString());
+ java.net.URL url = new java.net.URL(context, urlStr);
+ // The following two lines simply verify that the URL is valid.
+ // Strictly speaking, they are not required, although it allows
+ // us to have a fallback position.
+ java.io.InputStream is = url.openStream();
+ is.close();
+ pd = parseURL(url.toExternalForm());
+ }
+ catch (Exception e) {
+ pd = super.parseURL(baseURL, urlStr);
+ }
--
Keven Ring | They called it Paradise, I don't know why
Lead Software Systems Engineer | When you call someplace Paradise
The MITRE Corporation | Kiss it goodbye....
(703)883-7026 | The Eagles, The Last Resort
Re: [Fwd: RE: Problem with Batik source code]
Posted by Keven Ring <ke...@mitre.org>.
Thomas E Deweese wrote:
>>>>>>"KR" == Keven Ring <ke...@mitre.org> writes:
>>>>>>
>>>>>>
>
>
>
>>> Could you please provide additional tests for absolute JAR sub
>>>URL's? This will help ensure that you have covered all cases and
>>>that we don't break anything you are depending on in the future.
>>>
>>>
>
>KR> Sure. How would you like me to supply additional tests? Via this
>KR> reflector?
>
> Yes, that would be great.
>
>KR> I'll let you know when I have the tests up and working.
>
> Thanks.
>
>
>
>>>Comments on the code:
>>>
>>>I'm a little disappointed with the use of java.net.URL here (as
>>>opposed to directly handling the absolute and relative cases),
>>>although I've most convinced myself that this doesn't introduce a
>>>significant compatability issue (given the data protocol URL
>>>requiremenst of SVG, java.net.URL makes me twichy :).
>>>
>>>
>
>KR> I fully understand [ yes, I did read the javadoc comments
>KR> regarding ParsedURL, and why it exists ]. However, I was in a bit
>KR> of a dilema - I could parse it myself, and construct a suitable
>KR> URI, but since the JAR URL specification is really Sun's
>KR> specification, I would rather 'trust' their implementation of
>KR> constructing a new JAR URL [irrespective of the type of URL being
>KR> constructed, whether it is absolute, absolute in the same JAR, or
>KR> relative in the same JAR] than attempting to write a [possibly
>KR> poor] substitute, and at the same time, not have to worry about
>KR> possible format specification changes by Sun.
>
>KR> Because ParsedURL performs the same type of functionality as the
>KR> java URL class, if ParsedURL was modified to support these three
>KR> classes of modifications to an existing ParsedURL, then, I agree,
>KR> it would be the cleanest approach. However, because other
>KR> protocols require ParsedURL to operate correctly, I was hesitant
>KR> to make the necessary modifications, somewhat due to the
>KR> unwillingness (at the time) to fully understand what ParsedURL was
>KR> doing in the plethora of cases.
>
> As I said I don't think it's a huge problem (I don't think a
>'jar:data:xxxx!foo.svg' is something that we really need to deal with
>at this point).
>
>
To be honest, I'm not sure I even know what this is... :)
>
>
>>>As you seem to suspect I might, I really don't like the openStream
>>>call. Do you have a case that you expect to encounter here? If not
>>>I would rather drop it.
>>>
>>>
>
>KR> The case that I wanted to support, which, in the long run, isn't
>KR> very important, is the ability to fall back to the original URL
>KR> merging algorithm. Assuming that the overall issue is resolved,
>KR> then the opening and closing of the resource is certainly
>KR> superflous.
>
> I guess I was asking if you had a specific type of input that you
>thought the original code handled and the new code wouldn't.
>
>
Nope. Simply a CYA check.
> I ran the regard tests with your update any except for improved
>results from one of the relative cases
>("jar:file:dir/file.jar!/p/a/t/h/../base.svg" ->
>"jar:file:dir/file.jar!/p/a/t/base.svg" - which means that relative
>references will work now :) everything gave the same results. So the
>patch is looking pretty good without this check.
>
Great! I'll add the absolute path checks, and run a 'before and after'
change modification, and post the additional tests, and the before and
after to the reflector.
>
>
>
>>> Finally, you are better off using 'constructParsedURLData(url)'
>>>rather than calling 'parseURL(url.toExternalForm())'.
>>>
>>>
>
>KR> Great! If there is a preferred/more efficient way of constructing
>KR> a ParsedURL instance given a URL, then, by all means, make
>KR> changes.
>
> I made this change.
>
>KR> My purpose for posting the original message, and the submitted
>KR> change, was simply to identify an issue, and to provide a possible
>KR> solution. The solution is based on my knowledge and understand of
>KR> Batik, as well as the time available to investigate the matter. I
>KR> welcome the inclusion, or modification of the solution, more to
>KR> further extend the capabilities of the entire system.
>
> Sure I understand, of course part of my job is to get the people
>who "feel the itch" to finish the job (a few tests please ;) so everyone
>can use it.
>
> Thanks for the help!
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: batik-dev-unsubscribe@xml.apache.org
>For additional commands, e-mail: batik-dev-help@xml.apache.org
>
>
>
--
Keven Ring | They called it Paradise, I don't know why
Lead Software Systems Engineer | When you call someplace Paradise
The MITRE Corporation | Kiss it goodbye....
(703)883-7026 | The Eagles, The Last Resort
Re: [Fwd: RE: Problem with Batik source code]
Posted by Keven Ring <ke...@mitre.org>.
Thomas E Deweese wrote:
>>>>>>"KR" == Keven Ring <ke...@mitre.org> writes:
>>>>>>
>>>>>>
>
>KR> Attached are the ParsedURL tests which I added, along with the
>KR> html results from the original code. Also, I have attached the
>KR> changes to the code which you have suggested.
>
>KR> If there is anything else, please let me know!
>
> No, this looks good. It be committed shortly[1]
>
> Thanks,
>
>
For the $64000 question, which official release will this likely be in?
B5? When is that release scheduled?
Thanks!
>---
>
>[1] Shortly is a relative term, that in this case may mean up to a week
> or more, due to temporary CVS issues :/
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: batik-dev-unsubscribe@xml.apache.org
>For additional commands, e-mail: batik-dev-help@xml.apache.org
>
>
>
--
Keven Ring | They called it Paradise,
Lead Software Systems Engineer | I don't know why
The MITRE Corporation | Call someplace Paradise
(703)883-7026 | and kiss it goodbye..
Re: [Fwd: RE: Problem with Batik source code]
Posted by Thomas E Deweese <th...@kodak.com>.
>>>>> "KR" == Keven Ring <ke...@mitre.org> writes:
KR> Attached are the ParsedURL tests which I added, along with the
KR> html results from the original code. Also, I have attached the
KR> changes to the code which you have suggested.
KR> If there is anything else, please let me know!
No, this looks good. It be committed shortly[1]
Thanks,
---
[1] Shortly is a relative term, that in this case may mean up to a week
or more, due to temporary CVS issues :/
---------------------------------------------------------------------
To unsubscribe, e-mail: batik-dev-unsubscribe@xml.apache.org
For additional commands, e-mail: batik-dev-help@xml.apache.org
Re: [Fwd: RE: Problem with Batik source code]
Posted by Keven Ring <ke...@mitre.org>.
Thomas E Deweese wrote:
>>>>>>"KR" == Keven Ring <ke...@mitre.org> writes:
>>>>>>
>>>>>>
>
>
>
>>> Could you please provide additional tests for absolute JAR sub
>>>URL's? This will help ensure that you have covered all cases and
>>>that we don't break anything you are depending on in the future.
>>>
>>>
>
>KR> Sure. How would you like me to supply additional tests? Via this
>KR> reflector?
>
> Yes, that would be great.
>
>KR> I'll let you know when I have the tests up and working.
>
> Thanks.
>
>
>
Attached are the ParsedURL tests which I added, along with the html
results from the original code. Also, I have attached the changes to
the code which you have suggested.
If there is anything else, please let me know!
--
Keven Ring | They called it Paradise, I don't know why
Lead Software Systems Engineer | When you call someplace Paradise
The MITRE Corporation | Kiss it goodbye....
(703)883-7026 | The Eagles, The Last Resort
Re: [Fwd: RE: Problem with Batik source code]
Posted by Thomas E Deweese <th...@kodak.com>.
>>>>> "KR" == Keven Ring <ke...@mitre.org> writes:
>> Could you please provide additional tests for absolute JAR sub
>> URL's? This will help ensure that you have covered all cases and
>> that we don't break anything you are depending on in the future.
KR> Sure. How would you like me to supply additional tests? Via this
KR> reflector?
Yes, that would be great.
KR> I'll let you know when I have the tests up and working.
Thanks.
>> Comments on the code:
>>
>> I'm a little disappointed with the use of java.net.URL here (as
>> opposed to directly handling the absolute and relative cases),
>> although I've most convinced myself that this doesn't introduce a
>> significant compatability issue (given the data protocol URL
>> requiremenst of SVG, java.net.URL makes me twichy :).
KR> I fully understand [ yes, I did read the javadoc comments
KR> regarding ParsedURL, and why it exists ]. However, I was in a bit
KR> of a dilema - I could parse it myself, and construct a suitable
KR> URI, but since the JAR URL specification is really Sun's
KR> specification, I would rather 'trust' their implementation of
KR> constructing a new JAR URL [irrespective of the type of URL being
KR> constructed, whether it is absolute, absolute in the same JAR, or
KR> relative in the same JAR] than attempting to write a [possibly
KR> poor] substitute, and at the same time, not have to worry about
KR> possible format specification changes by Sun.
KR> Because ParsedURL performs the same type of functionality as the
KR> java URL class, if ParsedURL was modified to support these three
KR> classes of modifications to an existing ParsedURL, then, I agree,
KR> it would be the cleanest approach. However, because other
KR> protocols require ParsedURL to operate correctly, I was hesitant
KR> to make the necessary modifications, somewhat due to the
KR> unwillingness (at the time) to fully understand what ParsedURL was
KR> doing in the plethora of cases.
As I said I don't think it's a huge problem (I don't think a
'jar:data:xxxx!foo.svg' is something that we really need to deal with
at this point).
>> As you seem to suspect I might, I really don't like the openStream
>> call. Do you have a case that you expect to encounter here? If not
>> I would rather drop it.
KR> The case that I wanted to support, which, in the long run, isn't
KR> very important, is the ability to fall back to the original URL
KR> merging algorithm. Assuming that the overall issue is resolved,
KR> then the opening and closing of the resource is certainly
KR> superflous.
I guess I was asking if you had a specific type of input that you
thought the original code handled and the new code wouldn't.
I ran the regard tests with your update any except for improved
results from one of the relative cases
("jar:file:dir/file.jar!/p/a/t/h/../base.svg" ->
"jar:file:dir/file.jar!/p/a/t/base.svg" - which means that relative
references will work now :) everything gave the same results. So the
patch is looking pretty good without this check.
>> Finally, you are better off using 'constructParsedURLData(url)'
>> rather than calling 'parseURL(url.toExternalForm())'.
KR> Great! If there is a preferred/more efficient way of constructing
KR> a ParsedURL instance given a URL, then, by all means, make
KR> changes.
I made this change.
KR> My purpose for posting the original message, and the submitted
KR> change, was simply to identify an issue, and to provide a possible
KR> solution. The solution is based on my knowledge and understand of
KR> Batik, as well as the time available to investigate the matter. I
KR> welcome the inclusion, or modification of the solution, more to
KR> further extend the capabilities of the entire system.
Sure I understand, of course part of my job is to get the people
who "feel the itch" to finish the job (a few tests please ;) so everyone
can use it.
Thanks for the help!
---------------------------------------------------------------------
To unsubscribe, e-mail: batik-dev-unsubscribe@xml.apache.org
For additional commands, e-mail: batik-dev-help@xml.apache.org
Re: [Fwd: RE: Problem with Batik source code]
Posted by Keven Ring <ke...@mitre.org>.
>
>
> -------- Original Message --------
> Subject: RE: Problem with Batik source code
> Date: Mon, 3 Feb 2003 13:27:00 -0500
> From: Thomas E Deweese <th...@kodak.com>
> To: Keven Ring <ke...@mitre.org>
> CC: deweese@apache.org
> References: <3E...@mitre.org>
>
>
>
>Hi Keven,
>
> I would rather discuss this on batik-dev than off list, if
>possible.
>
>>>>>> "KR" == Keven Ring writes:
>
>KR> We are using batik to render a large (>2000) SVG image set. These
>KR> images consist of references to other SVGs, for compositional
>KR> purposes. We plan on distributing the SVGs in a jar file, due
>KR> both to the large number of images, and because we plan on
>KR> utilizing JNLP for distribution purposes. The images are grouped
>KR> into categories and subcategories appropriately, with each
>KR> category represented by a directory.
>
> Sounds cool.
>
>KR> When we originally developed the image set, we were rendering
>KR> the icons as they resided on the filesystem, ie, not in a JAR
>KR> file. As a result, the icons had relative path locations to the
>KR> other SVG components. These relative paths often went up one or
>KR> more directories, via the standard '..' notation.
>
>KR> When we put the icons in a JAR file, the previous directory
>KR> notation ['..'] no longer applied.
>
> Why? I'm under the impression that relative references should
>work within a JAR URL...
>
Sun's implementation of the JAR file specification does not support
them, from what I can tell. Specifically, given a JAR file, and a file
which exists in that JAR file, say /foo/bar.txt, one can look up the
resource /foo/bar.txt, but not /foo/../foo/bar.txt. I suspect that the
reason for this is that there is no entry in the JAR/ZIP file called
/foo/../foo/bar.txt.
>
>
>KR> We converted all of the references to use absolute paths from the
>KR> context of the jar file itself. Thus, all xlink targets start
>KR> with a 'root directory' notation, or '/'.
>
>KR> None of the icons rendered appropriately in batik when we did
>KR> this. We tracked it down to the way that the
>KR> ParsedURLDefaultProtocolHandler constructed 'relative' URLs,
>KR> specifically relating to jar files. Because it appeared that only
>KR> jar files were affected, we made a change to the
>KR> ParsedURLJarProtocolHandler. The patch for this file is attached.
>
> Thanks, I'm more or less happy to accept a change (I have some
>comments on it below). I knew that I wasn't really handling the
>absolute case.
>
>KR> Specifically, the default protocol handler removed the jar file
>KR> reference (ie, the in the syntax: jar: jar
>file> !) when a relative path was provided. We
>file> believe
>KR> this to be incorrect, and instead, the should
>KR> be replaced or appended to (depending on the absolute or relative
>KR> nature of the path provided).
>
> Well I'm 99% certain that relative paths work just as you
>describe. There are a bunch of existing tests for this in the
>ParsedURL testSuite (see
>'xml-batik/test-resources/org/apache/batik/util/unitTesting.xml' tests
>'ParsedURL.15' -> 'ParsedURL.21').
>
I will double check the problems that we had. I certainly could be wrong!
>
> Could you please provide additional tests for absolute JAR sub
>URL's? This will help ensure that you have covered all cases and that
>we don't break anything you are depending on in the future.
>
Sure. How would you like me to supply additional tests? Via this
reflector?
>
> In order to run regard (our test tool) you will need to do
>one of two things.
>
> 1) comment out the line:
>
>
>
> In 'xml-batik/test-resources/org/apache/batik/test/regard.xml'.
>
> 2) Get a copy of the SVG Working Groups BE test suite. and unpack
> it in the same directory as xml-batik (so beSuite is a sibling
> of xml-batik).
>
> Then to run the ParsedURL tests simply use:
>
> % build.sh regard ParsedURL
>
I'll let you know when I have the tests up and working.
>
>---
>
>Comments on the code:
>
> I'm a little disappointed with the use of java.net.URL here (as
>opposed to directly handling the absolute and relative cases),
>although I've most convinced myself that this doesn't introduce a
>significant compatability issue (given the data protocol URL
>requiremenst of SVG, java.net.URL makes me twichy :).
>
I fully understand [ yes, I did read the javadoc comments regarding
ParsedURL, and why it exists ]. However, I was in a bit of a dilema - I
could parse it myself, and construct a suitable URI, but since the JAR
URL specification is really Sun's specification, I would rather 'trust'
their implementation of constructing a new JAR URL [irrespective of the
type of URL being constructed, whether it is absolute, absolute in the
same JAR, or relative in the same JAR] than attempting to write a
[possibly poor] substitute, and at the same time, not have to worry
about possible format specification changes by Sun.
Because ParsedURL performs the same type of functionality as the java
URL class, if ParsedURL was modified to support these three classes of
modifications to an existing ParsedURL, then, I agree, it would be the
cleanest approach. However, because other protocols require ParsedURL
to operate correctly, I was hesitant to make the necessary
modifications, somewhat due to the unwillingness (at the time) to fully
understand what ParsedURL was doing in the plethora of cases.
>
>
> As you seem to suspect I might, I really don't like the openStream
>call. Do you have a case that you expect to encounter here? If not I
>would rather drop it.
>
The case that I wanted to support, which, in the long run, isn't very
important, is the ability to fall back to the original URL merging
algorithm. Assuming that the overall issue is resolved, then the
opening and closing of the resource is certainly superflous.
>
> Finally, you are better off using 'constructParsedURLData(url)'
>rather than calling 'parseURL(url.toExternalForm())'.
>
Great! If there is a preferred/more efficient way of constructing a
ParsedURL instance given a URL, then, by all means, make changes.
My purpose for posting the original message, and the submitted change,
was simply to identify an issue, and to provide a possible solution.
The solution is based on my knowledge and understand of Batik, as well
as the time available to investigate the matter. I welcome the
inclusion, or modification of the solution, more to further extend the
capabilities of the entire system.
Hope this helps!
>
>
>+ // It's relative so base it off baseURL
>+ try {
>+ java.net.URL context = new java.net.URL(baseURL.toString());
>+ java.net.URL url = new java.net.URL(context, urlStr);
>+ // The following two lines simply verify that the URL is valid.
>+ // Strictly speaking, they are not required, although it allows
>+ // us to have a fallback position.
>+ java.io.InputStream is = url.openStream();
>+ is.close();
>+ pd = parseURL(url.toExternalForm());
>+ }
>+ catch (Exception e) {
>+ pd = super.parseURL(baseURL, urlStr);
>+ }
>
>
>--
>Keven Ring | They called it Paradise, I don't know why
>Lead Software Systems Engineer | When you call someplace Paradise
>The MITRE Corporation | Kiss it goodbye....
>(703)883-7026 | The Eagles, The Last Resort
>
--
Keven Ring | They called it Paradise, I don't know why
Lead Software Systems Engineer | When you call someplace Paradise
The MITRE Corporation | Kiss it goodbye....
(703)883-7026 | The Eagles, The Last Resort