Ticket #2662 (closed enhancement: fixed)

Opened 12 years ago

Last modified 11 years ago

Support extended mouse clicks beyond 223

Reported by: egmont Owned by: slavazanko
Priority: minor Milestone: 4.8.1
Component: mc-tty Version: 4.8.0
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: commited-master

Description

The ancient way of reporting mouse coordinates only supports coordinates up to 231, so if your terminal is wider (or taller, but that's unlikely), you cannot use your mouse in the rightmost columns.

There are two (doh) new extensions to report mouse coordinates beyond 231. It would be nice to add support for them to MC.

I'm planning to come up with a patch as my time permits.

See technical details in the next comment...

Attachments

xterm-276-urxvt-mouse-mode.patch (9.7 KB) - added by egmont 12 years ago.
mc-4.8.0-extended-mouse-coordinates.patch (7.3 KB) - added by egmont 12 years ago.
putty-0.61-extended-mouse.patch (2.4 KB) - added by egmont 12 years ago.
mc-4.8.0-extended-mouse-broke-ctrl-space.patch (3.3 KB) - added by egmont 12 years ago.
fix for the ctrl+space bug

Change History

comment:1 Changed 12 years ago by egmont

  • The old way of reporting mouse coordinates is the following:
    • Output DECSET 1000 to enable mouse
    • Expect escape sequences in the format \e[M<action+32><x+32><y+32> whereas <action+32>, <x+32> and <y+32> are single bytes. (Action is 0 for left click, 1 for middle click, 2 for right click, 3 for release, or something like this.)
    • Disadvantages of this format:
      • x and y can only go up to 231.
      • Coordinates above 95 are not ascii-compatible, so any character set converting layer (e.g. luit) messes them up.
      • The stream is not valid UTF-8, even if everything else is
  • The first new extension, introduced by xterm-262, is the following:
    • Output DECSET 1000 to enable mouse, followed by DECSET 1005 to activate extended mode.
    • Expect escape sequences in the format \e[M<action+32><<x+32>><<y+32>> whereas <<x+32>> and <<y+32>> each can be up to two bytes long: coordinate+32 is encoded in UTF-8.
    • Disadvantates of this format:
      • There's still a limit of 2015 rows/columns (okay, it's not a real life problem).
      • Doesn't solve the luit issue.
      • It is "horribly broken" (quoting urxvt's changelog) in terms of compatibility with the previous standard. There is no way for an application to tell whether the underlying terminal supports this new mode (whether DECSET 1005 did actually change the behavior or not), but depending on this a completely different user action might generate the same input. Example:
        • If the terminal doesn't support this extension, then clicking at (162, 129) generates \e[M<32><194><161>
        • If the terminal supports this extension, then clicking at (129, 1) [bit of math: 129+32 = 161, U+0161 in UTF-8 is 194 161] generates \e[M<32><194><161><33>
        • so there's no way to tell whether the terminal ignored the 1005 escape sequence, the user clicked on (162, 129) and then typed an exclamation mark; or whether the terminal recognized the escape, and the user clicked on (129, 1).
      • Due to this horrible brokenness, there's no way to implement support it without explicitly asking the user (via a setting) if the terminal can speak this extension.
  • The second new extension, introduced by rxvt-unicode-9.10, is the following:
    • Output DECSET 1000 to enable mouse, followed by DECSET 1015 to activate this extended mode.
    • Expect escape sequences in the format \e[{action+32};{x};{y}M where this time I used the braces to denote spelling out the numbers in decimal, rather than using raw bytes
    • The only thing I don't understand is why they kept the offset of 32 at action, but other than that, this format is totally okay, and solves all the weaknesses of the previous ones.

Currently, at least the following terminal emulators have support for these:

  • xterm supports the xterm extension
  • rxvt-unicode supports both extensions
  • iterm2 supports both extensions

I'll file feature requests for the authors of other well-known terminals (including xterm) to implement at least the urxvt extension.

Given the reasons above how horribly broken the xterm extension is, my plan is to implement support for the urxvt extension only in MC. I'll come back as soon as I have a patch.

comment:2 Changed 12 years ago by andrew_b

  • Component changed from mc-core to mc-tty

This ticket looks like duplicate of #2399.

comment:3 Changed 12 years ago by egmont

Oh yes you're right, I didn't find that one.
I know it's impolite to mark the original one as the dupe of the new one, but given the amount of technical information in the tickets, could you please make an exception and mark that one as a duplicate of this? (Sorry, gms, and thanks for the original report!)

comment:4 Changed 12 years ago by andrew_b

  • Blocking 2399 added

I've marked this ticket as blocking of #2399, and we'll close both tickets at the same time.

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

comment:5 Changed 12 years ago by egmont

First patch attached, please take a look :)

Expected behavior:

  • In urxvt and iterm2 (and maybe other terminals which support the urxvt extension): mouse works flawlessly even beyond column 232.
  • In other terminals (including xterm which only supports the brain-damaged extension): no user-visible change, that is, mouse still only works up to column 231.

Right now I could only test it on iterm2, but urxvt really should behave the same.

comment:6 Changed 12 years ago by egmont

  • Summary changed from Support extended mouse clicks beyond 231 to Support extended mouse clicks beyond 223

So apparently 255-32 = 223, and not 231. Sorry it seems I can't do maths :D

comment:7 Changed 12 years ago by angel_il

key.c:938:46: warning: declaration shadows a variable in the global scope
      [-Wshadow]
parse_extended_mouse_coordinates (const int *keys)
                                             ^
key.c:508:17: note: previous declaration is here
static key_def *keys = NULL;

comment:8 Changed 12 years ago by angel_il

patch for xterm to allow generate mouse ESC-sequence beyond 223 col.

diff -r button.c button.utf-8-fix.c
--- a/button.c  Sat Aug 14 08:23:00 2010 +0200
+++ b/button.c  Thu Aug 26 16:16:48 2010 +0200
@@ -3994,1 +3994,27 @@
-#define MOUSE_LIMIT (255 - 32)
+#define MOUSE_LIMIT (2047 - 32)
+#define MOUSE_UTF_8_START (127 - 32)
+
+static unsigned
+EmitMousePosition(Char line[], unsigned count, int value)
+{
+    /* Add pointer position to key sequence
+     * 
+     * Encode large positions as two-byte UTF-8 
+     *
+     * NOTE: historically, it was possible to emit 256, which became
+     * zero by truncation to 8 bits. While this was arguably a bug,
+     * it's also somewhat useful as a past-end marker so we keep it.
+     */
+    if(value == MOUSE_LIMIT) {
+       line[count++] = CharOf(0);
+    }
+    else if(value < MOUSE_UTF_8_START) {
+       line[count++] = CharOf(' ' + value + 1);
+    }
+    else {
+       value += ' ' + 1;
+       line[count++] = CharOf(0xC0 + (value >> 6));
+       line[count++] = CharOf(0x80 + (value & 0x3F));
+    }
+    return count;
+}
@@ -4001,1 +4027,1 @@
-    Char line[6];
+    Char line[9]; /* \e [ > M Pb Pxh Pxl Pyh Pyl */
@@ -4021,2 +4047,0 @@
-    else if (row > MOUSE_LIMIT)
-       row = MOUSE_LIMIT;
@@ -4028,1 +4052,5 @@
-    else if (col > MOUSE_LIMIT)
+
+    /* Limit to representable mouse dimensions */
+    if (row > MOUSE_LIMIT)
+       row = MOUSE_LIMIT;
+    if (col > MOUSE_LIMIT)
@@ -4090,2 +4118,2 @@
-       line[count++] = CharOf(' ' + col + 1);
-       line[count++] = CharOf(' ' + row + 1);
+       count = EmitMousePosition(line, count, col);
+       count = EmitMousePosition(line, count, row);

comment:9 Changed 12 years ago by angel_il

so... Konsole, gnome Termital do not support this feature...

comment:10 Changed 12 years ago by egmont

The xterm patch you inlined made it into xterm-262, and only supports the broken extension (which is not compatible with my MC patch). I made a patch yesterday to add urxvt-extension to xterm and sent it to the author; for the record I also attach it here.

gnome-terminal patch is here: http://bugzilla.gnome.org/show_bug.cgi?id=662423

I've also sent a feature request (without patch) to PuTTY, and I'm planning to file a feature request for Konsole too.

Changed 12 years ago by egmont

Changed 12 years ago by egmont

comment:11 Changed 12 years ago by egmont

Patch updated to address the warning of comment 7.

comment:12 Changed 12 years ago by egmont

And just for reference, the Konsole ticket is here: http://bugs.kde.org/show_bug.cgi?id=285984

Changed 12 years ago by egmont

comment:13 Changed 12 years ago by egmont

(A word about GNU Screen: It seems that screen doesn't forward any escape sequence it is not familiar with, including the request to switch to the urxvt 1015 mode. This means that MC with this urxvt patch should make no difference inside screen: mouse will still only work up to 223. This is good news for me because I was afraid that something might break inside screen, but luckily nothing breaks. I'll see if I can patch screen to properly forward the urxvt 1015 request, as well as the new style coordinates.)

comment:15 Changed 12 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Votes for changeset set to andrew_b
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.1

Thanks. Applied with trivial changes.

Branch: 2662_extended_mouse_clicks (parent: master).
changeset:ab9a66406a684d89e26bd14ce4cf90fa28e11c3b

But I'd note that urxvt >= 9.10 is required to support mouse clicks beyond 223.

comment:16 Changed 12 years ago by egmont

Thanks Andrew!

Yes currently urxvt >= 9.10 or iTerm2 is needed. Hopefully other terminals will follow, e.g. in the gnome-terminal ticket they already had a quite positive response.

I hope you don't mind if I keep this ticket as a starting point for keeping track of generic support of this feature in terminals and maybe other apps as well.

comment:17 Changed 12 years ago by angel_il

egmont:
can you write automation tests for parse_extended_mouse_coordinates (void)
and i think need add param into parse_extended_mouse_coordinates. what you think about?

comment:18 Changed 12 years ago by slavazanko

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

comment:19 Changed 12 years ago by egmont

angel_li: I'll take a look, I guess it's not hard, I just have to find some spare time :)

comment:20 Changed 12 years ago by andrew_b

  • Keywords stable-candidate added
  • Votes for changeset changed from andrew_b slavazanko to committed-master
  • Blocking 2399 removed
  • Branch state changed from approved to merged

comment:21 Changed 12 years ago by andrew_b

  • Status changed from accepted to testing
  • Resolution set to fixed

comment:22 Changed 12 years ago by egmont

The change broke Ctrl+Space (directory size calculation), it causes mc to hang.

That key combo sends a 0 byte. parse_extended_mouse_coordinates() looks at the buffer of pending_keys to decide if it is a valid prefix of an extended mouse coordinate, but incorrectly assumes that pending_keys is a 0-terminated buffer. Hence when it includes a 0 as part of its actual content, it always reports that it is a valid prefix, and hence mc waits for the sequence to complete, but it will never complete.

The solution I guess is to use seq_append as the end of the buffer, instead of a 0 value.

I'll try to come up with a patch, but I'm quite busy nowadays. If it's blocking the release then feel free to revert and we'll fix it for the next release.

(I was about to refactor the code a little bit anyways, I promised to write unittests and figured out that for testing it would probably be a better design if the same parse_mouse_coordinates() method was responsible for both the old and the new style coordinates.)

comment:23 Changed 12 years ago by egmont

  • Status changed from testing to reopened
  • Resolution fixed deleted

Changed 12 years ago by egmont

fix for the ctrl+space bug

comment:24 Changed 12 years ago by egmont

I added a patch that seems to work at a first glimpse. I'll keep on using this patch (although apparently I never use Ctrl+Space:)).

I'm still planning to do some refactoring later on to make it less hackish and easier to unittest.

comment:25 Changed 12 years ago by slavazanko

  • Owner changed from andrew_b to slavazanko
  • Status changed from reopened to accepted

comment:26 Changed 12 years ago by slavazanko

  • Branch state changed from merged to on review

Created branch 2662_fix_space_calculation, parent branch: master.

Review, please.

comment:27 Changed 12 years ago by slavazanko

  • Votes for changeset changed from committed-master to slavazanko

comment:28 Changed 12 years ago by andrew_b

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

comment:29 Changed 12 years ago by slavazanko

  • Status changed from accepted to testing
  • Keywords stable-candidate removed
  • Votes for changeset changed from slavazanko andrew_b to commited-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master:

git log --pretty=oneline f507987..b491287

comment:30 Changed 12 years ago by slavazanko

  • Status changed from testing to closed

comment:31 Changed 11 years ago by egmont

(Just FYI: Continuing in ticket #2956)

Note: See TracTickets for help on using tickets.