Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Tom Gillespie]

Adam Porter <adam@alphapapa.net> writes:

> May I recommend using the rx macro for regexps?  They are much easier
> for humans to parse, which helps reduce errors like the ones mentioned
> here.  And they are about to gain some very useful new features
> in Emacs 27.

Yep. I'll switch the regex in over to use rx.


An unrelated question.

I've written some basic tests for this and I couldn't find any other
tests that seemed to deal with fontification at all. In order to get
fontification tests to work I added a call to `font-lock-ensure' inside
`org-test-with-temp-text' (see excerpted patch bit below). Given how
frequently `org-test-with-temp-text' is used, does it make sense to
create a separate version of that macro just for testing with
fontification? I have no idea what the performance impact would be, so
any guidance is appreciated.

diff --git a/testing/org-test.el b/testing/org-test.el
index c3e21eb30..e97e2eaa4 100644
--- a/testing/org-test.el
+++ b/testing/org-test.el
@@ -198,7 +198,8 @@ otherwise place the point at the beginning of the
inserted text."
               (insert (replace-match "" nil nil inside-text))
               (goto-char (1+ (match-beginning 0))))
           (insert inside-text)
-          (goto-char (point-min))))
+          (goto-char (point-min)))
+        (font-lock-ensure (point-min) (point-max)))
        ,@body)))
 (def-edebug-spec org-test-with-temp-text (form body))

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Nicolas Goaziou]

Hello,

Tom Gillespie <tgbugs@gmail.com> writes:

> I've written some basic tests for this and I couldn't find any other
> tests that seemed to deal with fontification at all. In order to get
> fontification tests to work I added a call to `font-lock-ensure' inside
> `org-test-with-temp-text' (see excerpted patch bit below). Given how
> frequently `org-test-with-temp-text' is used, does it make sense to
> create a separate version of that macro just for testing with
> fontification? I have no idea what the performance impact would be, so
> any guidance is appreciated.

FWIW, I simply suggest not bothering writing tests for fontification.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Bastien]

Hi Tom,

Tom Gillespie <tgbugs@gmail.com> writes:

> An unrelated question.
>
> I've written some basic tests for this and I couldn't find any other
> tests that seemed to deal with fontification at all. In order to get
> fontification tests to work I added a call to `font-lock-ensure' inside
> `org-test-with-temp-text' (see excerpted patch bit below). Given how
> frequently `org-test-with-temp-text' is used, does it make sense to
> create a separate version of that macro just for testing with
> fontification? I have no idea what the performance impact would be, so
> any guidance is appreciated.

I tested this patch for a week or so, running tests not as often as I
should (obviously), but I noticed to performance issue.  I applied it
in master.

Thanks!

-- 
 Bastien

[PATCH] Fix verbatim block fontification to end blocks on headlines

[Tom Gillespie]

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

This patch is a change to how org fontifies verbatim source blocks re: [1].
Hopefully it answers Nicolas's question from that thread. Best!
Tom

On Sun, Dec 8, 2019 at 12:05 AM Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> I do not understand. Source and example blocks are verbatim blocks,
> whereas verse blocks are not. There is nothing to match, is there?

[1] Bug: headlines escape blocks:
https://lists.gnu.org/archive/html/emacs-orgmode/2019-12/msg00133.html

[-- Attachment #2: 0001-org.el-Fix-verbatim-block-fontification-to-end-block.patch --]
[-- Type: application/x-patch, Size: 2632 bytes --]

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Nicolas Goaziou]

Hello,

Tom Gillespie <tgbugs@gmail.com> writes:

> This patch is a change to how org fontifies verbatim source blocks re: [1].
> Hopefully it answers Nicolas's question from that thread. Best!

OK. Now I see what you meant. Thank you.

>  	  (when (re-search-forward
> -		 (concat "^[ \t]*#\\+end" (match-string 4) "\\>.*")
> +		 (concat "\\(^\\*\\|^[ \t]*#\\+end" (match-string 4) "\\>.*\\)")

I haven't tested it, but I think this will not produce the expected
result:

  #+begin_example
  *bold*
  #+end_example

I.e., headlines are asterisks at column 0 /followed by a space/.
  
>  		 nil t)  ;; on purpose, we look further than LIMIT
>  	    ;; We do have a matching #+end line
>  	    (setq beg-of-endline (match-beginning 0)
> @@ -5326,10 +5326,11 @@ by a #."
>  	    (add-text-properties
>  	     beg (if whole-blockline bol-after-beginline end-of-beginline)
>  	     '(face org-block-begin-line))
> -	    (add-text-properties
> -	     beg-of-endline
> -	     (min (point-max) (if whole-blockline (min (point-max) (1+ end-of-endline)) end-of-endline))
> -	     '(face org-block-end-line))
> +	    (when (not (string= (match-string 1) "*"))

`when' + `not' -> `unless'

> +	      (add-text-properties
> +	       beg-of-endline
> +	       (min (point-max) (if whole-blockline (min (point-max) (1+ end-of-endline)) end-of-endline))
> +	       '(face org-block-end-line)))

Arguably, I think 

  (min (point-max) (1+ end-of-endline))

could be replaced, for clarity, by

  (let (... (beg-of-next-line (line-beginning-position 2)) ...)
   ...
   (add-text-properties
    beg-of-endline 
    (min (point-max) (if whole-blockline beg-of-next-line end-of-endline))))

I'm also not convinced that the (min (point-max) ...) left is necessary.

Could you send an updated patch (on top of maint)?

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Tom Gillespie]

Thank you very much for the feedback. I will make the additional fixes
against maint along with the changes for clarity and send them along
tomorrow. Additional replies in line. Best,
Tom

On Thu, Dec 12, 2019 at 12:40 AM Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
>
> Hello,
>
> Tom Gillespie <tgbugs@gmail.com> writes:
>
> > This patch is a change to how org fontifies verbatim source blocks re: [1].
> > Hopefully it answers Nicolas's question from that thread. Best!
>
> OK. Now I see what you meant. Thank you.
>
> >         (when (re-search-forward
> > -              (concat "^[ \t]*#\\+end" (match-string 4) "\\>.*")
> > +              (concat "\\(^\\*\\|^[ \t]*#\\+end" (match-string 4) "\\>.*\\)")
>
> I haven't tested it, but I think this will not produce the expected
> result:
>
>   #+begin_example
>   *bold*
>   #+end_example
>
> I.e., headlines are asterisks at column 0 /followed by a space/.

Yep, you are exactly right.

>
> >                nil t)  ;; on purpose, we look further than LIMIT
> >           ;; We do have a matching #+end line
> >           (setq beg-of-endline (match-beginning 0)
> > @@ -5326,10 +5326,11 @@ by a #."
> >           (add-text-properties
> >            beg (if whole-blockline bol-after-beginline end-of-beginline)
> >            '(face org-block-begin-line))
> > -         (add-text-properties
> > -          beg-of-endline
> > -          (min (point-max) (if whole-blockline (min (point-max) (1+ end-of-endline)) end-of-endline))
> > -          '(face org-block-end-line))
> > +         (when (not (string= (match-string 1) "*"))
>
> `when' + `not' -> `unless'
>
> > +           (add-text-properties
> > +            beg-of-endline
> > +            (min (point-max) (if whole-blockline (min (point-max) (1+ end-of-endline)) end-of-endline))
> > +            '(face org-block-end-line)))
>
> Arguably, I think
>
>   (min (point-max) (1+ end-of-endline))
>
> could be replaced, for clarity, by
>
>   (let (... (beg-of-next-line (line-beginning-position 2)) ...)
>    ...
>    (add-text-properties
>     beg-of-endline
>     (min (point-max) (if whole-blockline beg-of-next-line end-of-endline))))

Probably right. I will review since I didn't look carefully at that
code, but it is a good opportunity to make some improvements while we
are looking at it.

>
> I'm also not convinced that the (min (point-max) ...) left is necessary.
>
> Could you send an updated patch (on top of maint)?
>
> Regards,
>
> --
> Nicolas Goaziou

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Adam Porter]

May I recommend using the rx macro for regexps?  They are much easier
for humans to parse, which helps reduce errors like the ones mentioned
here.  And they are about to gain some very useful new features
in Emacs 27.

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Tom Gillespie]

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

Hi,
    Here is the updated patch, including the fix to only trigger on
actual headlines (and not bold or similar), the readability
improvement for beg-of-next-line and a fix to call point-max only once
per branch. I also switched all the regex over to use the rx macro. I
left out the tests because since they would entail quite a bit of
additional work that is out of scope for this patch. The rx changes
mean that the patch is now over the tiny change limit, I went ahead
and sent a request to assign@gnu.org so we don't have to wait around
if things look good. Best,
Tom

[-- Attachment #2: 0001-org.el-Fix-verbatim-block-fontification-to-end-block.patch --]
[-- Type: application/x-patch, Size: 5139 bytes --]

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Nicolas Goaziou]

Hello,

Tom Gillespie <tgbugs@gmail.com> writes:

> Subject: org.el: Fix verbatim block fontification to end blocks on
> headlines

Applied. Thank you.

I added TINYCHANGE cookie at the end of your commit message since I do
not know if you have signed FSF papers already. If you did, please let
me know so I can record it in our contributors file.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix verbatim block fontification to end blocks on headlines

[Bastien]

Hi Tom and Nicolas,

just a heads up on this thread to confirm that Tom assignment has been
registered by the FSF on Dec. 23rd, so the patch is included in 9.3.1.

Thanks again for the patch, Tom.

Best,

-- 
 Bastien