pre-compute entry visibility once via count-all-refs()#189
pre-compute entry visibility once via count-all-refs()#189joelsa wants to merge 1 commit intotypst-community:masterfrom
count-all-refs()#189Conversation
|
I am unsure whether using |
There was a problem hiding this comment.
Hi @joelsa, thanks for the patch!
Can you provide a test/showcase as well? I'd like to be able to confirm that your divergence test case is indeed fixed by this patch. I can confirm on my side that that tests pass locally. I'll run the workflow once you've got a test written up.
Using __glossary_final_counts() directly would be cleaner, right?
Yes, use internal functions as you like as long as the result is clear and concise. count-refs/count-all-refs are just more practical. In practice, I don't think the optimization is even visible because the Typst compiler does its job pretty well.
count-refs is called from print-gloss which means it's already called for each entry².
Let's not modify show-all to mean something else other than the current argument. Instead, you should add a new argument to user-print-reference and propagate it. I'll help with double checking that it's done correctly.
Then we can write in default-print-gloss
- if show-all == true or count-refs(entry.at(KEY)) >= minimum-refs {
+ if show-all == true or entry-visible == true {
This way, things are indeed pre-computed.
I've had some problems with non-convergent layouts. I've found in many of these cases the problem is an
oscillationdue to conditional content emission.From my point of view, the ref counts can be pre-computed once. Is there a reason why this was not done before?
This PR does just that, pre-computing the reference counts. I think this will make the layout more stable and should have no negative consequences.