feat(commands): allows cycling option values at runtime

[alevinval]

This patch updates the :toggle command to supports cycling of values at runtime. When the config is a boolean, it flips the value, otherwise cycles the argument strings. See the demo below:

[archseer]

Looks similar to #4085

[alevinval]

Looks similar to #4085

You are right, with the addition that cycle allows us to cycle settings that are not booleans. Let me know the best way to move forward.

[alevinval]

Maybe I could remove the logic that flips booleans, so boolean settings are handled with toggle. String settings are handled with cycle.

[archseer]

Ah sorry, I forgot about this PR and merged #4085 first. Can you rebase and update the toggle implementation? I think your behaviour is preferrable but I'd still call the command toggle.

I also think it would be interesting if it allowed you to cycle between options without having to specify possible values.

[David-Else]

Thanks for this, I am really hoping it will have the ability to toggle line numbers:

gutters = ["diagnostics", "spacer", "spacer", "diff"] # no line-numbers

gutters = ["diagnostics", "spacer", "spacer", "diff", "line-numbers"]

Is that possible yet?

[divarvel]

Ah sorry, I forgot about this PR and merged #4085 first. Can you rebase and update the toggle implementation? I think your behaviour is preferrable but I'd still call the command toggle.

I also think it would be interesting if it allowed you to cycle between options without having to specify possible values.

I thought about it when independently working on a toggle command. With enumerated values we have enough information to cycle, but the way things are currently implemented rely on the toml encoding, which doesn't have this information anymore.

I think this is especially important to avoid boolean blindness and avoid nudging implementors using booleans when 2-valued enums would be better.

Another reason why it would be interesting to do that is that the underlying mechanism could also be used to provide completion for setting values as well, not just keys. If there's no active work on this, i can give it a try.

[alevinval]

@archseer you want to update the toggle implementation to what I propose in the cycle one?

[alevinval]

Updated toggle to support cycling of options 👍

[pascalkuthe]

great that you are picking this back up!

[pascalkuthe]

LGTM this makes :toggle much more useful 👍

[David-Else]

@alevinval Thanks, great contribution!

I am fiddling around trying to answer my own question but can't get anywhere, could you quickly let me know if #4411 (comment) is possible?

[alevinval]

To keep it short: no, the current implementation is quite limited in supporting cycling of configuration values that have a string value out of several possibilities. What you are suggesting would require adding/removing elements from the vector. I could look into extending the implementation to deal with arrays, such that if you do :toggle gutters.layout line-numbers it simply adds/removes the element from the array. I'll see when I have some time on my hands.

[alevinval]

Looking back into this... I've just realised that I wrote some poor code here, I did not realise that I could pattern match on serde_json::Value cause it's an Enum... I've opened a tiny follow-up to this PR here:

#7240

It does two things:

• Gives better error message feedback to the user, when trying to toggle something that is not supported.
• Simplifies code, as the type conversion happens within the matching
• Makes it obvious on what is the way forward: which missing enum variants are pending to be implemented

Based on that clean-up PR, if we were to do something like that:

diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs
index 824abbf4..656e4aa3 100644
--- a/helix-term/src/commands/typed.rs
+++ b/helix-term/src/commands/typed.rs
@@ -1,3 +1,4 @@
+use std::cmp::min;
 use std::fmt::Write;
 use std::ops::Deref;
 
@@ -1824,7 +1825,22 @@ fn toggle_option(
                     .to_string(),
             )
         }
-        Value::Null | Value::Object(_) | Value::Array(_) | Value::Number(_) => {
+        Value::Array(ref mut arr) => {
+            ensure!(
+                args.len() == 3,
+                "Bad arguments. For array configurations use: `:toggle key val index`",
+            );
+
+            let value = Value::String(args[1].to_string());
+            let index: usize = min(arr.len(), args[2].parse()?);
+            if arr.contains(&value) {
+                arr.retain(|e| *e != value)
+            } else {
+                arr.insert(index, value)
+            }
+            Value::Array(arr.to_owned())
+        }
+        Value::Null | Value::Object(_) | Value::Number(_) => {
             anyhow::bail!("Configuration {key} does not support toggle yet")
         }
     };

Screen.Recording.2023-06-06.at.18.26.33.mov

Supporting toggling on Array seems quite convoluted to implement, as you now have to figure out how to deal with the inner type so we will get plenty of branches again for figuring out the parsing... Unless of course, we limit to arrays of strings only.

You also get issues with repeated values, as the gutters layout contains a couple of spacer entries, so depending on the order of running the commands you end up with a mess. All in all, I think this is very tricky and not sure is worth pursuing.

Let me know thoughts on this.

[David-Else]

@alevinval With great joy I realized that a toggleable zen mode is almost within reach, but this does not work :(

e = ":toggle gutters.line-numbers.min-width 48,0"

Configuration gutters.line-numbers.min-width does not support toggle yet

Would it be easy to fix? Would be a massive feature!

[alevinval]

I did not implement Number because I did not see a straightforward way to get a Value::Number(Number) out of a String. Without this, you would need to parse it into some float or an integer. This seems flaky, maybe we have floating point settings, and other settings are integers, so you would need to implement some smart (and easy to get wrong) logic. I feel there should be a way to get serde numbers from strings... But I don't see how, since the serde API for parsing number from strings is not public.

[David-Else]

Cheers, using this for now :)

[keys.normal.space]

Z = ":set gutters.line-numbers.min-width 3"
z = ":set gutters.line-numbers.min-width 48"

[gabydd]

Is there reason you can't just use parse like in set-option for everything that's not a string or a bool: https://github.com/helix-editor/helix/blob/master/helix-term/src/commands/typed.rs#L1802

[alevinval]

Ah you're genius! Here's the PR to add support to toggle numbers: #7877