commit a951b44fd9d2acf060067b24c994604351a27888
Author: Deri James <deri@chuzzlewit.myzen.co.uk>
Date:   Tue Apr 30 15:25:49 2024 +0100

    [gropdf] Re-arrange pattern matches.

    * src/devices/gropdf/gropdf.pl: Correct order of pattern
    match.

    Fixes https://savannah.gnu.org/bugs/?65585 (again!)

comment #2:

> The only thing I think which is still missing is the proper SS font for devpdf, which produces "more perfect" typeset equations.

Yes, I want that in groff 1.24 as well.  It's on my to-merge list.
 

> For the NEWS file perhaps you can use some of the information in the attached pdf.

I think that's a good idea.

> My condolences for your bereavement.

Thank you.

Closing as fixed.  Instead of citing the commit IDs I'll use the ChangeLog entries, since I added detail to them.

2024-04-12  Deri James  <deri@chuzzlewit.myzen.co.uk>

        Correct and improve linear search introduced in commit
        cd9fde325f, 4th March.

        * tmac/pdf.tmac: Add mark counter, `pdf:bm.nk`.
        (pdfbookmark, pdf*href-M): ...and use it.
        (pdf:lookup): Add string `pdf:lookup-value` to record the `.val`
        attribute of a PDF bookmark, distinctly from its `.tag`.
        (pdfnote, pdfbookmark, pdf*href-M, pdf*href): Use `\A` escape
        sequence to validate *roff strings as (parts of) identifiers
        {seen in Keith Marshall's "pdfmark.tmac"}.
        (pdf*href-M, pdf*href, pdfbookmark): Stop using old `pdf:look`
        method of bookmark tag lookup.  Drop conditionals on presence of
        mom(7) `PRINTSTYLE` macro to select its use.
        (pdfbookmark): Drop logic for cleaning bookmark tag of material
        invalid in *roff identifiers, since the foregoing ensure that
        we no longer use such in their construction.
        (pdfmarksuspend, pdfmarkrestart): Drop management of mark
        suspension state; "pdf.tmac" now subsumes this function.

        Fixes <https://savannah.gnu.org/bugs/?65585>.

2024-04-12  Deri James  <deri@chuzzlewit.myzen.co.uk>

        Support UTF-16-encoded PDF bookmarks.

        * utils/afmtodit/afmtodit.pl: Generate comment field holding the
        UTF-16 code for each groff character.

        * font/devps/AB:
        * font/devps/ABI:
        * font/devps/AI:
        * font/devps/AR:
        * font/devps/BMB:
        * font/devps/BMBI:
        * font/devps/BMI:
        * font/devps/BMR:
        * font/devps/HB:
        * font/devps/HBI:
        * font/devps/HI:
        * font/devps/HNB:
        * font/devps/HNBI:
        * font/devps/HNI:
        * font/devps/HNR:
        * font/devps/HR:
        * font/devps/NB:
        * font/devps/NBI:
        * font/devps/NI:
        * font/devps/NR:
        * font/devps/PB:
        * font/devps/PBI:
        * font/devps/PI:
        * font/devps/PR:
        * font/devps/TB:
        * font/devps/TBI:
        * font/devps/TI:
        * font/devps/TR:
        * font/devps/ZCMI: Regenerate with updated aftmtodit.pl.

        * man/groff_font.5.man: Document use of comment field to hold
        UTF-16 code.

        * src/devices/gropdf/gropdf.pl (LoadFont): Extract UTF-16 code
        from font comment field (rather than a new field).
        (ParsePDFValue): Validate input more strictly.
        (do_x): Manage state more strictly; don't suspend a mark
        {hotspot} when one is not active, and don't restart it when one
        is not suspended.

2024-04-12  Deri James <deri@chuzzlewit.myzen.co.uk>

        * om.tmac: Switch to using Branden's linear search so that we
        don't have to maintain two different methods of looking up PDF
        bookmarks.

These can of course be further clarified if I didn't describe them well enough.

Hi Deri,

Thanks for giving my changes a close look.  I'm obviously pretty inexperienced with this part of the groff code base.

original submission:

> I have been looking at the current state of pdf.tmac and have found a few issues. The changes which have been committed are:-
>
> Introduction of a new flag, -S, which replaces my change which introduced the concept of passing a single pipe character as a hotspot meant that the actual text to form the actual hotspot would be "piped" to the document stream, terminated by doing a markstop. This is what allowed Branden to implement the man macros which place contents into a diversion before emitting a hotspot. I have no objection to this change, although there were several ways of setting up a hotspot containing a single pipe character before this change, and most users are probably familiar with piping data. (Perhaps I should have used "<" as the single character). I would be very annoyed if someone sent me a pdf with a single "|" as a hotspot - would take me about 5 minutes to click it with the mouse!

I don't object to the mnemonic of "|" here; what made me uneasy was the use of in-band signaling to the macro file when faced with arbitrary user input.  Someone somewhere would want to make a lone "|" hotspot and would, I predict, be flabbergasted that they weren't allowed.  Yes, we could advise them to work around the restriction by sticking a dummy character (or similar) adjacent to it, but that doesn't feel to me like advice we could be proud of.

Also I don't think "pdf.tmac" is the right layer at which to have groff warn the user about accessibility flaws.  Maybe there isn't such a layer, apart from excoriating the document author.

And if someone set a "|" in 36- or 48-point type, presumably it wouldn't be as much of a problem.  So a preclusion of it (unadorned) as a hotspot seems to me a bit of a severe measure.
 

> Another change was an attempt to only allow a restart after a preceding suspend. This is a good idea, but it is in the wrong place, it should be implemented in gropdf rather than pdf.tmac, since a user may use \X'pdf: markrestart' as documented in the gropdf man page. Which bypasses the check in pdf.tmac.

I'll need to understand this better, but I admit that it did feel to me like only half of a fix.  My instinct is that it would be better to expose "markstart" and "markend" directly with convenience macros, and update gropdf(1) such that users could understand what they were for and how they might write their own hyperlinking macros.

I'm pretty uncomfortable with "pdfmark.tmac"'s approach, which assumes that it has anticipated every user's every need (but also stops documenting its all-singing, all-dancing comprehensiveness in section 4 of its manual), and all you need to do to use it is write a macro call resembling an ffmpeg command to get results. 

There's even a place for that--but not, I think, via the means of going straight to device control commands.  I think we should have a more granular API for access to PDF features, and I think your approach in "pdf.tmac" has taken several steps down that path already.

> Branden has implemented a looping construct to hold tags replacing an associative array. It is not used if the user is using mom macros (it does not work for mom documents).

I'm aware of it and it's on my to-do list.  I spoke of it on the mailing list in March.

Long story short, I plan to get my new approach working for mom(7) but that means I have to solve the danged old "node embedding in \X commands" problem that has vexed us for a while.

> In fact it only works reliably for the man macros producing a "book" of man pages, where it is used to differenciate between an internal link to another page in the book or a "man:" URL to an external man page. The problem is that the new code does not implement:-

> .pdfhref L -D name

> Which should emit the descriptive text associated with the named tag at the time it was defined, but instead it just emits the "name". This is documented in pdfmark.pdf, and used several times in pdfmark.ms, so it is reasonable to assume users may be using this in their own documents if they use "pdfmom --roff  -mspdf". So it is wrong to assume the new code is suitable for everything except mom.

Okay.  That's a defect/implementation gap I was unaware of.  It might be a good idea to spawn a ticket off for that and have this one depend on it.

> There is also an issue with the speed of the new code. One test file I used went from an elapsed time of 0m3.08s to 11m31.62s. (About 670 times slower!). This was producing a large document (LinuxManBook) from a single file, using the command:-

> time ~/groff-git/groff/test-pdfmom  --roff -Tpdf -mandoc -petk LinuxManBook.trf -z

> (test-pdfmom is the same as test-groff but calling the pdfmom in the build directory). I don't know whether this slow down in groff will be acceptable to most users,

That's freaking awful and I agree that it's unacceptable for release.  I am wondering why Alex Colomar hasn't rung my bell about it.  Maybe he hasn't tried groff Git HEAD lately.  He did complain that it was tedious to work with unreleased groff versions, and asked for a 1.24 ASAP.  I demurred, as I think there's still a solid chunk of work to be done--a few months' worth.

> I'm meant to be on a sabatical so I am not going to argue either way, but be aware that is Branden's attempt to solve one issue which was solved months ago in my branch waiting for merging. My use of stringhex was described as "obfuscation", which is rather insulting if the intention was to imply a deliberate attempt to obscure the purpose of my code.

That certainly was not my intention.  I meant simply that the stringhexed string did not have obvious meaning when dumping device-independent or approximated output (with "groff -Z" or "groff -a").  I want these output formats to be accessible and comprehensible to users so that they feel more competent to troubleshoot problems (and send us better-informed bug reports!).

> The correct interpretation of its use is to protect the data from interpretation by groff so that any byte sequence can be used as a string name in groff. This is analagous to using base64 to protect binary data in emails.

I understand.  But by the same token, no human reads base64.  They decode it first.  But we don't have an "unstringhex" command for performing this transformation on "groff -Z" or "groff -a" output, and I'd suggest that we shouldn't need one.

Admittedly, any accessibility my approach gains is going to be traded away again for documents that don't predominantly use the Latin alphabet.  On net I think my approach makes groff -[aZ] output more accessible for many users and is no worse for the rest.
 

> Although Branden's loopy code solves some issues with 1.23.0 it fails in a number of ways which are dealt with successfully by the pdf.tmac in my branch (i.e. t.trf in #64576).

Okay, I will have to take a look at that when my availability recovers.  (Since you're not on the groff list for a while, you may not know that I had a death in the immediate family this week and, in an employment context, I'd be on bereavement leave right now.)

> There is a bug in Branden's code. The attached file, pdf-L.trf, illustrates the issue. According to the documentation in pdfmark.pdf, if you use .pdfhref L with a destination name but no descriptive text, the descriptive text given when the destination is named is used. With Branden's code, instead of using the descriptive text it uses the name of the destination instead. The two pdfs called pdf-L illustrate the problem.

Okay.  Thanks for supplying reproducers.  This should be looked into.

> Another bug results in entries in the array Branden loops over get over-vritten in certain circumstances. This code illustrates the bug.

> .ig
>   groff -Tpdf -dPDF.EXPORT=1 -z pdf-M.trf
>
> Results:-
>
> .ds pdf:bm1.tag one
> .ds pdf:bm1.tag two
>
> (The bm1.tag has been overwritten!!!)

Hmm, yes, that does sound incorrect.

> But,
>
>   groff -Tpdf -dPDF.EXPORT=1 -dPRINTSTYLE=1 -z pdf-M.trf
>
> Results:-
>
> .ds pdf:look(one) Once upon...
> .ds pdf:look(two) This is two
>
> ..
> .pdfbookmark -T one 1 Once upon...
> .pdfhref M -D two -E This is two

> Setting "PRINTSTYLE=1" bypasses Branden's changes, because he uses the original code if mom macros are used. I don't understand why it is desirable to have two separate methods.

It isn't desirable.  Supporting both was a temporary measure to avoid regressing mom documents.

> I have fixed all the problems listed above, plus a speed up of Branden's loopy code. I have also included Keith's clever solution to polluting the pdf flags with data which can cause the errors about illegal characters in identifier names if the optional "--" marker not used.

I would like to review that; Keith and I seem frequently to disagree.

> There is a minor regression in this code, not caught by any of the check tests, but when producing our pdf documents you will notice these new warnings:-

> gropdf:man/groff.7: warning: Can't convert 'rs' to unicode
> gropdf:../contrib/mom/examples/sample_docs.mom: warning: Can't convert 'em' to unicode

I have not seen any of these warnings in any builds I have done at any point...oh, you mean these pop up with your new code?

> They are caused because gropdf currently has no way of knowing what the UTF-16 for these groff characters. Previously the "em" character was handled by a:-

> .tr \[em]-

> In pdf.tmac. I was never happy with this "fudge" and the correct handling was part of my branch. I have changed the method slightly in view of what Branden wrote on the list. Afmtodit does not add an extra column to the font files it creates but adds the UTF-16 code as a comment. The groff_font man page has been amended appropriately. The grops standard fonts have also had the comment added, which corrects the regression.

Okay.  I acknowledge I haven't yet reached a conclusion about what means of mapping groff special characters back to Unicode seems best to me here.  We have glyphuni.cpp, and it seems an awful damn shame not to leverage it.  At the same time, Perl/C++ interfacing doesn't seem like something we should just leap into.

Maybe we can write a simple/stupid executable that links with libgroff, dumps the "glyphuni" map to some well structured text file (or even a valid Perl hash), and gropdf could read that when it starts up.  Or even have it embedded as part of the build.  We already have something in the source tree that is similar; I just can't remember what it is right now.

> I am enjoying my sabbatical away from groff, but I thought I should correct these recent changes, since they would cause issues if released, but I would be grateful if other major changes to pdf production could wait until I am back, fighting fit to do any QA.

My plan had been to finish merging the features you had broken out into that HTML dependency graph, which I keep handy at all times on my tablet, call those the groovy new things for gropdf in 1.24, and then leave it alone.

I say merging the features rather than merging the code because I'm uneasy with merging code I don't understand.  Since you've gone on sabbatical the FLOSS community has gotten a lesson reminding us of the hazards of such things.  Andreas Freund even got himself interviewed on NPR (the U.S. public broadcaster) yesterday.  His next performance review at Microsoft should be exciting for him.

> Of course, if you find any problems with the code, please let me know through this bug report.

If Dave could help organize the list you've presented above into a punch list of new/old (probably mostly new) tickets upon which this one can depend, that would be helpful to me and likely others to direct efforts in getting the bugs squashed.

I would add that I think it's important for us to get more regression tests for groff's PDF output.  If we have them, then when I screw something up, I'm more likely to know it.  It's better to have my own builds tell me I've f***ed up than to not find out until you, or a user with a damaged document (or three more days of beard growth by the time groff finishes rendering), tells me.

Once again I appreciate your close attention to the changes I made.

Regards,
Branden