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

Fix and simplify unjustify, remove LineData::alignment #271

Merged
merged 5 commits into from
Feb 18, 2025
Merged

Conversation

tomcur
Copy link
Member

@tomcur tomcur commented Feb 8, 2025

unjustify doesn't mirror align, so it performs the wrong calculations, and re-line-breaking or re-aligning currently lead to the wrong result after a justified alignment.

As the alignment function requires further iteration (e.g. for floated boxes), this patch removes the specialized unjustify implementation and instead uses the same code for aligning and unjustifying. This is a trade-off between efficiency and correctness. If this is merged in, we can always split the two again in the future once alignment has stabilized.

This stores the width the layout was aligned to, which is necessary for unjustifying.

@tomcur tomcur added this to the 0.3 Release milestone Feb 8, 2025
Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

This is a bit awkward, but it seems reasonable. If it fixes unjustify then I'm happy for it to be merged.

Copy link
Contributor

@wfdewith wfdewith left a comment

Choose a reason for hiding this comment

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

Since it fixes and simplifies things, it makes sense to merge this, but the whole idea of 'unjustifying' text feels somewhat wrong to me.

`unjustify` doesn't mirror `align`, so it performs the wrong
calculations, and re-line-breaking or re-aligning currently lead to the
wrong result after a justified alignment.

As it's the alignment function requires further iteration (e.g. for
floated boxes), this patch removes the specialized `unjustify`
implementation and instead uses the same code for aligning and
unjustifying. This is a tradeoff between efficiency and correctness. If
this is merged in, we can always split the two again in the future once
alignment has stabilized.

This stores the width the layout was aligned to, which is necessary for
unjustifying.
@tomcur tomcur enabled auto-merge February 18, 2025 09:14
@tomcur tomcur added this pull request to the merge queue Feb 18, 2025
Merged via the queue into main with commit 9751217 Feb 18, 2025
21 checks passed
@tomcur tomcur deleted the unjustify branch February 18, 2025 09:17
@tomcur
Copy link
Member Author

tomcur commented Feb 18, 2025

Agreed with both comments. Doing it a different way has some tradeoffs: I think it would either require more memory even if justified alignment is not used (e.g., adding another field like ClusterData::adjusted_advance that would be set by alignment and used for positioning, keeping ClusterData::advance as the original value), or it would require more operations for positioning (e.g., some place where we store cluster adjustments, which are added to the cluster advances as they are being iterated for drawing).

github-merge-queue bot pushed a commit that referenced this pull request Feb 18, 2025
…struct (#278)

- Fixes #247
- Builds on top of #271 as
otherwise these would conflict

A small but nice improvement to the public API.

Signed-off-by: Nico Burns <nico@nicoburns.com>
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