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