-
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
Expose global commands at chrome://extensions/shortcuts #3785
base: master
Are you sure you want to change the base?
Conversation
Also adds a tab jump list, but might split that out if this PR has interest.
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'm not a maintainer, but I had a few suggestions.
@@ -1,8 +1,416 @@ | |||
{ | |||
"name": "vimium", | |||
"version": "1.0.0", | |||
"lockfileVersion": 1, | |||
"lockfileVersion": 2, |
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'd revert this file since you aren't making any package.json
changes.
@@ -350,6 +360,58 @@ const BackgroundCommands = { | |||
} | |||
}; | |||
|
|||
chrome.commands.onCommand.addListener(function(command) { | |||
|
|||
const sendCommandToCurrentTab = function(requestName) { |
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.
A better name for this variable would be command
.
}); | ||
break; | ||
default: | ||
console.error('unrecognized command: ', command); |
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.
You don't need to add a space after the string. The logger joins multiple things together with spaces by default.
@@ -7,6 +7,32 @@ | |||
"48": "icons/icon48.png", | |||
"128": "icons/icon128.png" }, | |||
"minimum_chrome_version": "69.0", | |||
"commands": { | |||
"jump-back-tab": { | |||
"description": "Jump back in the tab stack." |
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.
Everywhere else you call it a tab list (instead of a stack). These should be consistent.
this.deletedTabs = new Set(); | ||
} | ||
|
||
isCoherent(currentTabId) { |
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.
Did you mean isCurrent
here?
|
||
constructor(tabs) { | ||
this.tabs = tabs; | ||
this.activeIdx = tabs.length - 1; |
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 name could be more descriptive/explicit. Is that activeTabId
?
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.
Um, I suggest you use git rebase
to modify this commit and remove package-lock.json
from this PR.
"switch-to-previous-tab": { | ||
"description": "Switch to the previously used tab." | ||
} | ||
}, |
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.
Just a personal opinion:
Maybe we can use command names of Vimium directly, like what I've done in https://github.com/gdh1995/vimium-c/blob/dc5e026083db67fc6f471c907e5ca37b7b5a5316/manifest.json#L38-L63 .
Then we may add new syntaxes to key mappings to assign options to such global "shortcuts" (like https://github.com/gdh1995/vimium-c/wiki/Trigger-commands-in-an-input-box#shortcuts)
Allow Global Access to Some Commands
This PR adds the ability to set up hotkeys that are passed directly to the extension, without needing any of the hot key interceptions in a normal page. Visiting
chrome://extensions/shortcuts
, you see something like the attached image.You need to define these in
manifest.json
, so they are static. I only bothered adding ones that I was interested in for now. If there is interest in adding something like this, I can try to be more complete.This CL also adds a tab jump list (see
:h jumplist
in vim), which lets you move back and then forward through previously visited tabs. That isn't really related to this PR, but separating them cleanly before knowing if there's interest was more work than I wanted to do.Some related issues are #3367, #3279, #3254, and #3190.
@gdh1995 has had some comments on this and might know more, including here.