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

\par creates space in the wrong location #1647

Closed
raphCode opened this issue Dec 6, 2022 · 2 comments · Fixed by #1752
Closed

\par creates space in the wrong location #1647

raphCode opened this issue Dec 6, 2022 · 2 comments · Fixed by #1752
Assignees
Labels
bug Software bug issue
Milestone

Comments

@raphCode
Copy link
Contributor

raphCode commented Dec 6, 2022

In this document:

\begin[papersize=a9]{document}
\set[parameter=document.parskip, value=30pt]
\par  % troublemaker
aaa
\vfill
bottom text
\par
\break
\end{document}

(papersize a9 is just to create small screenshots)

The first \par creates a space on the bottom of the page, lifting the "bottom text" from the bottom of the content frame:
image

Adding a \hbox at the beginning of the document or removing the first \par yields the correct result with the bottom text aligned to the bottom:
image

Is that the intended behavior?

@Omikhleia
Copy link
Member

Omikhleia commented Mar 10, 2023

Running with -d all and comparing the ouput with the \par and without it...

So far so good, when pagebuilder:findBestBreak() works, the only difference is the 30pt top parskip.
image

Note: all vboxes are dumped in the debug, but afterwards the method skips top glues for the purpose of measuring the vertical content and finding an appropriate break. That's quite correct in my opinion (see further below). One may appreciate, though, the fanciness of that piece of art (absence of comments, going back and forth in indexes, complex restart logic which I hope is not used at all -- erm, seems commented out indeed elsewhere, pfeww).

But then...

image

"ouputting" is the last debug from pagebuilder:findBestBreak().
After, we are in typesetter.setVerticalGlue(), which is responsible for adjusting stretchable/shrinkable glues. And we see that the supposedly ignored top glue has been taken into account. And that is a bug, resulting in the other glues (here the infinite vfill) not to be stretched as much as expected.

From "OUTPUTTING", we now are in typesetter:outputLinesToPage() and the latter ignores top explicit/discardable glues with a spiffy / wacky comment "Annoyingly, explicit glue should disappear at the top of a page. if you don't want that, add an empty vbox or something." -- That's consistent with what findBestBreak did... Er. Or is it? The latter skipped glues regardless of them being explicit/discardable. Oh my. Anyway, that's not consistent with what setVerticalGlue() does...

Short recap conclusion:

  • It's indeed a bug, more important than the edge-case issue that raised it (that \par inserted a parskip, but any vertical glue would have been subject to the same problem)
  • The implementation shows lots of discrepancies... EDIT: And heavy code smell. I'm staying polite, due to the age of SILE. Nah, I'm just being ironic.
  • I'm feeling sad nobody tried at least to do what I did = just run the MWE and compare debug outputs) EDIT: And that @raphCode had to wait 3 months for at least a feedback...

@raphCode
Copy link
Contributor Author

Thanks for taking the time to analyze this! :)
I still don't understand SILE internals enough to fully comprehend your description so I can just say Kudos!


  • I'm feeling sad [...] that @raphCode had to wait 3 months for at least a feedback...

I don't really mind waiting, I know people are busy and the bug is not a blocker for me.
I just want this project to succeed / become better so I filed a bug.

  • The implementation shows lots of discrepancies... EDIT: And heavy code smell.

To me, the project looks like it evolved historically and that always comes with non-optimal code. That is often even required for some time to explore the desired direction and goals of the project / algorithms.
If it has been figured out how things should work, it paves the way for a beautiful rewrite someday.
I saw some nicer code in other places and noticed some refactors / rewrites in the development history, so I am confident everything will improve eventually :)

@Omikhleia Omikhleia added the bug Software bug issue label Mar 25, 2023
@Omikhleia Omikhleia self-assigned this Mar 25, 2023
@Omikhleia Omikhleia added this to the v0.14.9 milestone Mar 27, 2023
Omikhleia added a commit to Omikhleia/resilient-types that referenced this issue Mar 28, 2023
Looking closely, this test expection had an extra parskip at the
bottom of the first page, exactly the same symptom as sile-typesetter#1647.
Omikhleia added a commit to Omikhleia/resilient-types that referenced this issue Mar 28, 2023
Looking closely, this test expection had an extra parskip at the
bottom of the first page, exactly the same symptom as sile-typesetter#1647.
Omikhleia added a commit to Omikhleia/resilient-types that referenced this issue Mar 28, 2023
It's harder to say how it relates to sile-typesetter#1647, but checking with
-d all, this test showed a small vertical space at the bottom of
page 3, meaning the content frame was not stretched as much as
expected. It seems to be some lineskip. Now the text seems fully
stretched within the content frame.
Omikhleia added a commit to Omikhleia/resilient-types that referenced this issue Mar 28, 2023
Looking closely, this test expection had an extra parskip at the
bottom of the first page, exactly the same symptom as sile-typesetter#1647.
Omikhleia added a commit to Omikhleia/resilient-types that referenced this issue Mar 28, 2023
It's harder to say how it relates to sile-typesetter#1647, but checking with
-d all, this test showed a small vertical space at the bottom of
page 3, meaning the content frame was not stretched as much as
expected. It seems to be some lineskip. Now the text seems fully
stretched within the content frame.
Omikhleia added a commit to Omikhleia/resilient-types that referenced this issue Mar 28, 2023
The changes from sile-typesetter#1647 impact this test on page 3 (last line is
slightly lower), this doesn't seem worse though the test is hard
to check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Software bug issue
Projects
None yet
2 participants