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 paragraph motion and textobject #1627

Merged
merged 7 commits into from Apr 2, 2022
Merged

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Feb 7, 2022

Also improved testing facility.

Fix #1580
Superseeds #784

Although this is not done I think it would be good to help review the new test helpers.

TODO

  • prev paragraph (test)
  • next paragraph
  • inside/around paragraph
  • support extend mode?

Not sure if we want to support extend mode for this even though inspiration is taken from kakoune, kakoune does not have extend mode but we have that, maybe we can make use of it to extend an additional paragraph? I didn't do it since I don't think I need it.

@pickfire pickfire force-pushed the paragraph branch 3 times, most recently from 610934b to 45f83fa Compare February 21, 2022 15:04
@pickfire pickfire marked this pull request as ready for review February 21, 2022 15:04
helix-term/src/keymap.rs Outdated Show resolved Hide resolved
@dead10ck
Copy link
Member

I think the syntax of the test helpers warrants its own whole discussion. Should we do that in this PR, or save it for another? It's also very relevant to the integration testing branch I've been working on.

@pickfire pickfire changed the title Add (prev) paragraph motion Add paragraph motion and textobject Feb 21, 2022
@pickfire
Copy link
Contributor Author

pickfire commented Feb 21, 2022

I think the syntax of the test helpers warrants its own whole discussion. Should we do that in this PR, or save it for another? It's also very relevant to the integration testing branch I've been working on.

I use that here because it's very painful to have to manually count \n while I was working on this, I spent a whole bunch of time counting them until I feel the pain to write it out, maybe we can change the test syntax later?

Yes, it was discussed but IIRC @sudormrfbin or another maintainer does not like it. Personally I think it is fine since test is very limited and we don't have to cover everything, but I am fine either way if someone comes up with something that can be escaped, I just don't see much benefit of doing that so I didn't do that.

@dead10ck
Copy link
Member

Yeah I felt that pain when writing the auto pairs tests as well, counting individual characters. I'd really like to improve the testing facilities too, but it was apparent in the chat room that there were some different ideas about how to mark selections and such, so it would be better to work out a scheme before it becomes much more difficult to change later because there are a ton of tests written with the chosen syntax. But this discussion may not be short, so I also don't want to hold up this PR.

@pickfire
Copy link
Contributor Author

pickfire commented Feb 21, 2022

But this discussion may not be short, so I also don't want to hold up this PR.

I rather hold up this PR and wait for that discussion (hopefully it didn't become logo discussion length), given that it is so error prone while writing index based test like I did previously (I ended up fixing test index rather than code which is wrong), usually it's fine but now \n visually 2 characters became 1 character and my eyes T_T.

@dead10ck
Copy link
Member

dead10ck commented Feb 21, 2022

Okay, I'm also in favor of discussing now.

Personally I think we will need to be able to write single special ASCII characters, especially in integration tests, where, for example, there may end up being lots of tests to verify things like our tree-sitter queries in actual real source code, so I think something about the syntax will need to change from the one in this PR.

So I think in order to avoid the whole issue of escaping, it might be better to use a multi character sigil to mark selections, so that the syntax is very unlikely to conflict with any input text.

I know @archseer mentioned that kakoune uses %() to denote selections. I'm not sure what other syntax there is, since it doesn't seem to be documented. This might work, although we should probably extend it further so we can indicate the head, as well as primary selection, like you tried to do in this PR.

Maybe to simplify the scanner, we could do %( )% and | at the beginning or end to mark the head? So

%(|some te)%xt would mean some te is the selection, with the head at the front.

And maybe secondary selections could be %[text]%.

Just spit balling. Anyone have any other ideas?

Also, I'd really like to pull in indoc so we can use multi line strings, so the input text will look much more like rendered text. I think in cases where it's more important to verify exact line endings, these can just not use indoc so you can be sure of the input.

@archseer
Copy link
Member

although we should probably extend it further so we can indicate the head,

It seems to me that % indicates the tail: %() vs ()%. For primary vs other selections we can use a different symbol, i.e. %[]

@dead10ck
Copy link
Member

dead10ck commented Feb 25, 2022

although we should probably extend it further so we can indicate the head,

It seems to me that % indicates the tail: %() vs ()%. For primary vs other selections we can use a different symbol, i.e. %[]

That would make sense; however, just thinking about it from an implementation perspective, it might be tricky to implement a parser for this without the same problem with escaping. e.g. if we're testing some Rust source code

let pair = ((bar(), baz()))%;

How do we know where the selection starts? We'd have to do some kind of backtracking and possibly escaping or matching of other () pairs.

I think having both ends be two chars will make it easier to parse, as well as easier to see visually what the selection is.

@pickfire
Copy link
Contributor Author

pickfire commented Feb 26, 2022

Not quite the example you pointed out but I imagine this would be more common. I think maybe using # is better since I rarely see #().

x = (a-b)%(c-b)

@dead10ck
Copy link
Member

Not quite the example you pointed out but I imagine this would be more common. I think maybe using # is better since I rarely see #().

x = (a-b)%(c-b)

Yeah, this case could be easily fixed by adding whitespace, but #()# works just as well I think.

@archseer
Copy link
Member

archseer commented Mar 1, 2022

I don't have a strong opinion here so I'm okay with whatever you two settle on. I'd keep it simple and pick something "good enough" even if it has some edge cases, we can avoid those by changing the test specs.

@dead10ck
Copy link
Member

dead10ck commented Mar 1, 2022

Ok, @pickfire I'm good with #(|)# and #[|]# if you are.

@pickfire
Copy link
Contributor Author

pickfire commented Mar 1, 2022

Ok, @pickfire I'm good with #(|)# and #[|]# if you are.

I don't think that is a good idea since we don't know which is head and anchor. But #[] for primary and #() for others should be good enough I think.

@dead10ck
Copy link
Member

dead10ck commented Mar 1, 2022

@pickfire read my earlier comment for how to mark the head and anchor.

#(|hello)# world would indicate hello is the selection with the head coming before the anchor. #(hello|)# world would indicate hello with the head at the end.

#(foo|)# bar
#[baz|]# quux

would indicate foo is the primary selection, baz is a secondary, and the head is at the end of both.

@pickfire
Copy link
Contributor Author

pickfire commented Mar 1, 2022

Wouldn't it be better to have #[|]# for primary given that primary selection is always needed and used more often than secondary selection, so it shouldn't have the extra shift needed on most layouts?

@dead10ck
Copy link
Member

dead10ck commented Mar 1, 2022

Wouldn't it be better to have #[|]# for primary given that primary selection is always needed and used more often than secondary selection, so it shouldn't have the extra shift needed on most layouts?

Sure, that sounds good to me 👍

helix-core/src/test.rs Outdated Show resolved Hide resolved
@pickfire pickfire force-pushed the paragraph branch 2 times, most recently from 1bb1085 to 5751684 Compare March 7, 2022 13:09
@cyruseuros
Copy link

I'd just like to chime in on adding extend mode support. I personally use it with vim quite a bit, but that's beside the point. I believe it should be implemented for consistency's sake - why would motions suddenly not work when I happen to be selecting things? In addition, in obscure languages where treesitter fails/is not complete, or when editing markdown documents, there's really no good way to select an entire class/multiple paragraphs other than extend mode.

PS: Is there a reason why existing objects are not listed in the popup when one types mi/ma?

@sudormrfbin
Copy link
Member

PS: Is there a reason why existing objects are not listed in the popup when one types mi/ma?

Fixed in master: #1686

@pickfire
Copy link
Contributor Author

I'd just like to chime in on adding extend mode support. I personally use it with vim quite a bit, but that's beside the point. I believe it should be implemented for consistency's sake - why would motions suddenly not work when I happen to be selecting things? In addition, in obscure languages where treesitter fails/is not complete, or when editing markdown documents, there's really no good way to select an entire class/multiple paragraphs other than extend mode.

Note that some parts of extend (or normal select behavior) are still inconsistent, I am looking to send fixes as I see fit.

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.

Looks good!

@archseer
Copy link
Member

Just needs a rebase to fix the conflict

pickfire and others added 7 commits April 1, 2022 09:05
Also improved testing facility.

Fix helix-editor#1580
Change parameter/argument key from p to a since paragraph only have p
but parameter are also called arguments sometimes and a is not used.
@archseer archseer merged commit ec21de0 into helix-editor:master Apr 2, 2022
@sudormrfbin
Copy link
Member

Thanks @pickfire for working on this 🎉 Holding x was getting really tiring :P

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.

Add paragraph + indenation text object/motion
6 participants