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

Show infobox to hint textobjects with mi and ma #1686

Conversation

EpocSquadron
Copy link
Contributor

We should probably add to the status area as well, but this is a significant improvement in discoverability for textobject surround selection.

@EpocSquadron
Copy link
Contributor Author

I cleaned up a bug that was swallowing input if you cancelled with escape while in the middle of a mi or ma. I also improved the help text a bit.

@EpocSquadron
Copy link
Contributor Author

Also added the pending keys below the status bar. This should help in cases where people turn off the info box in their config. I can remove that part as it's a bit hackier than the rest. Let me know and i'll force push to remove it and maybe open a separate pr after this one is merged.

Copy link
Member

@sudormrfbin sudormrfbin left a comment

Choose a reason for hiding this comment

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

Ideally every textobject would be split into it's on bindable function so that #1595 gets completely resolved (and we would not have to manually show the infobox either). But it's currently blocked on not being able to execute a fallback function if none of the keys match, which would be required to make the surround objects like mi( work. I think it's good enough as a temporary solution (at the risk of it slipping into being the permanent solution :P).

Showing pending keys should also be ideally reworked to automatically work based on if there's an on_next_key callback registered. This has the benefit of working everywhere like f, mi, etc.

@pickfire
Copy link
Contributor

But it's currently blocked on not being able to execute a fallback function if none of the keys match, which would be required to make the surround objects like mi( work.

I think we should just add a function for each of these rather than having a fallback. An exhaustive list should work fine like kakoune, then we don't have to handle wildcard.

@sudormrfbin
Copy link
Member

sudormrfbin commented Feb 23, 2022

Defining a function for every single surround pair ((, [, {, ', ", <, `, and these are only the opening pairs) would be wasteful, and it would definitely clutter up the infobox. Besides it would be useful to be able to have any character act as a surround delimiter (for example using mi~ to select inside strikethrough text in markdown).

@EpocSquadron
Copy link
Contributor Author

I agree with @sudormrfbin on this. Another case where it is useful is if you have an @ delimited regex in a web context, or some markup using a pair of delimiters like {% %}.

I've been mulling over how to refactor to address the other feedback. I'm probably a couple days away from being able to fully address.

@pickfire
Copy link
Contributor

That is good too but not every places uses the same method, I have seen some text that uses ` and ' for opening and closing quote and we have no way around that. Yeah, I guess we could have a fallback, IIRC I did fallback for the older paragraph pull request I sent but I scraped that changes.

@EpocSquadron EpocSquadron force-pushed the epocsquadron/show-infobox-for-match-around-inside branch from b97d9bf to 5a817bc Compare February 27, 2022 14:14
@EpocSquadron
Copy link
Contributor Author

Ideally every textobject would be split into it's on bindable function so that #1595 gets completely resolved (and we would not have to manually show the infobox either). But it's currently blocked on not being able to execute a fallback function if none of the keys match, which would be required to make the surround objects like mi( work. I think it's good enough as a temporary solution (at the risk of it slipping into being the permanent solution :P).

Showing pending keys should also be ideally reworked to automatically work based on if there's an on_next_key callback registered. This has the benefit of working everywhere like f, mi, etc.

I don't have the bandwidth to do this level of refactor yet. Hoping we can get this in as a first pass and revisit later.

@archseer archseer merged commit b13d441 into helix-editor:master Mar 1, 2022
@EpocSquadron EpocSquadron deleted the epocsquadron/show-infobox-for-match-around-inside branch March 1, 2022 04:01
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.

None yet

4 participants