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

Use new CAP() macro for arrow keys #206

Closed
wants to merge 4 commits into from
Closed

Use new CAP() macro for arrow keys #206

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 3, 2017

No description provided.

@mptre
Copy link
Owner

mptre commented Jul 4, 2017

Great work!

Regarding the build failure on macOS: turns out kbs=^H for xterm on
macOS. I would therefore only use \b for backspace in the tests in
order to make all tests pass on all platforms.

@ghost
Copy link
Author

ghost commented Jul 4, 2017

Thanks again @mptre !

@mptre
Copy link
Owner

mptre commented Jul 4, 2017

With this diff applied I can't use the arrow keys on neither OpenBSD nor
macOS. As an example, pressing DOWN emits \e[B while the terminfo
entry equals \eOB. Tested with Terminal.app and xterm with and without
tmux.

Is anyone else seeing this? /cc @calleerlandsson @teoljungberg

@teoljungberg
Copy link
Contributor

My arrow keys broke aswell. Tested on macOS, in Terminal.app, in tmux and outside.

@calleluks
Copy link
Collaborator

Same here in bash in Terminal.app on macOS. Backspace also doesn't work.

Do we need to anything other than calling setupterm the way we do, before we call tigetstr?

@ghost
Copy link
Author

ghost commented Jul 5, 2017

Yes, we do: tty_putp(keypad_xmit, 0); after setupterm will fix this issue with "xterm" ...

@ghost
Copy link
Author

ghost commented Jul 5, 2017

Tested with/without dvtm, tmux and screen in xterm and terminator ... I use rxvt-unicode here, where this issue did not occur ... more testings will follow if needed ...

pick.c Outdated
@@ -649,6 +649,7 @@ tty_init(void)
err(1, "fopen");

setupterm((char *)0, fileno(tty_out), (int *)0);
tty_putp(keypad_xmit, 0);
Copy link
Owner

@mptre mptre Jul 5, 2017

Choose a reason for hiding this comment

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

Nice find! I would prefer grouping the tty_putp calls and make sure to
exit from the same mode.

diff --git a/pick.c b/pick.c
index 2c23aeb..df185b9 100644
--- a/pick.c
+++ b/pick.c
@@ -655,6 +655,8 @@ tty_init(void)
 	if (use_alternate_screen)
 		tty_putp(enter_ca_mode, 0);
 
+	tty_putp(keypad_xmit, 0);
+
 	signal(SIGINT, handle_sigint);
 }
 
@@ -677,6 +679,7 @@ tty_restore(void)
 	tcsetattr(fileno(tty_in), TCSANOW, &original_attributes);
 	fclose(tty_in);
 
+	tty_putp(keypad_local, 0);
 	tty_putp(carriage_return, 1);	/* move cursor to first column */
 	tty_putp(clr_eos, 1);
 

@mptre
Copy link
Owner

mptre commented Jul 5, 2017

Everything works except backspace on macOS, I think we should keep the
following:

KEY(BACKSPACE,  "\177");

I appreciate the effort on adding tests for the Emacs bindings, could
that be a separate PR?

@ghost
Copy link
Author

ghost commented Jul 5, 2017

What the hell is wrong with macOS? ;-)

@mptre
Copy link
Owner

mptre commented Jul 9, 2017

Check out the cap branch, it includes some cleanups and is what I
propose on committing.

@ghost
Copy link
Author

ghost commented Jul 9, 2017

Tested and looks good!

@mptre
Copy link
Owner

mptre commented Jul 10, 2017

Great! Lets wait for another confirmation, I will then go ahead and merge this.

/cc @calleerlandsson @teoljungberg

@calleluks
Copy link
Collaborator

Hey @mptre and @DBOTW, I'm sorry I've been unresponsive; I've been out traveling the last week. The cap branch works well on my machine and looks great to merge! Thanks for your hard work!

@mptre
Copy link
Owner

mptre commented Jul 17, 2017

Thanks, merged!

@mptre mptre closed this Jul 17, 2017
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