feat: accept templated command for OpenEditor

[pickx]

this PR extends OpenEditor in the following way:

• BufferEditor's command field can now contain the following patterns: {file}, {line}, {col}
• reedline will automatically instantiate the fields from current position when OpenEditor is invoked

the usecase here is that pressing Ctrl+O in nushell would open the editor in the current position of the cursor. this branch works as-is in nushell without additional changes to nushell itself, other than the user needing to use a different format for buffer_editor. it is (or should be) backward-compatible with existing buffer_editor commands.

I tested this with:

• $env.config.buffer_editor = ["hx" "{file}:{line}:{col}"]
• $env.config.buffer_editor = ["code" "--wait" "--goto" "{file}:{line}:{col}"]

TODOs in the code will be removed in subsequent commits - they're just potential areas of discussion for the PR process.

[pickx]

I realized I was re-inventing the wheel, so I rebased away unnecessary code.

[fdncred]

i'm wondering if .chars() is right here. e.g. should it be graphemes or should it call is_valid()?

[pickx]

I'm not sure, actually. would appreciate some help here.

[fdncred]

it should probably use is_valid to ensure that the col you're inserting isn't in the middle of a unicode character and impossible to be in the middle of.

[pickx]

looks like we want grapheme_indices(true) here, after testing with a buffer containing non-unicode characters. chars() is definitely wrong

[fdncred]

your change is probably alright but `is_valid() does something similar

[pickx]

sorry, looks like I'm not getting you 🙂
are you suggesting we validate using this function because even going by grapheme is not a strong enough guarantee? or that we change the impl to something that looks like is_valid?

please spoon feed me, lol. or simply commit whatever change makes sense.

[fdncred]

No worries. What I saw saying is that I think we already have a function that is doing something similar to what you're trying to do. So, you could call this function to ensure where you were inserting is a good place.

    /// Check if the line buffer is valid utf-8 and the cursor sits on a valid grapheme boundary
    pub fn is_valid(&self) -> bool {
        self.lines.is_char_boundary(self.insertion_point())
            && (self
                .lines
                .grapheme_indices(true)
                .any(|(i, _)| i == self.insertion_point())
                || self.insertion_point() == self.lines.len())
            && std::str::from_utf8(self.lines.as_bytes()).is_ok()
    }

I'm not positive but if we set the insertion_point() and then call is_valid(), it should tell us if that's a good place.

Maybe I'm making too much of this or am just wrong and I'm just missing the point. I'm just trying to have less duplicate/similar code if we can.

We can just go with what you have if I'm being too confusing.

[fdncred]

Can we somehow add a test for this and test this in an example?

[pickx]

as I was writing the examples/tests, I decided that requiring {file} is surprising.
instead we now append the filename at the end, unless {file} is specified.
tell me if you think we should do things differently