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.

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

Add more shortcuts for vimium-like navigation #53

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

Conversation

srsudar
Copy link

@srsudar srsudar commented Mar 14, 2021

This adds the ability to set custom shortcuts for up/down in the picker,
so you can do ctrl-p and ctrl-n as you do with the omnibar and the
vomnibar in vimium. It also adds the ability to set custom shortcut
behavior, so ctrl-[ escapes as it does in vim.

Is this something you'd be interested in merging? For now I'm just playing around with in my private fork. I'd tried QuicKey in the past and gone back to vimium because I missed these shortcuts. Very easy to implement, though.

FWIW I'm not able to build it (at least not with grunt). There is an unrelated error that I also get on master.

This adds the ability to set custom shortcuts for up/down in the picker,
so you can do ctrl-p and ctrl-n as you do with the omnibar and the
vomnibar in vimium. It also adds the ability to set custom shortcut
behavior, so ctrl-[ escapes as it does in vim.
Without this, storage wasn't persisting when reloading the extension.
@fwextensions
Copy link
Owner

Thanks for the PR. I'm impressed you were able to figure out how to add keyboard shortcuts! One thing this is reminding me about is that I don't love how many places you need to touch to add a shortcut. It's something I'd like to clean up.

FWIW I'm not able to build it (at least not with grunt). There is an unrelated error that I also get on master.

I need to make this clear in the readme, but there's nothing that needs to be built during development. You can just load the unpacked extension from the /src directory, thanks to old school RequireJS. Then just reload it after making a change (which can be done by clicking the reload button next to QuicKey on chrome://extensions/ if you want to force the background page to reload as well).

Changing the "build" script in package.json to "grunt build" will let you run the local grunt, or just execute node_modules\.bin\grunt directly. (I realized that Webstorm adds the .bin directory to your path automatically in its embedded terminal, which is why I hadn't noticed it wouldn't work in a regular terminal.) All grunt build does, though, is bundle and minify the code, and outputs it to /build/out, in preparation for zipping it to submit a release to the Chrome store. Again, not necessary for development.

I have a fix for this, and the error in the build:popup script, but I'm currently banging my head against some weird babel behavior that just cropped up, so I haven't checked it in yet.

As for adding new selection keys, I'm a fan of customizable shortcuts, but I'm not entirely sure how these should be handled. The other customizable shortcuts in the extension have just the one binding to perform the action, but select down/up already has the down/up arrow keys assigned to those actions. So should the default shortcuts be the arrow keys, and then adding ctrl-N/P (or whatever else the user chose) would override those (and therefore, the arrow keys would stop working)? Perhaps ctrl-N/P should just be added as additional built-in shortcuts, without a way of customizing them. I need to think about that.

@srsudar
Copy link
Author

srsudar commented Mar 15, 2021

It took me maybe 30-60 minutes to find the relevant shortcut code, but after I had it I was actually very impressed with how clean it was to add them! The only gotcha I found was that the storage validator didn't like them because of the defaults issue. Not a big deal at all, though. Very nice code to work with.

You can just load the unpacked extension from the /src directory

Yep I figured that out from another open issue. I mentioned my grunt failure just so that if you ran it and saw it was broken you wouldn't think it was the PR that had broken it.

So should the default shortcuts be the arrow keys, and then adding ctrl-N/P (or whatever else the user chose) would override those (and therefore, the arrow keys would stop working)? Perhaps ctrl-N/P should just be added as additional built-in shortcuts, without a way of customizing them. I need to think about that.

As-is, both my arrow keys and <c-p>/<c-n> work with this PR. I 100% agree that the arrow keys should keep working, no question.

I could see <c-p>/<c-n> being added as defaults, maybe. The omnibox on mac already supports that behavior, though not on linux without some configuration. I guess you're right that it's kind of an odd thing to want to customize completely, perhaps.

@fwextensions
Copy link
Owner

The omnibox on mac already supports that behavior, though not on linux without some configuration.

Interesting. Windows doesn't support those shortcuts in the omnibox, presumably because they're used for new window and print, respectively. I think I'm inclined towards making them hard-coded, built-in additional shortcuts, which avoids the complexity of a customizable shortcut.

The ctrl-[ shortcut is a little trickier, as it's currently used on all platforms to move a tab to the left of the current tab. How important is that for clearing the query?

@srsudar
Copy link
Author

srsudar commented Mar 16, 2021

For my particular OCD workflow it is very much desired. I like it matching vim (and to a lesser extent vimium). It's a more somewhat standard *nix / terminal binding for escape, I believe. But I'm planning on maintaining my own fork anyways, for the tab jump list behavior I mentioned, so don't feel obligated to keep it on my account.

Do you not like the thought of including a shortcut without a default action?

@fwextensions
Copy link
Owner

It’s more that it’s a little hard to explain in the UI. All the shortcut labels say what the shortcut will do, like close the selected tab or move it. For esc, it would be Clear the search query (or Close the menu, depending on the radio button selection above, which is a further complication), but then you’d expect esc to be the default shortcut. But as currently implemented, you can’t use esc as a shortcut, because pressing it exits the listening state of the shortcut widget. I suppose the label could be something like Clear the search query (alternate shortcut) and not have a default key, but it still feels a bit weird for just that command to have an alternate.

Anyway, the option feels like an outlier, so I’d be inclined to avoid that complexity, especially if you’re thinking of maintaining a fork.

@srsudar
Copy link
Author

srsudar commented Mar 17, 2021

"Act as another escape key" sounds clear to me, but then again I guess it would, since that's what I wrote. It might not even be true, I suppose, if it's only an escape when the search box is open.

Without the tab jump list I will just maintain my own fork, so no hurt feelings if you don't merge.

Also, it looks like on the omnibox on mac <c-n>/<c-p> go up and down, but <c-[> does NOT act as escape in that situation. I'd forgotten that. I have workaround there to close and reclaim focus easily, but it wouldn't be odd if QuicKey didn't support <c-[>.

@srsudar
Copy link
Author

srsudar commented Dec 31, 2021

It's become something of a nuisance to maintain my own fork. Settings seem to un-sync and interfere with each other across different machines.

The other changes I made, liking changing the way the jump list works, I don't think are all that important after all, and I'd be happy to use the main branch.

You mention:

The ctrl-[ shortcut is a little trickier, as it's currently used on all platforms to move a tab to the left of the current tab.

I'm not sure this is actually the case after all. After messing around some with a Windows machine, I don't think <c-[> is a native shortcut for changing tabs. It's definitely not on Mac. I don't think it is on linux either, though I don't have a linux machine handy to test on. So I think it is free and wouldn't be a problem to just add as a default.

With that in mind, any chance you'd reconsider merging something like this? I think it would be:

  • <c-p> as an additional up in the menu
  • <c-n> as an additional down in the menu
  • <c-[> as an additional escape key while in the menu

None of these would be configurable, they'd just be defaults. And the defaults would be natural to people coming from a *nix environment.

@fwextensions
Copy link
Owner

Settings seem to un-sync and interfere with each other across different machines.

I haven't actually tested the settings across different machines with the same account. There are extension APIs for handling synching, but I haven't implemented them, so I assumed each machine's settings would be totally independent. Any more info on the bugs you're seeing would be helpful.

I don't think <c-[> is a native shortcut for changing tabs.

Sorry, I meant it's the default for moving the selected tab to the left while in the QuicKey menu.

image

as an additional up in the menu
as an additional down in the menu

These are already in 1.6.1, along with ctrl-J/K. Let me think about the option for adding another esc key.

@srsudar
Copy link
Author

srsudar commented Jan 1, 2022

Ohhhh duh. I see re <c-[>. I forgot about that b/c I disabled it in favor of my escape remap.

I'm not totally sure what the problem is on syncing. It seems to be something like my customizations just get overwritten by defaults in certain scenarios, though I'm not sure what those scenarios are. I'll update them, things will be stable for a while, and then they get reset.

Great news on the additional up downs! Thanks.

I can see why the additional escape might seem kind of niche. I'll try the default version and see if I can get used to it. Neither Chrome nor the Apple spotlight <cmd+space> search respect <c-[> either, though it is a nuisance.

@fwextensions
Copy link
Owner

It seems to be something like my customizations just get overwritten by defaults in certain scenarios, though I'm not sure what those scenarios are. I'll update them, things will be stable for a while, and then they get reset.

When you say the customizations get reset to the defaults, do you mean just the keyboard shortcuts or other things as well? Does the list of recent tabs disappear when this happens? If recent tabs are cleared, it's likely the whole storage object is being reset, either because Chrome decided to, or the stored data doesn't match the format the code expects.

If you're running a local unpacked extension, there should be an error in the background.html console when the storage is reset, showing the old data object.

As for synching settings across machines, my understanding from the docs is that since QK isn't using chrome.storage.sync.set(), nothing will be synched. So I'd be surprised if setting a shortcut on one machine would set the same shortcut on another, even if the same account is signed into Chrome on both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants