Fix DRY and Styling Issues (hardReload)

[UncleSnail]

Description

Removes duplicate hardReload functionality. I decided to go with removing the hardReload function entirely and mapping R to reload hard. The other obvious option would be to keep the hardReload function and have it call reload({count: count, tabId: tabId, registryEntry: {options: {hard: true}}}). This fixes the one main downside of my chosen implementation: it means the key mapping appears like this: "R, r Reload the page" which is not intuitive and does not make it clear that R is hard reload and r is regular reload.

Perhaps this calls for a change to how the commands are rendered in the help menu. If you would like, I can do that in this PR.

Since this is a refactor pull request, I used it to make two other changes that have been outstanding:

1. Rename mkRepeateCommands to createRepeatCommands.
2. Resolve the other deno fmt corrections.

I looked through the rest of the background_scripts/main.js file and there didn't seem to be any more obvious DRY violations or other easy refactors that should be done. If you have any more reactors you have been wanting done, let me know and I can add them to this PR.

The tests pass and with quick testing, the addon seems to still work correctly in Chromium and Firefox.

[philc]

All of the non-reload changes look good and can be merged; perhaps split the reload stuff out into a separate PR?

I agree that the UX of the help dialog is poor. I didn't realize that was an issue with the "reload hard" approach. If we maintain this approach, what is your suggested UX for the help dialog?

I'm surprised in commands.js specifying the command as "reload hard" actually works, and everything flows through Vimium's data structures correctly. This is the first example of that in the codebase. I'm not yet sure how I feel about encoding both the command name and its options as an unstructured string in the code, but it struck me as odd and I thought I'd mention the concern. I realize this is how the user's keymap configuration data is loaded and represented.

[UncleSnail]

I agree that the UX of the help dialog is poor. I didn't realize that was an issue with the "reload hard" approach. If we maintain this approach, what is your suggested UX for the help dialog?

Since it only requires minimal changes, a first step could be separating out commands with options as different commands. This would also fix the UI for the new zoom commands.

So currently if we set commands for many zoom levels, like z1, z2, z3 etc, it shows up in the help menu as z1, z2, z3, z4, z5, z6, z7, z8, z9 Set zoom level to a given value. E.g. map zz setZoom 1.5

We could just changing it to include the command in some way, like:

z1 setZoom 0.3 Set zoom level to a given value. E.g. map zz setZoom 1.5
z2 setZoom 0.75 Set zoom level to a given value. E.g. map zz setZoom 1.5

This would be pretty easy and would make minimal changes to the existing experience. If the multiple keys are mapped to the exact same command, we could just add them with commas, and we wouldn't need to show the commands that run. The better but more difficult to implement and maintain option would be to change the help message based on the command and the params. This would also mean that we would have different messages in the help menu (sometimes) from what is in the settings "all commands" menu. So the messages would appear as:

z1 Set zoom level to 0.3.
z2 Set zoom level to 0.75.
R Hard reload the page

We would need to have a system to select which message should be shown. This could be implemented to also help us define the valid options for commands on the "all commands" help list. I would propose allowing arrays of options/message tuples in the commandDescriptions list. If there is no array of accepted options, or if none of the options match, then we display the default message. If there is an options array, the first match is selected. It would be like a switch statement. We would need to make a format very clear, but that shouldn't be hard. Here is an example of how that might look in the commands.js file.

  showHelp: ["Show help", { topFrame: true, noRepeat: true }], // A normal command with one help message.
  reload: ["Reload the page", { background: true, options: [["hard", "Hard reload the page"]] }], // One opt matches to the literal "hard"
  setZoom: ["Set zoom level to a given value", { background: true, options: [["$level:float", "Set zoom level to $level"]] }], // $level's value will replace the help string. Float will be used in the "all commands" help section that specifies available options.
  example: ["Multiple arg opts", { options: [["literal", "Match literal first"], ["$var:bool", "Match var to $var second"]] }], // We could allow for multiple types of options, or for commands with multiple args.

This method is pretty easy to understand and implement. It also helps solve the "where do we list available options" issue because we can easily extract the options and types from the options lists. In this example, I included a type for non-literal options, which I think would be helpful to have in an options list. Note that I chose to use a list of "tuples" (lists) here instead of a dictionary because we need to decide which option will be matched first somehow. Lists allow us a convenient way to do this. There are other ways, but this seems like the most straight forward to me from both understandability and coding.

We can implement this for both the help messages and the settings page command options by:

Help Page:

If there are no options set on this shortcut's command, give the default message.
Otherwise, if there is an options list for the command, loop through it.
    If the option at `i` is a literal and it matches, put this help message.
    Otherwise, move on. If the option at `i` is variable (right now I think we only have floats and URLs) always match it (unless we want to do type matching, which is fine) and insert it into the help message as specified.
If we end the list and nothing matched, use the default message.

Settings Page:

If there is no options list, add no options to the list of available commands.
If there is an options list, loop through it.
    Add literal options as they are.
    Add variable options, noting their type if specified.

Hopefully that proposal all makes sense and is stated clearly enough. I believe I could implement this pretty easily, it should have no effect on existing commands that we don't change. It solves the help messages problems (that multiple shortcuts doing different things because of different options are grouped together and have the same message, and that we don't have help messages for commands like reset hard), and gives us the framework to solve the "unknown options" issue.

[philc]

Riffing on your proposal:

Showing a command's arguments in the help UI

We could show the arguments passed to the command, if present. This won't add much clutter, because none of the default key mappings provide arguments to their commands (except recently, for the reload hard command), so in the default configuration, the help dialog will look as it does today. If a person creates their own mapping to a command with arguments, then the arguments will be shown.

The help dialog text could look like this:

t Create new tab
;t0 Create new tab (url=https://...)
R Reload the page (hard)
z2 Zoom the page (zoom=1.2)

One difficulty with showing the argument values in help dialog UI is that the UI has to be robust to strings of varying lengths. E.g. createTab can have a URL parameter which can be very long. The standard solution for this is to ellipsize aggressively, and show the full command string on hover.

Second, with this approach it's possible for the help dialog to have many more rows than it does currently. But I think this is already handled well enough: today if the help dialog is too tall, the page will get a scroll bar (not the dialog itself) and one can scroll the page to see the full help dialog.

Parameterizing a command's help message

That's a fancy proposal, but adds a lot of complexity, and doesn't add much UX value if we instead show the arguments passed to the command.

Data model for a command's possible options

If it's not strictly required for our UX work, then I think it's out of scope and should be tackled apart from this hard reload / UX dialog work.

However, I think it's very valuable to do (see also #2827). It's annoying that one can't easily trace which options a command accepts when browsing commands.js.

[UncleSnail]

The help dialog text could look like this:

t Create new tab
;t0 Create new tab (url=https://...)
R Reload the page (hard)
z2 Zoom the page (zoom=1.2)

I like that UI quite well and I think it could be implemented, with the ellipsis etc. We could check what the length of the total line will be and only cut off how much we need. I think I will start working on this.

Second, with this approach it's possible for the help dialog to have many more rows than it does currently. But I think this is already handled well enough: today if the help dialog is too tall, the page will get a scroll bar (not the dialog itself) and one can scroll the page to see the full help dialog.

I also think the scrolling is handled well enough. I have advanced commands shown and have to scroll as it is. But you are correct that this could blow up the number of command messages shown, which might lead to future issues.

That's a fancy proposal, but adds a lot of complexity, and doesn't add much UX value if we instead show the arguments passed to the command.

Agreed. The only issue is being able to generate the possible options. If possible, I would like to see a solution where the possible options and the help messages for the commands are located in the same place in code because if we need to have a separate place to list available commands and their options, vs the place where we list what the help message will be, that is always a large hazard for them becoming out of sync (and it "doubles" the work of adding new command documentation). It's best to avoid that if we can, in my opinion. That's why I would like to find a solution where the help message and docs messages (in this case, params) are stored in one place.

Data model for a command's possible options

If it's not strictly required for our UX work, then I think it's out of scope and should be tackled apart from this hard reload / UX dialog work.

However, I think it's very valuable to do (see also #2827). It's annoying that one can't easily trace which options a command accepts when browsing commands.js.

That sounds good. We should definitely look into this in the future though. I can create a new issue for it or we can continue one of the old issues/PRs.

Another benefit of doing a "somewhat major" rework to the way commands/docs for them are defined is that we could give better developer and user guidance. As some (admittedly lofty) examples, we could have a unit test that checks to make sure if a command's code tries to access the options, then an option must be listed in the command docs. And in the "options" command mapper, we could add JS checking that uses the command definitions, and if someone enters an option on a command that accepts none, we could warn them.

Those are far-fetched and would probably never happen, but the point being that if we had a good command/options documentation method, then it would allow for implementing better tests/features in the future, the first of which being a better "available commands" page.

[UncleSnail]

The code in this pull request and the suggested changes/new features have been broken (as requested by @philc) into two new PRs: #4554 and #4553

This PR is no longer relevant (except for discussion reference) since all changes now live in other places.