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

Added change_case command #441

Merged
merged 6 commits into from
Jul 16, 2021
Merged

Conversation

luctius
Copy link
Contributor

@luctius luctius commented Jul 13, 2021

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

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
helix-term/src/keymap.rs Outdated Show resolved Hide resolved
@luctius
Copy link
Contributor Author

luctius commented Jul 14, 2021

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 Outdated Show resolved Hide resolved
.chars()
.map(|ch| {
if ch.is_lowercase() {
ch.to_uppercase().collect()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, good point.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I think the current implementation is fine for now :)

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();
Copy link
Member

Choose a reason for hiding this comment

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

You're using to_lowercase in the uppercase method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, copy paste error; apologies.

@archseer
Copy link
Member

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?

@luctius luctius requested a review from archseer July 15, 2021 06:57
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

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

Screenshot_20210715_190948
Screenshot_20210715_190956
Screenshot_20210715_191120

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.

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

I think this should fix the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

@luctius
Copy link
Contributor Author

luctius commented Jul 15, 2021

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.

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

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

@cessen
Copy link
Contributor

cessen commented Jul 16, 2021

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 archseer merged commit 722cfed into helix-editor:master Jul 16, 2021
@archseer
Copy link
Member

Thanks! 🎉

@luctius luctius deleted the features/change_case branch July 16, 2021 17:28
@cessen
Copy link
Contributor

cessen commented Jul 17, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants