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

The reordering methods are a bit of a mess #106

Open
Manishearth opened this issue Mar 21, 2023 · 0 comments
Open

The reordering methods are a bit of a mess #106

Manishearth opened this issue Mar 21, 2023 · 0 comments

Comments

@Manishearth
Copy link
Member

I've improved the docs in #105 but I think our API surface for reordering is kinda bad.

We have the following APIs (I recommend going through the docs in #105 as well if it's not been landed and published):

  • reordered_levels() and reordered_levels_per_char() which run L1
  • reorder_visual(), which runs L2.
    • Returns a visual-to-logical map given a list of levels. This matches a similar API exposed by ICU4C for people who want to manage their own levels.
    • The corresponding ICU4C API is used by skia; this is actually useful even though it may seem like a weird slice of the API. In particular, you can do things like feeding it a list of levels after you have collapsed down combining chars for L3.
    • A tricky thing about this is that it will happily reorder within byte indices if it has been fed byte indices.
  • visual_runs().
    • Logically, this is the combination of reordered_levels_per_char() and reorder_visual(), except instead of returning a visual-to-logical map, it returns a vector of level runs in visual order
    • It makes some sense for this API to not return a visual-to-logical map since a visual-to-logical map that reorders every byte will be bad. Level runs are in general a very useful API for this since callers can handle L3 (combining chars) themselves.
    • However it still kind of makes sense to be able to get such a map at a per-character level.
  • reorder_line(), which returns a line reordered char-by-char according to the result of visual_runs().
    • This can be misleading since it does not handle L3 (combining chars) or L4 (mirroring), so it'll produce a weird string with combining characters in the wrong place.
    • I'm not actually sure if this API makes sense at all: this crate makes the sensible decision to defer L3 and L4 to the caller engine (since it's entangled with engine-specific concerns), but this is a convenience API for reordering which goes straight-to-user.

Concrete issues:

I think it might be worth doing a holistic overhaul of these after figuring out use cases, and deprecating the old APIs. I'm not particularly fond of the API surface of ICU4C either, I've always found it confusing.

cc @mbrubeck @sffc and maybe @markusicu

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

No branches or pull requests

1 participant