Skip to content

feat(developer): double markers once again 🙀#10541

Merged
srl295 merged 6 commits intomasterfrom
feat/core/10317-double-marker-epic-ldml
Feb 1, 2024
Merged

feat(developer): double markers once again 🙀#10541
srl295 merged 6 commits intomasterfrom
feat/core/10317-double-marker-epic-ldml

Conversation

@srl295
Copy link
Copy Markdown
Member

@srl295 srl295 commented Jan 29, 2024

#10516

@keymanapp-test-bot skip

@srl295 srl295 self-assigned this Jan 29, 2024
@srl295 srl295 force-pushed the feat/core/10317-double-marker-epic-ldml branch from 7693408 to 3767f7a Compare January 29, 2024 21:44
@srl295 srl295 marked this pull request as draft January 30, 2024 00:09
@srl295 srl295 marked this pull request as ready for review January 30, 2024 21:10
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM. My head is hurting now but it seems to all make sense

Comment on lines +351 to +365
// now, push the rest of the glue chars as an NFD sequence.
// For example, `\m{m}\u0344` will create the following stream:
// { ch: 0308, end: true}
// { ch: 0308, marker: 1}
// { ch: 0301, end: true} // added because of decomp
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see so the marker gets glued to the last character in the decomposition. Definitely slightly unintuitive -- more so than other gluings.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should be FIRST char

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not entirely following?

\m{m}\u0344 ==> \m{m}\u0308\u0301 or \u0308\m{m}\u0301?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

\m{m}\u0308\u0301 - adding a test. markers are re-added in reverse order.

@srl295 srl295 force-pushed the feat/core/10516-reorder-norm-epic-ldml branch from bf69802 to 7cb4697 Compare January 31, 2024 21:30
@srl295 srl295 requested a review from rc-swag as a code owner January 31, 2024 21:30
@srl295 srl295 force-pushed the feat/core/10317-double-marker-epic-ldml branch from 06b6e7f to 6b60ac4 Compare January 31, 2024 21:42
@srl295 srl295 force-pushed the feat/core/10516-reorder-norm-epic-ldml branch from 9d2731c to 2c92cfe Compare January 31, 2024 23:44
- use an 'end' marker for each NFD

Fixes: #10516
- revert comment
- reorder tests

Fixes: #10516
- fix off by one in adding denormalized glue chars

Fixes: #10516
- add another greek double reorder.  with chips.

Fixes: #10516
- clear the marker map at the end of remove_markers if no markers were found.
- add some suggested tests from @jahorton and @mcdurdin

For: #10317
@srl295 srl295 force-pushed the feat/core/10317-double-marker-epic-ldml branch from 6b60ac4 to 1988c43 Compare February 1, 2024 00:04
Base automatically changed from feat/core/10516-reorder-norm-epic-ldml to master February 1, 2024 05:18
@srl295 srl295 merged commit ab00f0c into master Feb 1, 2024
@srl295 srl295 deleted the feat/core/10317-double-marker-epic-ldml branch February 1, 2024 20:28
@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.259-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants