Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add reflow command #2128

Merged
merged 5 commits into from
May 2, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions book/src/generated/typable-cmd.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
| `:set-option`, `:set` | Set a config option at runtime |
| `:sort` | Sort ranges in selection. |
| `:rsort` | Sort ranges in selection in reverse order. |
| `:reflow` | Hard-wrap the current selection of lines to a given width. |
| `:tree-sitter-subtree`, `:ts-subtree` | Display tree sitter subtree under cursor, primarily for debugging queries. |
| `:config-reload` | Refreshes helix's config. |
| `:config-open` | Open the helix config.toml file. |
1 change: 1 addition & 0 deletions helix-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ encoding_rs = "0.8"
chrono = { version = "0.4", default-features = false, features = ["alloc", "std"] }

etcetera = "0.3"
textwrap = "0.15.0"

[dev-dependencies]
quickcheck = { version = "1", default-features = false }
1 change: 1 addition & 0 deletions helix-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub mod syntax;
pub mod test;
pub mod textobject;
mod transaction;
pub mod wrap;

pub mod unicode {
pub use unicode_general_category as category;
Expand Down
7 changes: 7 additions & 0 deletions helix-core/src/wrap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
use smartstring::{LazyCompact, SmartString};

/// Given a slice of text, return the text re-wrapped to fit it
/// within the given width.
pub fn reflow_hard_wrap(text: &str, max_line_len: usize) -> SmartString<LazyCompact> {
textwrap::refill(text, max_line_len).into()
}
38 changes: 38 additions & 0 deletions helix-term/src/commands/typed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,37 @@ fn sort_impl(
Ok(())
}

fn reflow(
cx: &mut compositor::Context,
args: &[Cow<str>],
_event: PromptEvent,
) -> anyhow::Result<()> {
// TODO: Can we instead take an Option<Cow<str>>, and then if the user
// 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()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@vlmutolo vlmutolo Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line_wrapping_width?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go to

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

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


let (view, doc) = current!(cx.editor);
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?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pickfire pickfire Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@vlmutolo vlmutolo Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 //)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor Author

@vlmutolo vlmutolo Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// that single selection is part of.
let selection = doc.selection(view.id);
let transaction = Transaction::change_by_selection(rope, selection, |range| {
let fragment = range.fragment(rope.slice(..));
let reflowed_text = helix_core::wrap::reflow_hard_wrap(&fragment, max_line_len);

(range.from(), range.to(), Some(reflowed_text))
});

doc.apply(&transaction, view.id);
doc.append_changes_to_history(view.id);

Ok(())
}

fn tree_sitter_subtree(
cx: &mut compositor::Context,
_args: &[Cow<str>],
Expand Down Expand Up @@ -1474,6 +1505,13 @@ pub const TYPABLE_COMMAND_LIST: &[TypableCommand] = &[
fun: sort_reverse,
completer: None,
},
TypableCommand {
name: "reflow",
aliases: &[],
doc: "Hard-wrap the current selection of lines to a given width.",
fun: reflow,
completer: None,
},
Comment on lines +1516 to +1522
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just bind it to gq.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TypableCommand {
name: "tree-sitter-subtree",
aliases: &["ts-subtree"],
Expand Down