Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hfill-command parameter to TOC #1650

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raphCode
Copy link
Contributor

@raphCode raphCode commented Dec 8, 2022

This PR adds the parameter tableofcontents.hfill-command to control the way the space between the TOC entry and the page number is filled.
By default this is dotfill which is the original behavior. But now it is trivially to change the setting inside e.g. tableofcontents:level1item to create different fill styles for the TOC levels:

image

@@ -97,6 +97,15 @@ function package:_init ()
self:deprecatedExport("moveTocNodes", self.moveTocNodes)
end

function package.declareSettings (_)
SILE.settings:declare({
parameter = "tableofcontents.hfill-command",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right approach to solve this problem. I understand why it works for your use case, but I don't think it generalizes very well. Trying to setup a TOC style that does different things at different levels is going to be a messy fiasco.

That being said part of the problem in my view in the original TOC :item commands (\tableofcontents:level1item and friends) were incorrectly setup as "pre" commands that run before an item but don't actually output the item itself and hence are limited in the amount of reconfiguring they can do.

By comparison the TOC implementation I use is the one in CaSILE (code here)) makes those commands responsible for actually outputting the line. The fill is part of the line, but since the point where it is output is in the per-level commands, each level can have different default options for whether to turn dotfill on or off.

For example a book project may run something like this:

SILE.setCommandDefaults("tableofcontents:level1item", { dotfill = false })
SILE.setCommandDefaults("tableofcontents:level2item", { dotfill = true })

I think it would be worth reworking this PR as part of a complete TOC package overhaul that –one way or another– allows for more complete styling hooks all the way through. I believe @Omikhleia also has another TOC implementation that handles styling better. We probably need to pair up and fine some best-of approach to styling.

In the mean time I'm kind of hesitant to just stuff hacks on the current implementation that aren't going to scale well in the long run for most users. This implementation wouldn't be too hard to copy into a library of your own to include in any projects you want to use it in, but since it doesn't handle multiple levels very well I don't think we should make it the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @Omikhleia also has another TOC implementation that handles styling better.

Very experimental. Revisiting "generic" styling is a long due task in my todos... I am not satisfied with its API. The "old" way can be consulted here, p. 40 §1.2.1 and is used on p. 3-5. I insist this is considered experimental and that my eventual implementation might differ (though it likely will keep the core concept of splitting styling decisions from implementation and using some king of "lite CSS" approach to style definition templates)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SILE.setCommandDefaults("tableofcontents:level1item", { dotfill = false })
SILE.setCommandDefaults("tableofcontents:level2item", { dotfill = true })

Wow, I was completely unaware of command defaults. I agree this is a good idea if it works*

It is a bit unconventional in the programming sense where all arguments / options are traditionally passed at the call site. So by just inspecting the SILE.call("level1item") it is not clear how options.dotfill can ever be populated, but once you are aware of the concept it's fine.
Also the implementation of SILE.call doesn't hint at the possibility of additional options being passed, since SILE.setCommandDefaults just creates a new closure that applies the defaults.

*In my understandung your casile TOC doesn't work this way - the level1item function doesn't even use its options argument. To make it work, it would have to pass on the argument to the content. And since that is a closure from tableofcontents:item it needs to take an option argument as well. Currently, the options in the closure body there is just an upvalue to the options from tableofcontents:item` .

If my understanding is correct I think this hints more to a general problem in SILE with nesting content closures which makes it very hard to pass arguments to a specific content function. I am happy to elaborate on that or discuss solutions if that is desired.
This is also why I settled for a SILE setting in this PR - it seemed to me with the current architecture it is the most elegant solution to access a "global" - while it is still ugly from a programming perspective.

In the mean time I'm kind of hesitant to just stuff hacks on the current implementation that aren't going to scale well in the long run for most users.

I am perfectly okay with this, I don't want anything merged that doesn't serve the project well in the long run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit unconventional in the programming sense where all arguments / options are traditionally passed at the call site.

True, but we're talking about document markup here. One cannot expect a document to modify every call to a tag to pass the same options over and over every time. That would be absurdly complex for large projects and get in the way of the content. Instead we allow documents to redefine commands and people to write their own libraries that replace the functions used to typeset the markup.

One of the most common use cases turned out to be using the same typesetting function but changing the default options—using some non-default option by default. Hence I added a programming shortcut for it. It's just syntax sugar for creating a wrapper function that calls the original function with some options by default, but it's a lot less cumbersome than making everybody write their own stub functions.

If you need something other than a different { options } table you'll still need to write your own function of course...

So by just inspecting the SILE.call("level1item") it is not clear how options.dotfill can ever be populated, but once you are aware of the concept it's fine.

Also the implementation of SILE.call doesn't hint at the possibility of additional options being passed

You can pass a table op options like so: SILE.call("command", { options_here }, ...), and that doesn't change when a command has been wrapped with setCommandDefaults(). You can still override options that are passed in, only the default options are changed.

*In my understandung your casile TOC doesn't work this way - the level1item function doesn't even use its options argument.

It's a little bit obtuse, but it does yet used ;-) The \tableofcontents:level1item function doesn't reference it directly, but it does process the content it is passed. The content it is passed in this situation is actually a lambda function sent from the body of \tableofcontents:item. That then eventually gets evaluated and spots the option. I would code that up a lot cleaner if I were to do it again today, but at the time I figured it out it got the job done right for the books I was printing.

If my understanding is correct I think this hints more to a general problem in SILE with nesting content closures which makes it very hard to pass arguments to a specific content function.

Yes. There are actually a lot of ways to do it using Lua features, but few/none that are intuitive and well documented.

I am happy to elaborate on that or discuss solutions if that is desired. This is also why I settled for a SILE setting in this PR - it seemed to me with the current architecture it is the most elegant solution to access a "global" - while it is still ugly from a programming perspective.

Ya I understand why you reached for it. I think settings though are better scoped to situations when more than one command would be affected: i.g. changing the behavior of an entire package or module (or multiple modules). When just one command is in play we probably need something else. A standardized way of options inheritance would help.

Also note another way to deal with this would be to write a \dotfill replacement that uses bread crumbs to give different output depending on what other commands it was nested in.

Copy link
Contributor Author

@raphCode raphCode Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little bit obtuse, but it does yet used ;-)

Sorry, I am still not convinced. I even made up a little test document to prove it, something along the lines:

\begin[class=book]{document}
\use[module=packages.cabook-commands]
\script{
SILE.setCommandDefaults("tableofcontents:l1item", { dotfill = false })
}
\tableofcontents
\chapter{aaa}
\end{document}

(I hacked your cabook-commands file (gist is here) to make it work with this minimal document outside casile, especially calling registerCommands() again from init() to overwrite the TOC commands the book class pulls in.)

I then replaced this line with pl.pretty.dump(options) to dump what options the lambda function you mentioned sees.
As expected, it sees the upvalue from tableofcontents:item, and no amount of
SILE.setCommandDefaults("tableofcontents:level1item", { dotfill = false }) changes that.
SILE.setCommandDefaults("tableofcontents:item", { dotfill = false }) is the only command that changes it, which doesn't enable per-level customisation.

The problem is that content is already a closure when it is passed to tableofcontents:level1item and no options can be passed into it anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Embarrassingly (for me) you might be right. I made the assertion based on looking at a book project that used CaSILE and had mix-matched dotfills and I could see it was responding ... but it turns out that book has its own forked copy of the TOC code from CaSILE that I made some other changes to and apparently fixed the option scoping bug without upstreaming it even to CaSILE much less SILE. Gaveraboglesplitişlöbğ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to see how you addressed the problem! I am happy to discuss in a dedicated issue or PR

@alerque alerque added the enhancement Software improvement or feature request label Dec 9, 2022
@alerque alerque marked this pull request as draft December 12, 2022 21:19
@alerque alerque added this to the v0.x.y milestone Dec 12, 2022
@raphCode
Copy link
Contributor Author

I noticed the following messages from @Omikhleia in my email inbox but can't find them here. I will answer for completeness:

Why a setting and not a tableofcontent parameter?

I am not sure what you mean?

And I don't get how it is expected to work if some toc levels should have dots but not all.

The idea was since tableofcontents:level1item is called with the actual TOC entry as the content, one can redefine the command and change the setting before processing the content:

self:registerCommand("tableofcontents:level1item", function (_, content)
  SILE.settings:temporarily(function()
    SILE.settings:set("tableofcontents.hfill-command", "hfill")
    SILE.call("font", { size = 14, weight = 800 }, content)
  end)
end)

@Omikhleia
Copy link
Member

I noticed the following messages from @Omikhleia in my email inbox but can't find them here. I will answer for completeness:

Sorry about that. Too early posts, and I deleted them quite immediately after posting - It sounded all wrong to me to comment on a package I do not really use ^^.
For completeness too: I do think that the whole "hook" paradigm to be very TeX-like, but quite wrong with respect to styling - my rationale is explained in the document I linked above in a further message - and that adding an additional layer of settings and complexity to them is inherently flawed, especially linking to a random command by name, but I shouldn't have commented anyhow regarding that tableofcontents package, since I use replacement(s) of thereof in my serious projects and don't really care what it does. In addition, it only would help, in what seems to me a very contrived "ad-hoc" way, addressing one specific aspects of ToC (the presence or absence of dot leaders), while many other aspects remain unaddressed (e.g. absence of page numbering for some levels, typically "parts" - but I don't use the "book" class either, so heh... ;) ).

@Omikhleia Omikhleia added the modules:packages Issue relates to core or 3rd party packages label May 6, 2024
@Omikhleia
Copy link
Member

This old PR now has conflicts, and last discussions pointed it would not occur as-is.
I am closing it, but please feel free to re-open a dedicated issue and/or PR if a different approach should be discussed.

@Omikhleia Omikhleia closed this Oct 19, 2024
@alerque alerque reopened this Oct 29, 2024
@alerque alerque self-assigned this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Software improvement or feature request modules:packages Issue relates to core or 3rd party packages
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants