Ticket #3697 (closed defect: fixed)

Opened 10 months ago

Last modified 3 months ago

ctrl+\ (Directory hotlist) broken

Reported by: Mihail Zenkov Owned by: andrew_b
Priority: major Milestone: 4.8.20
Component: mc-tty Version: master
Keywords: Cc: retnyg@…, dickey@…, egmont, andrey.gursky@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

ctrl+\ (Directory hotlist) broken on linux. Regression after http://www.midnight-commander.org/changeset/38d4c655d322837574e957b4a824f4a0d1bb3b86

Change History

comment:1 Changed 10 months ago by zaytsev

  • Cc dickey@… added

Hi Thomas,

I can confirm that this patch causes Ctrl+\ to generate SIGQUIT with ncurses on Linux, instead of freeing this key for use in mc. I'm not sure why this is happening; we've got the patch from NetBSD curses maintainer claiming that this is the way to do it to remain compatible both with ncurses and NetBSD curses by relying on the official interface instead of private implementation details.

Could you please be so kind to have a look and maybe tell us how to do it the right way? You are definitively the authority on this issue, and hopefully that would only take you a couple of moments.

Many thanks!

comment:2 Changed 10 months ago by egmont

  • Cc egmont added

FYI: mc-4.8.18 on Ubuntu Yakkety beta; any of xterm / gnome-terminal / Linux console: Ctrl+\ brings up the directory hotlist dialog for me as expected.

comment:3 Changed 10 months ago by egmont

/me stupid. Forget my previous comment. I was building with slang. My bad.

Indeed I can confirm this bug.

comment:4 Changed 10 months ago by dickey

Control-backslash is normally the quit character (

stty -a
speed 38400 baud; rows 40; columns 80; line = 0;
intr = ^C; quit = ^\; erase = ^H; kill = ^U; eof = ^D; eol = <undef>;
eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R;
werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0;
-parenb -parodd cs8 -hupcl -cstopb cread -clocal -crtscts
-ignbrk -brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr icrnl ixon -ixoff
-iuclc -ixany -imaxbel -iutf8
opost -olcuc -ocrnl onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0
isig icanon iexten echo echoe echok -echonl -noflsh -xcase -tostop -echoprt
echoctl echoke

).

ncurses sets up signal handlers during initscr/newterm

It sounds as if ncurses does its initialization for this after midnight commander.

comment:5 Changed 10 months ago by zaytsev

Hmmmm, the call to initscr() is right above the code in question that tries to override the signal handlers.

Click here to see the functions in full:

I would hate it to have to hardcode an ifdef to distinguish ncurses and NetBSD curses :-(

comment:6 Changed 10 months ago by ag

  • Cc andrey.gursky@… added

Changeset [1] claims:

The code that manipulates the ncurses backend into changing
the key combination to generate SIGINT from CTRL-c to CTRL-g does
so by accessing undocumented internal ncurses data structures.

Is that true? ncurses has curses.priv.h, but the datastructure in question (struct term) and cur_term are in public /usr/include/term.h and I see no comment, that it is only for private use.

[1] http://www.midnight-commander.org/changeset/38d4c655d322837574e957b4a824f4a0d1bb3b86

comment:7 follow-up: ↓ 8 Changed 10 months ago by dickey

Just reading the ncurses source, the indicated change will not work because in the calls to raw(), etc., ncurses uses the values from cur_term to make updates - not simply the current state of the terminal.

I'm looking at this chunk -

https://github.com/ThomasDickey/ncurses-snapshots/blob/master/ncurses/tinfo/lib_raw.c#L93

which uses this assignment:

buf = termp->Nttyb;

Agreeing with the comments about private data structures, NetBSD has made private a number of data items which are publicly accessible in other versions of curses, and has added functions for accessing those (but not done a complete job of allowing access - form/menu libraries have problems for portability). I've a page giving my perspective:

http://invisible-island.net/ncurses/ncurses-netbsd.html

So it looks as if you need an ifdef: to handle NetBSD where it differs from other versions of curses.

Last edited 10 months ago by dickey (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 10 months ago by andrew_b

Replying to dickey:

So it looks as if you need an ifdef: to handle NetBSD where it differs from other versions of curses.

If I understand you correctly, we need something like

#ifdef BSDCURSES
...
#else /* other curses */
...
#endif

not

#ifdef NCURSES
...
#else /* other curses */
...
#endif

ncurses provides the NCURSES_VERSION macro which identifies the ncurses library. I can't find such unique identifier for NetBSD curses.

comment:9 Changed 9 months ago by zaytsev-work

So, since there haven't been any feedback from rofl0r so far, my suggestion would be to simply revert this commit and wait until someone comes back with a proper fix.

As it appears, the only viable solution is to introduce conditionals, but I'd be unhappy to put the condition on curses. It would be much better to put a conditional on NetBSD curses, but as Andrew has pointed out, there doesn't seem to be an obvious way to detect it, and if they are uncooperative, we should probably just follow the path of least resistance.

comment:10 Changed 9 months ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Version changed from master to 4.8.18
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.19

Branch: 3697_ncurses_bsdcurses
Initial changeset:b77820b9d78862bd21e69f852ba664c876b9d943

comment:11 Changed 9 months ago by zaytsev

  • Votes for changeset set to zaytsev
  • Branch state changed from on review to approved

comment:12 Changed 8 months ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from zaytsev to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [fc3d153b6715d0018e3f83b5fca22a9bf489a39a].

git log --pretty=oneline f3509db..fc3d153

comment:13 Changed 8 months ago by andrew_b

  • Status changed from testing to closed

comment:14 Changed 7 months ago by zaytsev

Ticket #3743 has been marked as a duplicate of this ticket.

comment:15 follow-up: ↓ 16 Changed 4 months ago by dickey

With the upcoming ncurses 6.1, you'll need a different fix, since the enclosing structure is opaque:

diff --git a/lib/tty/tty-ncurses.c b/lib/tty/tty-ncurses.c
index a7a11f368..6e5304ba9 100644
--- a/lib/tty/tty-ncurses.c
+++ b/lib/tty/tty-ncurses.c
@@ -194,14 +194,6 @@ tty_init (gboolean mouse_enable, gboolean is_xterm)
     ESCDELAY = 200;
 #endif /* HAVE_ESCDELAY */

-#ifdef NCURSES_VERSION
-    /* use Ctrl-g to generate SIGINT */
-    cur_term->Nttyb.c_cc[VINTR] = CTRL ('g');   /* ^g */
-    /* disable SIGQUIT to allow use Ctrl-\ key */
-    cur_term->Nttyb.c_cc[VQUIT] = NULL_VALUE;
-    tcsetattr (cur_term->Filedes, TCSANOW, &cur_term->Nttyb);
-#else
-    /* other curses implementation (bsd curses, ...) */
     {   
         struct termios mode;

@@ -212,7 +204,8 @@ tty_init (gboolean mouse_enable, gboolean is_xterm)
         mode.c_cc[VQUIT] = NULL_VALUE;
         tcsetattr (STDIN_FILENO, TCSANOW, &mode);
     }
-#endif /* NCURSES_VERSION */
+    /* curses remembers the "in-program" modes after this call */
+    def_prog_mode();

     tty_start_interrupt_key ();

comment:16 in reply to: ↑ 15 Changed 4 months ago by andrew_b

Replying to dickey:

With the upcoming ncurses 6.1, you'll need a different fix, since the enclosing structure is opaque:

Is this fix correct for ncurses < 6.1?

Last edited 4 months ago by andrew_b (previous) (diff)

comment:17 Changed 4 months ago by dickey

yes, the change should work with any version of ncurses.

comment:18 Changed 3 months ago by zaytsev

  • Status changed from closed to reopened
  • Votes for changeset committed-master deleted
  • Version changed from 4.8.18 to master
  • Branch state changed from merged to no branch
  • Milestone changed from 4.8.19 to 4.8.20
  • Resolution fixed deleted

Reopening so that this doesn't get forgotten. Getting rid of ifdef sounds awesome. Thank you Thomas!

comment:19 Changed 3 months ago by andrew_b

  • Branch state changed from no branch to on review

comment:20 Changed 3 months ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:21 Changed 3 months ago by andrew_b

  • Status changed from reopened to closed
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged
Note: See TracTickets for help on using tickets.