Showframe should have an option to apply for every page just like \background. #1909
Conversation
|
I've never felt the need for this because if I'm debugging an entire document then Not saying this can't go in, just thinking out loud about ergonomics... |
What a shame, I had no idea of this command 🤦. I'm aware of a debugging facility but never tried it so far... Well, anyway, thanks for telling me that, I'll take a look at the debugging part of documentation... |
| if id == "all" then | ||
| for _, frame in pairs(SILE.frames) do | ||
| SILE.outputter:debugFrame(frame) | ||
| options.allpages = options.allpages or true |
There was a problem hiding this comment.
This should default to false and use SU.boolean() to do so.
| options.allpages = options.allpages or true | ||
|
|
||
| local show = function() | ||
| if id == "all" then |
There was a problem hiding this comment.
Even local functions should not use variables outside of their scope with a documented reason for needing to. This is called a side effect and has lots of downsides, not to mention will make porting harder down the road. Just add an id field to the function and pass it through explicitly.
| end | ||
| end | ||
|
|
||
| if options.allpages and options.allpages ~= "false" then |
There was a problem hiding this comment.
See above, but since this should already be cast to a boolean at this point the logic is much simpler: if not options.allpages then.
|
|
||
| if options.allpages and options.allpages ~= "false" then | ||
| show() | ||
| local oldNewPage = SILE.documentState.documentClass.newPage |
There was a problem hiding this comment.
No, don't do this. I know we do funky things like this elsewhere but we're trying to move away from this, not towards it. Set a new page hook instead and just leave the class:newPage() function alone.
There was a problem hiding this comment.
Sorry, I don't know how to play with hooks for now. I read this part of the documentation and searched for uses of it in this repository, but still can't manage to set hooks around there... Can you give me at least an example?
There was a problem hiding this comment.
Sure, happy to help. I think in this case this whole section of code can be reduced as in this code suggestion comment.
| class:loadPackage("frametricks") | ||
|
|
||
| SILE.call("showframe",{ id = "all"}) | ||
| for i = 1, 50 do |
There was a problem hiding this comment.
When writing regression test, we want as little output as possible to prove the case. In this case just one supereject and a tiny bit of content should do the trick, no need for 50 pages. Unless you expect to stop functioning after passing 42 for ... galaxy reasons.
alerque
added
enhancement
| local oldNewPage = SILE.documentState.documentClass.newPage | ||
| SILE.documentState.documentClass.newPage = function (self_) | ||
| local page = oldNewPage(self_) | ||
| show() | ||
| return page | ||
| end |
There was a problem hiding this comment.
Following up from this review comment, I had to start a new thread to get all the relevant lines tagged in the comment. This replacement should get the job done with a bit less fuss:
| local oldNewPage = SILE.documentState.documentClass.newPage | |
| SILE.documentState.documentClass.newPage = function (self_) | |
| local page = oldNewPage(self_) | |
| show() | |
| return page | |
| end | |
| self.class:registerHook("newpage", show) |
There was a problem hiding this comment.
Thanks for the tip. I got the following error when testing:
`Using code at: ./core/frame.lua:261: Passed a table, expected a string
So when I do self.class:registerHook("newpage", show(id) the error isn't raised, but it still gives the same output: just the first page is debugged...
|
Sorry for the waste of time, after using the |
I missed an option for this and here is a possible solution.