~protesilaos/denote

15 3

[PATCH] Display context of identifier in backlinks buffer with xref

Details
Message ID
<86r0yvzm12.fsf@nobiot.com>
DKIM signature
missing
Download raw message
Hi Prot,

I am sending another idea.  This patch is also related to the discussion I have seen in another thread:

   Show some context in the Backlinksbuffer
https://lists.sr.ht/~protesilaos/denote/%3C74322f7a-f3a3-b115-ffa6-aad237f8f765%40eh-is.de%3E#%3C87czaz21zy.fsf@protesilaos.com%3E

You may considered this change to be too large a change to consider, or goes
against the simplicity and contained scope of Denote's design philosophy.  In
this case, there is no issue on my end; I will understand and respect your
approach.  I would like to show a possibility for a discussion.

To facilitate the conversation, I'm also attaching some screen shots to show what the result of the changes will look like.  Note that I have completely personalized the file naming schema to my own one but the functionality is unchanged (except for this patch). You will see that in the screen shots.  I have also added the following minor modes to the backlinks-mode:

(add-hook 'denote-backlinks-mode-hook #'olivetti-mode)
(add-hook 'denote-backlinks-mode-hook #'outline-minor-mode)
Details
Message ID
<87o7tzl1zv.fsf@protesilaos.com>
In-Reply-To
<86r0yvzm12.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Tue, 25 Oct 2022 22:27:37 +0200
>
> Hi Prot,

Good day Noboru,

> I am sending another idea.  This patch is also related to the
> discussion I have seen in another thread:
>
>    Show some context in the Backlinksbuffer
> https://lists.sr.ht/~protesilaos/denote/%3C74322f7a-f3a3-b115-ffa6-aad237f8f765%40eh-is.de%3E#%3C87czaz21zy.fsf@protesilaos.com%3E
>
> You may considered this change to be too large a change to consider, or goes
> against the simplicity and contained scope of Denote's design philosophy.  In
> this case, there is no issue on my end; I will understand and respect your
> approach.  I would like to show a possibility for a discussion.

I installed your patch and pushed it to the main branch as commit
fcefc1d.  Thank you!  I now need to update the manual accordingly.

Regarding the scope: I think your change here improves the interface of
what we had available.  That is fine.

> To facilitate the conversation, I'm also attaching some screen shots
> to show what the result of the changes will look like.  Note that I
> have completely personalized the file naming schema to my own one but
> the functionality is unchanged (except for this patch). You will see
> that in the screen shots.  I have also added the following minor modes
> to the backlinks-mode:
>
> (add-hook 'denote-backlinks-mode-hook #'olivetti-mode)
> (add-hook 'denote-backlinks-mode-hook #'outline-minor-mode)

I like it!  I now wonder if we should update the user option
'denote-link-backlinks-display-buffer-action' to make it easier for the
user to display the buffer in a side window.  I say we keep the default
of showing it below the current window, just so we do not disrupt too
many established patterns at once.

* * *

Some comments about the patch:

1. Yes, I like the option to re-use the backlinks buffer.  This can
   probably be combined with the aforementioned display buffer action to
   make the buffer dedicated to its window.

2. I am not familiar with xref--xref-buffer-mode, though deriving the
   'denote-backlinks-mode' from it feels wrong because it is a private
   form.  Is this change away from special-mode necessary?  Do we have
   another option?

3. We need to find a workaround for 'xref--apply-truncation' so that we
   can continue to depend on Emacs 27.2.

4. Perhaps we can prettify the backlinks' buffer further by adding more
   space and/or faces to it.

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<86lep24jp9.fsf@nobiot.com>
In-Reply-To
<87o7tzl1zv.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Hi Prot,

Sorry, again I could not reply to the list.  How do you do it?

Thank you for merging.  I've creted two patches (attached as separate
files).  More coming.

Replying to your comments:

  > Some comments about the patch:
  > 1. Yes, I like the option to re-use the backlinks buffer.  This can
  >    probably be combined with the aforementioned display buffer action
  >    to make the buffer dedicated to its window.

Patch '0002-Add-user-option-to-use-original-backlinks-buffer-or-.patch'
addresses this.

  > 2. I am not familiar with xref--xref-buffer-mode, though deriving the
  >    'denote-backlinks-mode' from it feels wrong because it is a private
  >    form.  Is this change away from special-mode necessary?  Do we have
  >    another option?

We can go back to special-mode.  No problem.  Derivation from that mode
was mostly for convenience to use the same keybindings in
'denote-backlinks-mode' as in the xref mode.  Let me work on the patch
right after this email.

  > 3. We need to find a workaround for 'xref--apply-truncation' so that
  >    we can continue to depend on Emacs 27.2.

I will install 27.2 and see issues myself.

  > 4. Perhaps we can prettify the backlinks' buffer further by adding
  >    more space and/or faces to it.

Any recommendation?  I will add a blank space.  I'm not good at faces
(neither design and implemenation).  Perhpas you have a good idea?

Thanks!
nobiot
Details
Message ID
<86tu3qwl7w.fsf@nobiot.com>
In-Reply-To
<87o7tzl1zv.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Two more patches attached, addressing your comments 2 and 4.  I will
look into your comment 3 about Emacs version 27.2 later.  I don't think
it will be today, but hopefully soon.

Thanks.
nobiot
Details
Message ID
<86sfja78ik.fsf@nobiot.com>
In-Reply-To
<87o7tzl1zv.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Hi Prot,

One more for the evening :)

Has Denote been ever tested with Emacs 27.2?

I have got Emacs 27.2, checked out commit bc035823, which is the one
before my first patch for backlinks buffer refactoring.  There is
already an error where function 'xref--alistify' is called (line 1137 in
the source code in this commit).  It requires 3 arguments in version
27.2.  In version one this has changed and it requires only 2 arguments
(I'm attaching an image I took from version 27.2).

I can only assume that it would have failed before my patches.  If you
have tested Denote with 27.2 successfully, how did you manage the
difference in the signature of the built-in function?

nobiot
Details
Message ID
<86ilk6uzf4.fsf@nobiot.com>
In-Reply-To
<87o7tzl1zv.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Made patches to make the current version compatible with 27.2.  I have
tested both cases where user option 'denote-backlilnks-show-context' is
on and off for both 27.2 and 28.1.

I have noted this in the commit text: TODO: It is likely that these two
functions can be made into one to be comptabile for both Emacs versions.
Further work is requried to refactor them.

Thanks.
nobiot
Details
Message ID
<87pmeef4zv.fsf@protesilaos.com>
In-Reply-To
<86sfja78ik.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Wed, 26 Oct 2022 20:19:15 +0200
>
> Hi Prot,

Hello Noboru,

> One more for the evening :)

Thank you!  I am just replying to this message, though I got all your
patches.  I will work on them now.

> Has Denote been ever tested with Emacs 27.2?

Only lightly.  I think we should rethink this dependency, as it holds us
back and may not be worth the trouble.

> I have got Emacs 27.2, checked out commit bc035823, which is the one
> before my first patch for backlinks buffer refactoring.  There is
> already an error where function 'xref--alistify' is called (line 1137 in
> the source code in this commit).  It requires 3 arguments in version
> 27.2.  In version one this has changed and it requires only 2 arguments
> (I'm attaching an image I took from version 27.2).
>
> I can only assume that it would have failed before my patches.  If you
> have tested Denote with 27.2 successfully, how did you manage the
> difference in the signature of the built-in function?

Yes, it would have failed in that case.  I did not test it.

* * *

I will now work on your patches.  If I have any specific comments, I
will let you know.  Thank you once again!

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<87zgdi6idt.fsf@protesilaos.com>
In-Reply-To
<86ilk6uzf4.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Wed, 26 Oct 2022 22:01:51 +0200
>
> Made patches to make the current version compatible with 27.2.  I have
> tested both cases where user option 'denote-backlilnks-show-context' is
> on and off for both 27.2 and 28.1.
>
> I have noted this in the commit text: TODO: It is likely that these two
> functions can be made into one to be comptabile for both Emacs versions.
> Further work is requried to refactor them.

Hello again Noboru,

I implemented some of your patches, but left out those that (i) are for
Emacs 27 and (ii) prettify the backlinks' buffer.

This is the current commit log, since our last exchange:

*  (HEAD -> main, origin/main, gitlab-mirror/main) ee69dae 2022-10-27 06:28:19 +0300 Protesilaos Stavrou: Update sample configuration, per b313dfc
*  7c22231 2022-10-27 06:26:39 +0300 Protesilaos Stavrou: Update minimum required Emacs version to 28.1
*  b313dfc 2022-10-27 06:23:12 +0300 Protesilaos Stavrou: Update backlinks' buffer documentation in the manual
*  9b6cf70 2022-10-27 06:16:01 +0300 Protesilaos Stavrou: Add wrapper motions for backlinks' buffer
*  f90f79f 2022-10-27 05:13:42 +0300 Protesilaos Stavrou: Make denote-link-fontify-backlinks obsolete
*  3da08ea 2022-10-27 05:13:12 +0300 Protesilaos Stavrou: Reword denote-backlilnks-show-context docstring
*  d304d79 2022-10-27 05:11:27 +0300 Protesilaos Stavrou: Reword denote--retrieve-files-in-xrefs doc string
*  bdc94d3 2022-10-26 18:30:31 +0200 Noboru Ota: Add user option to use original backlinks buffer or with context
*  bbdcc49 2022-10-26 18:07:08 +0200 Noboru Ota: Fix 'denote-link-find-backlink'

Please let me know if you are okay with them.  The "wrapper motions" are
an alternative approach to the "n" and "p" keys in the backlinks'
buffer, so that we retain the old behaviour but otherwise keep all of
the Xref functionality.  My hesitation with the xref--xref-buffer-mode
was exaggerated, as I noticed the double hyphens and thought they
signified something we should not touch.  But the way it is implemented
suggests that the maintainers simply forgot to remove the extra dash:
this code is reliable.

Also, I think it is better to have something that "just works" than ask
users to bind their own keys to the denote-backlilnks-mode-map.

I believe the way I implemented those wrappers accommodates both the old
backlinks' buffer and the new one with the added context.  Please let me
know if this is true on your end.

About Emacs 27, I think it is better we avoid all those workarounds and
just depend on Emacs 28.  No-one reported the error which indicates that
this new requirement should not pose any problems.

With regard to the prettification of the backlinks' buffer, I decided to
deprecate the denote-link-fontify-backlinks in order to avoid confusion
caused by the presence of too many options: we just handle this
internally when denote-backlinks-show-context is nil.  The reason is
that such an option only worked nicely in the generic backlinks' buffer,
but caused subtle presentation problems in the Xref-capable buffer with
the added context.  I like the standard Xref style, as it clearly
distinguishes file names from the context, while it also highlights the
matching identifier (particularly useful for multiple matches).

The other tweaks I made are stylistic (rewording phrases and the like).

I believe I have not missed anything from the latest set of patches you
sent.  If I did, it was not intentional, and please tell me about it.

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<86h6zpmhpd.fsf@nobiot.com>
In-Reply-To
<87o7tzl1zv.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
> This is the current commit log, since our last exchange:
> [... omitted ...]
> Please let me know if you are okay with them.
> [... omitted ...]
> I believe I have not missed anything from the latest set of patches you
> sent.  If I did, it was not intentional, and please tell me about it.

All well on my end.  I tested commit ee69dae; it works for (both
with/without context). Thank you. Your reasoning makes sense to me; I
agree.


> I believe the way I implemented those wrappers accommodates both the old
> backlinks' buffer and the new one with the added context.  Please let me
> know if this is true on your end.

One thing I noticed is you use 'funcall'.  You could just call the
function (?)


One additional thing I have noticed.  I am sending a patch; no
functional change. Just to align the variable name 'xrefs-alist' (xrefs,
plural) to the built-in library (Xref uses singular 'xref-alist') while
it is still "hot". 

Cheers,
nobiot
Details
Message ID
<877d0lwazr.fsf@protesilaos.com>
In-Reply-To
<86h6zpmhpd.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Thu, 27 Oct 2022 17:03:58 +0200
>
>> This is the current commit log, since our last exchange:
>> [... omitted ...]
>> Please let me know if you are okay with them.
>> [... omitted ...]
>> I believe I have not missed anything from the latest set of patches you
>> sent.  If I did, it was not intentional, and please tell me about it.
>
> All well on my end.  I tested commit ee69dae; it works for (both
> with/without context). Thank you. Your reasoning makes sense to me; I
> agree.

You are welcome and thank you for trying it out!

>> I believe the way I implemented those wrappers accommodates both the old
>> backlinks' buffer and the new one with the added context.  Please let me
>> know if this is true on your end.
>
> One thing I noticed is you use 'funcall'.  You could just call the
> function (?)

My mistake!  I just pushed an update.

> One additional thing I have noticed.  I am sending a patch; no
> functional change. Just to align the variable name 'xrefs-alist' (xrefs,
> plural) to the built-in library (Xref uses singular 'xref-alist') while
> it is still "hot". 

I installed the patch.  Thank you!

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<86edutmgmq.fsf@nobiot.com>
In-Reply-To
<87o7tzl1zv.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
All is good then :)

Thank you.  See you around.

nobiot.
Details
Message ID
<86ilk47yyz.fsf@nobiot.com>
In-Reply-To
<87o7tzl1zv.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Hi Prot,

I wanted to refactor the implementation, so I went ahead and created
patches (attached).  I have elaborated on the intent and implementation
detail in the commit message.

I will be on two-week vacation from Saturday tomorrow; this is the main
reason why I really wanted to send all the patches to you.

I realize that you would be busy with the updates on Modus theme -- it's
the theme I use and love.  Thank you :).  Please treat these patches as
another discussion starter.  There should be more elegant way to get the
backlinks logic closer to the built-in way with Xref.  But I would like
to believe that the current way is not so bad.  At least functionally, I
have tested them on my Emacs 28.1 for all the four cases of backlinks
(context on/off and two commands 'denote-link-backlinks' and
'denote-link-find-backlink').

I will not carry my laptop to the vacation but I should be able to read
emails.  If there is anything I can/should do, I will see what I can do
in mid November.

Warm regards,
nobiot
Details
Message ID
<875yg3orbo.fsf@protesilaos.com>
In-Reply-To
<86ilk47yyz.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Fri, 28 Oct 2022 17:24:36 +0200
>
> Hi Prot,

Hello nobiot,

> I wanted to refactor the implementation, so I went ahead and created
> patches (attached).  I have elaborated on the intent and implementation
> detail in the commit message.

Thank you for preparing those and for the detailed explanation!  I
installed the patches and pushed them to the main branch.  I just
followed them up with a minor change to a couple of doc strings which
placates the byte compiler.

> I will be on two-week vacation from Saturday tomorrow; this is the main
> reason why I really wanted to send all the patches to you.

Enjoy your vacation!  Don't worry about this: it can wait.

> I realize that you would be busy with the updates on Modus theme -- it's
> the theme I use and love.  Thank you :).

You are welcome!

> Please treat these patches as another discussion starter.  There
> should be more elegant way to get the backlinks logic closer to the
> built-in way with Xref.  But I would like to believe that the current
> way is not so bad.  At least functionally, I have tested them on my
> Emacs 28.1 for all the four cases of backlinks (context on/off and two
> commands 'denote-link-backlinks' and 'denote-link-find-backlink').

The only thing I can think of right now is whether we need 'user-error'
forms when no backlinks are available.  Maybe a simple 'message' is
better, since this case is not technically an "error".

> I will not carry my laptop to the vacation but I should be able to read
> emails.  If there is anything I can/should do, I will see what I can do
> in mid November.

From my side, I promise NOT to send you anything: vacations are to be
respected.  It is okay to revisit whatever remaining work afterwards.
Also, version 1.1.0 of Denote was published on 2022-10-20, so we are not
in a hurry to release a new version---the package is stable as-is.

In other words: have fun!

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<87sfhf0xp8.fsf@posteo.net>
In-Reply-To
<87zgdi6idt.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Protesilaos Stavrou <info@protesilaos.com> writes:

> About Emacs 27, I think it is better we avoid all those workarounds and
> just depend on Emacs 28.  No-one reported the error which indicates that
> this new requirement should not pose any problems.

I just saw your announcement on your blog, so I am not sure if I am
missing something.  That being said, have you considered using xref from
GNU ELPA?  It is a core package but maintains compatibility back until
Emacs 26.  Of course a line must be drawn, but it would be a pity to
drop compatibility if it weren't necessary to do so, especially
considering that Debian Stable still distributes 27.1.
Details
Message ID
<87tu1ud6kh.fsf@protesilaos.com>
In-Reply-To
<87sfhf0xp8.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Fri, 16 Dec 2022 22:51:15 +0000

Good day Philip,

> Protesilaos Stavrou <info@protesilaos.com> writes:
>
>> About Emacs 27, I think it is better we avoid all those workarounds and
>> just depend on Emacs 28.  No-one reported the error which indicates that
>> this new requirement should not pose any problems.
>
> I just saw your announcement on your blog, so I am not sure if I am
> missing something.  That being said, have you considered using xref from
> GNU ELPA?  It is a core package but maintains compatibility back until
> Emacs 26.  Of course a line must be drawn, but it would be a pity to
> drop compatibility if it weren't necessary to do so, especially
> considering that Debian Stable still distributes 27.1.

The best case scenario is to support Emacs 27.  The problem was that
Denote was broken for a while on Emacs 27 and we never heard anything
about it.  This led me to believe that no-one is using it there.
Providing a broken package is not nice for users and it also reflects
poorly on us, as we try to deliver something usable.

Xref was not the only reason.  I have had other cases in the recent past
where I was caught off guard.  Compare, for instance, the arguments of
'mapconcat' between Emacs 27 and 29, respectively:

    (mapconcat FUNCTION SEQUENCE SEPARATOR)

    (mapconcat FUNCTION SEQUENCE &optional SEPARATOR)

So I write it without the optional separator, get no warnings, and then
my code is broken on Emacs 27...

The problem I have is that I do not know how to catch those cases, other
than testing every little thing on the older versions.  A fully fledged
test suite would probably be the real solution, though I don't know how
to do this yet.

As for Debian specifically, note that we were requiring 27.2 because it
provided some Org-related feature that is not in 27.1 (this is what the
linter was saying).

If we can address those, then I am happy to do it.  There is no
principled opposition to this cause.

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<87359e5qkn.fsf@posteo.net>
In-Reply-To
<87tu1ud6kh.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Protesilaos Stavrou <info@protesilaos.com> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Fri, 16 Dec 2022 22:51:15 +0000
>
> Good day Philip,
>
>> Protesilaos Stavrou <info@protesilaos.com> writes:
>>
>>> About Emacs 27, I think it is better we avoid all those workarounds
>>> and
>>> just depend on Emacs 28.  No-one reported the error which indicates
>>> that
>>> this new requirement should not pose any problems.
>>
>> I just saw your announcement on your blog, so I am not sure if I am
>> missing something.  That being said, have you considered using xref
>> from
>> GNU ELPA?  It is a core package but maintains compatibility back until
>> Emacs 26.  Of course a line must be drawn, but it would be a pity to
>> drop compatibility if it weren't necessary to do so, especially
>> considering that Debian Stable still distributes 27.1.
>
> The best case scenario is to support Emacs 27.  The problem was that
> Denote was broken for a while on Emacs 27 and we never heard anything
> about it.  This led me to believe that no-one is using it there.
> Providing a broken package is not nice for users and it also reflects
> poorly on us, as we try to deliver something usable.

One must always also consider the possibility that if something
fundamental is immediately broken (not by necessity, just be accident),
that people are less inclined to report the error because they just
started using a package and don't have any investment or a will to
contribute -- I know that writing a sensible bug report hat kept me back
from reporting issues I have with some software...

> Xref was not the only reason.  I have had other cases in the recent past
> where I was caught off guard.  Compare, for instance, the arguments of
> 'mapconcat' between Emacs 27 and 29, respectively:
>
>     (mapconcat FUNCTION SEQUENCE SEPARATOR)
>
>     (mapconcat FUNCTION SEQUENCE &optional SEPARATOR)
>
> So I write it without the optional separator, get no warnings, and then
> my code is broken on Emacs 27...
>
> The problem I have is that I do not know how to catch those cases, other
> than testing every little thing on the older versions.  A fully fledged
> test suite would probably be the real solution, though I don't know how
> to do this yet.

Using `package-lint' should be a good start.  Since you are also a
paying sourcehut user, you could set up a CI that would run tests or at
least just byte-compile some files.

> As for Debian specifically, note that we were requiring 27.2 because it
> provided some Org-related feature that is not in 27.1 (this is what the
> linter was saying).
>
> If we can address those, then I am happy to do it.  There is no
> principled opposition to this cause.

I would be glad to help.  As I said, this is currently not a solved
problem on SourceHut, but this is not the first time I have seen
interest in the issue (there was a thread on the Org mailing list a
short while ago, and I will need the same thing with Compat eventually).
The foundations have already been set with the following Guix package
definitions, defining packages back until Emacs 24.3
(https://git.sr.ht/~pkal/guix-emacs-historical), and Sourcehut Builds
supports Guix per se.  Ludovic Courtès has offered helping out making
these more practical, so that they could be used together with Guix
images (https://man.sr.ht/builds.sr.ht/compatibility.md#guix-system).

> All the best,
> Prot
Reply to thread Export thread (mbox)