Added change_case command #441

luctius · 2021-07-13T20:18:50Z

In vim I use the chance case function quite a lot and found it missing here.
Here is my attempt at adding it.

archseer

Thanks, this looks good! Note that we also want a command for uppercase and one for lowercase.

Kakoune uses these keybindings:

` to lower case
~ to upper case
<a-`> swap case

I kind of like having the swap case on ~ though.

helix-term/src/commands.rs

helix-term/src/keymap.rs

luctius · 2021-07-14T11:53:30Z

Ok, I've implemented to_lowercase and to_uppercase, and placed the keymaps in a slightly more sensible manner.

That said; I did notice a bug, but I'm not sure if this is the right place to solve it. When activating either case switch method on a selection > 1, the selection will grow with each press. However the same thing occurs with the replace command; it simple is less easy to trigger it.

Can any-one confirm this behaviour?

book/src/keymap.md

helix-term/src/commands.rs

Renamed change_case to switch_case.

archseer · 2021-07-15T02:19:43Z

helix-term/src/commands.rs

+                .chars()
+                .map(|ch| {
+                    if ch.is_lowercase() {
+                        ch.to_uppercase().collect()


Ah, these methods return an iterator, hmm. Maybe we can use flat_map and directly process these?

Ah yes, good point.

So with flat_map you should be able to return these iterators directly without .collect(). Might be a problem since all the branches return a different type though?

I must admit; this isn't my area of expertise, but I don't see much of an alternative. Or at-least no ones which also allocate.
With the exception of a custom enum combining the three branches, which also implements Iterator.

Any other suggestion?

I think the current implementation is fine for now :)

archseer · 2021-07-15T02:20:08Z

helix-term/src/commands.rs

+    let (view, doc) = current!(cx.editor);
+    let transaction =
+        Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| {
+            let text: Tendril = range.fragment(doc.text().slice(..)).to_lowercase().into();


You're using to_lowercase in the uppercase method.

Whoops, copy paste error; apologies.

archseer · 2021-07-15T02:21:20Z

That said; I did notice a bug, but I'm not sure if this is the right place to solve it. When activating either case switch method on a selection > 1, the selection will grow with each press. However the same thing occurs with the replace command; it simple is less easy to trigger it.

Yeah I think this is a side-effect of the fact that ranges are end inclusive, but we +1 to expand the range here.
\cc @cessen this is fixed by #376 right?

pickfire

I tested it and it seemed to extend the cursor by one character.

One more issue. Lowercase didn't work for me because alt-\`` is bound to something else. I think it would be better to have `` to lowercase it, and alt to uppercase it rather than flipping it like how it is currently.

pickfire · 2021-07-15T11:14:07Z

helix-term/src/commands.rs

+        Transaction::change_by_selection(doc.text(), doc.selection(view.id), |range| {
+            let text: Tendril = range.fragment(doc.text().slice(..)).to_lowercase().into();
+
+            (range.from(), range.to() + 1, Some(text))


Suggested change

(range.from(), range.to() + 1, Some(text))

(range.from(), range.to(), Some(text))

I think this should fix the bug.

While this does fix it when having a selection of >1 character, it causes it not to work when having a selection of 1 character. While this is solvable; the replace command has the exact same issue.

If I understand correctly, this behaviour is currently due to ranges being inclusive. archseer above suggested that this behaviour should be fixed later on in #376.

If so, should I work around the bug, or accept it for now?

I think it's fine, you should accept it for now and we'll deal with it in #376

luctius · 2021-07-15T11:40:42Z

One more issue. Lowercase didn't work for me because alt-\ is bound to something else. I think it would be better to have ```` to lowercase it, and alt to uppercase it rather than flipping it like how it is currently.

Sure, no problem.

pickfire

Yeah, maybe can just leave the fix to another time.

cessen · 2021-07-16T04:58:04Z

Yeah I think this is a side-effect of the fact that ranges are end inclusive, but we +1 to expand the range here.
\cc @cessen this is fixed by #376 right?

Having taken a quick look, my suspicion is that #376 isn't necessary to solve the issue, but it will probably solve it kind of incidentally anyway...? Not totally sure. At the very least, it should make the issue easier to reason about.

When I get back from vacation and resume work on the PR, I'll take a closer look.

archseer · 2021-07-16T16:13:07Z

Thanks! 🎉

cessen · 2021-07-17T18:31:29Z

I'm back from vacation. I merged the changes into #376, and it does indeed fix the selection extending issue.

Added change_case command

1223606

luctius force-pushed the features/change_case branch from d006469 to 1223606 Compare July 13, 2021 20:43

archseer requested changes Jul 14, 2021

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

helix-term/src/keymap.rs Outdated Show resolved Hide resolved

luctius force-pushed the features/change_case branch from ddfe79a to ef52af3 Compare July 14, 2021 14:57

pickfire reviewed Jul 14, 2021

View reviewed changes

book/src/keymap.md Outdated Show resolved Hide resolved

pickfire reviewed Jul 14, 2021

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

pickfire reviewed Jul 14, 2021

View reviewed changes

helix-term/src/commands.rs Outdated Show resolved Hide resolved

Cor added 2 commits July 14, 2021 21:13

Added switch_to_uppercase and switch_to_lowercase

48df076

Renamed change_case to switch_case.

Updated the Keymap section of the Book

e613403

luctius force-pushed the features/change_case branch from ef52af3 to e613403 Compare July 14, 2021 19:14

luctius requested a review from archseer July 14, 2021 20:23

archseer requested changes Jul 15, 2021

View reviewed changes

Cor added 2 commits July 15, 2021 08:55

Use flat_map instead of map + flatten

d8aacc0

Fix switch_to_uppercase using to_lowercase

e0b0745

luctius requested a review from archseer July 15, 2021 06:57

pickfire requested changes Jul 15, 2021

View reviewed changes

pickfire reviewed Jul 15, 2021

View reviewed changes

Switched 'Alt-' to uppercase and '' to lowercase

e3beccd

pickfire approved these changes Jul 15, 2021

View reviewed changes

archseer merged commit 722cfed into helix-editor:master Jul 16, 2021

luctius deleted the features/change_case branch July 16, 2021 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added change_case command #441

Added change_case command #441

luctius commented Jul 13, 2021

archseer left a comment

luctius commented Jul 14, 2021

archseer Jul 15, 2021

luctius Jul 15, 2021

archseer Jul 15, 2021

luctius Jul 16, 2021

archseer Jul 16, 2021

archseer Jul 15, 2021

luctius Jul 15, 2021

archseer commented Jul 15, 2021

pickfire left a comment

pickfire Jul 15, 2021

luctius Jul 15, 2021

archseer Jul 15, 2021

luctius commented Jul 15, 2021

pickfire left a comment

cessen commented Jul 16, 2021

archseer commented Jul 16, 2021

cessen commented Jul 17, 2021

	(range.from(), range.to() + 1, Some(text))
	(range.from(), range.to(), Some(text))

Added change_case command #441

Added change_case command #441

Conversation

luctius commented Jul 13, 2021

archseer left a comment

Choose a reason for hiding this comment

luctius commented Jul 14, 2021

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

archseer commented Jul 15, 2021

pickfire 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

luctius commented Jul 15, 2021

pickfire left a comment

Choose a reason for hiding this comment

cessen commented Jul 16, 2021

archseer commented Jul 16, 2021

cessen commented Jul 17, 2021