bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

Tags: patch

LSP servers like clangd generate trailing whitespace, that when rendered
as markdown causes parts of the buffer to be underlined.  Unless there
is a reason for this, I'd suggest helping out by deleting trailing
whitespace before continuing to process the documentation.



In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.41, cairo version 1.18.0) of 2024-03-08 built on peregrine
Repository revision: 4818022c7c45b04c55cf2a80ef689ee2681a5d78
Repository branch: master
System Description: Fedora Linux 39 (Workstation Edition)

Configured using:
 'configure --with-pgtk --with-native-compilation=aot
 --with-imagemagick'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Delete-trailing-whitespace-when-formatting-LSP-docum.patch --]
[-- Type: text/patch, Size: 859 bytes --]

From 0b0232193c40a39000576e49cd8fa7aada3a49e9 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Fri, 8 Mar 2024 16:02:40 +0100
Subject: [PATCH 1/2] Delete trailing whitespace when formatting LSP
 documentation

* lisp/progmodes/eglot.el (eglot--format-markup): Use
'delete-trailing-whitespace'.
---
 lisp/progmodes/eglot.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 1ec186e75f8..50820e81107 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1823,6 +1823,7 @@ eglot--format-markup
     (with-temp-buffer
       (setq-local markdown-fontify-code-blocks-natively t)
       (insert string)
+      (delete-trailing-whitespace (point-min) (point-max))
       (let ((inhibit-message t)
             (message-log-max nil)
             match)
-- 
2.44.0


[-- Attachment #3: Type: text/plain, Size: 37 bytes --]


-- 
	Philip Kaludercic on peregrine

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[João Távora]

On Fri, Mar 8, 2024 at 3:49 PM Philip Kaludercic <philipk@posteo.net> wrote:
>
> Tags: patch
>
> LSP servers like clangd generate trailing whitespace, that when rendered
> as markdown causes parts of the buffer to be underlined.  Unless there
> is a reason for this, I'd suggest helping out by deleting trailing
> whitespace before continuing to process the documentation.

Go ahead and push. But be warned I've been bitten by bugs when doing what
I thought were innocent changes to this function, so keep an eye out.

Ideally though the formatting (including these fixups) should be done
entirely by
entities other than Eglot.  I think markdown.el has something for that trailing
whitespace.  Anyway I don't have time for that, so go ahead and push.

João

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Jim Porter]

On 3/8/2024 7:48 AM, Philip Kaludercic wrote:
> Tags: patch
> 
> LSP servers like clangd generate trailing whitespace, that when rendered
> as markdown causes parts of the buffer to be underlined.  Unless there
> is a reason for this, I'd suggest helping out by deleting trailing
> whitespace before continuing to process the documentation.

In Markdown, two trailing spaces at the end of a line means "insert a 
line break here". I'm not sure we should remove that; instead, 
markdown-mode should handle this as appropriate in its view modes when 
'markdown-hide-markup-in-view-modes' is non-nil (the default).

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[João Távora]

On Fri, Mar 8, 2024 at 8:54 PM Jim Porter <jporterbugs@gmail.com> wrote:
>
> On 3/8/2024 7:48 AM, Philip Kaludercic wrote:
> > Tags: patch
> >
> > LSP servers like clangd generate trailing whitespace, that when rendered
> > as markdown causes parts of the buffer to be underlined.  Unless there
> > is a reason for this, I'd suggest helping out by deleting trailing
> > whitespace before continuing to process the documentation.
>
> In Markdown, two trailing spaces at the end of a line means "insert a
> line break here". I'm not sure we should remove that; instead,

That's precisely what I was fearing in the other thread.

> markdown-mode should handle this as appropriate in its view modes when
> 'markdown-hide-markup-in-view-modes' is non-nil (the default).

And that's probably the ideal solution too.  But as far as I remember,
markdown-view-mode (or whatever it's called) is not used for viewing the
documentation.  it is used somewhere in the pipeline to render the text in a
buffer which is then yanked using `buffer-substring` (with properties).

So I don't know how that'd work.  Maybe Philip's patch can be tweaked to leave
those specific bits of trailing whitespace alone?  And do we really want to
preserve the LSP server's notion of a line break anyway?  It has no
concept of how wide
the  Emacs window is.  I'd say paragraphs yes, line breaks no.  But
I'm probably
mistaken there too,

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Jim Porter]

On 3/8/2024 1:02 PM, João Távora wrote:
> And that's probably the ideal solution too.  But as far as I remember,
> markdown-view-mode (or whatever it's called) is not used for viewing the
> documentation.  it is used somewhere in the pipeline to render the text in a
> buffer which is then yanked using `buffer-substring` (with properties).

Yep. 'eglot--format-markup' uses 'gfm-view-mode' in a temp buffer and 
returns it as a string after calling 'buffer-string'. That should be 
fine though. For example, last year I fixed a similar issue with Eglot's 
usage of Markdown mode where backslash escapes still showed up in the 
Eldoc buffer:

   https://github.com/joaotavora/eglot/issues/188
   https://github.com/jrblevin/markdown-mode/pull/756
   https://github.com/jrblevin/markdown-mode/issues/766

I expect that something similar could be done for the trailing spaces case.

> So I don't know how that'd work.  Maybe Philip's patch can be tweaked to leave
> those specific bits of trailing whitespace alone?

I think we should leave the trailing whitespace alone in Eglot entirely; 
the underline that Philip mentioned in the original report is from 
'markdown-line-break-face', which Markdown mode uses[1] to fontify only 
the trailing whitespace that means "insert a line break".

> And do we really want to preserve the LSP server's notion of a line
> break anyway?  It has no concept of how wide the  Emacs window is.
> I'd say paragraphs yes, line breaks no.
I think we do in this case, yes. (Although I guess this depends on how 
Markdown mode handles things.) These are "hard" line breaks 
corresponding to the "<br>" tag when you convert Markdown->HTML, so they 
should always get turned into a newline.

Normally if I have some text like "foo bar\nbaz" in Markdown, that 
should get rendered as "foo bar baz": the newline is just there to make 
the source text readable, but it's treated like a space. 
'markdown-view-mode' (and 'gfm-view-mode') don't do this though, so a 
soft newline stays a newline in the "view" output. This also means that 
the Markdown line break sequence ("  \n") isn't necessary in 
'markdown-view-mode' to get a line break. Just the "\n" is sufficient. 
(In this sense, we could remove the "  " and there'd be no issues with 
the *current* version of markdown-mode.)

If I were going to make a complete fix for this, I'd do most of it in 
markdown-mode and would do the following things:

1. Translate "\n" to " " in 'markdown-view-mode'
2. Translate "  \n" to "\n" in 'markdown-view-mode'
3. Turn on 'visual-line-mode' in the ElDoc buffer to get soft line wrapping

However, as a partial fix, you could probably just set 
'markdown-line-break-face' to be invisible when 
'markdown-hide-markup[-in-view-modes]' is non-nil somewhere in Markdown 
mode. I think that would be easy, although I tried to do this and the 
obvious method didn't work, so who knows?

[1] Or *should* use, anyway. I haven't tested all possible cases.

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On 3/8/2024 1:36 PM, Jim Porter wrote:
> However, as a partial fix, you could probably just set 
> 'markdown-line-break-face' to be invisible when 
> 'markdown-hide-markup[-in-view-modes]' is non-nil somewhere in Markdown 
> mode. I think that would be easy, although I tried to do this and the 
> obvious method didn't work, so who knows?

I did a big more digging and came up with this patch to markdown-mode, 
implementing the partial fix I described above.

Does this work for you, Philip? If so, I'll submit it to the 
markdown-mode maintainers.

[-- Attachment #2: 0001-Hide-trailing-whitespace-when-hiding-markup.patch --]
[-- Type: text/plain, Size: 3265 bytes --]

From 150e14c5a48b8e38dc73130d4cb01ddf0a1afb5e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 8 Mar 2024 19:05:08 -0800
Subject: [PATCH] Hide trailing whitespace when hiding markup

See <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=69647>.
---
 CHANGES.md             |  1 +
 markdown-mode.el       |  6 +++++-
 tests/markdown-test.el | 12 ++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/CHANGES.md b/CHANGES.md
index 1e4608d..f9e694e 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -11,6 +11,7 @@
     - Add functions to move to the beginning and end of lines
       (`markdown-beginning-of-line` and `markdown-end-of-line`), and the
       variable `markdown-special-ctrl-a/e`, like Org mode.
+    - Trailing whitespace for line breaks are hidden when using `markdown-hide-markup`
 
 *   Bug fixes:
     - Don't highlight superscript/subscript in math inline/block [GH-802][]
diff --git a/markdown-mode.el b/markdown-mode.el
index 8094e02..1514edb 100644
--- a/markdown-mode.el
+++ b/markdown-mode.el
@@ -1855,6 +1855,10 @@ START and END delimit region to propertize."
   '(face markdown-markup-face invisible markdown-markup)
   "List of properties and values to apply to markup.")
 
+(defconst markdown-line-break-properties
+  '(face markdown-line-break-face invisible markdown-markup)
+  "List of properties and values to apply to line break markup.")
+
 (defconst markdown-language-keyword-properties
   '(face markdown-language-keyword-face invisible markdown-markup)
   "List of properties and values to apply to code block language names.")
@@ -2298,7 +2302,7 @@ Depending on your font, some reasonable choices are:
     (markdown--match-highlighting . ((3 markdown-markup-properties)
                                      (4 'markdown-highlighting-face)
                                      (5 markdown-markup-properties)))
-    (,markdown-regex-line-break . (1 'markdown-line-break-face prepend))
+    (,markdown-regex-line-break . (1 markdown-line-break-properties prepend))
     (markdown-match-escape . ((1 markdown-markup-properties prepend)))
     (markdown-fontify-sub-superscripts)
     (markdown-match-inline-attributes . ((0 markdown-markup-properties prepend)))
diff --git a/tests/markdown-test.el b/tests/markdown-test.el
index ae8820b..21c7a03 100644
--- a/tests/markdown-test.el
+++ b/tests/markdown-test.el
@@ -2257,6 +2257,18 @@ See GH-245."
     (should (invisible-p (point)))
     (should-not (invisible-p (1+ (point))))))
 
+(ert-deftest test-markdown-markup-hiding/line-break ()
+  "Test hiding markup for line break markup."
+  (markdown-test-string "hello  \nworld"
+    (markdown-test-range-has-property (+ 5 (point)) (+ 6 (point)) 'invisible 'markdown-markup)
+    (should-not (invisible-p (point))) ;; part of "hello"
+    (should-not (invisible-p (+ 5 (point)))) ;; part of line break
+    (should-not (invisible-p (+ 7 (point)))) ;; part of "world"
+    (markdown-toggle-markup-hiding t)
+    (should-not (invisible-p (point))) ;; part of "hello"
+    (should (invisible-p (+ 5 (point)))) ;; part of line break
+    (should-not (invisible-p (+ 7 (point)))))) ;; inside "world"
+
 ;;; Markup hiding url tests:
 
 (ert-deftest test-markdown-url-hiding/eldoc ()
-- 
2.25.1

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Philip Kaludercic]

Jim Porter <jporterbugs@gmail.com> writes:

> On 3/8/2024 1:36 PM, Jim Porter wrote:
>> However, as a partial fix, you could probably just set
>> 'markdown-line-break-face' to be invisible when
>> 'markdown-hide-markup[-in-view-modes]' is non-nil somewhere in
>> Markdown mode. I think that would be easy, although I tried to do
>> this and the obvious method didn't work, so who knows?
>
> I did a big more digging and came up with this patch to markdown-mode,
> implementing the partial fix I described above.
>
> Does this work for you, Philip? If so, I'll submit it to the
> markdown-mode maintainers.

Seems fine.  The only concern I have is if someone who enabled
`show-trailing-whitespace' would still get highlighted line-endings, but
I guess that's on markdown-mode to solve.

Does my patch have to be updated, or can we discard it then?

-- 
	Philip Kaludercic on peregrine

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Jim Porter]

I've filed a PR against markdown-mode with my patch here: 
<https://github.com/jrblevin/markdown-mode/pull/826>.

On 3/9/2024 1:02 AM, Philip Kaludercic wrote:
> Seems fine.  The only concern I have is if someone who enabled
> `show-trailing-whitespace' would still get highlighted line-endings, but
> I guess that's on markdown-mode to solve.

At least in this case, I think we should be ok, since I made 
markdown-view-mode *hide* the trailing spaces entirely (rather than just 
not fontifying them). However, there could be other trailing spaces that 
*aren't* Markdown line break syntax that could leak through.

> Does my patch have to be updated, or can we discard it then?

I think we could discard that one, unless there are still some issues 
with trailing whitespace in the Eldoc buffer.

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Philip Kaludercic]

Jim Porter <jporterbugs@gmail.com> writes:

> I've filed a PR against markdown-mode with my patch here:
> <https://github.com/jrblevin/markdown-mode/pull/826>.

1+

> On 3/9/2024 1:02 AM, Philip Kaludercic wrote:
>> Seems fine.  The only concern I have is if someone who enabled
>> `show-trailing-whitespace' would still get highlighted line-endings,
>> but
>> I guess that's on markdown-mode to solve.
>
> At least in this case, I think we should be ok, since I made
> markdown-view-mode *hide* the trailing spaces entirely (rather than
> just not fontifying them). However, there could be other trailing
> spaces that *aren't* Markdown line break syntax that could leak
> through.
>
>> Does my patch have to be updated, or can we discard it then?
>
> I think we could discard that one, unless there are still some issues
> with trailing whitespace in the Eldoc buffer.

No, that was my only issue.

-- 
	Philip Kaludercic on icterid

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Jim Porter]

On 3/9/2024 10:28 AM, Jim Porter wrote:
> I've filed a PR against markdown-mode with my patch here: 
> <https://github.com/jrblevin/markdown-mode/pull/826>.

This PR is now merged into markdown-mode, so I believe we can close this 
bug?

bug#69647: [PATCH 1/2] Delete trailing whitespace when formatting LSP documentation

[Philip Kaludercic]

Jim Porter <jporterbugs@gmail.com> writes:

> On 3/9/2024 10:28 AM, Jim Porter wrote:
>> I've filed a PR against markdown-mode with my patch here:
>> <https://github.com/jrblevin/markdown-mode/pull/826>.
>
> This PR is now merged into markdown-mode, so I believe we can close
> this bug?

Right, marking it as done.

-- 
	Philip Kaludercic on peregrine