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.devel Subject: Re: [PATCH] User-defined fringe tooltips (a request for review) Date: Sun, 24 Dec 2023 18:54:15 +0200 Message-ID: <8334vrczig.fsf@gnu.org> References: <83sf3xgimq.fsf@gnu.org> <83plyzfoe4.fsf@gnu.org> <83plyxca0t.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="34506"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Vladimir Kazanov Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Dec 24 17:55:21 2023 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 1rHRl2-0008gY-3W for ged-emacs-devel@m.gmane-mx.org; Sun, 24 Dec 2023 17:55:20 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rHRk7-0005ZY-GJ; Sun, 24 Dec 2023 11:54:23 -0500 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 1rHRk5-0005ZM-Cd for emacs-devel@gnu.org; Sun, 24 Dec 2023 11:54:21 -0500 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 1rHRk5-00086w-3v; Sun, 24 Dec 2023 11:54:21 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=UAECKHmt+UAw4IWlJdAB5fewbnox90eres+Z9NMa/Vc=; b=RMF8WzCzm6a4 J3DPrHtobyCSOY0wM6jNDWvis+Tx142L6M3P+40/kILPQEOne/r6AU/2EszvqU+QdMnw7bLRHa4kz eEUqyl8/MfHkmFnsnrwxi3pSrFmVgwrTLfz0hpKUhCi1LF0TCnU2b25lVZB0EGfIZ/SDLyp7VNFf9 /9r4Zj8ay0nWNufb2Wzfn3nxXVYvGktxKdSnC2Harq82n83dG+aibe/BAuXwwqX5Mwh6kaQASB9CA 3gCJuvhBgp+P/a95AX7Y0NRzuPLAATPeakYUz587HQl8jkCcR0RxaUeaVUjnqR6t0X0xrU5gwawdr hxd8f21PToSBL92YnqPPAA==; In-Reply-To: (message from Vladimir Kazanov on Sun, 24 Dec 2023 11:31:28 +0000) 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:314143 Archived-At: > 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.