-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Command options (in help message) #4554
base: master
Are you sure you want to change the base?
Conversation
This truncation method is that requested by philc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worked through a few different methods for implementing this, and have tried to explain why I believe this is the best given the existing code. If you have any more questions about why I chose this implementation, please ask!
background_scripts/commands.js
Outdated
} | ||
(commandToKey[registryEntry.command][optionString] != null | ||
? commandToKey[registryEntry.command][optionString] | ||
: (commandToKey[registryEntry.command][optionString] = [])).push(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section changes the commandToKey
from a 1 level dictionary like:
{
"setZoom": ["z1", "z2"],// old design
}
to a 2 level dictionary mapping from command to options set, like:
{
"setZoom: {// new design
"1.1": ["z1"],
"1.2": ["z2"],
}
}
Commands with no options will have the empty string options set, like:
{
"zoomIn: {
"": ["zi"],//No options
}
}
This allows us to split key mappings to their respective command/options combo, while still allowing us to directly check if we have any mappings to a command with commandToKey[command]
as we did before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the new structure, the current name is actively misleading. The map should be called commandToOptionStringToKeys
, or commandToOptionsToKeys
. A shorter name if possible would be nice, too.
And it would be nice to include your illustrative example data structure in a comment, which makes the name of this variable less important, because after reading your comment, it's easy to hold the shape of the data structure in one's head as one reads the code. The example should indicate that a command with no options will have an empty string as its key.
The use of the ternary operator here for control flow is an artifact of the coffeescript->js conversion process awhile ago. Let's change it to less dense and easier to read statements:
commandToKey[registryEntry.command][optionString] ||= [];
commandToKey[registryEntry.command][optionString].push(key);
advanced: this.advancedCommands.includes(command), | ||
options: options, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now have a 2-level dictionary, we now have a nested loop to loop through both commands and each variation of the command with different options. This also allows us to remove the keys check for null because we provide a default empty array in the loop.
So, for each command
we either get each option set and add a line for each, or if there are no mappings to the key we use { "": [] }
which is like saying that the empty (default) options set (no options) has no key mappings. This all works correctly with the other code, including with the options "show available commands".
Note that now we are also passing the options with the command entry.
@@ -409,7 +416,7 @@ const defaultKeyMappings = { | |||
"d": "scrollPageDown", | |||
"u": "scrollPageUp", | |||
"r": "reload", | |||
"R": "hardReload", | |||
"R": "reload hard", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also change the hardReload
to reload hard
and remove its associated code to avoid DRY violations and keep the code standard/maintainable.
zoomIn: ["Increase zoom", { background: true }], | ||
zoomOut: ["Decrease zoom", { background: true }], | ||
setZoom: ["Set zoom", { background: true }], | ||
zoomIn: ["Zoom in", { background: true }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the command descriptions for zoom in/out to make them match the command name and provide better intuition for the default mappings "zi" and "zo" since that name/description is where the mappings come from.
const desiredOptionsLength = Math.max(0, MAX_LENGTH - command.description.length - 3); | ||
const optionsTruncated = command.options.substring(0, desiredOptionsLength); | ||
let ellipis = ""; | ||
if ((command.description.length + command.options.length) > MAX_LENGTH) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we needed to truncate, add a set of ellipsis and set the title to the full options list so that we can see it on hover.
} | ||
const optionsString = command.options ? ` (${optionsTruncated}${ellipis})` : ""; | ||
const fullDescription = `${command.description}${optionsString}`; | ||
$$(descriptionElement, ".vimiumHelpDescription").textContent = fullDescription; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this implementation works exactly as you described in your comment on #4518 where the options list is truncated, but not the ending ")" or the command description itself.
I tried the other methods of truncation, and I like this one the most. I agree with your design, it works very well.
I also just pushed a commit to allow for a command + options set to be considered advanced. This way the If you do not like this feature, I can easily remove this commit. It seems like hard reloading should be an advanced command, so I wanted to let us mark it as such, but it does add some complexity and the potential for the advanced command list to explode with advanced option choices. Another implementation is to consider any command with options to be advanced, but then user mappings of simple commands (like new tab) with options are considered advanced and can't be seen in help without showing advanced options. I can imagine many users want to see their custom mappings without having "see advanced" checked, so I went with the implementation above. But, all options commands being advanced is a simpler solution to the problem with less scope-creep and still lets |
@@ -386,11 +395,11 @@ const Commands = { | |||
"enterVisualLineMode", | |||
"toggleViewSource", | |||
"passNextKey", | |||
"hardReload", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously (last PR), hardReload
was considered an advanced command, but reload is not (so we add reload hard
as an advanced command and add support for this).
@philc Let me know if there is anything you would like me to fix. Also, if all of the comment explanations/justifications are more overwhelming than helpful I can try to limit them in the future; just let me know your preference. |
@UncleSnail Will do. Thanks for posting this. I'll return to the project and review it soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be great! Overall it looks good; I've left some comments.
background_scripts/commands.js
Outdated
} | ||
(commandToKey[registryEntry.command][optionString] != null | ||
? commandToKey[registryEntry.command][optionString] | ||
: (commandToKey[registryEntry.command][optionString] = [])).push(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the new structure, the current name is actively misleading. The map should be called commandToOptionStringToKeys
, or commandToOptionsToKeys
. A shorter name if possible would be nice, too.
And it would be nice to include your illustrative example data structure in a comment, which makes the name of this variable less important, because after reading your comment, it's easy to hold the shape of the data structure in one's head as one reads the code. The example should indicate that a command with no options will have an empty string as its key.
The use of the ternary operator here for control flow is an artifact of the coffeescript->js conversion process awhile ago. Let's change it to less dense and easier to read statements:
commandToKey[registryEntry.command][optionString] ||= [];
commandToKey[registryEntry.command][optionString].push(key);
let ellipis = ""; | ||
if ((command.description.length + command.options.length) > MAX_LENGTH) { | ||
ellipis = "..."; | ||
$$(descriptionElement, ".vimiumHelpDescription").title = command.options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to have a comment above this line saying "The non-ellipsized option list will be visible on-hover."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed all the suggested changes and added a few comments where needed to help understandability.
I reran the formatter to ensure I didn't introduce any new formatting mistakes and tested the extension lightly in Chrome. Everything still appears to function as it should/did in my previous PR testing.
Description
Show command options in the help message dialogue as specified in this comment and the associated PR discussion.
Handles truncating based on the full length of the line, not just the length of the options, and shows the full options on hover.
Always indicates the presence of options even on lines with very long descriptions.
Correctly handles flags (without showing them as flag=true)
Handles commands with multiple options.
This PR also switches the new
hardReload
for the oldreload hard
to avoid repeated code/functionality.Implementation
There could be cleaner implementations, so let me know if you have a better idea, but I believe I have found a good method that requires minimal changes and fits very well into the existing mechanism. I will describe the implementation and logic more in comments on the code.
I have formatted all new code (see #4553 for the other changes from
deno fmt
) and all tests pass.