Ticket #3640 (closed defect: fixed)

Opened 8 years ago

Last modified 8 years ago

Subshell cursor position lost after window resizing

Reported by: yurikhan Owned by: andrew_b
Priority: major Milestone: 4.8.17
Component: mc-tty Version: 4.8.15
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

To reproduce:

  1. Start xterm.
  1. Start tmux with default configuration:
(xterm)$ tmux -f /dev/null
  1. Start mc.
(tmux)$ mc
  1. Hide panels (Ctrl+O).
  1. Resize the xterm window.
  1. Note the cursor position.
  1. Switch panels on and off again (Ctrl+O Ctrl+O).
  1. Note the cursor position.

Expected behavior: cursor position is the same in steps 6 and 8.

Observed behavior: cursor moves to the top line.

Versions:

  • Ubuntu 16.04
  • tmux 2.1
  • mc 4.8.15 compiled with libslang2 2.3.0
$ mc -V
GNU Midnight Commander 4.8.15
Built with GLib 2.47.3
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm and Linux console
With support for X11 events
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ext2undelfs, ftpfs, sftpfs, fish
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

Attachments

0001-Ticket-3640-Do-not-duplicate-alternate-screen-switch.patch (8.1 KB) - added by yurikhan 8 years ago.

Change History

comment:1 Changed 8 years ago by yurikhan

Cause:

  • Functions do_enter_ca_mode and do_exit_ca_mode (in lib/tty/win.c) use the ESC [ ? 4 7 h and ESC [ ? 4 7 l sequences to switch to and from the alternate screen, and ESC 7 and ESC 8 to save and restore the cursor position.
  • SLsmg_init_smg and SLsmg_reset_smg (in slang) use the smcup and rmcup sequences to switch to and from the alternate screen and save and restore the cursor position.
  • Both facilities are called from Midnight Commander on startup, shutdown and when toggling panels.
  • Additionally, do_enter_ca_mode is called from dialog_change_screen_size which gets called after window resize.

So what happens, chronologically:

  • User resizes window while panels are off
    • sigwinch_handler sets winch_flag to 1
  • User toggles panels on
    • do_enter_ca_mode saves correct cursor position (ESC 7)
    • do_enter_ca_mode switches to the alternate screen without saving the cursor position (ESC [ ? 4 7 h)
    • SLsmg_init_smg requests to switch to the alternate screen and save the cursor position (ESC [ ? 1 0 4 9 h). Since the alternate screen is already active, tmux ignores this request.
    • update_panels re-reads directories. As part of this, cursor is moved to the top right corner and rotating dash displayed and erased there.
    • Finally (for toggle_panels), winch_flag is handled by calling dialog_change_screen_size, which resets winch_flag, calls tty_change_screen_size, and then calls do_enter_ca_mode again.
    • do_enter_ca_mode overwrites the previously saved correct cursor position with that in the top right corner.
    • do_enter_ca_mode requests to switch to the alternate screen without saving the cursor position. This is also a no-op, because the alternate screen is already active.
  • User toggles panels off
    • SLsmg_reset_smg switches to the normal screen and restores the cursor position (ESC [ ? 1 0 4 9 l). Since the last successful switch to the alternate screen did not save the position, cursor position may now be bogus.
    • do_exit_ca_mode requests to switch to the normal screen without restoring the cursor position (ESC [ ? 4 7 l). This is a no-op, because the normal screen is already active.
    • do_exit_ca_mode restores the last saved cursor position, which was in the top right corner.
    • Prompt is redisplayed at the start of the line with cursor, which is the top line.

I propose that at least in the slang build the functions do_enter_ca_mode and do_exit_ca_mode should do nothing. A patch will follow shortly.

comment:2 Changed 8 years ago by andrew_b

Indent the library-specific #ifdef into library-independent code is not good way. do_enter_ca_mode and do_exit_ca_mode should be reimplemented like other library-specific tty functions.

Version 0, edited 8 years ago by andrew_b (next)

comment:3 follow-up: ↓ 4 Changed 8 years ago by yurikhan

I agree but I’d like for someone to confirm that this code is in fact needed in the ncurses build. If not, we could remove these two functions altogether.

comment:4 in reply to: ↑ 3 Changed 8 years ago by andrew_b

Replying to yurikhan:

I agree but I’d like for someone to confirm that this code is in fact needed in the ncurses build.

With ncurses (5.9 in my case) and without this code, cursor moves to the 1st column in the prompt line:

  1. Run mc
  2. Press ctrl-o to switch to subshell. Everything looks fine.
  3. Press ctrl-o to return to panels
  4. Press ctrl-o to switch to subshell again. Cursor is in the 1st column.
  5. And in all following switches to subshell cursor will be in the 1st column.

comment:5 Changed 8 years ago by yurikhan

Yes, I see that. The second time panels are hidden, there is no prompt.

Would it be okay to move the existing functions do_enter_ca_mode and do_exit_ca_mode into tty-ncurses.c, renaming them tty_enter_ca_mode and tty_exit_ca_mode, and provide empty implementations with the same names in tty-slang.c? Do I just delete calls to them from within tty-slang.c, or keep calls to known empty functions?

comment:6 Changed 8 years ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.17

I split the patch in two commits.

Branch: 3640_subshell_cursor_position
Initial changeset:fb5d1317df39a7a2d27214990c79ea2f58f9556e

comment:7 Changed 8 years ago by yurikhan

Thanks. Please see also #3639 — it is related and arguably more severe than this one.

comment:8 Changed 8 years ago by zaytsev

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

comment:9 Changed 8 years 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: [7fd0f172ef8b1a6cca5f6162764198aa97c7a6ba].

git log --pretty=oneline c4a8882..7fd0f17

comment:10 Changed 8 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.