Implemented fontification for displaying links

[haji-ali]

A follow-on from #340

This pull request implements a custom user option denote-link-display-style and a minor mode denote-fontify-links-mode which controls how links are displayed; as buttons (default), fontified or fontified and folded. Setting denote-link-display-style on its own is not sufficient. If it is set to button, the function denote-link-buttonize-buffer should be called or added to a hook. If it's set to fontify or fontify-and-fold then the newly defined minor mode denote-fontify-links-mode needs to be enabled (enabling this mode also calls denote-link-buttonize-buffer when denote-link-display-style is button). I also had to change the regex for links for org and md to capture the link captions.

I am putting this as a draft, since I am fairly new to denote and I haven't studied it extensively, so there might be something I am doing wrong or some design decisions that need to be discussed.

[protesilaos]

Thank you @haji-ali!

I will review this on Sunday.

Two quick notes:

• This change is more than ~15 lines of code. Have you assigned copyright to the Free Software Foundation? If yes, we are all good. If not, you need to do it before I can merge this. Is that okay?
• Do you think we really need the denote-link-display-style? I think that having a minor mode is basically an opt-in/opt-out mechanism. Adding an option on top may cause confusion.

[haji-ali]

• This change is more than ~15 lines of code. Have you assigned copyright to the Free Software Foundation? If yes, we are all good. If not, you need to do it before I can merge this. Is that okay?

Yes, I've done this before when I contributed to AUCTeX.

• Do you think we really need the denote-link-display-style? I think that having a minor mode is basically an opt-in/opt-out mechanism. Adding an option on top may cause confusion.

I agree about the possibility for confusion. But I think we need an option to allow the three modes of highlighting links (buttons as currently done, font-lock and font-lock with folding).

Unless you're suggesting that we set the value of the minor mode to be one of these options. I think this probably could be done, but I've never seen it before.

[haji-ali]

@protesilaos, I've removed the dependence on org-fold, removed the extra option (now always fontifying and folding), and removed the old function calls which added buttons.

There is some old code related to buttons which I haven't removed yet, in case you want to keep it for backward compatibility.

[protesilaos]

Thank you! I will review it tomorrow morning.

[protesilaos]

Thank you @haji-ali! I have just merged your patch. I made some further tweaks on top of it. I also deprecated the old code we had, as I think we do not need it going forward.

Note that I saved your PR as a patch file and installed it locally. Then I made changes on top and pushed everything. But GitHub does not seem to recognise this. I don't know if there is a way to mark this as "Merged". Your commit is here:

commit 4d1245223c726ef4e740a10a619e264e1ffa20bd
Author: Al Haji-Ali <a.hajiali@hw.ac.uk>
Date:   Fri May 10 14:49:06 2024 +0100

  Implemented a fontification method for displaying links and removed buttons

 denote.el | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 10 deletions(-)

[protesilaos]

Just to note that we have these two to still deal with:

commit ce1f59af6a06fd3c63ec8258c7083bfebbda92ad
Author: Protesilaos Stavrou <info@protesilaos.com>
Date:   Wed Jun 19 10:40:02 2024 +0300

    Add TODO and FIXME for new link fontification mode
---
 denote.el | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/denote.el b/denote.el
index 1ae00a0..f8e6c77 100644
--- a/denote.el
+++ b/denote.el
@@ -4191,6 +4191,10 @@ (eval-after-load 'markdown-mode
   '(add-hook 'markdown-follow-link-functions #'denote-link-markdown-follow))

 ;;;;; Link fontification
+
+;; TODO 2024-06-19: We need to bind RET and maybe even C-c C-o to a
+;; command that opens the link at point.  Then we may also rename this
+;; keymap.
 (defvar denote-link-mouse-map
   (let ((map (make-sparse-keymap)))
     (define-key map [mouse-2] #'denote-link-open-at-mouse)
@@ -4261,6 +4265,9 @@ (defun denote--get-link-file-path-at-point ()

 (defvar thing-at-point-provider-alist)

+;; FIXME 2024-06-19: We are missing a function that clean up all those
+;; properties when the mode is disabled.  Otherwise, we are left with
+;; invisible text, which looks broken.
 (define-minor-mode denote-fontify-links-mode
   "A minor mode to fontify and fold Denote links."
   :init-value nil

[haji-ali]

Note that I saved your PR as a patch file and installed it locally. Then I made changes on top and pushed everything. But GitHub does not seem to recognise this. I don't know if there is a way to mark this as "Merged". Your commit is here:

I am also not sure, but I can just close this PR and rebase my fork to your changes.

+;; TODO 2024-06-19: We need to bind RET and maybe even C-c C-o to a
+;; command that opens the link at point.  Then we may also rename this
+;; keymap.

I am not sure this is needed or advisable (at least by default). The reason for the thingat integration is that typically a more generic function/global bindings (such as org-open-at-point-global) opens links and this would work with this fontification. This is the approach that is followed in org-mode at least and that I was trying to replicate. I also believe RET should insert a new line, even if inside a link.

+;; FIXME 2024-06-19: We are missing a function that clean up all those
+;; properties when the mode is disabled.  Otherwise, we are left with
+;; invisible text, which looks broken.

Are you referring to the text properties? When the mode is disabled the font-lock keyword is removed and font-lock-update is called, so the properties won't be added.

Make denote-fontify-links-mode not do anything in Org mode protesilaos

I personally would prefer documenting that the mode should simply not be enabled in org-mode, or issue a warning.

[protesilaos]

From: Abdul-Lateef Haji-Ali @.***> Date: Thu, 20 Jun 2024 03:46:14 -0700

Note that I saved your PR as a patch file and installed it locally. Then I made changes on top and pushed everything. But GitHub does not seem to recognise this. I don't know if there is a way to mark this as "Merged". Your commit is here:

I am also not sure, but I can just close this PR and rebase my fork to your changes.

Sure!

+;; TODO 2024-06-19: We need to bind RET and maybe even C-c C-o to a
+;; command that opens the link at point.  Then we may also rename this
+;; keymap.

I am not sure this is needed or advised (at least by default). The reason for the thingat integration is that typically a more generic function/global bindings (such as org-open-at-point-global) opens links and this would work with this fontification. This is the approach that is followed in org-mode at least and that I was trying to replicated. I also believe RET should insert a new line, even if inside a link.

We cannot assume that the user will opt for 'org-open-at-point-global'. There are those who do not want to depend on Org in their workflow.

Before, we had the buttonisation bind RET to visit a link. This is why I thought we would want to keep this. Personally, I do not mind having another key, such that RET always inserts a newline.

+;; FIXME 2024-06-19: We are missing a function that clean up all those
+;; properties when the mode is disabled.  Otherwise, we are left with
+;; invisible text, which looks broken.

Are you referring to the text properties? When the mode is disabled the font-lock keyword is removed and font-lock-update is called, so thef properties won't be added.

I have not checked it again, but in my test when I would disable the mode the link would not be unfolded/unhidden. I tried it in a txt file.

Make denote-fontify-links-mode not do anything in Org mode protesilaos

I personally would prefer documenting that the mode should simply not be enabled in org-mode, or issue a warning.

Is this safe though? My concern is with the scenario where the user adds the mode via the text-mode-hook, which applies to Org as well. Will our fontification have any weird interaction with that of Org? If not, then covering it only in the docs is fine.

-- Protesilaos Stavrou https://protesilaos.com

[haji-ali]

We cannot assume that the user will opt for 'org-open-at-point-global'.
There are those who do not want to depend on Org in their workflow.

org-open-at-point-global was just an example for a function that uses thing-at-point 'url to get a url and navigate to it. In any case, I see the point of adding a key-binding in the local keymap, but I personally would probably remove the mapping in my config since I rarely want to shadow key-bindings with text properties (I would find it too unpredictable).

Closing this issue as the other points are being discussed in #383