Issue 258740043: Table Of Contents crash with negative first-page-number (Closed)
|
Created:
9 years, 10 months ago by thomasmorley651 Modified:
9 years, 4 months ago Reviewers:
dak CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionTable Of Contents crash with negative first-page-number
Issue 4494
Let \with-link point to the physical page-number
Patch Set 1 #
Total comments: 2
Patch Set 2 : Davids comments #Patch Set 3 : better coding #Patch Set 4 : oversight #Patch Set 5 : David's second thoughts #Patch Set 6 : Deal with non-existing labels #MessagesTotal messages: 13
Sign in to reply to this message.
https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:542: (+ table-page-number (+ 1 (* -1 first-page-number)))) Uh, (- table-page-number (1- first-page-number)) ? Do the links actually work when first-page-number is, say, 10? If so, did they not work before? https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.sc... scm/define-markup-commands.scm:548: (ly:stencil-add What's the reason for delaying this part of the stencil expression as well?
Sign in to reply to this message.
Davids comments
Sign in to reply to this message.
On 2015/07/12 17:01:06, dak wrote: > https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.scm > File scm/define-markup-commands.scm (right): > > https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.sc... > scm/define-markup-commands.scm:542: (+ table-page-number (+ 1 (* -1 > first-page-number)))) > Uh, (- table-page-number (1- first-page-number)) ? done > > Do the links actually work when first-page-number is, say, 10? yes > If so, did they > not work before? No, it was broken. > > https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.sc... > scm/define-markup-commands.scm:548: (ly:stencil-add > What's the reason for delaying this part of the stencil expression as well? No reason, corrected.
Sign in to reply to this message.
On 2015/07/12 17:49:44, thomasmorley651 wrote: > > Do the links actually work when first-page-number is, say, 10? > > yes > > > If so, did they > > not work before? > > No, it was broken. Well, then we probably have to thank Ghostscript for bringing this long-standing problem to our attention. It may be worth trying if this makes the rather awkward fix for issue 3388 unnecessary, but I would guess that the problem is likely sufficiently different that that fix is still needed.
Sign in to reply to this message.
On 2015/07/12 18:09:03, dak wrote: > On 2015/07/12 17:49:44, thomasmorley651 wrote: > > > > Do the links actually work when first-page-number is, say, 10? > > > > yes > > > > > If so, did they > > > not work before? > > > > No, it was broken. > > Well, then we probably have to thank Ghostscript for bringing this long-standing > problem to our attention. As far as I can tell other first-page-numbers than 1 are leading to wrong links right from the initial implementation of \with-link. Rereading the discussion http://lilypond.1069038.n5.nabble.com/PDF-hyperlinks-td93369.html and regarding the commit Author: Reinhold Kainhofer <reinhold@kainhofer.com> 2011-04-12 12:16:59 Committer: Reinhold Kainhofer <reinhold@kainhofer.com> 2011-04-15 15:49:25 Parent: 9f651e1f5143787aeda825a4061f82eede4943a3 (Doc: addition to MIDI and repeats by Tom Cloyd.) Child: 98b9439fda0b019a67cfe63b0c0c5bb99ded618e (Fix TOC: don't add links to non-existing labels in the .ps file) Branches: dev/toc-crash-with-link, master and many more (42) Follows: release/2.13.59-1 Precedes: release/2.13.60-1 Add feature to link to a particular page or a page containing a given label The main part of this patch was done by Dan Eble and posted to the lilypond-devel mailinglist on December 27, 2010. Like \with-url, this works only with the PDF backend. -) Add markup functions \with-link (links to the page containing the given label) and \page-link (links to the given page number) -) Internally, they are handled like the \with-url markup function, creating a pdfmark in the .ps file that creates the link in the PDF file. -) Add links to the pages for all TOC entries. I'd say it was never considered > It may be worth trying if this makes the rather awkward fix for issue 3388 > unnecessary, but I would guess that the problem is likely sufficiently different > that that fix is still needed. As far as I can tell my patch will not cure issue 3388. Though, I don't fully understand 3388. Maybe better to recheck yourself. The only other somehow related thing I've found is: Author: Reinhold Kainhofer <reinhold@kainhofer.com> 2011-04-12 12:16:59 Committer: Reinhold Kainhofer <reinhold@kainhofer.com> 2011-04-15 15:49:25 Parent: 9f651e1f5143787aeda825a4061f82eede4943a3 (Doc: addition to MIDI and repeats by Tom Cloyd.) Child: 98b9439fda0b019a67cfe63b0c0c5bb99ded618e (Fix TOC: don't add links to non-existing labels in the .ps file) Branches: dev/toc-crash-with-link, master and many more (42) Follows: release/2.13.59-1 Precedes: release/2.13.60-1 Add feature to link to a particular page or a page containing a given label The main part of this patch was done by Dan Eble and posted to the lilypond-devel mailinglist on December 27, 2010. Like \with-url, this works only with the PDF backend. -) Add markup functions \with-link (links to the page containing the given label) and \page-link (links to the given page number) -) Internally, they are handled like the \with-url markup function, creating a pdfmark in the .ps file that creates the link in the PDF file. -) Add links to the pages for all TOC entries.
Sign in to reply to this message.
On 2015/07/12 20:58:41, thomasmorley651 wrote: > On 2015/07/12 18:09:03, dak wrote: > > > It may be worth trying if this makes the rather awkward fix for issue 3388 > > unnecessary, but I would guess that the problem is likely sufficiently > different > > that that fix is still needed. > > As far as I can tell my patch will not cure issue 3388. > Though, I don't fully understand 3388. Maybe better to recheck yourself. > > The only other somehow related thing I've found is: > C/P-error should read: Author: Reinhold Kainhofer <reinhold@kainhofer.com> 2011-04-15 18:31:17 Committer: Reinhold Kainhofer <reinhold@kainhofer.com> 2011-04-15 18:31:17 Parent: 3d571d9c80b7855422c96ecc6966bcbfa4dfb9ff (Add feature to link to a particular page or a page containing a given label) Child: 7703d5ed1ce797d92449a6a79ce593e6e5601396 (Fix problem with multiple markups on chord glissandos.) Branches: dev/toc-crash-with-link, master and many more (42) Follows: release/2.13.59-1 Precedes: release/2.13.60-1 Fix TOC: don't add links to non-existing labels in the .ps file
Sign in to reply to this message.
better coding
Sign in to reply to this message.
oversight
Sign in to reply to this message.
On 2015/07/12 17:49:44, thomasmorley651 wrote: > On 2015/07/12 17:01:06, dak wrote: > > https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.scm > > File scm/define-markup-commands.scm (right): > > > > > https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.sc... > > scm/define-markup-commands.scm:542: (+ table-page-number (+ 1 (* -1 > > first-page-number)))) > > Uh, (- table-page-number (1- first-page-number)) ? > > done Now I get second thoughts. The problem is that I actually have no clue how this calculation comes about. Is it because logical pages are counted from 1? If so, this likely should rather be (1+ (- table-page-number first-page-number)) instead. Basically, the calculation should reflect the underlying logic. Whatever that may be.
Sign in to reply to this message.
David's second thoughts
Sign in to reply to this message.
On 2015/07/12 23:39:52, dak wrote: > On 2015/07/12 17:49:44, thomasmorley651 wrote: > > On 2015/07/12 17:01:06, dak wrote: > > > > https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.scm > > > File scm/define-markup-commands.scm (right): > > > > > > > > > https://codereview.appspot.com/258740043/diff/1/scm/define-markup-commands.sc... > > > scm/define-markup-commands.scm:542: (+ table-page-number (+ 1 (* -1 > > > first-page-number)))) > > > Uh, (- table-page-number (1- first-page-number)) ? > > > > done > > Now I get second thoughts. The problem is that I actually have no clue how this > calculation comes about. Is it because logical pages are counted from 1? If you mean that the pdf's first page is always page 1 of the document (whatever page-number is printed actually), then yes. > If so, this likely should rather be (1+ (- table-page-number first-page-number)) > instead. Basically, the calculation should reflect the underlying logic. > Whatever that may be. Changed to your proposal.
Sign in to reply to this message.
|
