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.

Sign up for GitHub

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

Jump to bottom

Fix hyphen duplication rule for some languages #4058

Merged
merged 3 commits into from May 15, 2024
Merged

Fix hyphen duplication rule for some languages #4058

merged 3 commits into from May 15, 2024

Conversation

gabriel-araujjo
Copy link
Contributor

@gabriel-araujjo gabriel-araujjo commented May 2, 2024 •
edited

This pr closes #3235.
Implement hyphen duplication rule for Czech, Croatian, Lower Sorbian, Polish, Portuguese, Slovak and Spanish.

So the input

#set page(width: 4cm, height: 4cm, margin: 2mm)
#set text(lang: "pt", region: "BR")
#set par(justify: true)

Alguma coisa no arco-da-velha é algo que está muito longe.

Um médico otorrinolaringologista cuida da garganta do paciente.

generates

right hyphen in portuguese

Tasks

  • implementation
  • Tests
  • More complete rules on should_repeat_hyphen() function.

@gabriel-araujjo gabriel-araujjo marked this pull request as draft May 2, 2024 04:02
@gabriel-araujjo gabriel-araujjo force-pushed the hyphenation-impls branch 8 times, most recently from ae55845 to 257b556 Compare May 4, 2024 16:43
@gabriel-araujjo gabriel-araujjo marked this pull request as ready for review May 4, 2024 16:48
@gabriel-araujjo gabriel-araujjo changed the title Fix hyphenation rules for Portuguese and Polish Fix hyphenation rules for some languages May 4, 2024
@gabriel-araujjo gabriel-araujjo changed the title Fix hyphenation rules for some languages Fix hyphen duplication rule for some languages May 4, 2024
PgBiel
PgBiel reviewed May 4, 2024
View reviewed changes
Copy link
Contributor

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

Some initial observations:

crates/typst/src/layout/inline/dash.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
@laurmaedje laurmaedje added the waiting-on-review This PR is waiting to be reviewed. label May 6, 2024
laurmaedje
laurmaedje reviewed May 7, 2024
View reviewed changes
crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/shaping.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/dash.rs Outdated Show resolved Hide resolved
@laurmaedje laurmaedje removed the waiting-on-review This PR is waiting to be reviewed. label May 7, 2024
laurmaedje
laurmaedje reviewed May 13, 2024
View reviewed changes
crates/typst/src/layout/inline/mod.rs Outdated
Comment on lines 1120 to 1125
if prepend_hyphen && before.is_empty() {
reshaped.prepend_hyphen(engine, p.fallback);
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too fond of the duplication in doing it potentially in this branch (for the last item) and then again in the later branch (for the first item, if first != last). Maybe we could just do it at the end, similarly to maybe_adjust_first_glyph?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a do_prepend_hyphen variable. So I can reuse it on both places.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant was more to have the prepend_hyphen itself just a single time right before if maybe_adjust_first_glyph { with a construction similar to the first.as_mut().or(last.as_mut()) used there. Do you think that'd be possible and make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got it now.

crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
crates/typst/src/layout/inline/mod.rs Outdated Show resolved Hide resolved
@gabriel-araujjo gabriel-araujjo force-pushed the hyphenation-impls branch 2 times, most recently from 3d27b49 to bb68389 Compare May 14, 2024 11:07
@gabriel-araujjo
Fix hyphenation for some languages
66cf0fd 
- Czech
- Croatian
- Lower Sorbian
- Polish
- Portuguese
- Slovak
- Spanish

Fix typst#3235
@laurmaedje laurmaedje added this pull request to the merge queue May 15, 2024
@laurmaedje
Copy link
Member

Thank you!

Merged via the queue into typst:main with commit 017f2f4 May 15, 2024
6 checks passed
@gabriel-araujjo gabriel-araujjo deleted the hyphenation-impls branch May 15, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PgBiel PgBiel Awaiting requested review from PgBiel

@laurmaedje laurmaedje Awaiting requested review from laurmaedje

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect hyphenation in situations including dashes for some languages (e.g. polish)
3 participants
@gabriel-araujjo @laurmaedje @PgBiel