Regression - Command Options Errors

Describe the bug

Many valid options are now being marked as errors in the "Custom Key Mappings." This regression is cased by 29cf576a5b31c964a63fa70041a45ad4ce149d15

To Reproduce

1. Go to Vimium Options
2. Add a mapping with a valid option that is not the name of the option.

Vimium 2.3, all browsers/OS

Cause

This is because the valid checker either checks for a key in the options dict or a valid URL. Most valid options are not either the option name or a URL, but rather have several defined valid values.

For example, take openCopiedUrlInNewTab. In all_commands.js we see:

  {
    name: "openCopiedUrlInNewTab",
    desc: "Open the clipboard's URL in a new tab",
    group: "navigation",
    noRepeat: true,
    options: {
      position: "Where to place the tab in the tab bar. " +
        "One of `start`, `before`, `after`, `end`. `after` is the default.",
    },
  },

The valid options are not ["position"] (that is actually not a valid option), but should be ["start", "before", "after", "end"].

But in the current implementation:

Notice that the incorrect option "position" is marked as correct.

There are also numeric options, such as setZoom:

  {
    name: "setZoom",
    desc: "Set zoom",
    group: "tabs",
    advanced: true,
    background: true,
    options: {
      level: "The zoom level. This can be a range of [0.25, 5.0]. 1.0 is the default.",
    },
  },

Solution:

We need to introduce a way to define valid option values, if we are going to be checking them in the options validity checker. It needs to support:

• Arbitrary text
• URLs
• Set of valid strings
• Numbers
• For future proofing, it should probably be flexible to support a combination, like an integer, or "all", as this is a somewhat common practice in config systems. There are other ways around this though, so it's not necessary.

Note that one useful example of where allowing for text or numbers would be the position option. It could support a number of tabs before/after, or a valid position, like -2, 2, start, or end. It doesn't currently, but the use case isn't far-fetched.

I propose that the simplest solution right now is to use a valid array of strings with "special keywords" for other types (and is similar to the way you are handling URLs right now). However, this is a controversial approach and can lead to complexity, errors, and pain in the future! It would look something like:

valid: ["start", "before", "after", "end", "int"]

Note that this doesn't allow for int range, etc. Another less-controversial option is to create a few option classes so that we can assign valid values. This can solve the complexity of different options for different types.

valid: [enumOption(["start", "before", "after", "end"]), intOption(0, 5)]

The two relevant files are commands.js and all_commands.js. Because of these questions, I'll wait on implementing the fix PR until we decide on a preferred implementation.

Start with the validity checker in commands.js and the option definitions in all_commands.js, especially openCopiedUrlInNewTab and setZoom. Review the existing handling for option names and URLs before deciding how valid values should be represented. Done means valid string and numeric option values are accepted while invalid values are rejected, with the preferred representation agreed first.