Bug 46980

Summary: [PATCH]internal named destinations
Product: Fop - Now in Jira Reporter: michaelrichardson89
Component: pdfAssignee: fop-dev
Status: NEW ---    
Severity: enhancement CC: michaelrichardson89
Priority: P2 Keywords: PatchAvailable
Version: all   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: patch file
updated patch proposal

Description michaelrichardson89 2009-04-07 07:01:53 UTC
Created attachment 23450 [details]
patch file

I was experiencing an issue where svg graphics would enterpret links of the format:
<a xlink:href="#named_destination">

as external web-links opening up a web browser.

I have now fixed this issue (given in the attached patch) so that links in the above format now properly go to an internal named destinations.

The named destination must be specified with the tag:
<fox:destination internal-destination="named_destination" />

I hope this is helpfull.
Comment 1 Andreas L. Delmelle 2009-04-08 11:26:28 UTC
Thanks for the patch! Seems like a good starting point.
Only some very minor details (code-style) I'd change:

"if (namedDestination == true) ..." 

No idea if it's just me, but boolean comparisons almost always seems a bit silly, as you can just write:
"if (namedDestination) ..."

Consequently, I'd also rename that member to something like 'isInternal' or 'hasInternalDestination', yielding the self-explanatory:
"if (this.isInternal) ..."

For the rest, the small piece of added code in PDFFactory can probably better be added to getGoToPDFAction(), to be executed in case the parameter 'filename' is null. (opportunity to avoid code-duplication)

Anyone with further suggestions before I commit this one, including the above modifications?
Comment 2 Andreas L. Delmelle 2009-04-08 11:33:50 UTC
(In reply to comment #1)
> Consequently, I'd also rename that member to something like 'isInternal' or
> 'hasInternalDestination', yielding the self-explanatory:
> "if (this.isInternal) ..."

Just noticed that 'hasNamedDestination' is probably better, since PDFGoTos are always internal... :-/
Comment 3 michaelrichardson89 2009-04-08 15:25:24 UTC
I'm glad my changes will be commited so others can benifit from my (small) contribution. :D
Comment 4 Jeremias Maerki 2009-04-09 00:37:59 UTC
+1 to commit the patch with the modifications suggested by Andreas. Ideal would, of course, be a little bit of documentation somewhere.
Comment 5 Andreas L. Delmelle 2011-01-09 08:54:21 UTC
Created attachment 26466 [details]
updated patch proposal

Found this one still open...

Took a quick look again, and I had applied the changes locally, slightly refined as in the attached patch.

It can be tested with a fo:basic-link.
<fox:destination internal-destination="dest" />

<fo:basic-link internal-destination="dest">...

yields behavior similar to
<fo:basic-link external-destination="#dest">...

Some reservations still: a PDFGoTo can be used to jump to either a page (with or without explicit coordinates) or a named destination.
After the patch, I still have the feeling the implementation somehow is slightly messy. 
In some contexts, the 'destination' member was already used to pass in a string containing the x/y coordinates. As this was likely done as a shortcut, to avoid creation of unnecessary Point2Ds (only to have to reconstruct the String later in toPDFString()), for now, I have split that into two members positionDestination and namedDestination, to avoid confusion. The latter is made final since I saw no immediate reason to be able to change it after creation.
There is still a PDFGoTo constructor that takes one String as a parameter, which leads to some ambiguity. Is the String a page reference or a destination name? If not for the parameter name, that would not always be clear. Given the parameter name, creating a shortcut for a named destination required adding a new constructor, taking a String and a boolean. I have not yet deprecated the existing String-only constructor, but would be inclined in that direction.
Another point of concern might be the semantics when compared to external links. 
In case of 'real' external links, pointing to a named destination would have to happen via "....pdf#dest=...", whereas in this case, it is just "#...". Would we also have to cater for "#dest=..." (and possibly "#page=...")? And what about, vice versa, allowing "....pdf#..." to point to a named destination? (The latter is enabled with the patch.)

Note also that this behavior only applies to external links with a protocol other than 'file' or 'http'. In those cases, respectively a PDFLaunch or a PDFUri action is created rather than a PDFGoTo or PDFGoToRemote.
Comment 6 Glenn Adams 2012-04-07 01:42:26 UTC
resetting P2 open bugs to P3 pending further review
Comment 7 Glenn Adams 2012-04-11 03:20:32 UTC
increase priority for bugs with a patch