Re: [PATCH] fix broken mouse in Cygwin 1.7.14 and 1.7.15

[Christopher Faylor <cgf-use-the-mailinglist-please at cygwin dot com>]

On Thu, May 17, 2012 at 01:20:06AM +0200, Mikulas Patocka wrote:
>On Thu, 17 May 2012, Mikulas Patocka wrote:
>
>> > Corinna Vinschen:
>> >
>> > > 2012-04-03  Thomas Wolff
>> > > 
>> > > 	* fhandler.h (class dev_console): Two flags for extended mouse modes.
>> > > 	* fhandler_console.cc (fhandler_console::read): Implemented 
>> > > 	extended mouse modes 1015 (urxvt, mintty, xterm) and 1006 (xterm).
>> > > 	Not implemented extended mouse mode 1005 (xterm, mintty).
>> > > 	Supporting mouse coordinates greater than 222 (each axis).
>> > > 	Also: two { wrap formatting consistency fixes.
>> > > 	(fhandler_console::char_command) Initialization of enhanced 
>> > > 	mouse reporting modes.
>> > 
>> > 
>> > Patch applied with changes.  Please use __small_sprintf rather than
>> > sprintf.  I also changed the CHangeLog entry slightly.  Keep it short
>> > and in present tense.
>> 
>> Hi
>> 
>> The change (sprintf -> __small_sprintf) that Corinna applied actually 
>> broke mouse reporting. It is broken in Cygwin 1.7.14 and 1.7.15.
>> 
>> When the user clicks with the first button, the mouse down event is 
>> reported incorrectly, the mouse down event always looks like this:
>>  1b 5b([) 4d(M) 30(0) 78(x) 32(2)
>> Note that there is 0x30 (instead of 0x20) as the button. And there are 
>> always fixed coordinates (0x78, 0x32), regardless of where the user 
>> clicks.
>> 
>> This bug breaks the Links textmode browser (if you click on the top line, 
>> you should see the menu, but you don't with Cygwin 1.7.14 and 1.7.15). It 
>> also break Midnight Commander (if you start it with "TERM=xterm mc") and 
>> all other programs that use xterm-style mouse reporting.
>> 
>> The reason is that __small_sprintf and sprintf aren't equivalent. 
>> __small_sprintf processes '%c' format string differently from sprintf. A 
>> piece of code from smallprint.cc:
>> 
>> case 'c':
>>   {
>>     int c = va_arg (ap, int);
>>     if (c > ' ' && c <= 127)
>>       *dst++ = c;
>>     else
>>       {
>>         *dst++ = '0';
>>         *dst++ = 'x';
>>         dst = __rn (dst, 16, 0, c, len, pad, LMASK);
>>       }
>>   }
>>   break;
>> 
>> We see that if the character is outside the range 0x21..0x7f, 
>> __small_sprintf prints 0x and the hex value. On the other hand, sprintf 
>> copies the byte unchanged.
>> 
>> __small_sprintf("%c", 32) doesn't print space, it prints 0x20 --- and this 
>> breaks mouse click reporting. It also breaks the extended coordinate 
>> reporting implemented by Thomas because it need to print characters >= 
>> 0x80.
>> 
>> The attached patch fixes the mouse bug by changing __small_sprintf back to 
>> sprintf. Alternatively, you can fix __small_sprintf to process "%c" 
>> consistently with sprintf, but I don't know how much other code is 
>> dependent on the current peculiar "%c" processing. So it's safer to change 
>> mouse reporting calls to use sprintf.

I don't know where that odd code came from.  It's apparently been in
smallprint.{c,cc} forever.  I'm nuking it.  I see one potential problem
in fhandler_socket::bind.  It won't be hard to work around if that
function was really relying on it.

Thanks for tracking this down.  The change will be in the next snapshot.

cgf

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple