From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: mouse-face and help echo support for xterm mouse Date: Wed, 04 Nov 2020 13:47:29 -0500 Message-ID: References: <31ff05295c806c4596c54fdcc8994a5f@finder.org> <3e7e0f6d1e272d03913e97254b2eabff@finder.org> <83imalazw7.fsf@gnu.org> <5928a03c79bcf0afee270de1df4d9fa5@finder.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="9594"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: Eli Zaretskii , emacs-devel@gnu.org To: Jared Finder Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Nov 04 20:06:47 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kaO7L-0002Qk-6v for ged-emacs-devel@m.gmane-mx.org; Wed, 04 Nov 2020 20:06:47 +0100 Original-Received: from localhost ([::1]:48222 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kaO7K-0003eF-9a for ged-emacs-devel@m.gmane-mx.org; Wed, 04 Nov 2020 14:06:46 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:53634) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kaNom-0001d7-Tk for emacs-devel@gnu.org; Wed, 04 Nov 2020 13:47:37 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:25819) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kaNok-0002tG-Gj; Wed, 04 Nov 2020 13:47:36 -0500 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id B6508441728; Wed, 4 Nov 2020 13:47:32 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 8C58D441724; Wed, 4 Nov 2020 13:47:30 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1604515650; bh=4r9W5rQwSp0XVnNWbmS80vloh3KxY4pUIRtjhQDr+0k=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=EaeYrVEUXDQeDOHwblVf5kzD1IMB0TSpiuRKhUsNA4ZuE7YvFdUUv6fe1rqdgBNaE b7pd1Hen2U7pcFcUyZ85nNhbDtW613C+w06194+URy5VweOznLOAexpmFElq45gyUd 0PC8ldJR47wwDOYFB1x5VBbsTGDtjEdxo5yvrbAbmeqGboRteNI7fNktyPlVgQDeSF 4t44xuKWIJROqchwmsxMMm58xGLu6dB+waggPUSypbhqjK1ZOAEVsaiEst4pHxuEwh 26NMnk559rdA+i3nZH79qh0ZU1Lew8GF1lqU4xFLVspYnhKwY/ajgKfzzP2uuDCgTY ynJ76uamOadQw== Original-Received: from alfajor (unknown [157.52.9.240]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 5B8D81201FA; Wed, 4 Nov 2020 13:47:30 -0500 (EST) In-Reply-To: <5928a03c79bcf0afee270de1df4d9fa5@finder.org> (Jared Finder's message of "Wed, 04 Nov 2020 09:54:11 -0800") Received-SPF: pass client-ip=132.204.25.50; envelope-from=monnier@iro.umontreal.ca; helo=mailscanner.iro.umontreal.ca X-detected-operating-system: by eggs.gnu.org: First seen = 2020/11/04 13:07:54 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] X-Spam_score_int: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:258695 Archived-At: >> - done: >> - if (ie.kind != NO_EVENT) >> + if (ie.kind != NO_EVENT) /* FIXME: We retest the previous if's >> condition! */ >> { >> + /* FIXME: `hold_quit` could already hold a previous event, >> + so it risks crushing that previous event. >> + What am I missing? */ >> kbd_buffer_store_event_hold (&ie, hold_quit); > > This is the meat of the refactor. > > Given the current code hold_quit will never hold a previous > event. tty_read_avail_input initializes hold_quit right before calling > handle_one_term_event and and sets kind to NO_EVENT. This is the first and > only time in this function hold_quit is passed to any other function. The way I read it: EVENT_INIT (gpm_hold_quit); gpm_hold_quit.kind = NO_EVENT; [...] while (gpm = Gpm_GetEvent (&event), gpm == 1) { nread += handle_one_term_event (tty, &event, &gpm_hold_quit); } `gpm_hold_quit->kind` could hold a different value from `NO_EVENT` on the second iteration (and subsequent ones) of the loop. > Because it is the first time passed, we can safely deduce that > hold_quit->kind will still be NO_EVENT *unless* a previous loop iteration > changed it. That's right. > Because it is the only time passed, we can safely deduce that > hold_quit->kind can only changed in a previous loop iteration by this > function storing a quit event. However, this can not happen. hold_quit > would get set by kbd_buffer_store_buffered_event which > kbd_buffer_store_event_hold calls. To get set, the following must all be > true: > > 1. An event of kind ASCII_KEYSTROKE_EVENT must be processed. > 2. And that event's character code must be quit_char. > 3. And that event must be on the current frame's KBOARD (not quite sure what > this means). > > However, 1 will never be true as the only events that ever reach this point > are of kind GPM_CLICK_EVENT, as deduced from the above asserts. Oh, I see, indeed. So we have the following: if (event->type & (GPM_MOVE | GPM_DRAG)) { [...] eassert (ie.kind == NO_EVENT); } else { [...] eassert (ie.kind == GPM_CLICK_EVENT); } if (ie.kind != NO_EVENT) /* FIXME: We retest the previous if's condition! */ { kbd_buffer_store_event_hold (&ie, hold_quit); count++; } where we can move the second `if` into the first (according to the FIXME) if (event->type & (GPM_MOVE | GPM_DRAG)) { [...] eassert (ie.kind == NO_EVENT); } else { [...] eassert (ie.kind == GPM_CLICK_EVENT); kbd_buffer_store_event_hold (&ie, hold_quit); count++; } and then we know that `hold_quit` starts at NO_EVENT and can only be changed via this call to `kbd_buffer_store_event_hold`, but this call will never change it because `ie.kind == GPM_CLICK_EVENT` and kbd_buffer_store_event_hold would only ever touch it if the kind was ASCII_KEYSTROKE_EVENT, so we know that `hold_quit->kind == NO_EVENT` always holds and we can drop it. The code is now (with added context): if (event->type & (GPM_MOVE | GPM_DRAG)) { [...] eassert (ie.kind == NO_EVENT); } else { [...] eassert (ie.kind == GPM_CLICK_EVENT); kbd_buffer_store_event (&ie); count++; } if (do_help /* FIXME: `hold_quit` is never NULL?! */ && !(hold_quit && hold_quit->kind != NO_EVENT)) { ... } and the last if's condition can now be simplified: bool do_help = false; [...] if (event->type & (GPM_MOVE | GPM_DRAG)) { [...] if (!NILP (help_echo_string) || !NILP (previous_help_echo_string)) do_help = true; eassert (ie.kind == NO_EVENT); } else { [...] eassert (ie.kind == GPM_CLICK_EVENT); kbd_buffer_store_event (&ie); count++; } if (do_help) { ... } and now we can again fold the second move into the tail of the first: bool do_help = false; [...] if (event->type & (GPM_MOVE | GPM_DRAG)) { [...] if (!NILP (help_echo_string) || !NILP (previous_help_echo_string)) do_help = true; eassert (ie.kind == NO_EVENT); if (do_help) { ... } } else { [...] eassert (ie.kind == GPM_CLICK_EVENT); kbd_buffer_store_event (&ie); count++; } and get rid of `do_help` altogether. [...] if (event->type & (GPM_MOVE | GPM_DRAG)) { [...] if (!NILP (help_echo_string) || !NILP (previous_help_echo_string)) { ... } } else { [...] eassert (ie.kind == GPM_CLICK_EVENT); kbd_buffer_store_event (&ie); count++; } So, I installed the patch below. Stefan diff --git a/src/keyboard.c b/src/keyboard.c index 2e0143379a..49a0a8bd23 100644 --- a/src/keyboard.c +++ b/src/keyboard.c @@ -7005,12 +7005,8 @@ tty_read_avail_input (struct terminal *terminal, if (gpm_tty == tty) { Gpm_Event event; - struct input_event gpm_hold_quit; int gpm, fd = gpm_fd; - EVENT_INIT (gpm_hold_quit); - gpm_hold_quit.kind = NO_EVENT; - /* gpm==1 if event received. gpm==0 if the GPM daemon has closed the connection, in which case Gpm_GetEvent closes gpm_fd and clears it to -1, which is why @@ -7018,13 +7014,11 @@ tty_read_avail_input (struct terminal *terminal, select masks. gpm==-1 if a protocol error or EWOULDBLOCK; the latter is normal. */ while (gpm = Gpm_GetEvent (&event), gpm == 1) { - nread += handle_one_term_event (tty, &event, &gpm_hold_quit); + nread += handle_one_term_event (tty, &event); } if (gpm == 0) /* Presumably the GPM daemon has closed the connection. */ close_gpm (fd); - if (gpm_hold_quit.kind != NO_EVENT) - kbd_buffer_store_event (&gpm_hold_quit); if (nread) return nread; } diff --git a/src/term.c b/src/term.c index ff1aabfed2..3a13da165e 100644 --- a/src/term.c +++ b/src/term.c @@ -2550,67 +2550,63 @@ term_mouse_click (struct input_event *result, Gpm_Event *event, } int -handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event, - struct input_event *hold_quit) +handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event) { struct frame *f = XFRAME (tty->top_frame); struct input_event ie; - bool do_help = 0; int count = 0; EVENT_INIT (ie); ie.kind = NO_EVENT; ie.arg = Qnil; - if (event->type & (GPM_MOVE | GPM_DRAG)) { - previous_help_echo_string = help_echo_string; - help_echo_string = Qnil; + if (event->type & (GPM_MOVE | GPM_DRAG)) + { + previous_help_echo_string = help_echo_string; + help_echo_string = Qnil; - Gpm_DrawPointer (event->x, event->y, fileno (tty->output)); + Gpm_DrawPointer (event->x, event->y, fileno (tty->output)); - if (!term_mouse_movement (f, event)) - help_echo_string = previous_help_echo_string; + if (!term_mouse_movement (f, event)) + help_echo_string = previous_help_echo_string; - /* If the contents of the global variable help_echo_string - has changed, generate a HELP_EVENT. */ - if (!NILP (help_echo_string) - || !NILP (previous_help_echo_string)) - do_help = 1; + /* If the contents of the global variable help_echo_string + has changed, generate a HELP_EVENT. */ + if (!NILP (help_echo_string) + || !NILP (previous_help_echo_string)) + { + Lisp_Object frame; - goto done; - } - else { - f->mouse_moved = 0; - term_mouse_click (&ie, event, f); - if (tty_handle_tab_bar_click (f, event->x, event->y, - (ie.modifiers & down_modifier) != 0, &ie)) - { - /* tty_handle_tab_bar_click stores 2 events in the event - queue, so we are done here. */ - count += 2; - return count; - } - } + if (f) + XSETFRAME (frame, f); + else + frame = Qnil; - done: - if (ie.kind != NO_EVENT) - { - kbd_buffer_store_event_hold (&ie, hold_quit); - count++; + gen_help_event (help_echo_string, frame, help_echo_window, + help_echo_object, help_echo_pos); + count++; + } } - - if (do_help - && !(hold_quit && hold_quit->kind != NO_EVENT)) + else { - Lisp_Object frame; - - if (f) - XSETFRAME (frame, f); - else - frame = Qnil; - - gen_help_event (help_echo_string, frame, help_echo_window, - help_echo_object, help_echo_pos); + f->mouse_moved = 0; + term_mouse_click (&ie, event, f); + /* eassert (ie.kind == GPM_CLICK_EVENT); */ + if (tty_handle_tab_bar_click (f, event->x, event->y, + (ie.modifiers & down_modifier) != 0, &ie)) + { + /* eassert (ie.kind == GPM_CLICK_EVENT + * || ie.kind == TAB_BAR_EVENT); */ + /* tty_handle_tab_bar_click stores 2 events in the event + queue, so we are done here. */ + /* FIXME: Actually, `tty_handle_tab_bar_click` returns true + without storing any events, when + (ie.modifiers & down_modifier) != 0 */ + count += 2; + return count; + } + /* eassert (ie.kind == GPM_CLICK_EVENT); */ + kbd_buffer_store_event (&ie); count++; } diff --git a/src/termhooks.h b/src/termhooks.h index d18b750c3a..6ab06ceff9 100644 --- a/src/termhooks.h +++ b/src/termhooks.h @@ -370,7 +370,7 @@ #define EVENT_INIT(event) memset (&(event), 0, sizeof (struct input_event)) #ifdef HAVE_GPM #include -extern int handle_one_term_event (struct tty_display_info *, Gpm_Event *, struct input_event *); +extern int handle_one_term_event (struct tty_display_info *, Gpm_Event *); #ifndef HAVE_WINDOW_SYSTEM extern void term_mouse_moveto (int, int); #endif