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 option to not overtype on closing pairs #10501

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SoahLi
Copy link

@SoahLi SoahLi commented Apr 17, 2024

Solution to #9595
Vscode uses three modes(always, auto - only skip over auto generated closing pairs, and never) for autoCloseOvertype, while my implementation only uses two(always and never).

@SoahLi
Copy link
Author

SoahLi commented Apr 17, 2024

This is an extension of #10301. I apologize for the mistake in closing the request, as I am still learning how to use git!

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Apr 21, 2024
[editor]
auto-pairs = false # defaults to `true`
[editor.auto-pairs]
config-type = false # defaults to `true`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really nice to break a config without much of a need. Perhaps it's better to add auto-pairs-overtype = <bool> param?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean adding an auto-pairs-overtype = <bool> param to the Editor? If so I can certainly do that. It seemed better to group it with the other auto-pairs configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I suggested this as an option but whether a breaking change like this would be accepted would be up to the maintainers. It feels better to group them together since we do that for e.g. soft-wrap and the file picker. It would be possible to avoid a breaking change by using an enum, but yeah it's up to the maintainers.

Copy link

Choose a reason for hiding this comment

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

The config-type naming is not very understandable to me, personally.

Also I think having both [editor.auto-pairs] and [editor.auto-pairs.<whatever_we_name_it>] complicates configuration. It would be nice if overtype was co-located with the pair definitions. This would also maintain backwards compatability, if you make it optional.

log::trace!("autopairs hook selection: {:#?}", selection);

if let Some(pair) = pairs.get(ch) {
if pair.same() {
return Some(handle_same(doc, selection, pair));
} else if pair.open == ch {
return Some(handle_open(doc, selection, pair));
} else if pair.close == ch {
} else if pair.close == ch && overtype {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the flag be handled inside handle_close?

Copy link
Author

Choose a reason for hiding this comment

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

I thought it might make more sense to just return and insert the closing pair instead of creating a new transaction and then checking. It definitely makes more sense inside though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants