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

Huge LaTeX fix #269

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

ahsoltan
Copy link

KACTL has long been plagued with LaTeX issues (#128), making it difficult to modify and compile without causing spacing problems. This PR changes the document class from extreport to extarticle, which surprisingly fixes all overlapping text and similar issues.

One notable side effect is that the spacing is slightly different in some places (mainly because penalties work differently for some reason), but this is not a major issue, as everything still fits within 25 pages. Additionally, as far as I know, the large spaces between code snippets disappear when more content is added.

@simonlindholm
Copy link
Member

Wow, this is super nice!

It feels to me when looking at the new and old pdf side by side that section titles now have a bit more vertical spacing around them, do you think some of the size growth could be clawed back by putting some negative margins there?

I get a build error on first build unless I remove the build directory, would be nice if we could make that happen automatically using some make rule (maybe add a new dummy file in build/ with a rule that wipes build/kactl.*, and have every build rule depend on that?).

@ahsoltan
Copy link
Author

The spacing around section titles seems very volatile for some reason.

I tried to modify the spacing to match the current kactlpkg:

\renewcommand\section{\@startsection {section}{1}{\z@}%
                                   {-0.5ex \@plus -1ex \@minus -.2ex}%
                                   {.4ex}%
                                   {\normalfont\LARGE\bfseries}}

\renewcommand\subsection{\@startsection{subsection}{2}{\z@}
	{1ex\@plus -1ex \@minus -.3ex}
	{.2ex}
	{\normalfont\large\bfseries}}

\renewcommand\subsubsection{\@startsection{subsubsection}{3}{\z@}
	{1ex\@plus -1ex \@minus -.3ex}
	{1ex \@plus .2ex}
	{\normalfont\large\bfseries}}

This should be pretty straightforward, since titlesec provides almost the exact same parameters as \@startsection, but after translating over even one of the rules:

\titlespacing{\subsection}{0pt}{-0.5ex plus -1ex minus -.2ex}{.4ex}

The spacing is reduced (but still different from the old PDF), however this happens:

image
Which is very weird, since this isn't even an area around a \subsection. I also tried using the default spacing values from the report document class, but that also caused overlaps.

Fixing the spacing seems hard, and it looks to me like most of the size growth is caused by tex trying to align headings with page breaks (for example 10.2, 10.3), maybe we should focus on that instead?

Regarding the build errors - they must've been caused by a conflict between with the old build files (from before my patches). I don't think we should clean them every time make is run, since it is convenient to only have to run tex once when working locally.

@simonlindholm
Copy link
Member

I don't think we should clean them every time make is run, since it is convenient to only have to run tex once when working locally.

Agreed. I'm only concerned about upgrade scenarios from before this PR to after. It adds some ugliness to the makefile to deal with a one-time event, but I think it's worth it given the number of KACTL users.

@ahsoltan
Copy link
Author

Added a flag file, should be a seamless upgrade now.

@IvanRenison
Copy link
Contributor

I get a build error on first build unless I remove the build directory, would be nice if we could make that happen automatically using some make rule (maybe add a new dummy file in build/ with a rule that wipes build/kactl.*, and have every build rule depend on that?).

I think that only happens when the last build was before the pull request, so once this pull request is merged you only need to remove the build directory once, is it necessarily to add the flag file only for that?

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.

3 participants