From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Vladimir Kazanov Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] User-defined fringe tooltips (a request for review) Date: Mon, 25 Mar 2024 15:55:16 +0000 Message-ID: References: <83sf3xgimq.fsf@gnu.org> <83plyzfoe4.fsf@gnu.org> <83plyxca0t.fsf@gnu.org> <8334vrczig.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="11684"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Mar 25 16:56:38 2024 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 1romgf-0002oi-SO for ged-emacs-devel@m.gmane-mx.org; Mon, 25 Mar 2024 16:56:37 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1romfg-00015O-6z; Mon, 25 Mar 2024 11:55:36 -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 1romfe-00015A-IF for emacs-devel@gnu.org; Mon, 25 Mar 2024 11:55:34 -0400 Original-Received: from mail-lf1-x12e.google.com ([2a00:1450:4864:20::12e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1romfc-0004lB-K8; Mon, 25 Mar 2024 11:55:34 -0400 Original-Received: by mail-lf1-x12e.google.com with SMTP id 2adb3069b0e04-513da1c1f26so5684681e87.3; Mon, 25 Mar 2024 08:55:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1711382129; x=1711986929; darn=gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=69w+cpwSGWL5DR0bc1+Pv9gbgEq5xOFcBFBgGDXeCgc=; b=fIGIe7et1O3jW7hJoYwsch4mnztH5fvaKe7zZEB5OZgDBMi7iV6QDVzHriRFZJr4d+ 9jW8vW3llfG9X0RxD6EzFkhJT17aHpui/0n9EXzukEP5UlpelakS/jGOMClkYwUa4O7S r3RB+ghCxLBeBNAJF0ABN2SRM0dO0139DkZlcWejVo8tWqqacNL350Gp5CzwQ5jjVKVm f1FnOt7K4SiSioVne+Ruhc7FXSnEu573iZFFXva0REWnGr8rffq5EPowgR/9dTNIuScD 2yIq0nNYr5iJBFCeKk5hd2AxKH4zBTATxWeN3wxeRWos/v3g3Mc9gyEbT9RSmyRSl3K1 omWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711382129; x=1711986929; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=69w+cpwSGWL5DR0bc1+Pv9gbgEq5xOFcBFBgGDXeCgc=; b=RB1QnfQs2w1sPsUQH3LWpI9tkZlnto6EsStawFF+/0pnNklFLaiwZBLI0kSgDmRr50 qtf3/fDIXUZcGqWaGGktPe0Q7941Se2edTSxFfGldKA3ELgW56ifO/2sfxzEaYK0AlcT tA8GEggECOZanY1M9Aeo1Xqd+OOU+xF+SoqWCTi3cuLD8UNbOZe12U7NtVU2lA2RIoWR N/e7yi0svX0zhcjZslHO4zDuc0nObMwMqA4ccDq/eNV5VyZM0i1KluYlRQogYHgSUYxc c0PG0dMnbagL8OalcoPmIajq72ALJ5kS+G8J+K1aoU0d2OJHYUtjPlmGH3KPg5d3EuWd kb/Q== X-Gm-Message-State: AOJu0YzjOM9/qGrhaB2mpGSK0fjRNQY1Y8BUE9xInfpE11mGrmkteM1e KuNWkyzQ9vieVjhUGw7ipTVPuNTyWRMp5VLpuaLASKCo0y+24PzYBG14qutUVYzEE7kgxgW561e 3ZcQgBAv2RTcyY7ONy/bat/sEehY1wy+TmHQp X-Google-Smtp-Source: AGHT+IGHJxzaM6Zbt8KH+lNMIfCU6oOLDkFaGky7kmpuod5bH69oA5lRUuxCTN7sVn4DrZUu1OdnWi2K57hr1XlmxyM= X-Received: by 2002:a2e:9484:0:b0:2d2:4783:872a with SMTP id c4-20020a2e9484000000b002d24783872amr4584151ljh.29.1711382128613; Mon, 25 Mar 2024 08:55:28 -0700 (PDT) In-Reply-To: <8334vrczig.fsf@gnu.org> Received-SPF: pass client-ip=2a00:1450:4864:20::12e; envelope-from=vekazanov@gmail.com; helo=mail-lf1-x12e.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, 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.29 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-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:317281 Archived-At: Hi Eli, Thank you for looking into this and sorry for the delay - life happened. Anyways, I have some time now to finish the patch. I've integrated all of the suggestions but the last one, related to many forms of display specs. The code in the (note_fringe_highlight) function works outside of the main (handle_single_display_spec) call. So to get to the :help-echo string it has to parse into the spec, or a list of specs, or a vector of specs, effectively duplicating parsing logic in handle_single_display_spec. This is not nice but doable. But every display spec can be wrapped in a (when CONDITION . SPEC) form. And because (handle_single_display_spec) happens at a time different from the (note_fringe_highlight) call time we can never be sure that the condition still holds even if the code reevaluates CONDITION. One approach would be to just go into SPEC of the condition without any reevaluations and get the :help-echo string anyway, hoping that there are no discrepancies. This would work, hopefully, most of the time but not all the time. What do you think? Thank you On Sun, 24 Dec 2023 at 16:54, Eli Zaretskii wrote: > > > From: Vladimir Kazanov > > Date: Sun, 24 Dec 2023 11:31:28 +0000 > > Cc: emacs-devel@gnu.org > > > > After figuring out a couple of nasty crashes I came up with a patch > > which works for all display specs coming from buffer text itself. > > > > But I've just realized that some fringe sources are still unhandled > > here. For example, when the fringe indicator is defined in non-buffer > > strings, like overlays with 'before-string/'after-string properties. > > Just iterating through text positions doesn't scan any of that. This > > means that we'd have to imitate more of the iterator logic when > > looking for display specs containing help-echo. > > > > Do we want to manage fringe help-echo text coming from various strings > > as well? Certainly doable. I wonder if this asks for a separate patch. > > There's no catastrophe if we only support some sources of this display > property; we already do that in other similar cases. We can later add > more sources if there's demand. > > I have some comments on the code and the documentation: > > > --- a/doc/lispref/display.texi > > +++ b/doc/lispref/display.texi > > @@ -5492,14 +5492,15 @@ Other Display Specs > > but it is done as a special case of marginal display (@pxref{Display > > Margins}). > > > > -@item (left-fringe @var{bitmap} @r{[}@var{face}@r{]}) > > -@itemx (right-fringe @var{bitmap} @r{[}@var{face}@r{]}) > > +@item (left-fringe @var{bitmap} @r{[}@var{face}@r{]} @r{[}@var{help-echo}@r{]}) > > +@itemx (right-fringe @var{bitmap} @r{[}@var{face}@r{]} @r{[}@var{help-echo}@r{]}) > > This display specification on any character of a line of text causes > > the specified @var{bitmap} be displayed in the left or right fringes > > for that line, instead of the characters that have the display > > specification. The optional @var{face} specifies the face whose > > -colors are to be used for the bitmap display. @xref{Fringe Bitmaps}, > > -for the details. > > +colors are to be used for the bitmap display. The optional > > +@var{help-echo} string can be used to display tooltips or help text in > > +the echo area. @xref{Fringe Bitmaps}, for the details. > > The @xref sentence should be after the sentence which ends with > "bitmap display", since it describes more details about that, and has > nothing to do with the new help-echo string feature. > > > ++++ > > +** Tooltips for user fringe indicators. > > +User fringe indicators defined in text display specifications now > > +support user-defined tooltips. See the "Other Display Specifications" > ^^ > Our conventions are to leave two spaces between sentences. > > > + /* Translate windows coordinates into a vertical window position. */ > > + int hpos, vpos, area; > > + struct window *w = XWINDOW (window); > > + x_y_to_hpos_vpos (w, x, y, &hpos, &vpos, 0, 0, &area); > > This was already done by the caller, so no need to do it again; > instead, pass the info as arguments to this function. > > > + /* Get to the current glyph row. */ > > + struct glyph_row *row = MATRIX_ROW (w->current_matrix, vpos); > > + > > + /* No fringe indicators - no need to look things up */ > > Our style for comments in C is to have them as complete sentences > ending in a period, and leave two spaces between the last sentence and > the closing "*/" comment delimiter. > > > + /* Check display specs for both visual and invisible text in the > > + row. */ > > + ptrdiff_t charpos; > > + ptrdiff_t charpos_start = row->minpos.charpos; > > + ptrdiff_t charpos_end = row->maxpos.charpos; > > + for (charpos = charpos_start; charpos <= charpos_end; charpos++) > > + { > > + /* Properties in question can be either in text props or > > + overlays, so check both. */ > > + Lisp_Object spec = get_char_property_and_overlay (make_fixnum (charpos), > > + Qdisplay, Qnil, NULL); > > To avoid punishing display of text that has no such properties, I > would first see if any Qdisplay properties are within this glyph row, > using Fnext_char_property_change, and only enter the loop if that call > returns a position in-range. > > > + /* Hovering over the right fringe - check the right-fringe > > + spec */ > > + /* Hovering over the left fringe - check the left-fringe > > + spec */ > > + if (!(EQ (XCAR (spec), Qleft_fringe) && part == ON_LEFT_FRINGE) && > > + !(EQ (XCAR (spec), Qright_fringe) && part == ON_RIGHT_FRINGE)) > > Please place each comment before the line of code on which it > comments. > > > + continue; > > We prefer not to over-use 'continue' in loops; instead, reverse the > condition in 'if' and do everything inside the 'if' block. > > > + /* Get the caption while ingoring all non-strings */ > > + Lisp_Object fringe_help_echo = CAR_SAFE (CDR_SAFE (CDR_SAFE (CDR_SAFE (spec)))); > > + if (!STRINGP (fringe_help_echo)) > > + continue; > > + > > + help_echo_string = fringe_help_echo; > > Here, you only support the simplest form of the display property's > value, i.e. > > (left-fringe BITMAP [FACE] HELP-STRING) > > But the property's value can have a more complex form: > > . it could be a vector or a list of several display specifications > . a single specification could have the form (when CONDITION . SPEC) > > So I think you need something more complex here; see the loop in > handle_display_prop for the details. > > > + else if (part == ON_LEFT_FRINGE || part == ON_RIGHT_FRINGE) { > > This is not our style of placing braces. We use this style: > > else if (CONDITION) > { > ... > } > > Thanks. -- Regards, Vladimir Kazanov