Split off :config-cycle from :set

Before, we allowed :set to take multiple values, which often lead to confusing
error messages when a user forgot to quote the value.

Now, we instead have a dedicated :config-cycle command for that.

See #1840, #2794

doc/help/commands.asciidoc

@@ -31,6 +31,7 @@ It is possible to run or bind multiple commands by separating them with `;;`.
 |<<bookmark-load,bookmark-load>>|Load a bookmark.
 |<<buffer,buffer>>|Select tab by index or url/title best match.
 |<<close,close>>|Close the current window.
+|<<config-cycle,config-cycle>>|Cycle an option between multiple values.
 |<<download,download>>|Download a given URL, or current page if no URL given.
 |<<download-cancel,download-cancel>>|Cancel the last/[count]th download.
 |<<download-clear,download-clear>>|Remove all finished downloads from the list.
@@ -198,6 +199,20 @@ Focuses window if necessary.
 === close
 Close the current window.
 
+[[config-cycle]]
+=== config-cycle
+Syntax: +:config-cycle [*--temp*] [*--print*] 'option' 'values' ['values' ...]+
+
+Cycle an option between multiple values.
+
+==== positional arguments
+* +'option'+: The name of the option.
+* +'values'+: The values to cycle through.
+
+==== optional arguments
+* +*-t*+, +*--temp*+: Set value temporarily until qutebrowser is closed.
+* +*-p*+, +*--print*+: Print the value after setting.
+
 [[download]]
 === download
 Syntax: +:download [*--mhtml*] [*--dest* 'dest'] ['url'] ['dest-old']+
@@ -773,7 +788,7 @@ Save a session.
 
 [[set]]
 === set
-Syntax: +:set [*--temp*] [*--print*] ['option'] ['values' ['values' ...]]+
+Syntax: +:set [*--temp*] [*--print*] ['option'] ['value']+
 
 Set an option.
 
@@ -781,7 +796,7 @@ If the option name ends with '?', the value of the option is shown instead. If t
 
 ==== positional arguments
 * +'option'+: The name of the option.
-* +'values'+: The value to set, or the values to cycle through.
+* +'value'+: The value to set.
 
 ==== optional arguments
 * +*-t*+, +*--temp*+: Set value temporarily until qutebrowser is closed.

qutebrowser/config/configcommands.py

@@ -37,11 +37,11 @@ class ConfigCommands:
         self._config = config
         self._keyconfig = keyconfig
 
-    @cmdutils.register(instance='config-commands', star_args_optional=True)
+    @cmdutils.register(instance='config-commands')
     @cmdutils.argument('option', completion=configmodel.option)
-    @cmdutils.argument('values', completion=configmodel.value)
+    @cmdutils.argument('value', completion=configmodel.value)
     @cmdutils.argument('win_id', win_id=True)
-    def set(self, win_id, option=None, *values, temp=False, print_=False):
+    def set(self, win_id, option=None, value=None, temp=False, print_=False):
         """Set an option.
 
         If the option name ends with '?', the value of the option is shown
@@ -51,7 +51,7 @@ class ConfigCommands:
 
         Args:
             option: The name of the option.
-            values: The value to set, or the values to cycle through.
+            value: The value to set.
             temp: Set value temporarily until qutebrowser is closed.
             print_: Print the value after setting.
         """
@@ -66,37 +66,38 @@ class ConfigCommands:
             return
 
         with self._handle_config_error():
-            if option.endswith('!') and option != '!' and not values:
+            if option.endswith('!') and option != '!' and value is None:
-                # Handle inversion as special cases of the cycle code path
                 option = option[:-1]
                 opt = self._config.get_opt(option)
-                if isinstance(opt.typ, configtypes.Bool):
+                if not isinstance(opt.typ, configtypes.Bool):
-                    values = ['false', 'true']
-                else:
                     raise cmdexc.CommandError(
                         "set: Can't toggle non-bool setting {}".format(option))
-            elif not values:
+                old_value = self._config.get_obj(option)
+                self._config.set_obj(option, not old_value, save_yaml=not temp)
+            elif value is None:
                 raise cmdexc.CommandError("set: The following arguments "
                                           "are required: value")
-            self._set_next(option, values, temp=temp)
+            else:
+                self._config.set_str(option, value, save_yaml=not temp)
 
         if print_:
             self._print_value(option)
 
-    def _print_value(self, option):
+    @cmdutils.register(instance='config-commands')
-        """Print the value of the given option."""
+    @cmdutils.argument('option', completion=configmodel.option)
+    @cmdutils.argument('values', completion=configmodel.value)
+    def config_cycle(self, option, *values, temp=False, print_=False):
+        """Cycle an option between multiple values.
+
+        Args:
+            option: The name of the option.
+            values: The values to cycle through.
+            temp: Set value temporarily until qutebrowser is closed.
+            print_: Print the value after setting.
+        """
+        if len(values) < 2:
+            raise configexc.CommandError("Need at least two values")
         with self._handle_config_error():
-            value = self._config.get_str(option)
-        message.info("{} = {}".format(option, value))
-
-    def _set_next(self, option, values, *, temp):
-        """Set the next value out of a list of values."""
-        if len(values) == 1:
-            # If we have only one value, just set it directly (avoid
-            # breaking stuff like aliases or other pseudo-settings)
-            self._config.set_str(option, values[0], save_yaml=not temp)
-            return
-
             # Use the next valid value from values, or the first if the current
             # value does not appear in the list
             old_value = self._config.get_obj(option, mutable=False)
@@ -111,6 +112,15 @@ class ConfigCommands:
                 value = values[0]
             self._config.set_obj(option, value, save_yaml=not temp)
 
+        if print_:
+            self._print_value(option)
+
+    def _print_value(self, option):
+        """Print the value of the given option."""
+        with self._handle_config_error():
+            value = self._config.get_str(option)
+        message.info("{} = {}".format(option, value))
+
     @contextlib.contextmanager
     def _handle_config_error(self):
         """Catch errors in set_command and raise CommandError."""

tests/unit/config/test_configcommands.py

@@ -189,6 +189,11 @@ class TestSet:
         with pytest.raises(cmdexc.CommandError, match="set: No option 'foo'"):
             commands.set(win_id=0, option='foo' + suffix)
 
+
+class TestCycle:
+
+    """Test :config-cycle."""
+
     @pytest.mark.parametrize('initial, expected', [
         # Normal cycling
         ('magenta', 'blue'),
@@ -201,20 +206,20 @@ class TestSet:
         """Run ':set' with multiple values."""
         opt = 'colors.statusbar.normal.bg'
         config_stub.set_obj(opt, initial)
-        commands.set(0, opt, 'green', 'magenta', 'blue', 'yellow')
+        commands.config_cycle(opt, 'green', 'magenta', 'blue', 'yellow')
         assert config_stub.get(opt) == expected
         assert config_stub._yaml[opt] == expected
 
-    def test_cycling_different_representation(self, commands, config_stub):
+    def test_different_representation(self, commands, config_stub):
         """When using a different representation, cycling should work.
 
         For example, we use [foo] which is represented as ["foo"].
         """
         opt = 'qt_args'
         config_stub.set_obj(opt, ['foo'])
-        commands.set(0, opt, '[foo]', '[bar]')
+        commands.config_cycle(opt, '[foo]', '[bar]')
         assert config_stub.get(opt) == ['bar']
-        commands.set(0, opt, '[foo]', '[bar]')
+        commands.config_cycle(opt, '[foo]', '[bar]')
         assert config_stub.get(opt) == ['foo']