GNU bug report logs - #66431
[PATCH] Fix reset treesit--explorer-last-node when explorer buffer was killed

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66431 in the body.
You can then email your comments to 66431 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: nvp <noah.v.peart <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix reset treesit--explorer-last-node when explorer buffer
 was killed
Date: Mon, 9 Oct 2023 23:02:48 -0700

[Message part 1 (text/plain, inline)]

Tags: patch


Bug: After `treesit-explorer-mode` is enabled in a buffer and its
associated `treesit--explorer-buffer` is killed, a subsequent call
to `treesit-explorer-mode` initially displays an empty explorer
buffer b/c `treesit--explorer-refresh` sees old value for
`treesit--explorer-last-node`.

* lisp/treesit.el (treesit-explorer-mode): reset
`treesit--explorer-last-node` when `treesit--explorer-buffer` was killed

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.33, cairo version 1.16.0) of 2023-10-05 built on noah-X580VD
Repository revision: 505c80623049d9e181918acdac8229c9a2041b1e
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Ubuntu 22.04.3 LTS

Configured using:
 'configure --prefix=/usr/local --with-modules --with-tree-sitter
--with-threads --with-x-toolkit=gtk3 --with-xwidgets --with-gnutls
--with-json --with-mailutils --with-jpeg --with-png --with-rsvg
--with-tiff --with-xml2 --with-xpm --with-imagemagick CC=gcc-12
CXX=gcc-12'

[Message part 2 (text/html, inline)]

[fix-treesit-explorer-last-node.patch (text/x-patch, attachment)]

Message #8 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: nvp <noah.v.peart <at> gmail.com>, Yuan Fu <casouri <at> gmail.com>
Cc: 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Sat, 14 Oct 2023 11:17:42 +0300

> From: nvp <noah.v.peart <at> gmail.com>
> Date: Mon, 9 Oct 2023 23:02:48 -0700
> 
> Tags: patch
> 
> Bug: After `treesit-explorer-mode` is enabled in a buffer and its
> associated `treesit--explorer-buffer` is killed, a subsequent call
> to `treesit-explorer-mode` initially displays an empty explorer
> buffer b/c `treesit--explorer-refresh` sees old value for
> `treesit--explorer-last-node`.
> 
> * lisp/treesit.el (treesit-explorer-mode): reset
> `treesit--explorer-last-node` when `treesit--explorer-buffer` was killed
> 
> In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
> 3.24.33, cairo version 1.16.0) of 2023-10-05 built on noah-X580VD
> Repository revision: 505c80623049d9e181918acdac8229c9a2041b1e
> Repository branch: master
> Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
> System Description: Ubuntu 22.04.3 LTS

Yuan, could you please look into this?  Is the patch OK to go in, and
if so, should it be installed on emacs-29?

Message #11 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: nvp <noah.v.peart <at> gmail.com>
Cc: 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Sat, 14 Oct 2023 10:08:09 -0700

> On Oct 9, 2023, at 11:02 PM, nvp <noah.v.peart <at> gmail.com> wrote:
> 
> Tags: patch
> 
> 
> Bug: After `treesit-explorer-mode` is enabled in a buffer and its
> associated `treesit--explorer-buffer` is killed, a subsequent call
> to `treesit-explorer-mode` initially displays an empty explorer
> buffer b/c `treesit--explorer-refresh` sees old value for
> `treesit--explorer-last-node`.
> 
> * lisp/treesit.el (treesit-explorer-mode): reset
> `treesit--explorer-last-node` when `treesit--explorer-buffer` was killed

Hmm, I can’t reproduce what you described. Besides, treesit--explorer-last-node is only set in the source buffer, not the explorer buffer. But the patch tries to reset it for the explorer buffer. Also treesit--explorer-last-node is reset at the end of treesit-explore-mode.

We can start from reliably reproducing the bug you are seeing, and see what’s the true cause of it.

Yuan

Message #14 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: nvp <noah.v.peart <at> gmail.com>, 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Sat, 14 Oct 2023 10:08:41 -0700

> On Oct 14, 2023, at 1:17 AM, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>> From: nvp <noah.v.peart <at> gmail.com>
>> Date: Mon, 9 Oct 2023 23:02:48 -0700
>> 
>> Tags: patch
>> 
>> Bug: After `treesit-explorer-mode` is enabled in a buffer and its
>> associated `treesit--explorer-buffer` is killed, a subsequent call
>> to `treesit-explorer-mode` initially displays an empty explorer
>> buffer b/c `treesit--explorer-refresh` sees old value for
>> `treesit--explorer-last-node`.
>> 
>> * lisp/treesit.el (treesit-explorer-mode): reset
>> `treesit--explorer-last-node` when `treesit--explorer-buffer` was killed
>> 
>> In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
>> 3.24.33, cairo version 1.16.0) of 2023-10-05 built on noah-X580VD
>> Repository revision: 505c80623049d9e181918acdac8229c9a2041b1e
>> Repository branch: master
>> Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
>> System Description: Ubuntu 22.04.3 LTS
> 
> Yuan, could you please look into this?  Is the patch OK to go in, and
> if so, should it be installed on emacs-29?

I’ll look at it, thanks for the reminder.

Yuan

Message #17 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: nvp <noah.v.peart <at> gmail.com>
To: Yuan Fu <casouri <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Sat, 14 Oct 2023 21:20:34 -0700

[Message part 1 (text/plain, inline)]

Hi,
The patch is supposed to reset `treesit--explorer-last-node` in the source
buffer, just before the `(with-current-buffer treesit--explorer-buffer
...)`.
Upon trying to reproduce it now, I realized it's harder to reproduce than
I had thought -- sorry about that.
I noticed the bug (if it is a bug) initially when I was adding a function
to jump b/w source and explorer buffers, like the following:

(defun my-treesit-explorer-jump ()
  "Pop b/w source and explorer buffers."
  (interactive)
  (let ((buf
         (cond
          ((eq major-mode 'treesit--explorer-tree-mode)
           (when (buffer-live-p treesit--explorer-source-buffer)
             treesit--explorer-source-buffer))
          (t
           (unless (and treesit-explore-mode
                        (buffer-live-p treesit--explorer-buffer))
             ;; *** Without the reset here, the explorer buffer doesn't
             ;; get redrawn the first time, when treesit--explorer-last-node
             ;; is non-nil in the source buffer ***
             ;; (setq-local treesit--explorer-last-node nil)
             (cl-letf (((symbol-function (function completing-read))
                        (lambda (&rest _) (symbol-name (treesit-language-at
(point))))))
               (treesit-explore-mode 1)))
           treesit--explorer-buffer))))
    (pop-to-buffer buf)))

Let me give a more precise recipe to reproduce:
1. From a c++-ts-mode buffer, call `treesit-explorer-mode`, select `cpp`.
Now there should be an explorer buffer.
2. Kill the associated explorer buffer.
3. Now, back in the c++-ts-mode buffer, `treesit--explorer-last-node`
should still have a value.
4. From that c++-ts-mode buffer, call `my-treesit-explorer-jump`, and the
explorer buffer should be empty, until
switching back to the source buffer.

This seems to me to be caused by `treesit--explorer-post-command` not
running until the source
buffer is active again.


On Sat, Oct 14, 2023 at 10:08 AM Yuan Fu <casouri <at> gmail.com> wrote:

>
>
> > On Oct 14, 2023, at 1:17 AM, Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> >> From: nvp <noah.v.peart <at> gmail.com>
> >> Date: Mon, 9 Oct 2023 23:02:48 -0700
> >>
> >> Tags: patch
> >>
> >> Bug: After `treesit-explorer-mode` is enabled in a buffer and its
> >> associated `treesit--explorer-buffer` is killed, a subsequent call
> >> to `treesit-explorer-mode` initially displays an empty explorer
> >> buffer b/c `treesit--explorer-refresh` sees old value for
> >> `treesit--explorer-last-node`.
> >>
> >> * lisp/treesit.el (treesit-explorer-mode): reset
> >> `treesit--explorer-last-node` when `treesit--explorer-buffer` was killed
> >>
> >> In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
> >> 3.24.33, cairo version 1.16.0) of 2023-10-05 built on noah-X580VD
> >> Repository revision: 505c80623049d9e181918acdac8229c9a2041b1e
> >> Repository branch: master
> >> Windowing system distributor 'The X.Org Foundation', version
> 11.0.12101004
> >> System Description: Ubuntu 22.04.3 LTS
> >
> > Yuan, could you please look into this?  Is the patch OK to go in, and
> > if so, should it be installed on emacs-29?
>
> I’ll look at it, thanks for the reminder.
>
> Yuan
>
>

[Message part 2 (text/html, inline)]

Message #20 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: nvp <noah.v.peart <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Wed, 18 Oct 2023 21:35:48 -0700

[Message part 1 (text/plain, inline)]

> On Oct 14, 2023, at 9:20 PM, nvp <noah.v.peart <at> gmail.com> wrote:
> 
> Hi,
> The patch is supposed to reset `treesit--explorer-last-node` in the source buffer, just before the `(with-current-buffer treesit--explorer-buffer ...)`.
> Upon trying to reproduce it now, I realized it's harder to reproduce than I had thought -- sorry about that.  
> I noticed the bug (if it is a bug) initially when I was adding a function to jump b/w source and explorer buffers, like the following:
> 
> (defun my-treesit-explorer-jump ()
>   "Pop b/w source and explorer buffers."
>   (interactive)
>   (let ((buf
>          (cond
>           ((eq major-mode 'treesit--explorer-tree-mode)
>            (when (buffer-live-p treesit--explorer-source-buffer)
>              treesit--explorer-source-buffer))
>           (t
>            (unless (and treesit-explore-mode
>                         (buffer-live-p treesit--explorer-buffer))
>              ;; *** Without the reset here, the explorer buffer doesn't
>              ;; get redrawn the first time, when treesit--explorer-last-node
>              ;; is non-nil in the source buffer ***
>              ;; (setq-local treesit--explorer-last-node nil)
>              (cl-letf (((symbol-function (function completing-read))
>                         (lambda (&rest _) (symbol-name (treesit-language-at (point))))))
>                (treesit-explore-mode 1)))
>            treesit--explorer-buffer))))
>     (pop-to-buffer buf)))
> 
> Let me give a more precise recipe to reproduce:
> 1. From a c++-ts-mode buffer, call `treesit-explorer-mode`, select `cpp`.  Now there should be an explorer buffer.
> 2. Kill the associated explorer buffer.
> 3. Now, back in the c++-ts-mode buffer, `treesit--explorer-last-node` should still have a value.
> 4. From that c++-ts-mode buffer, call `my-treesit-explorer-jump`, and the explorer buffer should be empty, until
> switching back to the source buffer.
> 
> This seems to me to be caused by `treesit--explorer-post-command` not running until the source 
> buffer is active again.

Thank you, I think I see the problem now. Could you try the below patch and see it fixes your problem? Also, sorry for the late reply, I meant to reply sooner but couldn’t find the time to figure out what exact was the cause :-) I was initially a bit confused since we already do set last-node to nil.

Yuan

[fix-last-node.diff (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

Message #23 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: nvp <noah.v.peart <at> gmail.com>
To: Yuan Fu <casouri <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Fri, 20 Oct 2023 14:22:37 -0700

[Message part 1 (text/plain, inline)]

That fixes the problem!

However, the reason I initially put the reset inside the `(unless
(buffer-live-p treesit--explorer-buffer) ...)`
in `treesit-explore-mode` was b/c it looked like there was an optimization
happening in
`treesit--explorer-refresh` where it does this check

           ;; If we didn't edit the buffer nor change the top-level
           ;; node, don't redraw the whole syntax tree.
           (highlight-only (treesit-node-eq
                            top-level treesit--explorer-last-node))

I don't know if that is something you'd want to keep, but just pointing it
out in case.  I think
the initial patch works as well, but still allows that check to work when
the explorer buffer hasn't
been killed.

Thankyou!

On Wed, Oct 18, 2023 at 9:36 PM Yuan Fu <casouri <at> gmail.com> wrote:

>
>
> > On Oct 14, 2023, at 9:20 PM, nvp <noah.v.peart <at> gmail.com> wrote:
> >
> > Hi,
> > The patch is supposed to reset `treesit--explorer-last-node` in the
> source buffer, just before the `(with-current-buffer
> treesit--explorer-buffer ...)`.
> > Upon trying to reproduce it now, I realized it's harder to reproduce
> than I had thought -- sorry about that.
> > I noticed the bug (if it is a bug) initially when I was adding a
> function to jump b/w source and explorer buffers, like the following:
> >
> > (defun my-treesit-explorer-jump ()
> >   "Pop b/w source and explorer buffers."
> >   (interactive)
> >   (let ((buf
> >          (cond
> >           ((eq major-mode 'treesit--explorer-tree-mode)
> >            (when (buffer-live-p treesit--explorer-source-buffer)
> >              treesit--explorer-source-buffer))
> >           (t
> >            (unless (and treesit-explore-mode
> >                         (buffer-live-p treesit--explorer-buffer))
> >              ;; *** Without the reset here, the explorer buffer doesn't
> >              ;; get redrawn the first time, when
> treesit--explorer-last-node
> >              ;; is non-nil in the source buffer ***
> >              ;; (setq-local treesit--explorer-last-node nil)
> >              (cl-letf (((symbol-function (function completing-read))
> >                         (lambda (&rest _) (symbol-name
> (treesit-language-at (point))))))
> >                (treesit-explore-mode 1)))
> >            treesit--explorer-buffer))))
> >     (pop-to-buffer buf)))
> >
> > Let me give a more precise recipe to reproduce:
> > 1. From a c++-ts-mode buffer, call `treesit-explorer-mode`, select
> `cpp`.  Now there should be an explorer buffer.
> > 2. Kill the associated explorer buffer.
> > 3. Now, back in the c++-ts-mode buffer, `treesit--explorer-last-node`
> should still have a value.
> > 4. From that c++-ts-mode buffer, call `my-treesit-explorer-jump`, and
> the explorer buffer should be empty, until
> > switching back to the source buffer.
> >
> > This seems to me to be caused by `treesit--explorer-post-command` not
> running until the source
> > buffer is active again.
>
> Thank you, I think I see the problem now. Could you try the below patch
> and see it fixes your problem? Also, sorry for the late reply, I meant to
> reply sooner but couldn’t find the time to figure out what exact was the
> cause :-) I was initially a bit confused since we already do set last-node
> to nil.
>
> Yuan
>
>
>
>

[Message part 2 (text/html, inline)]

Message #26 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: nvp <noah.v.peart <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Sat, 21 Oct 2023 11:33:20 -0700

> On Oct 20, 2023, at 2:22 PM, nvp <noah.v.peart <at> gmail.com> wrote:
> 
> That fixes the problem!
> 
> However, the reason I initially put the reset inside the `(unless (buffer-live-p treesit--explorer-buffer) ...)`
> in `treesit-explore-mode` was b/c it looked like there was an optimization happening in 
> `treesit--explorer-refresh` where it does this check 
> 
>            ;; If we didn't edit the buffer nor change the top-level
>            ;; node, don't redraw the whole syntax tree.
>            (highlight-only (treesit-node-eq
>                             top-level treesit--explorer-last-node))
> 
> I don't know if that is something you'd want to keep, but just pointing it out in case.  I think
> the initial patch works as well, but still allows that check to work when the explorer buffer hasn't 
> been killed.

Oh that’s fine, treesit-explore-mode always wipes everything and start from a clean slate. That optimization is for when the user moves point in the source buffer when explore-mode is on. If you’d like to send a patch that does roughly what I did in the patch I sent, I’d love to merge it. Otherwise I can fix it myself, too.

The initial patch could be a bit confusing to the readers since it sets last-node twice, and it’s not clear why.

Thanks,
Yuan

Message #29 received at 66431 <at> debbugs.gnu.org (full text, mbox):

From: nvp <noah.v.peart <at> gmail.com>
To: Yuan Fu <casouri <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 66431 <at> debbugs.gnu.org
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Sat, 21 Oct 2023 17:40:40 -0700

[Message part 1 (text/plain, inline)]

Ok that makes sense, thanks for clearing that up for me, your fix looks
good. I'm loving this package!

On Sat, Oct 21, 2023 at 11:33 AM Yuan Fu <casouri <at> gmail.com> wrote:

>
>
> > On Oct 20, 2023, at 2:22 PM, nvp <noah.v.peart <at> gmail.com> wrote:
> >
> > That fixes the problem!
> >
> > However, the reason I initially put the reset inside the `(unless
> (buffer-live-p treesit--explorer-buffer) ...)`
> > in `treesit-explore-mode` was b/c it looked like there was an
> optimization happening in
> > `treesit--explorer-refresh` where it does this check
> >
> >            ;; If we didn't edit the buffer nor change the top-level
> >            ;; node, don't redraw the whole syntax tree.
> >            (highlight-only (treesit-node-eq
> >                             top-level treesit--explorer-last-node))
> >
> > I don't know if that is something you'd want to keep, but just pointing
> it out in case.  I think
> > the initial patch works as well, but still allows that check to work
> when the explorer buffer hasn't
> > been killed.
>
> Oh that’s fine, treesit-explore-mode always wipes everything and start
> from a clean slate. That optimization is for when the user moves point in
> the source buffer when explore-mode is on. If you’d like to send a patch
> that does roughly what I did in the patch I sent, I’d love to merge it.
> Otherwise I can fix it myself, too.
>
> The initial patch could be a bit confusing to the readers since it sets
> last-node twice, and it’s not clear why.
>
> Thanks,
> Yuan

[Message part 2 (text/html, inline)]

Message #34 received at 66431-done <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: nvp <noah.v.peart <at> gmail.com>
Cc: 66431-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#66431: [PATCH] Fix reset treesit--explorer-last-node when
 explorer buffer was killed
Date: Sat, 21 Oct 2023 20:35:48 -0700

> On Oct 21, 2023, at 5:40 PM, nvp <noah.v.peart <at> gmail.com> wrote:
> 
> Ok that makes sense, thanks for clearing that up for me, your fix looks good. I'm loving this package!

Thanks, I pushed the fix to emacs-29. I’m glad you are finding it useful!

Yuan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.