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.

Sign up for GitHub

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

:trim to remove trailing whitespace #8366

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

:trim to remove trailing whitespace #8366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions book/src/generated/typable-cmd.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,4 @@
| `:move`, `:mv` | Move the current buffer and its corresponding file to a different path |
| `:yank-diagnostic` | Yank diagnostic(s) under primary cursor to register, or clipboard by default |
| `:read`, `:r` | Load a file into buffer |
| `:trim-trailing-whitespace`, `:trim` | Delete whitespace |
99 changes: 99 additions & 0 deletions helix-term/src/commands/typed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2520,6 +2520,97 @@ fn read(cx: &mut compositor::Context, args: &[Cow<str>], event: PromptEvent) ->
Ok(())
}

fn trim_whitespace_impl(doc: &Document, selection: &Selection) -> Transaction {
/// Find the last non-whitespace char of a line
fn find_trailing(l: RopeSlice) -> Option<usize> {
// Handle empty docs
if l.len_chars() == 0 {
return None;
}

// Returns the left-wise beginning of the trailing whitespace
// It is +1 the index of that char so that char is not deleted
l.chars_at(l.len_chars())
.reversed()
.position(|ch| !ch.is_whitespace())
.map(|n| l.len_chars() - n)
.or(Some(0))
}

let mut deletions: Vec<helix_core::Deletion> = Vec::new();
let mut delete = |start, end| {
// Don't push empty changes
if start != end {
deletions.push((start, end));
}
};

// Assume ranges are in order and not overlapping
for range in selection.ranges().iter().rev() {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if instead of operating on each selection's range we should use Selection::line_ranges like we do for :reset-diff-change now. I don't think it ever makes sense to not operate on full lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might have misunderstood the code or this review, but here are the pros & cons that come to mind right now:

  • Trimming selections is much more versatile, but comes at the cost of hitting x before the cmd (if we want to trim the full line)
  • Full-line-trim seems more common (in practice), so it should have a shorter key-sequence. But (mathematically) it's just 1 special case among an usize::MAX ("pseudo-Infinity") of potential selection-trims. Therefore, full-line-trim is rare (in theory)

Although, it seems fallacious to believe that FLT is common, simply because practically-all editors only support trimming all lines in a buffer, thereby forcing users to get used to this limitation

Copy link
Member

Choose a reason for hiding this comment

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

For a bit of a contrived example, say we have a file like:

····[········ffffffffffff···············
············ffffffffffff···············
············ffffffffffff·······]········

(with "·" being a space character and the selection being represented by the brackets)

I think :trim here should operate on all lines covered by the selection, resulting in

············ffffffffffff
············ffffffffffff
············ffffffffffff

where I think currently this PR would end up with

············ffffffffffff
············ffffffffffff
············ffffffffffff·······

I'm not sure it's ever useful to use the end of the selection range as the end of the area you're trimming

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Now I see why it doesn't seem useful.

············ffffffffffff
············ffffffffffff
············ffffffffffff·······

In my mind, I thought it would be:

············ffffffffffff············
············ffffffffffff············
············ffffffffffff·······

Which makes less sense, I guess?

Then I thought "what if there were non-space code-points somewhere between the consecutive spaces?", then I answered my own question:

In practice, typically, users want 0, 1, or (rare) 2 spaces between non-space chars (let's ignore indentation, for simplicity). So, why would users need to specify how many spaces they want to delete? It'd be much simpler to:

  • delete all spaces from all lines
  • delete all spaces from all lines, except 1 space from the last line
  • preserve some lines as-is

let slice = range.slice(doc.text().slice(..));
let lines = slice.lines_at(slice.len_lines()).reversed();

// Cap the `end` to not delete the line ending
let end_account_le = |line: RopeSlice, n: usize| {
let le_len = helix_core::line_ending::get_line_ending(&line)
.map(|le| le.len_chars())
.unwrap_or(0);
// Map `end` with respect to the whole doc
range.from() + n - le_len
};

// Ignore empty lines if `trailing` is true.
// If not `trailing`, delete trailing whitespace on lines.
let mut trailing = true;
for (idx, line) in lines
.enumerate()

{
// This subtraction cannot underflow because len_lines must be `> 0` to enter
// this loop and `idx` is always `< len_lines`.
let line_number = slice.len_lines() - 1 - idx;
if trailing {
// `n @ 1..` will ignore `Some(0)` from empty lines
if let Some(n @ 1..) = find_trailing(line) {
let start = range.from() + slice.line_to_char(line_number) + n;
// Needed to retain the last EOL of the selection, which would be selected. e.g. in `%:trim`
let end = end_account_le(slice, slice.len_chars());
delete(start, end);
trailing = false;
}
} else if let Some(n) = find_trailing(line) {
let start = range.from() + slice.line_to_char(line_number) + n;
let end = end_account_le(line, slice.line_to_char(line_number + 1));
delete(start, end);
}
}

// Delete empty selections
if trailing {
let start = range.from();
let end = end_account_le(slice, slice.len_chars());
delete(start, end);
}
}
Transaction::delete(doc.text(), deletions.into_iter().rev())
}

fn trim_whitespace(
cx: &mut compositor::Context,
_args: &[Cow<str>],
event: PromptEvent,
) -> anyhow::Result<()> {
if event != PromptEvent::Validate {
return Ok(());
}
let (view, doc) = current!(cx.editor);
let selection = doc.selection(view.id);
let tx = trim_whitespace_impl(doc, selection);
doc.apply(&tx, view.id);
doc.append_changes_to_history(view);
Ok(())
}

pub const TYPABLE_COMMAND_LIST: &[TypableCommand] = &[
TypableCommand {
name: "quit",
Expand Down Expand Up @@ -3141,6 +3232,14 @@ pub const TYPABLE_COMMAND_LIST: &[TypableCommand] = &[
fun: read,
signature: CommandSignature::positional(&[completers::filename]),
},
TypableCommand {
name: "trim-trailing-whitespace",
aliases: &["trim"],
doc: "Delete trailing whitespace from the current selections",
fun: trim_whitespace,
signature: CommandSignature::none(),

},
];

pub static TYPABLE_COMMAND_MAP: Lazy<HashMap<&'static str, &'static TypableCommand>> =
Expand Down
Loading