add reflow command #2128

vlmutolo · 2022-04-16T03:19:22Z

Users need to be able to hard-wrap text for many applications, including
comments in code, git commit messages, plaintext documentation, etc. It
often falls to the user to manually insert line breaks where appropriate
in order to hard-wrap text.

This commit introduces the "reflow" command (both in the TUI and core
library) to automatically hard-wrap selected text to a given column length.

Addresses #625.

helix-term/src/commands/typed.rs

archseer · 2022-04-16T04:19:19Z

helix-core/src/wrap.rs

+
+/// Given a slice of text, return the text re-wrapped to fit
+/// it within the given width.
+pub fn reflow_hard_wrap(text: RopeSlice, max_line_len: usize) -> String {


Have you evaulated simply using https://github.com/mgeisler/textwrap which will also solve for cases like multiwidth characters?

Not yet! That could clean up most of the logic. I'll definitely check it out.

Looked at it a bit, and it seems like the right approach. Thanks for the suggestion. I wish I'd asked around re: existing crates a bit earlier, but I wanted to delete most of this code anyway.

I'm especially impressed with the line-breaking algorithms they have. Not sure if using the "optimal" line breaking algorithm is worth the extra binary size. Personally, I'd want it for the extra ~50KB. Thoughts?

Users need to be able to hard-wrap text for many applications, including comments in code, git commit messages, plaintext documentation, etc. It often falls to the user to manually insert line breaks where appropriate in order to hard-wrap text. This commit introduces the "reflow" command (both in the TUI and core library) to automatically hard-wrap selected text to a given number of characters (defined by Unicode "extended grapheme clusters"). It handles lines with a repeated prefix, such as comments ("//") and indentation.

vlmutolo · 2022-04-16T16:28:17Z

I switched to using textwrap, which obviously made this PR a lot cleaner. The default feature set looked like what we want. It has the smawk feature enabled by default, which brings in one (transitive) dependency to do the linear algebra calculations to make the reflow optimal instead of greedy. Do we want to make this optional behind a (default?) feature, or just leave it always-on?

Also I left the textwrap crate to be imported in core inside a now-tiny module called wrap. The motivation here was to allow any Helix frontend to have access to the wrapping capabilities. Let me know if you think there's a better place for the function, or if we want to just stick it in core/lib.

Future work should probably include ensuring that reflowing from a single line with a known prefix (like a comment) preserves that prefix. Otherwise, the textwrap crate seems to do an excellent job.

vlmutolo · 2022-04-18T18:47:39Z

I'm not sure what the Build / Docs (pull_request) failure is. Is this something I should be trying to fix?

the-mikedavis · 2022-04-18T18:49:01Z

If you run cargo xtask docgen and commit the changes it'll clear up the ❌. That check makes sure all the available commands / languages / lsps are documented.

vlmutolo · 2022-04-18T18:55:20Z

That's a nice check to ensure up-to-date docs in the book. I'll have to remember that strategy for my own future projects.

dead10ck

Seems pretty straightforward to me with the external crate.

dead10ck · 2022-04-18T22:13:14Z

helix-term/src/commands/typed.rs

+    let rope = doc.text();
+
+    // TODO: If only a single character is selected, we should expand the
+    // selection to whatever "object" (can we use treesitter for this? text query?)


This would be tricky because of inconsistencies between grammars. For example, in Rust, each line in a comment is a separate node, so it would just end up reflowing a single line, instead of all the consecutive lines of comments together.

Hmm, that's interesting. We can scrap this TODO, then.

Plus, if we have a way of "expanding" the selection to a comment block or something, then we basically achieve the same effect because this operates on selections.

There's mio for selecting the comment block (selecting all adjacent comments nodes if the queries are specified correctly).

gqq like vim to default wrapping on a single line might be a reasonable default.

I think we probably also want to ignore comment in the reflow, at least it should be as convenient as vim, also note that rust supports quite a few form of comments and that should be handled as well here, like ////, //!, // ...

My experience with kakoune using external fmt command were especially bad with reflowing rust code given that the comment could appear in multiple forms, and the comment being wrapped along in unexpected ways.

The library we're using (textwrap) will detect a common prefix across the selected lines. This will work for most of our comments. For example, I just tested this:

// A B // C D E F

and it reflowed to:

// A B C D E F

We fall down a bit in trying to reflow from one line to many, since the library we're using can't detect the comment prefix when it's not repeated. We also still have an issue where the library will eat blank lines and reflow across them. I'd file both of those points under "future work" on this feature.

Agreed, that's future work. It could be defined in a language.toml as something like comment_prefix_tokens = ["///", "//!", "//"] where the first token in the list is prioritized (so rust /// doesn't get detected as //)

Hey folks! Textwrap author here :-) It's super cool to see textwrap::refill used. I'll be very interested in making textwrap::unfill (the actual engine behind textwrap:refill) more flexible so that it covers the necessary use-cases.

I wrote textwrap::unfill quickly so that it seems to do the right thing for a few languages, such as Markdown and programming languages which use # and / as line comments. But the code has not been tested much in the wild, so feedback is welcome :-)

@mgeisler textwrap is an awesome library! I started to implement this feature myself before I was aware textwrap existed, so I have a bit of an idea of the work that went into a more comprehensive solution. And the optimal line breaking is just really cool.

Re: unfill, what do you think about adding an API to provide a list of "known prefixes"? I should probably make an issue in textwrap to discuss this.

Thanks! I think we should do exactly that: change the API of refill to let you specify prefixes and so on. This probably means expanding unfill as well since that is the function which turns a wrapped paragraph into a single line of text.

dead10ck · 2022-04-18T22:24:11Z

helix-term/src/commands/typed.rs

+    // doesn't pass in a selection, default to 79 characters per line?
+
+    const DEFAULT_MAX_LEN: Cow<'static, str> = Cow::Borrowed("79");
+    let max_line_len: usize = args.get(0).unwrap_or(&DEFAULT_MAX_LEN).parse()?;


Maybe not necessarily in this PR, but it might also be cool to add a config option to set the default max line width if you prefer a wider width, but don't want to give it explicitly every time.

Agreed. I can add that real quick to this PR, and clean up some of the comments and TODOs.

I don't think this should be set on a global config especially when different languages very likely have different width, notably

rust 100 / 80 (some special case that we don't really want to handle but rustfmt should already do it)

python 80

git commit 52 IIRC

javascript shouldn't have a default especially when there are so many "standards", some say 100 some say 120

I think this should be within languages.toml

This makes to me, and I included it in the version I just pushed.

Any opinions on the name of the config value? I made it a top-level key called max_line_length and it currently gets parsed to a usize.

line_wrapping_width?

I think we can add some sensible defaults for the languages.toml like rust 100.

Hmm I'll have to look into that. Any tips on where those defaults might go? I was a bit confused by the code that processes the languages.toml file.

Go to

[[language]] name = "rust" max_line_len = 100

That is just an example, I think the variable name may not the that.

pickfire · 2022-04-25T16:15:15Z

helix-term/src/commands/typed.rs

+        TypableCommand {
+            name: "reflow",
+            aliases: &[],
+            doc: "Hard-wrap the current selection of lines to a given width.",
+            fun: reflow,
+            completer: None,
+        },


I wonder if we should just bind it to gq.

gq could be awkward given the goto menu g. We might be able to use = although I think that is currently saving a spot for LSP-driven format.

I don't know any other good bindings for it beside gq which is similar to vim, we might have to tweak goto menu to goto/misc menu

Seems like this decision shouldn't block the PR. I imagine it's pretty easy to add an alias later.

dead10ck

Looking good to me!

archseer

I'm not sure yet if the optional features of text-wrap are worth keeping on by default but I think this is good to merge for now 👍🏻

vlmutolo force-pushed the reflow branch from 805605a to 3defb03 Compare April 16, 2022 03:33

archseer reviewed Apr 16, 2022

View reviewed changes

vlmutolo force-pushed the reflow branch from 3defb03 to a254351 Compare April 16, 2022 04:52

Vince Mutolo added 2 commits April 16, 2022 08:50

reflow: consider newlines to be word separators

a3127d4

replace custom reflow impl with textwrap crate

e8c84a5

vlmutolo force-pushed the reflow branch from a254351 to e8c84a5 Compare April 16, 2022 16:07

vlmutolo marked this pull request as ready for review April 16, 2022 16:20

the-mikedavis linked an issue Apr 17, 2022 that may be closed by this pull request

Command to hard-wrap selected text. #625

Closed

Sync reflow command docs with book

363113e

dead10ck approved these changes Apr 18, 2022

View reviewed changes

pickfire reviewed Apr 25, 2022

View reviewed changes

reflow: add default max_line_len language setting

d6c2996

This was referenced Apr 26, 2022

Command to hard-wrap selected text. #625

Closed

Support soft wrap #136

Closed

add support for continuous hard wrap #2274

Open

dead10ck approved these changes Apr 26, 2022

View reviewed changes

archseer approved these changes May 2, 2022

View reviewed changes

archseer merged commit f9baced into helix-editor:master May 2, 2022

vlmutolo deleted the reflow branch May 3, 2022 04:00

poliorcetics mentioned this pull request May 4, 2022

Have a default max_line_length in config.toml to use when not set for the language in languages.toml #2393

Closed

vlmutolo mentioned this pull request Aug 20, 2022

Reflow doesn't work consistently with comments #3332

Open

jw013 mentioned this pull request May 21, 2023

Add a mappable version of existing :reflow command #7098

Closed

bonsairobo mentioned this pull request Oct 27, 2023

refilling single-line (Rust) comments mgeisler/textwrap#524

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add reflow command #2128

add reflow command #2128

vlmutolo commented Apr 16, 2022 •

edited

Loading

archseer Apr 16, 2022

vlmutolo Apr 16, 2022

vlmutolo Apr 16, 2022 •

edited

Loading

vlmutolo commented Apr 16, 2022 •

edited

Loading

vlmutolo commented Apr 18, 2022

the-mikedavis commented Apr 18, 2022

vlmutolo commented Apr 18, 2022

dead10ck left a comment

dead10ck Apr 18, 2022

vlmutolo Apr 19, 2022

sudormrfbin Apr 19, 2022

pickfire Apr 25, 2022 •

edited

Loading

vlmutolo Apr 26, 2022 •

edited

Loading

evanrichter Apr 26, 2022

mgeisler Jun 3, 2022

vlmutolo Jun 3, 2022 •

edited

Loading

mgeisler Jun 3, 2022

dead10ck Apr 18, 2022

vlmutolo Apr 19, 2022

pickfire Apr 25, 2022

vlmutolo Apr 26, 2022 •

edited

Loading

evanrichter Apr 26, 2022

pickfire Apr 28, 2022

vlmutolo Apr 28, 2022

pickfire Apr 29, 2022

pickfire Apr 25, 2022

the-mikedavis Apr 25, 2022

pickfire Apr 28, 2022

vlmutolo Apr 28, 2022

dead10ck left a comment

archseer left a comment

add reflow command #2128

add reflow command #2128

Conversation

vlmutolo commented Apr 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlmutolo Apr 16, 2022 • edited Loading

Choose a reason for hiding this comment

vlmutolo commented Apr 16, 2022 • edited Loading

vlmutolo commented Apr 18, 2022

the-mikedavis commented Apr 18, 2022

vlmutolo commented Apr 18, 2022

dead10ck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickfire Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

vlmutolo Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlmutolo Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlmutolo Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dead10ck left a comment

Choose a reason for hiding this comment

archseer left a comment

Choose a reason for hiding this comment

vlmutolo commented Apr 16, 2022 •

edited

Loading

vlmutolo Apr 16, 2022 •

edited

Loading

vlmutolo commented Apr 16, 2022 •

edited

Loading

pickfire Apr 25, 2022 •

edited

Loading

vlmutolo Apr 26, 2022 •

edited

Loading

vlmutolo Jun 3, 2022 •

edited

Loading

vlmutolo Apr 26, 2022 •

edited

Loading