From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: =?UTF-8?B?Sm/Do28gVMOhdm9yYQ==?= Newsgroups: gmane.emacs.devel Subject: Re: jit-lock-antiblink-grace Date: Sat, 12 Oct 2019 11:57:39 +0100 Message-ID: References: <834l0enw8c.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="00000000000079b9dd0594b4816e" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="149900"; mail-complaints-to="usenet@blaine.gmane.org" Cc: Stefan Monnier , emacs-devel To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Oct 12 12:58:58 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1iJF6v-000cq4-UR for ged-emacs-devel@m.gmane.org; Sat, 12 Oct 2019 12:58:58 +0200 Original-Received: from localhost ([::1]:60222 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iJF6u-0008JK-Ke for ged-emacs-devel@m.gmane.org; Sat, 12 Oct 2019 06:58:56 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:39400) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iJF5x-0008Gh-F3 for emacs-devel@gnu.org; Sat, 12 Oct 2019 06:57:59 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iJF5v-0001UE-Dn for emacs-devel@gnu.org; Sat, 12 Oct 2019 06:57:57 -0400 Original-Received: from mail-io1-xd2b.google.com ([2607:f8b0:4864:20::d2b]:40784) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iJF5s-0001TK-7b; Sat, 12 Oct 2019 06:57:52 -0400 Original-Received: by mail-io1-xd2b.google.com with SMTP id h144so26883565iof.7; Sat, 12 Oct 2019 03:57:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jJ3Vlg57Khd5Zjl48Lgpg67Dj9QvRSyvJ4BF8um6ovQ=; b=W6tZ6OF7FyaeOKlkCNlosOjFRd801C03tIFvZXXo49lYjhKDNd8hLNjyt8Z4197mjt QUTgGJx5pk7YWjbj52SFg6ewNAx6Lxl+Pf3hojdVWZjcqp1WhVzH23EELwpR4lZlQ97a g47pRv3tBtP30zQC8kmS/d7oR9CwmkHcnJXbUwiOdz8dDPYtq922F18OTuLdZxbuf6kP W7zPiUUEyD4TQ4IbGA8tpl99oIxGAts3E11PVldQYzOUo5wR2MOL4SFHC1xPAQwA6owA jQn9M8Iiu+P0z4st/AWp9x8zWpIgSTxyD2DHY9ivYW/mSwEoV7IrrqHJukmk56aVsKBR YSkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jJ3Vlg57Khd5Zjl48Lgpg67Dj9QvRSyvJ4BF8um6ovQ=; b=cE9QWbJ6Z77uQySLPErqxu5Y4fqz1hOpZqoHfJd4/C/opsBxEM60BfRPebt+kQ/KLq PNXjM/B3NaMwbtbzlZOTVbIEepk9X446j9ceSY2dHi9/BFUJKtH2uzXVexpnEdNDFulw ntGkWupCGSAVvl+p5pXCyVRX9vE9/I3h0+4ft2Td1XHNB9xfI/Wa6VydCBLeFf0jNaBj ZTcNMTULYiUOxVUHcsQXSoO0dG2MJon1qbeIBVAdXkSPCJA2KMu0hNzwQCCuYsHNbT5S RqrZwHGz3GvtHllAqUcUAR4AahR/hrahvVyJxb1FCymP7zHBD72KsgS9xIhvQ7fu9TaS tPPA== X-Gm-Message-State: APjAAAXjS6KsL76itMqrh7Ih7jnJq/i4WpcOpYK10kfOXRQYlMoR3PKL Zan23EHayaozwQLgjpP/OJE0XLhtjv6kqOyZ63hLP6d8 X-Google-Smtp-Source: APXvYqxCCwaLG72hXsPzeMSs410Vg4uzOtY0zmCwi9OOMVmeUgh2zWrUOlFO8E2j3pnkX9kclv2TshYFi9GWlEX5lag= X-Received: by 2002:a6b:4f03:: with SMTP id d3mr12514642iob.199.1570877870813; Sat, 12 Oct 2019 03:57:50 -0700 (PDT) In-Reply-To: <834l0enw8c.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::d2b 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.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:240909 Archived-At: --00000000000079b9dd0594b4816e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, Oct 12, 2019 at 10:34 AM Eli Zaretskii wrote: > > If you want to try it before that happens see the > > scratch/jit-lock-antiblink-cleaned-up branch. > > Bother: this unconditionally adds a post-command-hook, which will > necessarily slow down paging through a file. Everything slows down everything, the question is where and by how much. A noop will probably not slow paging very much. Can you tell me more about the kinds of files you're anxious about and exact meaning of "paging"? Is it C-n'ing from the first line to the last? I could benchmark. > If there's no better solution than using that over-used hook, My very first version relied on an extension of the existing jit-lock-context-time, but I seem to remember it broke down here and there sometimes. I agreed with Stefan (possibly off-list) to use a post-command-hook, which is safer. But I can have a look at the original version and re-study those problems more closely. > then at the very least we should give users a way of NOT adding a > post-command-hook when this feature is disabled. Given that I intend for this to be controlled via a customization variable, I only see it done via a `:set` hook or something like that. Or use some hack where the current timers detect the variable has been enabled/disabled. > Some more comments about the code: > > > +** New customizable variable 'jit-lock-antiblink-grace' > > This line should end in a period OK. > > +Setting this to a positive number of seconds helps avoid the > > +fontification "blinking" behaviour observed when adding temporarily > > +unterminated strings to source code. If the user has recently created > > +an unterminated string at EOL, jit fontification allows this idle > > +"grace" period to elapse before deciding it is a multi-line string and > > +fontifying the remainder of the buffer accordingly. > > This should be simplified and shortened. (In general, copy/paste of > doc strings into NEWS is not a good idea.) In particular, if the > default is to have this behavior (see below), the NEWS entry should > tell how to disable that. OK. Supposing you've already already gotten the idea, I invite you to submit a suggestion. > > +(defcustom jit-lock-antiblink-grace 2 > > + "Like `jit-lock-context-time' but for unterminated multiline strings= . > > +Setting this to a positive number of seconds helps avoid the > > +fontification \"blinking\" behaviour observed when adding > > +temporarily unterminated strings to source code. If the user has > > +recently created an unterminated string at EOL, allow for an idle > > +\"grace\" period to elapse before deciding it is a multi-line > > +string and fontifying the remainder of the buffer accordingly." > > + :type '(number :tag "seconds") > > + :group 'jit-lock) > > This new defcustom should have a :version tag. OK. > The doc string should say how to disable the feature. Also, the doc > string makes it sound like the default is not a positive number of > seconds by default, but it is. > (I question the wisdom of making this the default behavior, btw.) What's bothering you? (assuming all other objections you stated already are somehow dealt with satisfactorily.) > I don't understand the "at EOL" part: isn't any unterminated string > appear as if it is "at EOL"? An unterminated string at EOL might be terminated somewhere _after_ EOL, i.e. a perfectly legitimate (as "in your intentions") multiline string. Moreover this is a hint as to how the system is implemented, which some users may appreciate. > > +(defvar jit-lock--antiblink-l-b-p (make-marker) > > + "Last line beginning position (l-b-p) after last command (a marker).") > > +(defvar jit-lock--antiblink-i-s-o-c nil > > + "In string or comment (i-s-o-c) after last command (a boolean).") > > Please don't use such cryptic variable names, at least not on the file > level (preferably also not locally inside functions). The docstring explains the abbreviation. I'm afraid that given current naming practice (prefix, double dash, sub-feature) I couldn't do much better. I think jit-lock--antiblink-l-b-p is a better name than jit-lock--antiblink-pos, or jit-lock--pos, because it makes the reader "chase" the doc and learn of the exact meaning of the abbreviation. Do you really prever jit-lock--antiblink-in-string-or-comment and jit-lock--antiblink-line-beginning-position? I think it's much harder to follow. But I will abide, especially if you suggest alternatives. > > + (add-hook 'post-command-hook 'jit-lock--antiblink-post-command nil t) > > As mentioned above, this hook should not be added if the feature is > disabled. See above. > > + (when jit-lock--antiblink-grace-timer > > + (message "internal warning: `jit-lock--antiblink-grace-timer' not null")) > > We should in general avoid calling 'message' here, because such a > message will appear after every command, which is a nuisance. Is this > really needed? This is an internal consistency check, i.e. a run-time assertion. It should never happen, except when the program is buggy. I had this set to 'warn', but Stefan suggested I change it. What do you suggest? Perhaps I could warn and turn off the feature. Jo=C3=A3o --00000000000079b9dd0594b4816e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Sat, Oct 12, 2019 at 10:34 AM Eli Zaretskii <eliz@gnu.org> wrote:
&= gt; > If you want to try it before that happens see the
> > scr= atch/jit-lock-antiblink-cleaned-up branch.
>
> Bother: this un= conditionally adds a post-command-hook, which will
> necessarily slow= down paging through a file. =C2=A0

Everything slows down everything= , the question is where and by how much.
A noop will probably not slow p= aging very much.=C2=A0 Can you tell me more
about the kinds of files you= 're anxious about and exact meaning of
"paging"?=C2=A0 Is = it C-n'ing from the first line to the last?=C2=A0 I could
benchmark.=

> If there's no better solution than using that over-used ho= ok,

My very first version relied on an extension of the existing
= jit-lock-context-time, but I seem to remember it broke down here and
the= re sometimes.=C2=A0 I agreed with Stefan (possibly off-list) to use a
po= st-command-hook, which is safer.=C2=A0 But I can have a look at the
orig= inal version and re-study those problems more closely.

> then at = the very least we should give users a way of NOT adding a
> post-comm= and-hook when this feature is disabled.

Given that I intend for this= to be controlled via a customization
variable, I only see it done via a= `:set` hook or something like that.
Or use some hack where the current = timers detect the variable has been
enabled/disabled.

> Some m= ore comments about the code:
>
> > +** New customizable vari= able 'jit-lock-antiblink-grace'
>
> This line should en= d in a period

OK.

> > +Setting this to a positive numbe= r of seconds helps avoid the
> > +fontification "blinking&quo= t; behaviour observed when adding temporarily
> > +unterminated st= rings to source code.=C2=A0 If the user has recently created
> > += an unterminated string at EOL, jit fontification allows this idle
> &= gt; +"grace" period to elapse before deciding it is a multi-line = string and
> > +fontifying the remainder of the buffer accordingly= .
>
> This should be simplified and shortened. =C2=A0(In genera= l, copy/paste of
> doc strings into NEWS is not a good idea.) =C2=A0I= n particular, if the
> default is to have this behavior (see below), = the NEWS entry should
> tell how to disable that.

OK.=C2=A0 Su= pposing you've already already gotten the idea, I invite you to
subm= it a suggestion.

> > +(defcustom jit-lock-antiblink-grace 2> > + =C2=A0"Like `jit-lock-context-time' but for untermina= ted multiline strings.
> > +Setting this to a positive number of s= econds helps avoid the
> > +fontification \"blinking\" b= ehaviour observed when adding
> > +temporarily unterminated string= s to source code.=C2=A0 If the user has
> > +recently created an u= nterminated string at EOL, allow for an idle
> > +\"grace\&qu= ot; period to elapse before deciding it is a multi-line
> > +strin= g and fontifying the remainder of the buffer accordingly."
> >= ; + =C2=A0:type '(number :tag "seconds")
> > + =C2= =A0:group 'jit-lock)
>
> This new defcustom should have a := version tag.

OK.

> The doc string should say how to disabl= e the feature.=C2=A0 Also, the doc
> string makes it sound like the d= efault is not a positive number of
> seconds by default, but it is. = =C2=A0

> (I question the wisdom of making this the default behavi= or, btw.)

What's bothering you? (assuming all other objections y= ou stated already
are somehow dealt with satisfactorily.)

> I = don't understand the "at EOL" part: isn't any unterminate= d string
> appear as if it is "at EOL"?

An untermina= ted string at EOL might be terminated somewhere _after_ EOL,
i.e. a perf= ectly legitimate (as "in your intentions") multiline string.
M= oreover this is a hint as to how the system is implemented, which some
u= sers may appreciate.

> > +(defvar jit-lock--antiblink-l-b-p (m= ake-marker)
> > + =C2=A0"Last line beginning position (l-b-p)= after last command (a marker).")
> > +(defvar jit-lock--anti= blink-i-s-o-c nil
> > + =C2=A0"In string or comment (i-s-o-c)= after last command (a boolean).")
>
> Please don't us= e such cryptic variable names, at least not on the file
> level (pref= erably also not locally inside functions).

The docstring explains th= e abbreviation.=C2=A0 I'm afraid that given current
naming practice = (prefix, double dash, sub-feature) I couldn't do much
better.=C2=A0 = I think jit-lock--antiblink-l-b-p is a better name than
jit-lock--antibl= ink-pos, or jit-lock--pos, because it makes the reader
"chase"= the doc and learn of the exact meaning of the abbreviation.

Do you = really prever jit-lock--antiblink-in-string-or-comment and
jit-lock--ant= iblink-line-beginning-position? I think it's much harder to
follow.= =C2=A0 But I will abide, especially if you suggest alternatives.

>= ; > + =C2=A0 =C2=A0 =C2=A0(add-hook 'post-command-hook 'jit-lock= --antiblink-post-command nil t)
>
> As mentioned above, this ho= ok should not be added if the feature is
> disabled.

See above= .

> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (when jit-lock--ant= iblink-grace-timer
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= (message "internal warning: `jit-lock--antiblink-grace-timer' not= null"))
>
> We should in general avoid calling 'messa= ge' here, because such a
> message will appear after every comman= d, which is a nuisance.=C2=A0 Is this
> really needed?

This is= an internal consistency check, i.e. a run-time assertion.=C2=A0 It
shou= ld never happen, except when the program is buggy.=C2=A0 I had this set
= to 'warn', but Stefan suggested I change it.=C2=A0 What do you sugg= est?
Perhaps I could warn and turn off the feature.

Jo=C3=A3o --00000000000079b9dd0594b4816e--