From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Kaushal Modi Newsgroups: gmane.emacs.devel Subject: Re: Patch to highlight current line number [nlinum.el] Date: Mon, 18 Jul 2016 15:28:04 +0000 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=94eb2c1917a0ed6a6b0537ea9d69 X-Trace: ger.gmane.org 1468855717 18749 80.91.229.3 (18 Jul 2016 15:28:37 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 18 Jul 2016 15:28:37 +0000 (UTC) Cc: code.falling@gmail.com, Emacs developers To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Jul 18 17:28:36 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1bPATA-0001ds-S5 for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2016 17:28:33 +0200 Original-Received: from localhost ([::1]:48328 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPATA-0001d4-4z for ged-emacs-devel@m.gmane.org; Mon, 18 Jul 2016 11:28:32 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:41315) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPAT1-0001UU-2h for emacs-devel@gnu.org; Mon, 18 Jul 2016 11:28:24 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPASt-00042A-OH for emacs-devel@gnu.org; Mon, 18 Jul 2016 11:28:22 -0400 Original-Received: from mail-oi0-x22c.google.com ([2607:f8b0:4003:c06::22c]:33615) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPASt-00041y-Hy for emacs-devel@gnu.org; Mon, 18 Jul 2016 11:28:15 -0400 Original-Received: by mail-oi0-x22c.google.com with SMTP id j185so250562681oih.0 for ; Mon, 18 Jul 2016 08:28:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=LTYM84SwPAzxdnZK5tnPEpvMeVK2CE9wKgqW0swVgFM=; b=thDpKg42iCiTUrHpc0htPVCLEuQy3ztZ8JiAJ9fUw574IkjMHL9T0XwyGYb4uP3bl0 bWoBFswCZcBKAdE54WTQroldHNSaAMyxZKy3DGnOaPyf4Dm9Je9kOOjsxjpM0RJFatBR WXeQOzTDSAIfFo1tXYxcxCDiUiM8zmptcwQ+0IAA09FzIWI5Q/7zhGSPpjJKL+OTmTTG DXSZupS/sZBWIcd2fBMaKlw4lu0FRFrFZNo9TSOcEO3URoy0z/+v/HwAlxdZ6b5ut2jv 8xnk+GU9enDyO2YJW/fLyX5NPG8hEiHi4QgZ1QVbEJ1LCbNOD1A4jKB3aq3oVxKGMZqU hhBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=LTYM84SwPAzxdnZK5tnPEpvMeVK2CE9wKgqW0swVgFM=; b=RkyjBJnDITv8d+SqgWqbhjflMteQxMVvNe65vJOhTN2hf3d6vW2SwDFze4hlg+27Jo JKKO8E6xcJJRLoGTkB0BFzO6ToekK260TpoIUgjyU24QIIVh9ZcqH2YtxigRDYlHgScb sFnHBqZ97ENX6tR3lhXSMZJxJtrYIOliNn9vl9C/989gWm1lVU1l5R/8O82DO4yp4RIb Npu7ONwwrFnY90ntc4fZpfWtSz/IBijr62KyiWO4SMqiJDKJKjzcHqfqAM1D7Jfu+dSS dxldmSdbCb8bWUEiwfISlXAzrmb6hxQAzXYkoz+kpPgp0Rkp8dBtp9y9zj5ZJ0f+shBj /C9A== X-Gm-Message-State: ALyK8tJgwtUos6S5qo3jnMIzKkOdVTMsxFKFv3pH3yr+K2z30kieg0yUp6/4MTxhB2+68T1gP6HCzrtv8JM7Vg== X-Received: by 10.157.17.169 with SMTP id v38mr19232675otf.11.1468855695007; Mon, 18 Jul 2016 08:28:15 -0700 (PDT) In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2607:f8b0:4003:c06::22c X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 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:205809 Archived-At: --94eb2c1917a0ed6a6b0537ea9d69 Content-Type: text/plain; charset=UTF-8 Thanks Stefan, My comments are below. On Mon, Jul 18, 2016 at 9:40 AM Stefan Monnier wrote: > Actually, after thinking about it, I got to the conclusion that as long > as you don't try to handle the multiple-window case correctly, you're > probably better off with post-command-hook. > Yes, post-command-hook works fine for my use case. More or less. Try M-x nhexl-mode RET and the C-x 5 2 and then move > around the buffer and compare the two window's highlighting of the > "current line". You'll see that only one of the two is correct. > I see what you mean. I tried that. If I wanted to do something like that, C-x 4 c (indirect buffer cloning) seems like a good solution. > > From quick trial, I saw the line numbers change in the whole buffer even > > when I moved the cursor horizontally. > > You need to always call it with point at BOL. > Got it. Below works (setq nlinum--current-line (save-excursion ; works (unless (bolp) (forward-line 0)) (nlinum--line-number-at-pos))) > In the below updated patch, I attempt to do this (code commented out > > in the patch), but failed. I tried removing the text properties on > > the current and last lines, but it is not working. I'll give more > > time to understand tomorrow. But if you can point out the issue with > > those remove-text-properties, that will be great. > > Sorry, the problem didn't jump at me. They look pretty good. > Do you mean that the commented code to remove text properties from last/current lines should have worked? Here is a longish gif video (1 min 20 secs) demonstrating the issue I see when removing text properties from the whole visible window vs just curr+last lines: http://i.imgur.com/GN3zTlJ.gifv Note that nlinum--current-line-update is called in post-command-hook. > Clearly the above change should come with the corresponding change in > nlinum-current-line-face (i.e. it shouldn't inherit from `linum' face > any more). But either way is OK. > OK, I will then stick with linum inheritance in the defface itself so that people can change that in their themes if they wish. > Oh, and nowadays the convention is to use "-face" only for > names of variables whose value is a face, but not for the face names > themselves. The font-lock-foo-face faces are the main exceptions > because they're already so omnipresent. > OK, I have made that change locally; will be seen in the final patch. > > I have also made one cosmetic change .. Instead of `t' as argument > values, > > I have replaced them with `:local' or `:contextual' as appropriate. > > Fine. I often use 'local (rather than :local) for that same purpose, > but I don't really care about the color of this shed. > Thanks for confirming. > > +(defvar-local nlinum--last-line 0 > > + "Store line number where the point was before it moved to the current > > line.") > > No reason to keep this as a global var (but I'd rename > nlinum--current-line to nlinum--last-line). > Correct. nlinum--last-line does not need to be a defvar; I have now made it a let-bound var. I did not understand why nlinum--current-line should be renamed as nlinum--last-line; because that var is storing the current line number and we are using that to highlight the current line number. > +(defun nlinum--current-line-update () > > + "Update current line number, flush text properties for last and > current > > line." > > Actually, it shouldn't (and doesn't) flush text-properties. It should > only update the current-line highlighting or cause it to be > updated later. > I did not understand this too. I refer to the remove-text-properties action as "flushing". That function is updating the nlinum--current-line defvar and removing text properties from the whole visible window (or not working presently, current and last lines). -- Kaushal Modi --94eb2c1917a0ed6a6b0537ea9d69 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Thanks Stefan,

My comments are be= low.

On Mon, Jul 18, 20= 16 at 9:40 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
Actually, after thinking about it, I got to the conclusion that as l= ong
as you don't try to handle the multiple-window case correctly, you'= re
probably better off with post-command-hook.

=
Yes, post-command-hook works fine for my use case.

More or less.=C2=A0 Try M-x nhexl-mode RET = and the C-x 5 2 and then move
around the buffer and compare the two window's highlighting of the
"current line".=C2=A0 You'll see that only one of the two is = correct.

I see what you mean. I tried t= hat. If I wanted to do something like that, C-x 4 c (indirect buffer clonin= g) seems like a good solution.
=C2=A0
> From quick trial, I saw the line numbers change in the whole buffer ev= en
> when I moved the cursor horizontally.

You need to always call it with point at BOL.

Got it. Below works

(setq nlinum--current-l= ine (save-excursion =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0; works
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(unless (bolp)
=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(forward-line 0))
=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0(nlinum--line-number-at-pos)))

=
> In the below updated patch, I attempt t= o do this (code commented out
> in the patch), but failed.=C2=A0 I tried removing the text properties = on
> the current and last lines, but it is not working.=C2=A0 I'll give= more
> time to understand tomorrow.=C2=A0 But if you can point out the issue = with
> those remove-text-properties, that will be great.

Sorry, the problem didn't jump at me.=C2=A0 They look pretty good.
<= /blockquote>

Do you mean that the commented code to remo= ve text properties from last/current lines should have worked?
Here is a longish gif video (1 min 20 secs) demonstrating the = issue I see when removing text properties from the whole visible window vs = just curr+last lines:=C2=A0http= ://i.imgur.com/GN3zTlJ.gifv

Note that=C2=A0nli= num--current-line-update is called in post-command-hook.
=C2=A0
Clearly the above change should come wit= h the corresponding change in
nlinum-current-line-face (i.e. it shouldn't inherit from `linum' fa= ce
any more).=C2=A0 But either way is OK.

= OK, I will then stick with linum inheritance in the defface itself so that = people can change that in their themes if they wish.
=C2=A0
=
Oh, and nowadays the convention is to use "-face" only for
names of variables whose value is a face, but not for the face names
themselves.=C2=A0 The font-lock-foo-face faces are the main exceptions
because they're already so omnipresent.

=
OK, I have made that change locally; will be seen in the final patch.<= /div>
=C2=A0
> I have also made = one cosmetic change .. Instead of `t' as argument values,
> I have replaced them with `:local' or `:contextual' as appropr= iate.

Fine.=C2=A0 I often use 'local (rather than :local) for that same purpo= se,
but I don't really care about the color of this shed.
<= div>
Thanks for confirming.
=C2=A0
> +(defvar-local nlinum--last-line 0
> +=C2=A0 "Store line number where the point was before it moved to= the current
> line.")

No reason to keep this as a global var (but I'd rename
nlinum--current-line to nlinum--last-line).

=
Correct. nlinum--last-line does not need to be a defvar; I have now ma= de it a let-bound var. I did not understand why nlinum--current-line should= be renamed as nlinum--last-line; because that var is storing the current l= ine number and we are using that to highlight the current line number.

> +(defun nlinum--current-line-update ()
> +=C2=A0 "Update current line number, flush text properties for la= st and current
> line."

Actually, it shouldn't (and doesn't) flush text-properties.=C2=A0 I= t should
only update the current-line highlighting or cause it to be
updated later.

I did not understand thi= s too. I refer to the remove-text-properties action as "flushing"= . That function is updating the nlinum--current-line defvar and removing te= xt properties from the whole visible window (or not working presently, curr= ent and last lines).=C2=A0
--
<= /div>

Kaushal Modi

--94eb2c1917a0ed6a6b0537ea9d69--