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

Typesetter and package support for constructs spanning multiple lines #1977

Merged
merged 7 commits into from
Feb 2, 2024

Conversation

Omikhleia
Copy link
Member

@Omikhleia Omikhleia commented Jan 30, 2024

New approach to #1334 (with nicer user-facing API), based on Omikhleia/silex.sile#10 -- See the latter for illustration and screenshot.

Caveat: It does further break the broken "pushback" logic (due to additional reboxing)

@Omikhleia Omikhleia added bug Software bug issue enhancement Software improvement or feature request labels Jan 30, 2024
@Omikhleia Omikhleia requested a review from alerque as a code owner January 30, 2024 20:41
@Omikhleia
Copy link
Member Author

N.B. Proposed on "develop" for 0.15 as the pdf changes there make it easier.
The other changes (i.e. rules, color) were working on 0.14.16.

Is it me or GitHub doesn't notice the "Closes XXX" text in enumerations? ^^

typesetters/base.lua Outdated Show resolved Hide resolved
@alerque
Copy link
Member

alerque commented Jan 31, 2024

This passes all the regression and unit tests, but it won't build the manual (which is why the CI build failure). At far as code review I liked most of what I saw so far. I'm a little concerned about exposing all the public methods on typesetter. If those actually need to be exposed to the end user and packages etc. that's fine, but if there is any reason to consider them an implementation detail that could be refactored without having to maintaine support for the way the previous methods worked I would mark them as private (underscore prefix or make them local functions in the module).

@Omikhleia
Copy link
Member Author

Omikhleia commented Jan 31, 2024

(underscore prefix or make them local functions in the module)

I was thinking the same, and many other methods in the typesetters are kind of internal too. I am not fond of making them local, as I like the idea anyone can tinker with them, especially in derived classed, and was thinking of prefixes too. _xxx or internalXxx or privXxx ? (I'd like to convey the idea that they are more or less "protected" (in the C++ sense) and anyone tinkering with them does so at their own risks ^^)

@alerque
Copy link
Member

alerque commented Jan 31, 2024

I've already been using the convention (used in many other Lua projects) of prefixing functions and variables with _ if they are intended to be "internal". SILE has a bunch of them already, and the main point I'd make is that I don't have any qualms about refactoring them with breaking changes without marking the release as breaking or providing shims. In other words "tinker with this at your own risk". Lets make as many of the typesetter bits as we reasonable can this kind of "privateish" so we don't have to support old versions if we decide to refactor while still leaving things open to tinkering. Of course anything that obviously makes sense to be user or package facing can be public.

A construct for content spanning multiple lines and needing to be
wrapped for some effect on each line.
@Omikhleia Omikhleia requested a review from a team as a code owner February 2, 2024 11:28
@alerque alerque force-pushed the feat-liner-support branch from bfbc84d to 08b86a3 Compare February 2, 2024 11:32
@alerque alerque added this to the v0.15.0 milestone Feb 2, 2024
@alerque alerque force-pushed the feat-liner-support branch from 08b86a3 to b0027f8 Compare February 2, 2024 11:36
@alerque
Copy link
Member

alerque commented Feb 2, 2024

Rebase of old commits was just to tweak a commit message. The only change I actually made was the minor commit at the end to bludgeon CJK into cooperation.

This notably affects Latin (non CJK) text in JA language because the
spacer width node is computed at a different time and hence possible
using a different font. This result isn't necessarily *correct* but it
is not what this regression test is even testing.
@alerque alerque merged commit bd0746e into sile-typesetter:develop Feb 2, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Software bug issue enhancement Software improvement or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants