refilling single-line (Rust) comments

[bonsairobo]

As discussed here: helix-editor/helix#2128 (comment)

To support refilling single-line Rust comments (as desired in this helix issue), the most flexible proposal I've seen is to provide a list of know prefixes, e.g. ["//", "///", "//!"], and then if one of these prefixes is identified on a single line input text, propagate initial_indent to subsequent_indent.

FWIW I created this branch that does the indent propagation without any public function signature changes. All tests pass, but I figured there might be some cases where this isn't flexible enough. I'd love to get some feedback on cases where this solution would be insufficient.

[mgeisler]

Hey @bonsairobo,

I took a quick peek at your branch and we could add ! to the list of characters to skip.

However, I feel it would be wrong for a text editor to use textwrap::refill directly. I see that function as a bit of quick-and-dirty convenience function for small command line programs.

For a text editor, I would expect that you need and want more control, so I would recommend using the more low-level wrapping infrastructure in Textwrap directly. That is, implement your own Fragment and use that to wrap and re-wrap the text as you need.

But of course, if you tell me that refill is just what you need, then I guess you could use it 😄 Just note that it won't work for things like proportional fonts — which you can wrap if you implement your own Fragment: https://mgeisler.github.io/textwrap/.

I'll be happy to extend unfill like in your branch — perhaps it would be better if the prefix chars were an argument to the function?

[bonsairobo]

@mgeisler Thanks for the quick reply.

However, I feel it would be wrong for a text editor to use textwrap::refill directly. I see that function as a bit of quick-and-dirty convenience function for small command line programs. ...

I trust your opinion here. I can see how a text editor would want more control as it evolves to support more features.

But of course, if you tell me that refill is just what you need, then I guess you could use it 😄 Just note that it won't work for things like proportional fonts

I am not sure yet. It seems like the closest thing, and it's what Helix currently uses, but it does make enough assumptions that I worry the Helix use case could outgrow the refill abstraction eventually. At the very least, I can see a path to fixing the Helix issue I referenced in OP by making changes to refill.

FWIW, I doubt Helix would want to use proportional fonts, being a terminal text editor. But I am not a Helix maintainer so take my words with a grain of salt.

I'll be happy to extend unfill like in your branch — perhaps it would be better if the prefix chars were an argument to the function?

OK, only if you think it's a good change overall, not just for my sake.

In summary, I will take a look at the Fragment trait and see if it offers a good tradeoff for Helix's :rewrap command. I also considered using some combination of unfill and fill instead of refill directly. Perhaps that would be a reasonable middle ground, though I think users of unfill might like to know how many lines existed in the original text.

[bonsairobo]

After looking at the Fragment trait, I'm pretty sure Helix does not need to implement this, at least not in order to resolve the current issue. The Word implementation should be sufficient.

[bonsairobo]

I'm also fairly certain that fill will be exactly what is needed by Helix for the near future. By process of elimination, I think we only need to consider how to achieve the desired behavior for unfilling and detecting the correct initial_prefix and subsequent_prefix (perhaps with some help from the editor).

We have options:

1. Re-implement refill and unfill for Helix while reusing textwrap's fill. Not a terrible task, it would probably involve just copy-pasting the current implementation and making some opinionated changes. It would also allow for direct changes in the Helix source code in the future.
2. Make opinionated changes to unfill like I've done in my branch. It sounds like @mgeisler is open to this option.
3. Make refill/unfill more flexible by:

• providing a list of known prefixes (or prefix characters)
• making the behavior of choosing subsequent_prefix configurable in the case when there is only a single line of input text

[mgeisler]

1. Re-implement refill and unfill for Helix while reusing textwrap's fill. Not a terrible task, it would probably involve just copy-pasting the current implementation and making some opinionated changes.
2. Re-implement refill and unfill for Helix while reusing textwrap's fill. Not a terrible task, it would probably involve just copy-pasting the current implementation and making some opinionated changes.

In that case, then let's go for your second option:

2. Make opinionated changes to unfill like I've done in my branch. It sounds like @mgeisler is open to this option.

I would be happy to make the function fit your use case better. I implemented the function one day when I realized that I mostly had the infrastructure to do so... but I didn't actually have a use case for it myself 😄

If you need a breaking API change, then we just make a 0.18 release of the library. I would probably like to also hide the public Options fields (as discussed recently in #521 (comment)). I've been on the fence about this, but I guess it's better for future evolution.

[bonsairobo]

@mgeisler OK I think I will make another post in the Helix issue to make sure this decision is agreeable before doing more work on it. Just want to avoid going too far and ultimately getting a PR rejected b/c maintainers have other ideas.