Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Add c-n, c-j, c-p, c-k, c-m to console keybind #162

Merged
merged 5 commits into from
Nov 18, 2017

Conversation

heavenshell
Copy link
Contributor

@heavenshell heavenshell commented Nov 15, 2017

Hi, first thank you for create nice and useful extension!

I add following behaviors to Vim-vixen.

  • c-n, c-j for select next item.
  • c-p, c-k for select previous item.
  • c-m for select item.

These console keybinds are same as Vim(Vimperator)'s completion selector.

Please check and review this and it's very pleasure to give me advices.

Thank you.

Add `c-n`, `c-j` for select next item.
Add `c-p`, `c-k` for select previous item.
Add `c-m` for select item.

Above console keybinds are same as Vim(Vimperator)'s completion
selector.
@idlewan
Copy link
Contributor

idlewan commented Nov 15, 2017

This is great, I was wondering how to use that list, I didn't notice that you had to use the tab key.
In addition to all of these, the up/down arrow keys should probably also be working to select lines.

@heavenshell
Copy link
Contributor Author

In addition to all of these, the up/down arrow keys should probably also be working to select lines.

ah, right. I'll add later 👍

@heavenshell
Copy link
Contributor Author

Add up/down arrow keys for select next/prev item ☺️

e.stopPropagation();
e.preventDefault();
return this.onEntered(e.target.value);
this.doEnter(e);
Copy link
Contributor

@idlewan idlewan Nov 16, 2017

Choose a reason for hiding this comment

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

Does this need a return as well?
I think returning anything from a listener function (the ones passed to addEventListener) is deprecated behavior, so probably no need to change anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch! thx!

Copy link
Contributor Author

@heavenshell heavenshell Nov 16, 2017

Choose a reason for hiding this comment

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

oh, I added it and missed to read your fixed comment :-p
Original code have return.
I think we should confirm to @ueokande .

@ueokande
Copy link
Owner

@heavenshell Thanks for your PR.
Your patch is not acceptable. I agree to use <C-N> and <C-P> to focus next and previous items and <C-M> to select item. They are also used in Vim.
In my experience, <C-J>, <C-K>, and arrow keys have not been commonly used for select items in console. What is a reason for tha decision to use these keys?

@heavenshell
Copy link
Contributor Author

heavenshell commented Nov 18, 2017

@ueokande Thank you for review my PR!

In my experience, , , and arrow keys have not been commonly used for select items in console. What is a reason for tha decision to use these keys?

arrow key is also Vim's completion selector behavior. So I added.

$ vim --clean
i<c-x><c-f><arrow down>

<c-j>, <c-k> are not Vim's default behavior but Vimperator's behavior.
But I can understand <c-j>, <c-k> is not common behavior.
So, if I drop <c-j>, <c-k>, could you accept this?

Because these are not Vim's default behavior.
@heavenshell
Copy link
Contributor Author

Drop <C-j>, <C-k> 😊

@ueokande
Copy link
Owner

Arrow keys seem completion from command-line history in Vim. I have a plan to implement is in the future. Please drop arrow keys from key maps.

@heavenshell
Copy link
Contributor Author

@ueokande

Arrow keys seem completion from command-line history in Vim. I have a plan to implement is in the future. Please drop arrow keys from key maps.

This is so nice and make sense!
Drop arrow keys from console 👍

@ueokande
Copy link
Owner

@heavenshell In my environment (Linux, Firefox 58 developer edition), <C-N> does not work to select next item in completion. The new tab is opened by Firefox's key binding. How do I solve it?

@heavenshell
Copy link
Contributor Author

@ueokande Thank you for telling me this.
Oh, I usally use macOS and did't notice about confilict with Firefox's keybind.

It's better to do followings

  • if platform is Linux and state is opening console than ignore OS's default keybinds

IMHO, this is very difficult(I'm sorry, I'm WebExtensions newbie).

I'll continue to investigate this, but do you have any idea?
Sorry for bothering you...

P.S.
I found Shift-Ctrl-n move next at Linux(Ubuntu) + Firefox57...

@ueokande
Copy link
Owner

ueokande commented Nov 18, 2017

@heavenshell
I understood a situation on <C-N> binding issue. Some Firefox's short-cut keys are not overridden by WebExtensions. It is already reported in Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1320332

I would merge your PR, and register known problem in issues. I wish Mozilla will resolve it. 🙏
Thank you.

@ueokande
Copy link
Owner

#200

Copy link
Owner

@ueokande ueokande left a comment

Choose a reason for hiding this comment

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

It works

@ueokande ueokande merged commit bdae21b into ueokande:master Nov 18, 2017
@heavenshell heavenshell deleted the feature/console_key branch November 18, 2017 13:56
@heavenshell
Copy link
Contributor Author

Thank you so much including this!!

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

Successfully merging this pull request may close these issues.

3 participants