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

Update to current parley main #870

Merged

Conversation

waywardmonkeys
Copy link
Contributor

This updates to the version using the updated objc2 and binding crates for Fontique.

It also has a minor API change.

@@ -386,7 +386,7 @@ impl Widget for Label {
};
if self.alignment_changed {
self.text_layout
.align(Some(alignment_width), self.alignment);
.align(Some(alignment_width), self.alignment, false);
Copy link
Member

Choose a reason for hiding this comment

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

Obviously I'm not super happy about an ugly boolean parameter appearing here. For reference, it seems to be from linebender/parley#241, meaning whether we align when overflowing.
The lack of tests in that PR to make it more comprehensible isn't ideal...

In this case:

  1. I agree that false maintains the same behaviour here. I think that true would maybe be a better default, but that should be a follow-up
  2. I think that Replace boolean option in Layout::align with AlignmentOptions Struct parley#247 should block parley 0.3 (cc @tomcur); the fact that every use in Make alignment when free space is negative configurable parley#241 needed to be documented with the parameter name as a comment strongly supports this...

Copy link
Member

Choose a reason for hiding this comment

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

I think that linebender/parley#247 should block parley 0.3

Yes, that issue is blocking 0.3. Nico opened a PR fixing it yesterday: linebender/parley#278.

I agree that false maintains the same behaviour here. I think that true would maybe be a better default, but that should be a follow-up

I believe false (start-alignment for lines that overflow) is what web does, and currently that's the planned default for the AlignmentOptions introduced in Nico's PR.

This updates to the version using the updated objc2 and binding
crates for Fontique.

It also has a minor API change.
@waywardmonkeys waywardmonkeys force-pushed the update-to-current-parley-head branch from 09c9ddf to 375c4f1 Compare February 17, 2025 15:12
@waywardmonkeys
Copy link
Contributor Author

@DJMcNab I've removed this from the auto-merge because I had to update a single screenshot test. @tomcur You might want to take a peek just to be sure all is well.

@nicoburns
Copy link
Contributor

nicoburns commented Feb 18, 2025

@DJMcNab I've removed this from the auto-merge because I had to update a single screenshot test. @tomcur You might want to take a peek just to be sure all is well.

@waywardmonkeys It looks to me like the old version is including trailing whitespace in the width of the line for the purposes of alignment, and the new version is ignoring. If so, then I believe the new version is correct. Eyeballing it, the text looks more centered in the new screenshot.

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Feb 19, 2025
Merged via the queue into linebender:main with commit f63e709 Feb 19, 2025
18 checks passed
@waywardmonkeys waywardmonkeys deleted the update-to-current-parley-head branch February 19, 2025 02:56
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.

4 participants