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

Per syllable #4510

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Per syllable #4510

wants to merge 3 commits into from

Conversation

behdad
Copy link
Member

@behdad behdad commented Dec 2, 2023

Trying to fix #4509
but tests fail.

@behdad behdad requested a review from dscorbett December 2, 2023 12:16
@behdad
Copy link
Member Author

behdad commented Dec 2, 2023

The use test expectations might need to be updated.

@khaledhosny
Copy link
Collaborator

The failing tests were added in e3db84a from #1695

@behdad
Copy link
Member Author

behdad commented Dec 2, 2023

The failing tests were added in e3db84a from #1695

I think it happened to work that way by chance. IIRC we always ignore ZWNJ/ZWJ in the backtrack/lookahead. Let me wrap my head around this.

@khaledhosny
Copy link
Collaborator

The failing tests were added in e3db84a from #1695

CoreText seems to give the old output for both failing tests.

@behdad
Copy link
Member Author

behdad commented Dec 2, 2023

Thanks. Yeah, the ZWNJ should do something there...

The logic here was made to enable per-syllable matching for input,
and not for backtrack/lookahead. However, if input-length is 1,
this was setting per-syllable for lookahead, which caused the following:

#4509

I have no idea why the tests are failing. Needs investigation.

I also have no idea why this was not discovered before.
Another test fails now. Needs to be investigated.
@behdad
Copy link
Member Author

behdad commented Dec 6, 2023

I spent some more time on this. I have no idea what's going on.

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.

None yet

2 participants