Bug 46072

Created attachment 22786 [details]
First attempt patch to allow changing location with script.

I implemented the suggestions made by Cameron McCormack in hopes of giving the ability to assign to window.location to load a new file.

To complete this change, I would need to make a wrapper class for the location (I think) to make it scriptable and return it from getLocation(). Then I could implement the assign() and reload() methods.

However, this is functional and some feedback/review of the implementation is appreciated.

(In reply to comment #1)
> Created an attachment (id=22786) [details]
> First attempt patch to allow changing location with script.

Cool! :-)



> + * @version $Id: Loaction.java$

Sounds like a typo.



> null == location

I'd suggest the reverse (location == null) as it's more intuitive and seems to match the code 



> +     * @param url A string containing the URL where the user agent should
> +     *    navigate.

"should navigate [to]."? ;-)



> However, this is functional and some feedback/review of the implementation is
> appreciated. 

Intuitively it looks well, and I may help testing whenever/if this makes it into the trunk and/or if someone with more expertise (such as Cameron or Thomas, for example) states this looks good enough (by applying the patch locally). Sorry for not being able to provide a deeper review but I'm not that familiar with Batik codebase yet. ;-)

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=22786) [details] [details]
> > First attempt patch to allow changing location with script.
> 
> Cool! :-)
> 
> 
> 
> > + * @version $Id: Loaction.java$
> 
> Sounds like a typo.

Yeah. I've changed my copy. I'll send a new patch once I've got the LocationWrapper thing worked out.

> 
> > null == location
> 
> I'd suggest the reverse (location == null) as it's more intuitive and seems to
> match the code 

Since 'location == null' seems to be the convention, I'll switch to that. But as an old C, C++, etc. programmer I'd argue that it's not more "intuitive".
(Then again, I've been known to argue that nothing in programming is intuitive, but that's another discussion.<grin/>)

> > +     * @param url A string containing the URL where the user agent should
> > +     *    navigate.
> 
> "should navigate [to]."? ;-)

I was trying to avoid that for more formal English, but it does make more sense.
I've changed it in my copy.

> > However, this is functional and some feedback/review of the implementation is
> > appreciated. 
> 
> Intuitively it looks well, and I may help testing whenever/if this makes it
> into the trunk and/or if someone with more expertise (such as Cameron or
> Thomas, for example) states this looks good enough (by applying the patch
> locally). Sorry for not being able to provide a deeper review but I'm not that
> familiar with Batik codebase yet. ;-)

Thank you for the help. As I said, I'll have a new patch soon.

Created attachment 22794 [details]
Contains the location changes from the previous patch and the new LocationWrapper code.

In addition to including the previous code changes, this patch fixes the following:

1. Style issues and typos noted in Helder Magalhães's review.
2. Correcting compile issues that I didn't notice because I forgot how Ant handles dependencies.
3. Addition of new LocationWrapper that allows access to assign(url) and reload() methods on Location object from script as well as assignment through =.

This appears to cover everything needed to address my issue.

The change does not refactor the Window class to depend on the Global class as in the 1.2 specification.

Created attachment 22795 [details]
The real patch including files that were added.

This is the real patch. I forgot to included added files when I made the other patch.

Comment on attachment 22795 [details]
The real patch including files that were added.

Hi G. Wade.

Thanks for working on the patch.  I have a few comments.  (Some are just style/formatting, and although there isn't a documented coding style, there's a de facto one that most of the code adheres to.)

>Index: sources/org/apache/batik/swing/svg/JSVGComponent.java
...
>-        }
>+        } 

Looks like an accidental space on the end of the line, which doesn't need to be there.

>Index: sources/org/apache/batik/bridge/Location.java
...
>+        ((UserAgent)bridgeContext.getUserAgent()).loadDocument( url );

No spaces around "url".

>+    public void reload() {
>+        URL url = ((SVGOMDocument) bridgeContext.getDocument())
>+                    .getURLObject();
>+        ((UserAgent)bridgeContext.getUserAgent()).loadDocument( url.toString() );

You can use getDocumentURI() on AbstractDocument to get the the current document's URI more directly than getting the URL object and converting it to a string.  Note that you'll need to cast the returned Document to AbstractDocument so that the code will compile properly on JDKs that don't have the DOM Level 3 Core interfaces (since getDocumentURI() is a DOM 3 method).

Unnecessary spaces around the loadDocument() argument here, too.

>+    }
>+
>+    /**
>+     * Returns the URL of this location as a String.
>+     */
>+    public String toString() {
>+        URL url = ((SVGOMDocument) bridgeContext.getDocument())
>+                    .getURLObject();
>+        return url.toString();

Same as above; use getDocumentURI() instead.

>Index: sources/org/apache/batik/bridge/ScriptingEnvironment.java
...
>         public Window(Interpreter interp, String lang) {
>             interpreter = interp;
>             language = lang;
>+            location = null;

This line is unnecessary, since location will be initialised to null automatically.

>+        public Location getLocation() {
>+            if( location == null ) {

Write this as 'if (location == null) {'.

>+                location = new Location( bridgeContext );

Remove the spaces.

>Index: sources/org/apache/batik/script/rhino/WindowWrapper.java
...
>+        this.defineProperty("location", WindowWrapper.class,
>+                            ScriptableObject.PERMANENT );

Errant space before the ')'.

>     /**
>+     * Return the Location for this Window.
>+     */
>+    public LocationWrapper getLocation() {
>+        return new LocationWrapper( window.getLocation() );
>+    }

I don't think a wrapper class for the Location object is needed.  Wrapper classes are useful if you need to give the JS object some special behaviour that you can't get from the automatic reflection of Java objects to their JS counterparts.  Since the Location object is just a couple of simple methods, this shouldn't be needed, so you can make this method just return the Location object directly.

>+++ sources/org/apache/batik/script/rhino/LocationWrapper.java	(revision 0)

This class then wouldn't be needed.

>Index: sources/org/apache/batik/script/Window.java
...
>+import org.w3c.dom.Location;

This import isn't used.


With these addressed I'd be happy to check in the patch.  I see on http://people.apache.org/~jim/committers.html that you don't have a Contributor License Agreemnt on file (see http://www.apache.org/licenses/#clas).  You'll need to submit one of those before I can commit the patch.

(In reply to comment #6)

Thanks Cameron,

I'll modify the code and update the patch this evening.

> (From update of attachment 22795 [details])
> Hi G. Wade.
> 
> Thanks for working on the patch.  I have a few comments.  (Some are just
> style/formatting, and although there isn't a documented coding style, there's a
> de facto one that most of the code adheres to.)
> 
> >Index: sources/org/apache/batik/swing/svg/JSVGComponent.java
> ...
> >-        }
> >+        } 
> 
> Looks like an accidental space on the end of the line, which doesn't need to be
> there.
> 
> >Index: sources/org/apache/batik/bridge/Location.java
> ...
> >+        ((UserAgent)bridgeContext.getUserAgent()).loadDocument( url );
> 
> No spaces around "url".
> 
> >+    public void reload() {
> >+        URL url = ((SVGOMDocument) bridgeContext.getDocument())
> >+                    .getURLObject();
> >+        ((UserAgent)bridgeContext.getUserAgent()).loadDocument( url.toString() );
> 
> You can use getDocumentURI() on AbstractDocument to get the the current
> document's URI more directly than getting the URL object and converting it to a
> string.  Note that you'll need to cast the returned Document to

Seems like I tried that and it didn't work. But, I may be misremembering.

> AbstractDocument so that the code will compile properly on JDKs that don't have
> the DOM Level 3 Core interfaces (since getDocumentURI() is a DOM 3 method).
> 
> Unnecessary spaces around the loadDocument() argument here, too.
> 
> >+    }
> >+
> >+    /**
> >+     * Returns the URL of this location as a String.
> >+     */
> >+    public String toString() {
> >+        URL url = ((SVGOMDocument) bridgeContext.getDocument())
> >+                    .getURLObject();
> >+        return url.toString();
> 
> Same as above; use getDocumentURI() instead.
> 
> >Index: sources/org/apache/batik/bridge/ScriptingEnvironment.java
> ...
> >         public Window(Interpreter interp, String lang) {
> >             interpreter = interp;
> >             language = lang;
> >+            location = null;
> 
> This line is unnecessary, since location will be initialised to null
> automatically.

Habit. I tend to be explicit about these things. But, I'll change it.

> >+        public Location getLocation() {
> >+            if( location == null ) {
> 
> Write this as 'if (location == null) {'.
> 
> >+                location = new Location( bridgeContext );
> 
> Remove the spaces.
> 
> >Index: sources/org/apache/batik/script/rhino/WindowWrapper.java
> ...
> >+        this.defineProperty("location", WindowWrapper.class,
> >+                            ScriptableObject.PERMANENT );
> 
> Errant space before the ')'.
> 
> >     /**
> >+     * Return the Location for this Window.
> >+     */
> >+    public LocationWrapper getLocation() {
> >+        return new LocationWrapper( window.getLocation() );
> >+    }
> 
> I don't think a wrapper class for the Location object is needed.  Wrapper
> classes are useful if you need to give the JS object some special behaviour
> that you can't get from the automatic reflection of Java objects to their JS
> counterparts.  Since the Location object is just a couple of simple methods,
> this shouldn't be needed, so you can make this method just return the Location
> object directly.

I know I tried that and could not get it to work. Rhino kept reporting that it couldn't find the methods. I'll try it again, maybe I did something stupid. 

> >+++ sources/org/apache/batik/script/rhino/LocationWrapper.java	(revision 0)
> 
> This class then wouldn't be needed.
> 
> >Index: sources/org/apache/batik/script/Window.java
> ...
> >+import org.w3c.dom.Location;
> 
> This import isn't used.

Okay.

> With these addressed I'd be happy to check in the patch.  I see on
> http://people.apache.org/~jim/committers.html that you don't have a Contributor
> License Agreemnt on file (see http://www.apache.org/licenses/#clas).  You'll
> need to submit one of those before I can commit the patch.

I'll take care of it tonight. 

Thanks.
G. Wade

Created attachment 22837 [details]
Patch with corrections for Cameron's comments.

This contains all of the corrections suggested by Cameron McCormack.

LocationWrapper has been removed and the other changes were made.

Comment on attachment 22837 [details]
Patch with corrections for Cameron's comments.

The patch looks good.  Once the CLA has been processed, I'll commit the patch.

(In reply to comment #9)
> (From update of attachment 22837 [details])
> The patch looks good.  Once the CLA has been processed, I'll commit the patch.
> 

FYI: G. Wade Johnson's ICLA has been recorded a few minutes ago.

Great!  Landed in r712954.

Wade, note that I modified the patch very slightly to avoid the unintentional use of covariant return types of getLocation() and getParent() in BaseScriptingEnvironment.Window and ScriptingEnvironment.Window, which caused build failures on JDK 1.4.

(In reply to comment #11)
> Great!  Landed in r712954.

Should G. Wade be listed as a contributor [1]? I'd vote so, given the non-trivial contribution.

[1] http://xmlgraphics.apache.org/batik/contributors.html#contributors