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

enhance mark extraction to account for forced breaks within the material #1177

Merged
merged 23 commits into from
Jan 29, 2024

Conversation

FrankMittelbach
Copy link
Member

@FrankMittelbach FrankMittelbach commented Nov 10, 2023

Internal housekeeping

Status of pull request

  • Ready to merge

Checklist of required changes before merge will be approved

  • Test file(s) added
  • Version and date string updated in changed source files
  • Relevant \changes entries in source included
  • Relevant changes.txt updated
  • Rollback provided (if necessary)?
  • ltnewsX.tex (and/or latexchanges.tex) updated

Copy link
Member

@josephwright josephwright left a comment

Choose a reason for hiding this comment

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

Changes.txt entry?

@FrankMittelbach
Copy link
Member Author

Changes.txt entry?

sure, but too early for that in my opinion. That's why it says under development and nothing is checkmarked above (I'm one of those folks who are really using Will's checkmark list :-) ). So right now all I'm really interested in is whether the logical seems to be right.

@josephwright
Copy link
Member

Changes.txt entry?

sure, but too early for that in my opinion.

Obviously we view this a bit differently: I thought the changes.txt had to be updated when you edit the source, even if that means we have a series of edits to changes.txtduring a PR. I am though happy if you want to leave until the end.

@FrankMittelbach
Copy link
Member Author

FrankMittelbach commented Nov 13, 2023

@josephwright I agree that doing this in parallel is useful most of the time. However, in this case I expect many more changes (and possibly alternative implementation), so I'm going lightweight on programmer-level docs and do them when things are a bit more settled.

Use it to also provide \__mark_get_marks_for_reinsertion:nNN for use in multicols (and elsewhere) eventually
@FrankMittelbach
Copy link
Member Author

I altered and extended the implementation in anticipation what is necessary for multicols and/or in general for extracting marks out of box material to reinsert them afterwards. This is now closer to what I think is needed so change entries will come up soon @josephwright :-)

@muzimuzhi
Copy link
Contributor

To reduce the ambiguity, perhaps a new item can be added to section "Status of pull request", in pull request template, to indicate that the PR author is aware of the missing changes.txt entries and friends.

For example,

- In early stage (`changes.txt` entries and friends might be missing)

@FrankMittelbach
Copy link
Member Author

To reduce the ambiguity, perhaps a new item can be added to section "Status of pull request", in pull request template, to indicate that the PR author is aware of the missing changes.txt entries and friends.

sorry don't get it. Of course, I'm aware (and so should everybody else) since I didn't checkmark those item nor did I put n/a (not applicable) into the checkmark box. So it is all there at the very top. Why should a different string from

  • Feedback wanted
  • Under development

plus the checkmark list, provide any additional info? It is more that often people do not use the checkmark list, but that is not helped I think by asking for providing a different status string.

base/ltmarks.dtx Outdated
% executes \verb/#1{#3}/ to do something with the marks, e.g., to
% update the mark structure for the region.
%
% If it finds infinite negative glue it generates an error message
Copy link
Member

Choose a reason for hiding this comment

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

could you get any other tex error at this point, since the material is already a vlist?If not I suppose you could consider setting interactionmode and restoring it to suppress this error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting thought, but I don't think there is a way to detect then that the error has happened and it is an error, ie the user is losing the marks and so I think they should learn that this is the case. But perhaps if it is enough if that is something you can find in the .log? It would certainly has the advantage that then the processing doesn't stop if they can't avoid the infinite negative glue. Maybe.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, back to the drawing board perhaps. If we run with \interactionmode 0, the error only appears in the log and we actually get to the marks even if there is real infinite negative glue (because TeX makes it finite).

In other words, if we accept that we may see

! Infinite glue shrinkage found in box being split.
\__box_dim_eval:n ...box_dim_eval:w #1\scan_stop: 
l. ...   }
The box you are \vsplitting contains some infinitely
shrinkable glue, e.g., `\vss' or `\vskip 0pt minus 1fil'.
Such glue doesn't belong there; but you can safely proceed,
since the offensive shrinkability has been made finite.
! Infinite glue shrinkage found in box being split.

in the log then we can get at the marks in all cases, which seems better then issuing an error or a warning that it doesn't work.

If we want to we could additionally do

\wlog{Any immediately following 'Infinite glue shrinkage' error is harmless and can be ignored}

but I'm not sure this is better, because most of the time there will be none and we only end up with these extra lines. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

yes wlog would be confusing I think, for the reason you say.

Copy link
Member

@davidcarlisle davidcarlisle Nov 14, 2023

Choose a reason for hiding this comment

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

You could use \ignore_infinite_shrinkage_error defined as \let\ignore_infinite_shrinkage_error\__box_dim_eval:n perhaps....

Copy link
Member Author

@FrankMittelbach FrankMittelbach Nov 14, 2023

Choose a reason for hiding this comment

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

??? not sure I understand the suggestion, could you perhaps simply implement it? the branch is up to date and I don't intend to alter it right now

Copy link
Member

Choose a reason for hiding this comment

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


\expandafter\def\csname Ignore any infinite glue shrinkage error on this line\endcsname{
\setbox0\vbox{aaa \vss bbb}%
\setbox2=\vsplit0 to \maxdimen
}
\expandafter\def\expandafter\foo\expandafter{%
  \csname Ignore any infinite glue shrinkage error on this line\endcsname}
{
\interactionmode=0
\foo
}

makes a log

! Infinite glue shrinkage found in box being split.
\Ignore any infinite glue shrinkage error on this line ...
                                                  
l.13 \foo

Copy link
Member

Choose a reason for hiding this comment

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

I could probably try adding something like this later this evening

base/ltmarks.dtx Outdated Show resolved Hide resolved
Copy link
Contributor

@car222222 car222222 left a comment

Choose a reason for hiding this comment

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

Lots of commas (and a few "uncommas"), etc.

base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
base/ltmarks.dtx Outdated Show resolved Hide resolved
@davidcarlisle
Copy link
Member

davidcarlisle commented Nov 14, 2023

I pushed a change change the glue shrink message


  ! Infinite glue shrinkage found in box being split.
! \__box_dim_eval:n ...box_dim_eval:w #1\scan_stop: 

--- 113,126 ----

  ! Infinite glue shrinkage found in box being split.
! <argument> \Ignore infinite glue shrinkage error 

The test file xmarks-009 fails showing this diff, I didn't update it, to make it easier to back out this commit if you don't like it, and to highlight the diff.
It requires a bit of balancing to get the string at the right expansion level but it seems to work here without breaking the other ltmarks tests.

@FrankMittelbach
Copy link
Member Author

The test file xmarks-009 fails showing this diff, I didn't update it, to make it easier to back out this commit if you don't like it, and to highlight the diff. It requires a bit of balancing to get the string at the right expansion level but it seems to work here without breaking the other ltmarks tests.

I like the idea and it makes the code much simpler --- and we get correct results even if there is shrinkage in the material which is great. I altered the txt a bit (not sure if it is for the better). Right now the test suite fails in a very strange way though and I don't understand why.

@davidcarlisle
Copy link
Member

The test file xmarks-009 fails showing this diff, I didn't update it, to make it easier to back out this commit if you don't like it, and to highlight the diff. It requires a bit of balancing to get the string at the right expansion level but it seems to work here without breaking the other ltmarks tests.

I like the idea and it makes the code much simpler --- and we get correct results even if there is shrinkage in the material which is great. I altered the txt a bit (not sure if it is for the better). Right now the test suite fails in a very strange way though and I don't understand why.

yes lots of newlines in the raw log before the [4 page marker which looks like some extra empty \write but I don't see how that can be. eg the raw log for tlb-dox002 has

[3


]) (./tlb-dox002.gls



[4



])

On the terminal it looks like

[3]) (./tlb-dox002.gls



[4]) (./tlb-dox002.aux)

so the extra newlines are only going to the log

@FrankMittelbach
Copy link
Member Author

but I don't see how that can be. eg the raw log for tlb-dox002 has

neither can I but I really would like to understand before I update hundreds of test files

@davidcarlisle
Copy link
Member

@FrankMittelbach turns out \nonstopmode causes a newline in the log, if you keep all the code apart from this , the breaks go.

possibly from

@<Change the interaction...@>=
begin error_count:=0; interaction:=batch_mode+c-"Q";
print("OK, entering ");
case c of
"Q":begin print_esc("batchmode"); decr(selector);
  end;
"R":print_esc("nonstopmode");
"S":print_esc("scrollmode");
end; {there are no other cases}
print("..."); print_ln; update_terminal; return;
end

but my web is a bit rusty:-)

@davidcarlisle
Copy link
Member

davidcarlisle commented Nov 14, 2023

@FrankMittelbach


\wlog{===}
\wlog{===}
\nonstopmode
\interactionmode=1
\interactionmode=2
\wlog{===}

\bye

produces

===
===



===

but since the test suite uses \scrollmode anyway an alternative plan to avoid changing the tests and adding newlines to end user log files would be to skip saving and reseting \interactionmode unless its current value is 3 (errorstopmode) I think any of the values 0,1,2 would work for this so we don't need to switch?

@FrankMittelbach
Copy link
Member Author

I guess I'm more for fixing the tlgs instead of doing something tailored to our test suite. But I have another issue. When I locally run l3build check it seems too loop after processing (or while?) github-0094 and I don't find what happens there. When I run 0094 by itself (or the one that I think comes after it all seems fine

@davidcarlisle
Copy link
Member

@FrankMittelbach

If I run

l3build check github-0094

It stops in error stop mode at the terminal with

! Undefined control sequence.
\pdfstartlink ->\pdfextension 
                              startlink 
l.25 \leavevmode\pdfstartlink
                             
? 

@FrankMittelbach
Copy link
Member Author

and the same happens for l3build check tltc001 with xetex (as if the interaction mode it not properly restored), now why is that?

@davidcarlisle
Copy link
Member

and the same happens for l3build check tltc001 with xetex (as if the interaction mode it not properly restored), now why is that?

well why is it using luatex back end syntax with xetex

@davidcarlisle
Copy link
Member

me> well why is it using luatex back end syntax with xetex

because it's defined that way in the test file, this ought to be in a pdftex-luatex config, probably

@FrankMittelbach
Copy link
Member Author

All true but why is it not running scrollmode?

@FrankMittelbach
Copy link
Member Author

After the test suite issues have been identified and corrected, I think this is now ready to be merged. Open questions

  • Should there be a rollback (would be tricky and I tend to think no)
  • Is the error message ok, the way it is now? (there is not much space availabel, ie one has to stay with more or less that many chars)
  • Is the news entry ok?

Copy link
Member

@davidcarlisle davidcarlisle left a comment

Choose a reason for hiding this comment

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

looks good to me, I don't think it needs rollback, I made a couple of minor tweaks to the news text. Error text is OK, I played around in my initial draft but getting the text just to appear in the right place doesn't leave a lot of room, as you noted Your version is fine.

@FrankMittelbach FrankMittelbach merged commit d87b8fc into develop Jan 29, 2024
78 checks passed
@FrankMittelbach FrankMittelbach deleted the ltmarks-enhance branch January 29, 2024 09:13
jlaurens pushed a commit to jlaurens/latex2e that referenced this pull request Jun 6, 2024
…ial (latex3#1177)

* enhance mark extraction to account for forced breaks within the material (WIP)

* Generalize mark extraction code;
Use it to also provide \__mark_get_marks_for_reinsertion:nNN for use in multicols (and elsewhere) eventually

* added change entries
typos and corrections

* first attempt using \interactionmode (WIP)
code can be simplified if we go this way.

* improve infinite glue shrinkage log message

* some documentation of David's idea

* hmm

* much simpler this way now (and the marks always correct)
documentation not yet updated and old code still mostly inside commented out

* the ways of TeX are mysterious --- anybody any idea how this change can be triggered from ltmarks.dtx?

* more of those ... strange

* and another one

* some cleanup and many more test files showing extra lines in the log

* too much removed from the doc

* and a few more tests ...

* even more test files

* added ltnews entry

* minor wording adjustments

* a bit more docu cleanup

* tlg updates

* correct date

---------

Co-authored-by: David Carlisle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants