Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Command options (in help message) #4554

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Command options (in help message) #4554

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b3bdd5
Change zoom command description to match mapping intuition.
UncleSnail Oct 8, 2024
51e8753
Initial implementation of command options help messages.
UncleSnail Oct 8, 2024
034630e
Remove hardReload command and replace it with `reload hard`
UncleSnail Oct 8, 2024
0f93cb5
Change truncating logic to full description length and fmt.
UncleSnail Oct 8, 2024
63f266d
Allow commands with options to be advanced (for hard reload).
UncleSnail Oct 9, 2024
6fdbdd1
Fix advanced check styling.
UncleSnail Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions background_scripts/commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ const Commands = {

for (const line of configLines) {
const tokens = line.split(/\s+/);
const command = tokens[0].toLowerCase();
switch (command) {
const action = tokens[0].toLowerCase();
Copy link
Contributor Author

@UncleSnail UncleSnail Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is for understandability. Before there were two variables named command within this block:

  1. This "command" meaning map, unmap, etc.
  2. The "command" on line 71 which is the actual command to be run, like "newTab" or "setZoom".
    This caused understanding errors for me, so to avoid this for others in the future, I renamed the map vs unmap to the more fitting name action. Please suggest your name preference if different.

switch (action) {
case "map":
if (tokens.length >= 3) {
const [_, key, command, ...optionList] = tokens;
Expand Down Expand Up @@ -118,7 +118,7 @@ const Commands = {
}
break;
default:
logWarning(`"${command}" is not a valid config command in line:`, line);
logWarning(`"${action}" is not a valid config command in line:`, line);
}
}

Expand Down Expand Up @@ -260,21 +260,30 @@ const Commands = {
const commandToKey = {};
for (const key of Object.keys(this.keyToRegistryEntry || {})) {
const registryEntry = this.keyToRegistryEntry[key];
(commandToKey[registryEntry.command] != null
? commandToKey[registryEntry.command]
: (commandToKey[registryEntry.command] = [])).push(key);
const optionString = registryEntry.optionList?.length > 0
? registryEntry.optionList?.join(" ")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joining the option list here is the simplest way to turn the options back into a string. It does mean that differently written commands with the same options will be marked different. IE: command opt=example opt2=example2 is not equal to command opt2=example2 opt=example even though they are the same. I can do a mapping from the options object which would fix this problem if you would like, but it seemed like code bloat.

: "";
if (commandToKey[registryEntry.command] == null) {
commandToKey[registryEntry.command] = {};
}
(commandToKey[registryEntry.command][optionString] != null
? commandToKey[registryEntry.command][optionString]
: (commandToKey[registryEntry.command][optionString] = [])).push(key);
Copy link
Contributor Author

@UncleSnail UncleSnail Oct 8, 2024 •
edited
Loading

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.

}
const commandGroups = {};
for (const group of Object.keys(this.commandGroups || {})) {
const commands = this.commandGroups[group];
commandGroups[group] = [];
for (const command of commands) {
commandGroups[group].push({
command,
description: this.availableCommands[command].description,
keys: commandToKey[command] != null ? commandToKey[command] : [],
advanced: this.advancedCommands.includes(command),
});
for (const [options, keys] of Object.entries(commandToKey[command] ?? { "": [] })) {
commandGroups[group].push({
command,
description: this.availableCommands[command].description,
keys: keys,
advanced: this.advancedCommands.includes(command),
options: options,
});
}
Copy link
Contributor Author

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.

}
}
chrome.storage.session.set({ helpPageData: commandGroups });
Expand All @@ -297,7 +306,6 @@ const Commands = {
"scrollToLeft",
"scrollToRight",
"reload",
"hardReload",
"copyCurrentUrl",
"openCopiedUrlInCurrentTab",
"openCopiedUrlInNewTab",
Expand Down Expand Up @@ -386,7 +394,6 @@ const Commands = {
"enterVisualLineMode",
"toggleViewSource",
"passNextKey",
"hardReload",
Copy link
Contributor Author

@UncleSnail UncleSnail Oct 9, 2024 •
edited
Loading

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).

"setZoom",
"zoomIn",
"zoomOut",
Expand All @@ -409,7 +416,7 @@ const defaultKeyMappings = {
"d": "scrollPageDown",
"u": "scrollPageUp",
"r": "reload",
"R": "hardReload",
"R": "reload hard",
Copy link
Contributor Author

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.

"yy": "copyCurrentUrl",
"p": "openCopiedUrlInCurrentTab",
"P": "openCopiedUrlInNewTab",
Expand Down Expand Up @@ -499,7 +506,6 @@ const commandDescriptions = {
scrollFullPageUp: ["Scroll a full page up"],

reload: ["Reload the page", { background: true }],
hardReload: ["Hard reload the page", { background: true }],
toggleViewSource: ["View page source", { noRepeat: true }],

copyCurrentUrl: ["Copy the current URL to the clipboard", { noRepeat: true }],
Expand Down Expand Up @@ -562,9 +568,9 @@ const commandDescriptions = {
moveTabLeft: ["Move tab to the left", { background: true }],
moveTabRight: ["Move tab to the right", { background: true }],

setZoom: ["Set zoom level to a given value. E.g. map zz setZoom 1.5", { background: true }],
zoomIn: ["Increase zoom", { background: true }],
zoomOut: ["Decrease zoom", { background: true }],
setZoom: ["Set zoom", { background: true }],
zoomIn: ["Zoom in", { background: true }],
Copy link
Contributor Author

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.

zoomOut: ["Zoom out", { background: true }],
zoomReset: ["Reset zoom", { background: true }],

"Vomnibar.activate": ["Open URL, bookmark or history entry", { topFrame: true }],
Expand Down
6 changes: 0 additions & 6 deletions background_scripts/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,12 +405,6 @@ const BackgroundCommands = {
chrome.tabs.reload(tab.id, { bypassCache });
});
},

async hardReload({ count, tab }) {
await forCountTabs(count, tab, (tab) => {
chrome.tabs.reload(tab.id, { bypassCache: true });
});
},
};

async function forCountTabs(count, currentTab, callback) {
Expand Down
12 changes: 11 additions & 1 deletion pages/help_dialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,17 @@ const HelpDialog = {
});
}

$$(descriptionElement, ".vimiumHelpDescription").textContent = command.description;
const MAX_LENGTH = 50;
const desiredOptionsLength = Math.max(0, MAX_LENGTH - command.description.length - 3);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Find the length that the options list should be to make the full description the right length. Don't ever let it be less than 0, and leave room for the ellipsis "...".

Since this might not be clear, I will explain why I implement it this correct way. It bothers me how many people implement this wrong. Leaving room means that we fit as long lines as we can. For example, if we want a line fewer than 10 characters, not leaving room (not having the - 3) would mean we need to truncate anything over 7 characters so that the ... does not make the line too long. So we end up with:

"truncated!" -> "truncat..."

There is no reason to do this since the word could have fit on the line (word is 10 chars and we are fine with 10 chars). The other common wrong implementation is to truncate anything >= 10 and add the "..." giving:

"truncated!" -> "truncated!..."

This gives us a line that is longer than 10 chars!

The correct implementation (like this where we leave space) means only lines that need to be truncated are, and we never make a line too long:

"truncated!" -> "truncated!" (10 chars)
"even longer" -> "even lo..." (10 chars)

I hope this explains what the -3 is for. I thought about adding a quick comment in the code there since it might not be clear, and I will do so if you like.

const optionsTruncated = command.options.substring(0, desiredOptionsLength);
let ellipis = "";
if ((command.description.length + command.options.length) > MAX_LENGTH) {
Copy link
Contributor Author

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.

ellipis = "...";
$$(descriptionElement, ".vimiumHelpDescription").title = command.options;
}
const optionsString = command.options ? ` (${optionsTruncated}${ellipis})` : "";
const fullDescription = `${command.description}${optionsString}`;
$$(descriptionElement, ".vimiumHelpDescription").textContent = fullDescription;
Copy link
Contributor Author

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.


keysElement = $$(keysElement, ".vimiumKeyBindings");
let lastElement = null;
Expand Down