From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw Date: Thu, 17 Oct 2024 08:48:56 +0300 Message-ID: <86cyjzp9dj.fsf@gnu.org> References: <86r08goy76.fsf@gnu.org> <86msj3q4t0.fsf@gnu.org> <86iktrpe46.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33689"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 73838@debbugs.gnu.org To: Gerd =?UTF-8?Q?M=C3=B6llmann?= Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Oct 17 07:50:07 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@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 1t1JOg-0008ce-Nh for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 17 Oct 2024 07:50:07 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1t1JOJ-00065I-7C; Thu, 17 Oct 2024 01:49:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1t1JOH-000652-Vo for bug-gnu-emacs@gnu.org; Thu, 17 Oct 2024 01:49:42 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1t1JOH-0003OQ-LW for bug-gnu-emacs@gnu.org; Thu, 17 Oct 2024 01:49:41 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-version:References:In-Reply-To:From:Date:To:Subject; bh=xncBqViyquqslVg2QiXvOzYoL2VrZci/2LqP64sVY2A=; b=JQtfQ0sHSqlf07gEF1vpgpaVZClgL2yKUkw6VdutpkSydcZZ6zYc9JotPmrlGHgcP2mjmEuZhPCf2/oyyIGAo/pfMb6Oc1/TjRx8ClC7VusFDNilMu7rm7OC821I7Mw8TLphOJkja0zWHC++FdodH1ZwX2YqnmLcH4qanS1EnCXZn/F1P5ZhX88rxbmXhobn7J2xxef536dF9da1aPS1WI0R+py2Tt9i89xxIUZWjmQ4DrK5asRto6AbB95s7zPflUPpfyz836XL/QI6BMi+4VGllwhoUOemMH4KyzTHW0x8rrgULbdbUZczZwXFjJp7gphTrsLc9ZYiUY9R1iNKUA==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1t1JOc-00048O-3y for bug-gnu-emacs@gnu.org; Thu, 17 Oct 2024 01:50:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 17 Oct 2024 05:50:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 73838 X-GNU-PR-Package: emacs Original-Received: via spool by 73838-submit@debbugs.gnu.org id=B73838.172914416815840 (code B ref 73838); Thu, 17 Oct 2024 05:50:02 +0000 Original-Received: (at 73838) by debbugs.gnu.org; 17 Oct 2024 05:49:28 +0000 Original-Received: from localhost ([127.0.0.1]:33096 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t1JO3-00047O-QY for submit@debbugs.gnu.org; Thu, 17 Oct 2024 01:49:28 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:49970) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t1JO0-000476-79 for 73838@debbugs.gnu.org; Thu, 17 Oct 2024 01:49:25 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1t1JNa-0003C2-BK; Thu, 17 Oct 2024 01:48:58 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-version:References:Subject:In-Reply-To:To:From: Date; bh=xncBqViyquqslVg2QiXvOzYoL2VrZci/2LqP64sVY2A=; b=mhjXX72RowbUkbffPz85 9OAYoAYoCumYLKD3WOvf9viLODae1dBTy5EyW2yje9jdPOUM9NoSljxL/VzioSvNk2hicfc8RP4TK wy2dZJ6O0ULfEoQN2x/C3emedXyNpEmH2faNwn1iOcSp1Sk/S+/oWbcBFPnzvWJWt/17zLX5GUpi8 Up4nvb/RMrIg3DYTKPZqibTSQUmoETc/Q7PmKvj1mpa7ZHwhZqLNycjxpTyGk1QSh5uSYK1bFvoix +/ckUVx02uTHAi8nq/HBCsOONtuwkWJ7c4QQ2HinratJOzfkqGGueF+fmRWJ092WaNse5vpZED4sW ibLa7uqot1jyhw==; In-Reply-To: (message from Gerd =?UTF-8?Q?M=C3=B6llmann?= on Thu, 17 Oct 2024 07:07:44 +0200) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:293705 Archived-At: > From: Gerd Möllmann > Cc: 73838@debbugs.gnu.org > Date: Thu, 17 Oct 2024 07:07:44 +0200 > > Eli Zaretskii writes: > > > My suggestion is to extend 'struct tty_display_info' so that > > FRAME_OUTPUT_DATA on TTY frames will not access unrelated memory, when > > these macros/inline functions are called. Alternatively, we could have > > the macros/functions (FRAME_INTERNAL_BORDER etc.) test for TTY frame > > and DTRT. > > FRAME_OUTPUT_DATA is meaningful only for window system frames. Each > window system's "term" header defines it. For example > > xterm.h: > #define FRAME_X_OUTPUT(f) ((f)->output_data.x) > #define FRAME_OUTPUT_DATA(f) FRAME_X_OUTPUT (f) > > nsterm.h: > #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns) > > and so on. So using FRAME_OUTPUT_DATA is per se only valid if > FRAME_WINDOW_P. Which is equivalent to FRAME_NS_P in my case, or > whatever someone uses. > > #ifdef HAVE_X_WINDOWS > #define FRAME_WINDOW_P(f) FRAME_X_P (f) > #endif > #ifdef HAVE_NS > #define FRAME_WINDOW_P(f) FRAME_NS_P(f) > #endif > #ifndef FRAME_WINDOW_P > #define FRAME_WINDOW_P(f) ((void) (f), false) > #endif My suggestion is to change the last part. output_data is defined like so: union output_data { struct tty_output *tty; /* From termchar.h. */ struct x_output *x; /* From xterm.h. */ struct w32_output *w32; /* From w32term.h. */ struct ns_output *ns; /* From nsterm.h. */ struct pgtk_output *pgtk; /* From pgtkterm.h. */ struct haiku_output *haiku; /* From haikuterm.h. */ struct android_output *android; /* From androidterm.h. */ } output_data; So it exists in TTY frames as well, and we could have bitfields in it that correspond to the window-system's versions of output_data to specify internal-border and other decorations. We just need to make sure these bitfields are zero as long as TTY frames don't support those features. Then we could avoid the FRAME_WINDOW_P tests which you propose to add, and still have valid and future-proof code. I thought you were proposing a future-proof change that will avoid the need to dig into these macros and change them if and when TTY frames gain more functionality. If that's not what you are suggesting, I wonder what is wrong with the current code that we need to make changes which are basically half-solutions. If the problem is access to unrelated memory, that is easy to fix by just adding enough slack to tty_output definition, for example. Adding tests slows down redisplay, albeit by a very small amount in each case (but these slow-downs add up, since we use these macros all over the place). > I think changing that would be a major surgery. It's probably easier to > add checks like I did in the patch to FRAME_OUTPUT_DATA if the frame in > questioin is indeed a window system frame. It can be decided at run-time > only anyway. It might be easier, but if we are going to make changes, why not do it the right way to begin with? After all, if what's problematic in the current code is that valgrind or ASAN complain, we could simply regard that as false positives, since there are no problems in production builds. > The other idea is, IIUC, is to make code using FRAME_OUTPUT_DATA like > this one > > if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0 > && !NILP (get_frame_param (f, Qdrag_internal_border))) > { > enum internal_border_part part = frame_internal_border_part (f, x, y); > > switch (part) > { > case INTERNAL_BORDER_NONE: > if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor) > /* Reset cursor. */ > cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor; > > work by making sure that their if-conditions can't be true, if there > any. In the above case by making FRAME_INTERNAL_BORDER_WIDTH return 0 if the > frame is not FRAME_WINDOW_P. In other cases like this one > > if (EQ (window, f->tool_bar_window)) > { > note_tool_bar_highlight (f, x, y); > cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor; > > or here > > if (part == ON_MODE_LINE || part == ON_HEADER_LINE || part == ON_TAB_LINE > || part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN) > { > note_mode_line_or_margin_highlight (window, x, y, part); > > #ifdef HAVE_WINDOW_SYSTEM > if (part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN) > { > cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor; > /* Show non-text cursor (Bug#16647). */ > goto set_cursor; > } > else > #endif > return; > } > > by doing something else. > > I have to admit that I don't like that. I don't understand what is wrong > to check FRAME_WINDOW_P before using something (using FRAME_OUTPUT_DATA) > that requires FRAME_WINDOW_P to be valid. Let me try explaining once again what I think is wrong with testing FRAME_WINDOW_P in each of those cases. Imagine that someone develops a feature whereby TTY frames could have scroll bars or fringes. With the method you propose, we'd then need to change all the places where the code accesses scroll bars or fringes, and remove the FRAME_WINDOW_P and similar tests. If some of these tests are forgotten and not removed/rewritten, we'd have a subtle bug. By contrast, the way I propose it, whereby tty_output will have extended to have the corresponding fields, all we'd need is to set those fields to some non-zero values, and the rest will "just work". IOW, my proposal is to make the tests be based on supported functionalities rather than on some attribute of a frame which _today_ is associated with the lack of such functionalities, for the same reasons we prefer testing functionalities with HAVE_SOMETHING to testing version numbers or OS-specific symbols. Because the correlation between frame types and available functionalities can change in the future, and then adjusting the code to such changes is usually a tedious and error-prone job. We had ample examples of that when TTY frames learned to display colors, then again when they learned to show menus and dialogs. We should learn from those examples.