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

[unic-bidi] Bugs in visual_runs #272

Open
raphlinus opened this issue Sep 1, 2020 · 1 comment · May be fixed by #278
Open

[unic-bidi] Bugs in visual_runs #272

raphlinus opened this issue Sep 1, 2020 · 1 comment · May be fixed by #278

Comments

@raphlinus
Copy link

raphlinus commented Sep 1, 2020

I was looking at the code of unic-bidi, considering using it for skribo-layout and trying to understand it better, and came across what I am pretty sure are some logic errors.

fn main() {
    let text = "a אב ג";
    println!("{:x?}", text.as_bytes());
    let bidi = unic_bidi::BidiInfo::new(text, None);
    println!("{:?}", bidi);
    let para = &bidi.paragraphs[0];
    println!("{:?}", bidi.visual_runs(para, 0..9));
    println!("{:?}", bidi.visual_runs(para, 0..7));
    println!("{:?}", bidi.visual_runs(para, 2..7));
}

The first behaves as expected, the runs are [0..2, 2..9].

In the second, I expect the whitespace at the end of line to be ordered after the RTL run, i.e. I expect the runs to be [0..2, 2..6, 6..7], but I get [0..2, 6..7, 2..6]. I see the L1 logic (resetting the EOL whitespace chars to paragraph level) in the returned "levels" array, but the reversing logic (line 366 and 374) is referring to self.levels, not the locally computed levels. Patching this locally seems to resolve the issue.

In the third, I expect [2..6, 6..7] for similar reasons, but this time I get [2..7]. What seems to be going on here is that the i variable in the char_indices() enumeration (line 297) is an offset relative to the line start, but both the original_classes[] query and the levels[] mutation are applied relative to text start.

I have some other concerns, but will start with these. Happy to do a PR, as the fixes seem simple.

@mbrubeck
Copy link

This is fixed by #278. The fix is already applied to the original unicode-bidi crate in servo/unicode-bidi#55.

@mbrubeck mbrubeck linked a pull request May 20, 2021 that will close this issue
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 a pull request may close this issue.

2 participants