Ticket #3136 (new defect)

Opened 10 years ago

Last modified 2 years ago

Numpad 5 ("Begin") freezes mc for a while

Reported by: egmont Owned by:
Priority: minor Milestone:
Component: mc-tty Version: 4.8.11
Keywords: Cc: gotar@…
Blocked By: Blocking:
Branch state: no branch Votes for changeset:

Description

With numlock off, press the numpad 5. It is supposed to do nothing. Instead, it makes mc freeze for a second or two, not accepting any keyboard input.

Same happens with Ctrl + function keys under xterm, and probably other key combos too.

mc should recognize out of the box all the escape sequences that keys can generate with the most typical hardware, os, terminal emulator combinations and quietly ignore them so that it does not block on them. At least the numpad 5, because that's a single key, one can very easily accidentally hit it without any modifiers, and expect not to have to wait for seconds to continue the work.

Ideally there should also be a way (in config file, and preferably in the Learn Keys dialog too) to teach mc an arbitrary number of escapes that it recognizes but ignores.

Change History

comment:1 Changed 10 years ago by egmont

The behavior changed in mc 4.8.9, by ticket #2988.

Previously, numpad 5 (with numlock off) emitted an 'E' and you could immediately continue using mc, e.g. numpad 2 or 8 would move the cursor up and down, backspace would erase the undesired 'E', and so on.

Now, you have to press many keys to get out of this state, or wait for about a second or two without pressing anything. For example, you might press the Enter key about 20 times with the rate of about 1/sec, and still nothing happens. This very easily leads to the misbelief that mc has frozen, maybe cause the user to agressively press keys without waiting for feedback after each and every one, eventually (when mc comes back to life) maybe give it commands that the user didn't mean to execute, causing mc to do undesired things.

I firmly disagree with this change. In my opinion, emitting garbage characters is acceptable (although undesired), whereas hanging for many seconds, making the user believe that mc has totally stopped working is not acceptable. I strongly recommend that change to be reverted.

Even if you decide not to revert that change, it should further be improved to recognize characters that can't be part of escape characters (e.g. newline, space(?), the escape character itself) and immediately synchronize: drop the unrecognized sequence and immediately process that new character or new valid escape sequence. That way, when you press numpad 5 followed by for example the Up arrow or Enter, mc would immediately process the Up or the Enter action.

Nevertheless, in addition to this change, mc should really recognize the escape sequence of numpad 5 and know that it requires no action to be taken.

Version 0, edited 10 years ago by egmont (next)

comment:2 Changed 10 years ago by vda

Now, you have to press many keys to get out of this state, or wait for about a second or two without pressing anything.

This is the patch which should be causing this:

https://mail.gnome.org/archives/mc-devel/2012-October/msg00009.html

When mc sees "ESC ch1 ch2 ch3" and does not recognize it as a known ESC sequence, it reads and discards all chars:

        if (bad_seq)
        {
            /* This is an unknown ESC sequence.
             * To prevent interpreting its tail as a random garbage,
             * eat and discard all buffered and quickly following chars.
             * Small, but non-zero timeout is needed to reconnect
             * escape sequence split up by e.g. a serial line.
             */
            int paranoia = 20;
            while (getch_with_timeout (old_esc_mode_timeout) >= 0 && --paranoia != 0)
                ;
            goto nodelay_try_again;
        }

The intent of the code is to drop out of while loop as soon as there is no more input to drop.

The patch as I submitted it was

+                    int paranoia = 20;
+                    while (getch_with_timeout (50*1000) >= 0 && --paranoia != 0)
+                        continue;

that is, it was using not old_esc_mode_timeout (which by default is 1000 milliseconds = 1 second)
but much shorter 50 millisecond delay. This made while() loop exit imperceptibly fast.

How about reinstating 50*1000 here?
(It looks like comment needs to be tweaked so that people won't try to "fix" in again).

Last edited 10 years ago by andrew_b (previous) (diff)

comment:3 Changed 10 years ago by vda

it should further be improved to recognize characters that can't be part of escape characters (e.g. newline, space(?), backspace, tab, the escape character itself...) and immediately synchronize

With 50 millisecond delay in "discard garbage" code reinstated, it is quite challenging to press the next key so fast right after a bogus key so that it gets eaten too. I can do it, with difficulty, only when I try to achieve it on purpose.

When I do achieve it, the effect is that the next key get "eaten" too - hardly a disaster.

I don't think we need to add more code here, unless you can demonstrate that a problem does exist in practice here.

comment:4 Changed 10 years ago by egmont

I'm not a great fan of this whole timeout business - I don't see it guaranteed that various layers of transportation (e.g. ssh) can't split a valid escape sequence apart with a lag that'd cause mc to misinterpret that. (Well, on the other hand, as long as you type with normal human speed probably I guess there's no reason for any transport layer to split any sequence into multiple TCP packets or something similar.)

If you accidentally press numpad 5 and 8 together at roughly the same time, the sequence for 8 (Up) may not be sent due to 5's unknown sequence blocking it. But it's just the same as if you've accidentally hit 5 only, so it's not a big deal either.

That being said, I'm not super happy with the approach, but sounds like a reasonable compromise and saves development efforts.

comment:5 Changed 10 years ago by gotar

  • Cc gotar@… added

comment:6 Changed 8 years ago by andrew_b

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

comment:7 Changed 3 years ago by zaytsev

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

comment:8 Changed 2 years ago by andrew_b

  • Component changed from mc-core to mc-tty
  • Milestone Future Releases deleted
Note: See TracTickets for help on using tickets.