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

[pickx]

I would like to revive this PR.
main has drifted since this was opened, but after resolving conflicts, this still works.

1. is this a feature you would like to see in reedline? if so, anything else you want to see here? (if not, that's fine, and you can close this.)
2. due to the conflicts: do we prefer a merge commit, or a rebase+squash?
3. suggestions on how to test this? I've already added tests for render_editor_command, but I'm not sure if that is sufficient.
4. this substitution thing feels like it can be handled by ENV_CONVERSIONS...? of course, not without some additional changes. also, this requires nushell, so no longer keeps this contained to reedline.

[fdncred]

1. I think having this would be cool in nushell

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

However, I'd like it to happen without breaking changes so whatever we have needs to be optional. Maybe it's like that already?

2. I'm fine with either rebase+squash or merge commit since this is so old now.

3. I'm not sure about tests. We'd definitely want some in nushell too.

4. I'm not sure how ENV_CONVERSIONS would help but I'm just waking up too.

I'd like to get @kronberger-droid's thoughts on this one too.

[pickx]

(as far as I can tell, this isn't breaking, and is reverse-compatible with existing setups)

[kronberger-droid]

Sounds nice, I like the feature.

The new editor backend would maybe change the code just a little, but it general it looks fine already.

[pickx]

@kronberger-droid okay, now that the recent reedline changes appears to have landed:
I rebased this. not much actual code changes here now.
ready for review.

[pickx]

right now we do this even if the command is not templated.
is this is considered wasteful, we can avoid it by determining ahead of time if it's templated or not
please tell me if you think it matters.

[fdncred]

let's see what copilot thinks.

[Copilot]

Pull request overview

Adds cursor-aware command templates for external buffer editors.

Changes:

• Substitutes {file}, {line}, and {col} in editor arguments.
• Defers command construction until OpenEditor.
• Adds grapheme-based column calculation and renderer tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/engine.rs	Implements command rendering and editor lifecycle changes.
src/core_editor/line_buffer.rs	Adds zero-based cursor column calculation.

Suppressed comments (3)

src/engine.rs:270

• Every argument is converted with to_string_lossy, even when it contains no template token. On platforms that permit non-UTF-8 arguments, this silently replaces bytes and changes an otherwise valid preconfigured editor invocation. Only arguments that can actually be interpreted as UTF-8 templates should be rewritten; pass all others through unchanged.

            .map(OsStr::to_string_lossy)
            .map(|arg| arg.replace(FILE, &file))
            .map(|arg| arg.replace(LINE, &line))
            .map(|arg| arg.replace(COL, &col));

src/engine.rs:260

• This lossy conversion can make {file} point somewhere other than temp_file. A valid Unix PathBuf may contain non-UTF-8 bytes: Reedline writes the buffer to that exact path, but the spawned editor receives a path containing Unicode replacement characters, so it cannot edit the file Reedline later reads. Build the rendered OsString by inserting self.temp_file.as_os_str() without converting the path to UTF-8.

        let file = self.temp_file.to_string_lossy();

src/engine.rs:275

• Existing callers that already include the temporary path now receive it twice because the fallback checks only for {file}. The repository's own examples/demo.rs:123-126 configures vi <temp_file> explicitly, and the previous implementation deliberately avoided appending an exact duplicate. Preserve that compatibility check while adding placeholder support.

        if !has_file_placeholder {
            cmd.arg(&self.temp_file);
        }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pickx]

I admit, I don't understand what this is about.

[fdncred]

maybe @kronberger-droid understands?

[kronberger-droid]

Well my guess was building a new command just looses the env and some other stuff around it.

I sadly don't have time right now to go into it to much. But claude agreed and was quite clear that we loose quite some features and some of it is unrecoverable in the way we write it right now:

"render_command builds a fresh Command from get_program() and get_args(), so everything else the caller set stays behind on the old object. nushell leans on exactly that: nu-cli/src/repl.rs:713 does command.args(args).envs(envs) since $env isn't the process env, so the editor ends up with the bare inherited env instead. Copying get_envs() and get_current_dir() would fix some of it. env_clear(), the stdio config and the unix CommandExt settings have no getters at all, and a bare env_clear() reads back identical to a command that never touched the env, so that part isn't recoverable from a stored Command however we copy it."

I will take a closer look later.

[kronberger-droid]

But i think its essentially that that nushell hands its whole $env to the command via .envs(...), and a freshly built Command has no way to pick that back up.
get_envs() would recover that part at least.

[kronberger-droid]

So the env is easy enough to solve. But there are other methods which where possible on the prebuilt Command one would loose. So easiest would be to just ship the default implementation when no template is used and for the template at least add the env. Since nushell depends on it.

I would argue that we don't merge this until we have it ported and working in a branch of nushell.

[pickx]

so...

1. while testing, I noticed that a non-existent $env.config.buffer_editor command is an unhandled reedline error and crashes nushell. uh, this really seems like something nushell should be able to recover from.
2. the failure to propagate env makes this worse because it can make crashing more common. for example if the editor command was in the previous $env.PATH but env wasn't propagated.
3. any optimizations here (such as somehow reusing the Command, or storing the args separately, ...) don't really benefit us now, since I believe the REPL recreates this command regardless.