emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [PATCH] Fix verbatim block fontification to end blocks on headlines
@ 2019-12-12  3:35 Tom Gillespie
  2019-12-12  8:40 ` Nicolas Goaziou
  0 siblings, 1 reply; 10+ messages in thread
From: Tom Gillespie @ 2019-12-12  3:35 UTC (permalink / raw)
  To: emacs-orgmode; +Cc: Nicolas Goaziou

[-- 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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-12  3:35 [PATCH] Fix verbatim block fontification to end blocks on headlines Tom Gillespie
@ 2019-12-12  8:40 ` Nicolas Goaziou
  2019-12-12 10:27   ` Tom Gillespie
  0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Goaziou @ 2019-12-12  8:40 UTC (permalink / raw)
  To: Tom Gillespie; +Cc: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-12  8:40 ` Nicolas Goaziou
@ 2019-12-12 10:27   ` Tom Gillespie
  2019-12-12 12:20     ` Adam Porter
  2019-12-18  4:38     ` Tom Gillespie
  0 siblings, 2 replies; 10+ messages in thread
From: Tom Gillespie @ 2019-12-12 10:27 UTC (permalink / raw)
  To: Tom Gillespie, emacs-orgmode

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-12 10:27   ` Tom Gillespie
@ 2019-12-12 12:20     ` Adam Porter
  2019-12-18  4:38     ` Tom Gillespie
  1 sibling, 0 replies; 10+ messages in thread
From: Adam Porter @ 2019-12-12 12:20 UTC (permalink / raw)
  To: emacs-orgmode

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
@ 2019-12-13 23:25 Tom Gillespie
  2019-12-14 14:45 ` Nicolas Goaziou
  2020-02-12  8:38 ` Bastien
  0 siblings, 2 replies; 10+ messages in thread
From: Tom Gillespie @ 2019-12-13 23:25 UTC (permalink / raw)
  To: emacs-orgmode

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))

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-13 23:25 Tom Gillespie
@ 2019-12-14 14:45 ` Nicolas Goaziou
  2020-02-12  8:38 ` Bastien
  1 sibling, 0 replies; 10+ messages in thread
From: Nicolas Goaziou @ 2019-12-14 14:45 UTC (permalink / raw)
  To: Tom Gillespie; +Cc: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-12 10:27   ` Tom Gillespie
  2019-12-12 12:20     ` Adam Porter
@ 2019-12-18  4:38     ` Tom Gillespie
  2019-12-19 13:09       ` Nicolas Goaziou
  2019-12-26 18:47       ` Bastien
  1 sibling, 2 replies; 10+ messages in thread
From: Tom Gillespie @ 2019-12-18  4:38 UTC (permalink / raw)
  To: emacs-orgmode

[-- 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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-18  4:38     ` Tom Gillespie
@ 2019-12-19 13:09       ` Nicolas Goaziou
  2019-12-26 18:47       ` Bastien
  1 sibling, 0 replies; 10+ messages in thread
From: Nicolas Goaziou @ 2019-12-19 13:09 UTC (permalink / raw)
  To: Tom Gillespie; +Cc: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-18  4:38     ` Tom Gillespie
  2019-12-19 13:09       ` Nicolas Goaziou
@ 2019-12-26 18:47       ` Bastien
  1 sibling, 0 replies; 10+ messages in thread
From: Bastien @ 2019-12-26 18:47 UTC (permalink / raw)
  To: Tom Gillespie; +Cc: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Fix verbatim block fontification to end blocks on headlines
  2019-12-13 23:25 Tom Gillespie
  2019-12-14 14:45 ` Nicolas Goaziou
@ 2020-02-12  8:38 ` Bastien
  1 sibling, 0 replies; 10+ messages in thread
From: Bastien @ 2020-02-12  8:38 UTC (permalink / raw)
  To: Tom Gillespie; +Cc: emacs-orgmode

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-02-12  8:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-12  3:35 [PATCH] Fix verbatim block fontification to end blocks on headlines Tom Gillespie
2019-12-12  8:40 ` Nicolas Goaziou
2019-12-12 10:27   ` Tom Gillespie
2019-12-12 12:20     ` Adam Porter
2019-12-18  4:38     ` Tom Gillespie
2019-12-19 13:09       ` Nicolas Goaziou
2019-12-26 18:47       ` Bastien
  -- strict thread matches above, loose matches on Subject: below --
2019-12-13 23:25 Tom Gillespie
2019-12-14 14:45 ` Nicolas Goaziou
2020-02-12  8:38 ` Bastien

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).