01mf02 / codesnake

Pretty printer for non-overlapping code spans
MIT License
8 stars 0 forks source link

Bug in formatting 0 width parts #3

Closed Zheoni closed 3 months ago

Zheoni commented 3 months ago

Using a label to mark a postion in the code, so 0 len range, shifts the following graphics in the current line by 1 character because the lines used to mark the label takes 1 character not taken into account.

Example:

//...
let src = "labelstogether";
let labels = vec![
    Label::new(3..3)
        .with_text("I'm a label!")
        .with_style(|s| s.red().to_string()),
    Label::new(6..14)
        .with_text("Another one")
        .with_style(|s| s.blue().to_string()),
];
//...

image

After a couple of hours I managed to understand a bit of the code and fix this. I edited the following function https://github.com/01mf02/codesnake/blob/051c91e3dd999bc1493016c117e22f875ed7a219/src/lib.rs#L565-L584

fn fmt_inside<S1, S2>(&self, from: usize, s1: S1, s2: S2, f: &mut Formatter) -> fmt::Result
    where
        S1: Fn(usize) -> String,
        S2: Fn(usize, usize) -> String,
    {
        let before = width(&self.inside[..from]);
        write!(f, "{}", " ".repeat(before))?;

        let mut over = 0;

        for (code, label) in &self.inside[from..] {
            match label {
                Some((Some(_text), style)) => {
                    if code.width == 0 {
                        over = 1; // the line takes width of 1, even if the code is width 0
                    }
                    let (left, right) = code.left_right();
                    style(s2(left, right)).fmt(f)?
                }
                Some((None, style)) => style(s1(code.width)).fmt(f)?,
                None => {
                    " ".repeat(code.width.saturating_sub(over)).fmt(f)?;
                    over = 0;
                }
            }
        }
        Ok(())
    }

I won't open a pull request because it's only 3 lines edited and I'm not confident it doesn't break anything else...

01mf02 commented 3 months ago

Can you post the whole code that triggered this behaviour?

My guess is that you can support this by using something like this (taken from https://github.com/01mf02/jaq/blob/e36f57e43d16af218a9bcfbc9dca32bd0acd617a/jaq/src/main.rs#L658C1-L662C11):

Block::new(idx, labels).unwrap().map_code(|c| {
    let c = c.replace('\t', "    ");
    let w = unicode_width::UnicodeWidthStr::width(&*c);
    CodeWidth::new(c, core::cmp::max(w, 1))
})

The important part here is the max(w, 1), which assures that even code of width 0 will be counted as having width 1 for the calculation of the line positions.

If this solves your problem, I will add this code example to the documentation.

Zheoni commented 3 months ago

Makes sense. I would just explain map_code and CodeWidth in the documentation, I dind't fully understant it's purpose when I started using the lib.

This is the full code example with the your suggested CodeWidth added.

use codesnake::*;
use yansi::Paint;

fn main() {
    let src = "labelstogether";
    let labels = vec![
        Label::new(3..3)
            .with_text("I'm a label!")
            .with_style(|s| s.red().to_string()),
        Label::new(6..14)
            .with_text("Another one")
            .with_style(|s| s.blue().to_string()),
    ];
    let idx = LineIndex::new(src);
    let block = Block::new(&idx, labels).unwrap().map_code(|c| {
        let c = c.replace('\t', "    ");
        let w = unicode_width::UnicodeWidthStr::width(&*c);
        CodeWidth::new(c, core::cmp::max(w, 1))
    });
    println!("{block}");
}

It now shows all lines connected, but the blue label, underline and line, is still shifted by 1 character.

image

I guess this comes down to the same thing. If the fmt_inside method writes based on code.width, if it's 1, it will be all shifted after the part, and that's because lines connect. If code.width is 0, writing the line makes things after the line shifted.

Look at this for example (same code as above but changed the labels), more labels, more shifting.

let labels = vec![
    Label::new(2..2)
        .with_text("1")
        .with_style(|s| s.red().to_string()),
    Label::new(3..3)
        .with_text("2")
        .with_style(|s| s.red().to_string()),
    Label::new(4..4)
        .with_text("3")
        .with_style(|s| s.red().to_string()),
    Label::new(5..5)
        .with_text("4")
        .with_style(|s| s.red().to_string()),
    Label::new(6..14)
        .with_text("Another one")
        .with_style(|s| s.blue().to_string()),
];

image

When it should be like image

01mf02 commented 3 months ago

I understand now, @Zheoni. Thanks for taking the time and effort to explain this to me. May I ask whether you actually need the functionality of being able to create labels with zero width before other labels? For me, zero width labels are mostly a "hack" that is useful only for putting a label at EOF.

Zheoni commented 3 months ago

I use them to mark a position in the code where something is missing. For example in this warning in cooklang

image

I can rework the reports to not use them if you don't want to support this, it's not a big deal.

01mf02 commented 3 months ago

Hmmm, that use case sounds reasonable to me. I'll think about how to support it nicely.

01mf02 commented 3 months ago

I think I found a nice way to support zero-width labels: With the new version of codesnake (0.2.1), you can now do this:

let mut prev_empty = false;
let block = Block::new(&idx, labels).unwrap().map_code(|s| {
    let sub = usize::from(core::mem::replace(&mut prev_empty, s.is_empty()));
    let s = s.replace('\t', "    ");
    let w = unicode_width::UnicodeWidthStr::width(&*s);
    CodeWidth::new(s, core::cmp::max(w, 1) - sub)
});

In a nutshell, this uses the same technique from your original post, but it does this outside the codesnake library. This does not work in codesnake 0.2.0, because there, map_code is only Fn, whereas in codesnake 0.2.1, map_code accepts FnMut (to allow mutating prev_empty).

Zheoni commented 3 months ago

Yes, this works. Thank you! With this I think I'm ready to replace ariadne in the main branch of cooklang

01mf02 commented 3 months ago

Great!