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

feature: add a toggle-option command #5770

Closed
wants to merge 1 commit into from

Conversation

divarvel
Copy link
Contributor

@divarvel divarvel commented Feb 1, 2023

This commands allows to toggle boolean options, with a dedicated completer that only suggests boolean options
It does not support other enumerated types.

:toggle softwrap.enable

Alternative options:

:set !softwrap.enable # ok as well (as long as options cannot start with a !), consistent with vim, i think
:set softwrap.enable ! # possibly ambiguous

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 1, 2023
pascalkuthe
pascalkuthe previously approved these changes Feb 1, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

Implementation LGTM. I think this feature certainly makes sense so users can bind it to their keys. I was originally leaning towards a special :set syntax for this but I think a separate command is easier to implement, more discoverable and easier to remeber.

aliases: &["toggle"],
doc: "Toggle a config option at runtime.\nFor example to toggle smart case search, use `:toggle search.smart-case`.",
fun: toggle_option,
completer: Some(completers::setting),
Copy link
Contributor

Choose a reason for hiding this comment

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

How possible would it be to only complete boolean setting names instead of all of them?
BTW, this is fantastic, I would love to see this added.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, should be pretty trivial, all you would need to do is use value.is_boolean() in the get_keys function in helix-term/ui/mod.rs instead of !value.is_object()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i'll have a shot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in a new commit

.collect()
}

pub fn boolean_setting(_editor: &Editor, input: &str) -> Vec<Completion> {
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 would have preferred adding a helper function used by both setting and boolean_setting, but the static KEYS can't depend on a function parameter.

Copy link
Member

@pascalkuthe pascalkuthe Feb 4, 2023

Choose a reason for hiding this comment

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

you just have two static in the setting function:

static KEYS

and

static KEYS_BOOL

and dispatch which of those two to use with a simple if:

let keys = if only_booleans{
 KEYS_BOOL
}else{
  KEYS
};

Not a perfect solution either but would reduce code duplication slightly. I don't imagine that we would add more specialized settings keys in the future as more advanced dynamic configuration is better achieved with a future plugin system. Until then hardocind the bool keys seems like a good enough solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in a separate commit, i had to duplicate the .iter() call as well

@poliorcetics
Copy link
Contributor

#4085 has been merged, is this a duplicate ?

@poliorcetics
Copy link
Contributor

Ah, it's a partial duplicate: in #4085, the completer proposes all keys, not only boolean ones

@divarvel
Copy link
Contributor Author

Heh, I'll rebase and rename the PR accordingly

@divarvel
Copy link
Contributor Author

Amusing how my changes were close to #4085 without being aware of it.

I've only kept the commit adding dedicated completion.

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 16, 2023

Actually it seems like @archseer wants to merge #4411 too which allows cycling arbitrary settings by providing a list of available values. In that case we actually want to keep the completion for any settings. For example :toggle bufferline always multiple

I wasn't aware of those two PRs when you started working on this originally and somehow endedup missing them while searching. That was my bad! Sorry for wasting your time.

@divarvel
Copy link
Contributor Author

ah, support for enumerated values was next on my list. i waited before working on it, thankfully ^^

no worries, i'll close this one

@divarvel divarvel closed this Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants