Distinguish mutiple HTML5 footnote section ids

[ag-eitilt]

The HTML5 writer generates an <aside> tag grouping all footnotes written to a particular location, and gives it a #footnotes ID for ease of navigation. However, it gives that same ID for every <aside> if the footnotes occur in multiple locations within the file (--reference-location=block or =section). This PR adds a counter to the writer state to track how many footnote-blocks have been written, and uses that to uniquely identify each <aside>.

The first ID remains simply #footnotes for backwards compatibility with incoming links and for cases where only a single <aside> is needed, but all following ones are given a suffix #footnotes-2, #footnotes-3, and so on (one-indexed to match the individual-note numbering style). As far as I could tell, none of the other writers supporting --reference-location give the blocks a presumed-unique ID, and while it seems like that would be a good feature to add support for in the HTML4 format defined in the same file, I've not made that change since I don't know if there's some use case which expects that lack of ID for some reason.

I didn't see any good way to test this without creating a new output -- and continuing to test endnotes is definitely important -- so I hope the naming pattern I used (modeled on the dokuwiki_*.* tests) and the entry in the testrunner are acceptable.

[ag-eitilt]

I was sure I tested it after fixing the indexing, but apparently not; sorry! I've also realized that this might not round-trip, so I'll take a look into the reader code to see what that side of it all needs. One second.

[ag-eitilt]

Looks like the HTML reader doesn't make any effort to read the generated section, even when it just has the old #footnotes ID (there's a few checks for type and epub:type that might be intended to do it, but those aren't written out), so I just fixed the test and am not worrying about round-tripping.

[jgm]

We should not produce duplicate ids, so this is definitely something that needs fixing.

[ag-eitilt]

I (rightfully) don't have merge permissions, so was under the impression that this was ready but in your court at this point. Or am I missing a stage of the contribution process?

[jgm]

Sorry for the delay -- I just can't keep up sometimes!

This looks good, except that I'd suggest a more focused test. You've basically duplicated the old writer test with a new option. But most of this new test isn't testing the new feature. Simplest option would be to use a command test (see test/command directory) to test just this.

[ag-eitilt]

Sorry for the delay -- I just can't keep up sometimes!

No worries! Just didn't want there to be something you were waiting on from my end, which I didn't know about; it's not breaking any of my current workflows and won't break anything in the near future, so I'd definitely rather do it right than do it fast.

This looks good, except that I'd suggest a more focused test. You've basically duplicated the old writer test with a new option. But most of this new test isn't testing the new feature. Simplest option would be to use a command test (see test/command directory) to test just this.

Ah, yeah, that would be better. I didn't know how the tests were structured, found one spot with a lot of data, and then just monkey-coded something to match. I'll be rather busy the next week or two, but as I get time I'll write up something more properly targeted.

[jgm]

@ag-eitilt this is not far from being merge-able -- just needs a rebase and some tests.
Do you still plan to work on it?

[ag-eitilt]

I'll be honest, I took a vacation and promptly forgot I had done this much until your merge lifted it up through the morass of my email inbox. Now that I know about it again, I do definitely want to get this polished up for merging, so I'll fix the test up and try to get something pushed this weekend. Sorry for the delay!

[ag-eitilt]

Yeah, the command tests were definitely better than what I did initially; thanks for pointing me to them! I'm a bit less happy about moving the fromString inline with the prefixedId/A(5).id, but since the former requires a Text and the latter an AttributeValue (with no easy way I saw to convert between the two types directly) it seemed the best way to unify them -- I personally would prefer prefixedId for all three, but that's a different discussion I don't have the full context for, especially since #9008 is already open for something similar.

Again, sorry I disappeared like that, especially after I apparently pinged you about the same. I can read the thread and mostly remember writing my side of it, but I don't actually remember anything about writing the code in the original patch... Memory's a weird thing.

[mxngls]

@jgm, @ag-eitilt ; Bumping this issue as this PR seems fine to be merged now, but got kinda forgotten. I guess we are all too busy these days. Thanks for getting this done.

[jgm]

@ag-eitilt do you plan to rebase this?
If not I could probably take care of it.

[ag-eitilt]

Sorry, I was seeing the "has no conflicts" marker, and not thinking about the fact that the tests might have gotten stale. They are indeed passing, though; I'll squash it all down since most of these are merge commits, and apparently the line length of the auto-generated text for that is too long.

[jgm]

Many thanks! Looks good.

[mxngls]

Thanks!