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

Disable ncurses extended key values so that esc-codes can be used in … #1049

Closed
wants to merge 1 commit into from

Conversation

mckellygit
Copy link
Contributor

@mckellygit mckellygit commented Nov 13, 2020

…mappings for Shift/Ctrl/Alt Up/Dn/Lt/Rt/Hm/En/In/De/PgUp/PgDn

This is the other way to go, as compared to PR #1048

thx,
-m

…mappings for Shift/Ctrl/Alt Up/Dn/Lt/Rt/Hm/En/In/De/PgUp/PgDn
koutcher pushed a commit that referenced this pull request Dec 22, 2020
This makes it possible to use esc-codes in mappings for Shift/Ctrl/Alt
Up/Down/Left/Right/Home/End/Insert/Delete/PgUp/PgDn.

[tk: minor tweaks and NCURSES_EXT_FUNCS was only made visible in ncurses
 v5_6_20071117, so use NCURSES_VERSION instead for compatibility.]

References #1046
@koutcher
Copy link
Collaborator

Merged manually in aee8eb1.

@koutcher koutcher closed this Dec 22, 2020
@mckellygit
Copy link
Contributor Author

@koutcher thx. Something is weird tho, in my original PR a binding such as:

bind generic   <Esc>[1;5B   scroll-line-down

worked fine - but now it does not, and I need to use:

bind generic   <Ctrl-[>[1;5B   scroll-line-down

I will dig into it and post what I find.

@mckellygit
Copy link
Contributor Author

mckellygit commented Dec 23, 2020

@koutcher its a problem with my other commit that was merged and the values for Esc and C-_
I'll post a PR to fix it asap.
I think we just need an additional && key_value != KEY_ESC added.

@mckellygit
Copy link
Contributor Author

ok, PR added: #1058

@koutcher
Copy link
Collaborator

koutcher commented Dec 23, 2020

Good catch. I should have spotted that during the review. I was so focused on the possible side-effect of changing the < in <= that I was happy enough when I checked that CTRL('z') was handled separately and I completely overlooked that when going from 25 to 30, 27 was also invited to the party. Hopefully 28 is only ^\.

@mckellygit
Copy link
Contributor Author

mckellygit commented Jan 22, 2021

@koutcher @jonas just an fyi - I was working with xaizek on vifm for this same issue and he suggested a cleaner way to solve this.
We added this code -

#if defined(NCURSES_EXT_FUNCS) && NCURSES_EXT_FUNCS >= 20081102
	/* Disable 'unsupported' extended keys so that esc-codes will be
	 * received instead of unnamed extended keycode values (> KEY_MAX).
	 * These keys can then be mapped with esc-codes in rc files.
	 * An example could be <c-down>:
	 *     bind generic <esc>[1;5B scroll-line-down
	 * as without this change <c-down> can return a keycode value of 531
	 * (terminfo name kDN5), which is larger than KEY_MAX and has no
	 * pre-defined curses key name.
	 * NOTE: this MUST be called before initscr() */
	use_extended_names(false);
#endif

just BEFORE initscr() is called.

I just thought I'd mention it. If you want I can create a PR to remove the keyok() loop and use this instead ?
I tried this before, calling it before keypad(), but that didn't work. We just discovered it has to be called before initscr() to be effective.

thx,
-m

@koutcher
Copy link
Collaborator

It is much cleaner indeed. Why 20081102 ? NCURSES_EXT_FUNCS macro was only made visible in ncurses v5_6_20071117 while use_extended_names appeared in v4_2_990301, so using #ifdef NCURSES_VERSION instead would allow the last CentOS 5 users to also benefit from the PR.

@mckellygit
Copy link
Contributor Author

Ok sure, NCURSES_VERSION is good - I just used an old version thinking it was old enough.
I should have copied the same macro name you used before.
I'll create a PR

@mckellygit
Copy link
Contributor Author

Created #1074

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.

3 participants