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

Suppress hyphenation on the last line of pages #39

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

bertfrees
Copy link
Contributor

@bertfrees bertfrees commented Apr 29, 2022

@kalaspuffar @PaulRambags I've added a new method allowsEndingPageOnHyphen() to the FormatterConfiguration API along the lines of the existing allowsEndingVolumeOnHyphen() method, but I could also have merged the two into a single new method that accepts an enum (true|false|except-at-volume-breaks).

@fredrikschill rightly remarked that never allowing hyphenation at page breaks could also become the default behavior, and nobody would have any problems with this. This means dropping allowsEndingVolumeOnHyphen().

This morning we also mentioned that we might want to make an exception for compound words: we could allow compound words to be hyphenated across pages (except at volume boundaries maybe, but we also said that we should consider to try not breaking blocks across volumes by default, i.e. the current behavior when a volume-transition element is present).

Regarding compound words that contain a hyphen: CSS does not consider breaking the word after the hyphen as "hyphenation", so in Pipeline I follow this recommendation and so even when hyphenation is disallowed at page breaks, I do allow breaking after a hard hyphen. (I might provide ways to suppress it though, and/or I might also make the hyphenation smarter so that words like "t-shirt" and "e-mail" are not considered compound words.)

Regarding compound words that don't contain a hyphen: recognizing these words is not trivial, I'm afraid it would be way too much work for such a small optimization.

Finally, I'd also like to remind us of the related bug brailleapps/dotify.formatter.impl#46 (allowsEndingVolumeOnHyphen(false) does not work when a volume-transition element is present).

to suppress hyphenation on the last line of pages.
@kalaspuffar
Copy link
Collaborator

I think this change is fine; now sure if @PaulRambags will be able to review it in the near future. Are this one ready for merging, or do you want to add more? @bertfrees

@bertfrees
Copy link
Contributor Author

bertfrees commented May 30, 2022

No, it's ready.

I should add that I did not do exactly what we said in the last call, namely that we would change the default values of allowsEndingVolumeOnHyphen and the new allowsEndingPageOnHyphen to false, AND also remove the settings. On second thought I decided to not remove the settings because I thought that would be a too radical change. Now, in order for the regressions to pass, you will probably have to set both settings to true, but if the settings were removed completely you would have to update a lot of expected result files. Let me know if this is still OK for you.

@kalaspuffar
Copy link
Collaborator

@bertfrees Well if that is the expected behavior going forward then it's ok that this review will be a little longer in order to update the regression tests. What I need to do is verify the PR, do a release and then create a new baseline. Then we can continue using the library with the new default behavior.

@kalaspuffar
Copy link
Collaborator

PR approved, running regression tests.

@kalaspuffar
Copy link
Collaborator

Regression tests passed, merging.

@kalaspuffar kalaspuffar merged commit 2159fbf into mtmse:main Jun 10, 2022
@bertfrees bertfrees deleted the suppress-ending-page-on-hyphen branch June 28, 2022 11:00
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.

2 participants