Ticket #2988 (closed defect: fixed)

Opened 6 years ago

Last modified 6 years ago

When an unknown key is pressed, it is interpreted as garbage. It's better to interpret such keys as "nothing was pressed"

Reported by: vda Owned by: slavazanko
Priority: major Milestone: 4.8.9
Component: mc-tty Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

For example, Ctrl-Alt-Shift-Right_Arrow acts as if I pressed "1;8C" keys.

We need to ignore ALL bytes, not just a few of them, if we saw ESC byte, then, in rapid succession, some bytes which do not constitute a known key.

Patches were sent to ML with subject line "keyboard input: stop generating garbage input from unknown ESC sequences"

Change History

comment:1 Changed 6 years ago by slavazanko

  • Status changed from new to accepted
  • Owner set to slavazanko

comment:2 Changed 6 years ago by slavazanko

Hi Denys,

As i see, first patch in your patch-set is https://mail.gnome.org/archives/mc-devel/2012-October/msg00004.html

Is the patch is empty or something is wrong?

Thanks.

comment:3 Changed 6 years ago by vda

Is the patch is empty or something is wrong?

No, nothing is wrong.
"[PATCH 0/nnn]" email is used to precede a patch set with an overall explanation what entire set achieves (as opposed to per-patch comments which explain what each individual patch is doing).
"[PATCH 0/nnn]" email does not contain any patches. They start from "[PATCH 1/nnn]".

comment:4 Changed 6 years ago by slavazanko

  • Component changed from mc-core to mc-tty
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.9

Created branch 2988_ignore_unknown_esc_sequences

Initial changeset:0552611557e9835eccdf40b7b5b66bf04b6b8bf1

Review, please.

comment:5 Changed 6 years ago by slavazanko

MC produces garbage when we will press ESC and next Home or End key
For example, Esc,Home produces:

OH

comment:6 Changed 6 years ago by angel_il

  • Votes for changeset set to angel_il

comment:7 Changed 6 years ago by andrew_b

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

comment:8 Changed 6 years ago by slavazanko

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

Merged to master:

git log --pretty=oneline f09b319..664e7d3

comment:9 Changed 6 years ago by slavazanko

  • Status changed from testing to closed

comment:10 Changed 6 years ago by egmont

Please see #3136 for followup.

comment:11 Changed 6 years ago by vda

Patch has been changed a bit: "discard garbage" bit of code was

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

in my patch, but in mc-4.8.11 it is

                    int paranoia = 20;
                    while (getch_with_timeout (old_esc_mode_timeout) >= 0 && --paranoia != 0)
                        ;

This is wrong. The timeout value here controls how many microseconds mc waits to eat every next "bogus char". It must be big enough to work on serial lines, where it looks like this:

ESC ...<a few ms>... CH1 ...<a few ms>... CH2 ...<a few ms>...<nothing>

therefore I picked 50 ms as it should work even on stone-age ~1000 baud serial lines; yet, it must not be too big!
Else, a human pressing bogus key and then, relatively swiftly, pressing a valid key is:

ESC CH1 CH2 ...<quarter second of confusion>... VALID_CHAR

The code in 4.8.11 will eat VALID_CHAR too, because it is within old_esc_mode_timeout (1 second by default).

This what caused ticket #3136 to be created...

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

comment:12 Changed 6 years ago by ossi

the idea to use old_esc_mode_timeout was based on the assumption that it would be configured correctly for the expected maximal line delay between adjacent packets (which may be a lot longer than 50ms). if one configures it for the latency of a human pressing esc, the delay becomes unreasonably long for "real" sequences, but there is little that can be done about that from the timing side. the idea of "resetting" sequences when the next esc is received seems reasonable (i wouldn't make assumptions about any other characters).

comment:13 Changed 6 years ago by vda

the idea to use old_esc_mode_timeout was based on the assumption that it would be configured correctly for the expected maximal line delay between adjacent packets (which may be a lot longer than 50ms).

I have doubts there are non-degenerate cases where bytes from a single key sequence end up being split across e.g. ssh packets.

How about changing default value of old_esc_mode_timeout to a less paranoidally large value of 1000 ms? e.g. to 100 ms?

Note: See TracTickets for help on using tickets.