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.

Sign up for GitHub

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

Jump to bottom

Add hfill-command parameter to TOC #1650

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

Add hfill-command parameter to TOC #1650

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/tableofcontents/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

type = "string",
default = "dotfill",
help = "The sile command used to fill the space between a TOC entry and the corresponding page number"
})
end

function package:registerCommands ()

self:registerCommand("tableofcontents", function (options, _)
Expand Down Expand Up @@ -136,7 +145,7 @@ function package:registerCommands ()
end
end)
SILE.process(content)
SILE.call("dotfill")
SILE.call(SILE.settings:get("tableofcontents.hfill-command"))
SILE.typesetter:typeset(options.pageno)
end)
)
Expand Down