Xi-editor: [xi-rope][bug] prev::<LinesMetric> does not work

[ngortheone]

It looks like prev is broken. It moves cursor back once and that' s it.

    #[test]
    fn prev_line() {
        let rope = Rope::from_str("First line\nSecond line\r\nThird line\n").unwrap();
        let mut cursor = Cursor::new(&rope, 34);

        cursor.prev::<LinesMetric>();
        cursor.prev::<LinesMetric>();
        cursor.prev::<LinesMetric>();
        cursor.prev::<LinesMetric>();

        assert_eq!(cursor.pos(), 24);
}

[ngortheone]

Same behavior observed without '\r' too

[cmyr]

Okay, so maybe we've never really used prev::<LinesMetric>() from a boundary before?

This minimal case fails:

let rope = "one\ntwo\ntre".into();
let mut cursor = Cursor::new(&rope, rope.len());

cursor.prev::<LinesMetric>();
assert_eq!(cursor.pos(), rope.len());
cursor.prev::<LinesMetric>();
assert_eq!(cursor.pos(), 8);

[raphlinus]

Huh, LinesMetric::prev is wrong. The fix is simple enough I think (mostly use offset - 1, with an early out to None if offset is already 0) but should come with tests. It does look like we never actually used this.

[ngortheone]

@raphlinus @cmyr

I am working on PR to fix this

The fix looks like this (added offset -1)

    fn prev(s: &String, offset: usize) -> Option<usize> {
        memrchr(b'\n', &s.as_bytes()[..offset-1]).map(|pos| pos + 1)
    }

But now this test fails

    #[test]
    fn cursor_prev_misc() {
        cursor_prev_for("a\na\n");
    }

    fn cursor_prev_for(s: &str) {
        let r = Rope::from(s.to_owned());
        for i in 0..r.len() {
            let mut c = Cursor::new(&r, i);
            let it = c.prev::<LinesMetric>();
            let pos = c.pos();
            assert!(s.as_bytes()[pos..i-].iter().all(|c| *c != b'\n'), "missed linebreak");
// .....

The behavior is slightly changed. Now if cursor is on '\n' prev will return previous metric, not current one. So it fails assertion because line has '\n' at the end.

Not sure whether I got something wrong or test now is invalid. Please advise

[ngortheone]

I should mention that every other test apart from this one succeeds. So I am leaning towards thinking that the logic of this test is faulty since it was using buggy behavior.