orgtbl-mode and markdown

orgtbl-mode and markdown

[Vegard Vesterheim]

I had problems editing tables (using the minor mode orgtbl-mode) in
markdown files. 

To reproduce: 
 - visit an empty buffer in markdown mode
 - M-x orgtbl-mode
 - create a new table (C-c |)
 - try to edit a cell
 - observe that the edited text is misplaced at the end of the line

The following patch against 709bf92950fb3e9dd7425e01eb53eedad43c7262
seems to fix the problem

diff --git a/lisp/org-table.el b/lisp/org-table.el
index acad0bb..188a825 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -4233,10 +4233,10 @@ overwritten, and the table is not marked as requiring realignment."
 	    t)
 	   (eq N 1)
 	   (looking-at "[^|\n]*  +|"))
-      (let (org-table-may-need-update)
-	(goto-char (1- (match-end 0)))
-	(backward-delete-char 1)
-	(goto-char (match-beginning 0))
+      (let ((org-table-may-need-update) (mb (match-beginning 0)) (me (match-end 0)))
+	(goto-char (1- me))
+	(delete-backward-char 1)
+	(goto-char mb)
 	(self-insert-command N))
     (setq org-table-may-need-update t)
     (let* (orgtbl-mode

Re: orgtbl-mode and markdown

[Carsten Dominik]

On 18 dec. 2012, at 09:33, Vegard Vesterheim <vegard.vesterheim@uninett.no> wrote:

> I had problems editing tables (using the minor mode orgtbl-mode) in
> markdown files. 
> 
> To reproduce: 
> - visit an empty buffer in markdown mode
> - M-x orgtbl-mode
> - create a new table (C-c |)
> - try to edit a cell
> - observe that the edited text is misplaced at the end of the line
> 
> The following patch against 709bf92950fb3e9dd7425e01eb53eedad43c7262
> seems to fix the problem
> 
> diff --git a/lisp/org-table.el b/lisp/org-table.el
> index acad0bb..188a825 100644
> --- a/lisp/org-table.el
> +++ b/lisp/org-table.el
> @@ -4233,10 +4233,10 @@ overwritten, and the table is not marked as requiring realignment."
> 	    t)
> 	   (eq N 1)
> 	   (looking-at "[^|\n]*  +|"))
> -      (let (org-table-may-need-update)
> -	(goto-char (1- (match-end 0)))
> -	(backward-delete-char 1)
> -	(goto-char (match-beginning 0))
> +      (let ((org-table-may-need-update) (mb (match-beginning 0)) (me (match-end 0)))
> +	(goto-char (1- me))
> +	(delete-backward-char 1)
> +	(goto-char mb)
> 	(self-insert-command N))
>     (setq org-table-may-need-update t)
>     (let* (orgtbl-mode


This is really strange.  Does markdown mode define hooks which kick in on delete-backward-char and which to bad stuff with the match data?  Looks to me that this is a bug in markdown-mode, which should be reported to its authors.

- Carsten

Re: orgtbl-mode and markdown

[Vegard Vesterheim]

On Tue, 18 Dec 2012 14:07:36 +0100 Carsten Dominik <carsten.dominik@gmail.com> wrote:

> On 18 dec. 2012, at 09:33, Vegard Vesterheim <vegard.vesterheim@uninett.no> wrote:
>
>> I had problems editing tables (using the minor mode orgtbl-mode) in
>> markdown files. 
>> 
>> To reproduce: 
>> - visit an empty buffer in markdown mode
>> - M-x orgtbl-mode
>> - create a new table (C-c |)
>> - try to edit a cell
>> - observe that the edited text is misplaced at the end of the line
>> 
>> The following patch against 709bf92950fb3e9dd7425e01eb53eedad43c7262
>> seems to fix the problem
>> 
>> diff --git a/lisp/org-table.el b/lisp/org-table.el
>> index acad0bb..188a825 100644
>> --- a/lisp/org-table.el
>> +++ b/lisp/org-table.el
>> @@ -4233,10 +4233,10 @@ overwritten, and the table is not marked as requiring realignment."
>> 	    t)
>> 	   (eq N 1)
>> 	   (looking-at "[^|\n]*  +|"))
>> -      (let (org-table-may-need-update)
>> -	(goto-char (1- (match-end 0)))
>> -	(backward-delete-char 1)
>> -	(goto-char (match-beginning 0))
>> +      (let ((org-table-may-need-update) (mb (match-beginning 0)) (me (match-end 0)))
>> +	(goto-char (1- me))
>> +	(delete-backward-char 1)
>> +	(goto-char mb)
>> 	(self-insert-command N))
>>     (setq org-table-may-need-update t)
>>     (let* (orgtbl-mode
>
>
> This is really strange.  Does markdown mode define hooks which kick in
> on delete-backward-char and which to bad stuff with the match data?

Dunno. I just observed that (match-end 0) returned different results
immediately before and after (backward-delete-char 1). 

> Looks to me that this is a bug in markdown-mode, which should be
> reported to its authors.

Possibly, I am not proficient in elisp, hence the naive patch. But isn't
it good practise to copy the match positions immediately after matching
anyways? 

 - Vegard V -

Re: orgtbl-mode and markdown

[Carsten Dominik]

On 18 dec. 2012, at 14:39, Vegard Vesterheim <vegard.vesterheim@uninett.no> wrote:

> On Tue, 18 Dec 2012 14:07:36 +0100 Carsten Dominik <carsten.dominik@gmail.com> wrote:
> 
>> On 18 dec. 2012, at 09:33, Vegard Vesterheim <vegard.vesterheim@uninett.no> wrote:
>> 
>>> I had problems editing tables (using the minor mode orgtbl-mode) in
>>> markdown files. 
>>> 
>>> To reproduce: 
>>> - visit an empty buffer in markdown mode
>>> - M-x orgtbl-mode
>>> - create a new table (C-c |)
>>> - try to edit a cell
>>> - observe that the edited text is misplaced at the end of the line
>>> 
>>> The following patch against 709bf92950fb3e9dd7425e01eb53eedad43c7262
>>> seems to fix the problem
>>> 
>>> diff --git a/lisp/org-table.el b/lisp/org-table.el
>>> index acad0bb..188a825 100644
>>> --- a/lisp/org-table.el
>>> +++ b/lisp/org-table.el
>>> @@ -4233,10 +4233,10 @@ overwritten, and the table is not marked as requiring realignment."
>>> 	    t)
>>> 	   (eq N 1)
>>> 	   (looking-at "[^|\n]*  +|"))
>>> -      (let (org-table-may-need-update)
>>> -	(goto-char (1- (match-end 0)))
>>> -	(backward-delete-char 1)
>>> -	(goto-char (match-beginning 0))
>>> +      (let ((org-table-may-need-update) (mb (match-beginning 0)) (me (match-end 0)))
>>> +	(goto-char (1- me))
>>> +	(delete-backward-char 1)
>>> +	(goto-char mb)
>>> 	(self-insert-command N))
>>>    (setq org-table-may-need-update t)
>>>    (let* (orgtbl-mode
>> 
>> 
>> This is really strange.  Does markdown mode define hooks which kick in
>> on delete-backward-char and which to bad stuff with the match data?
> 
> Dunno. I just observed that (match-end 0) returned different results
> immediately before and after (backward-delete-char 1). 
> 
>> Looks to me that this is a bug in markdown-mode, which should be
>> reported to its authors.
> 
> Possibly, I am not proficient in elisp, hence the naive patch. But isn't
> it good practise to copy the match positions immediately after matching
> anyways? 

Well this is very safe - but if you have to assume that every elisp command does change match data behind your back, you would have to create a lot of extra code.  I think that you should report this to the markdown people.  I do not think we should have to make the change in org-mode.

I just checked markdown.el.  Below you can find a more complete description of what you can report to the markdown people:

Regards

- Carsten

Bug report for markdown.el
=====================================================================================

Markdown defines an after-change-function `markdown-check-change-for-wiki-link'.  This function is called after each buffer change.  The function contains a call to `markdown-fontify-region-wiki-links'.  If such a function would be called through the general font-lock stuff, the match data would be protected.  However, here it is called directly from a change function.  So this function should protect match data explicitly:

[lisp] Sir? diff -u markdown.el{.orig,}
--- markdown.el.orig	2012-12-18 15:28:18.000000000 +0100
+++ markdown.el	2012-12-18 15:46:50.000000000 +0100
@@ -1861,16 +1861,17 @@
 If a wiki link is found check to see if the backing file exists
 and highlight accordingly."
   (goto-char from)
-  (while (re-search-forward markdown-regex-wiki-link to t)
-    (let ((highlight-beginning (match-beginning 0))
-	  (highlight-end (match-end 0))
-	  (file-name
-	   (markdown-convert-wiki-link-to-filename (match-string 1))))
-      (if (file-exists-p file-name)
+  (save-match-data
+    (while (re-search-forward markdown-regex-wiki-link to t)
+      (let ((highlight-beginning (match-beginning 0))
+	    (highlight-end (match-end 0))
+	    (file-name
+	     (markdown-convert-wiki-link-to-filename (match-string 1))))
+	(if (file-exists-p file-name)
+	    (markdown-highlight-wiki-link
+	     highlight-beginning highlight-end markdown-link-face)
 	  (markdown-highlight-wiki-link
-	   highlight-beginning highlight-end markdown-link-face)
-	(markdown-highlight-wiki-link
-	 highlight-beginning highlight-end markdown-missing-link-face)))))
+	   highlight-beginning highlight-end markdown-missing-link-face))))))
 
 (defun markdown-extend-changed-region (from to)
   "Extend region given by FROM and TO so that we can fontify all links.

Re: orgtbl-mode and markdown

[Achim Gratz]

Carsten Dominik writes:
> Well this is very safe - but if you have to assume that every elisp
> command does change match data behind your back, you would have to
> create a lot of extra code.

Stefan Monnier advises the opposite (quote from
http://article.gmane.org/gmane.emacs.bugs/68688 ):

--8<---------------cut here---------------start------------->8---
> Okay.  On second though: it wouldn't be very useful to use the match
> data set by these functions.  Why not save the match-data here once so
> that no caller has to ever worry about it?  Is there a general policy
> that this kinda thing shouldn't be done?

Yes, the general policy is: all functions should be presumed to trash
the match-data, except for a very few exceptions.
--8<---------------cut here---------------end--------------->8---


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: orgtbl-mode and markdown

[Carsten Dominik]

On 18.12.2012, at 18:56, Achim Gratz <Stromeko@nexgo.de> wrote:

> Carsten Dominik writes:
>> Well this is very safe - but if you have to assume that every elisp
>> command does change match data behind your back, you would have to
>> create a lot of extra code.
> 
> Stefan Monnier advises the opposite (quote from
> http://article.gmane.org/gmane.emacs.bugs/68688 ):
> 
> --8<---------------cut here---------------start------------->8---
>> Okay.  On second though: it wouldn't be very useful to use the match
>> data set by these functions.  Why not save the match-data here once so
>> that no caller has to ever worry about it?  Is there a general policy
>> that this kinda thing shouldn't be done?
> 
> Yes, the general policy is: all functions should be presumed to trash
> the match-data, except for a very few exceptions.
> --8<---------------cut here---------------end--------------->8---

I am sure that Stefan would consider backward-delete-charater
such an exception, if you ask him directly.

- Carsten

> 
> 
> Regards,
> Achim.
> -- 
> +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+
> 
> SD adaptation for Waldorf rackAttack V1.04R1:
> http://Synth.Stromeko.net/Downloads.html#WaldorfSDada
> 
> 

Re: orgtbl-mode and markdown

[Carsten Dominik]

OK, I withdraw my opposition to fixing this issue in org - by my criticism toward the way markdown.el defines this hook remains.

- Carsten

On 20 dec. 2012, at 09:01, Carsten Dominik <carsten.dominik@gmail.com> wrote:

> 
> On 18.12.2012, at 18:56, Achim Gratz <Stromeko@nexgo.de> wrote:
> 
>> Carsten Dominik writes:
>>> Well this is very safe - but if you have to assume that every elisp
>>> command does change match data behind your back, you would have to
>>> create a lot of extra code.
>> 
>> Stefan Monnier advises the opposite (quote from
>> http://article.gmane.org/gmane.emacs.bugs/68688 ):
>> 
>> --8<---------------cut here---------------start------------->8---
>>> Okay.  On second though: it wouldn't be very useful to use the match
>>> data set by these functions.  Why not save the match-data here once so
>>> that no caller has to ever worry about it?  Is there a general policy
>>> that this kinda thing shouldn't be done?
>> 
>> Yes, the general policy is: all functions should be presumed to trash
>> the match-data, except for a very few exceptions.
>> --8<---------------cut here---------------end--------------->8---
> 
> I am sure that Stefan would consider backward-delete-charater
> such an exception, if you ask him directly.
> 
> - Carsten
> 
>> 
>> 
>> Regards,
>> Achim.
>> -- 
>> +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+
>> 
>> SD adaptation for Waldorf rackAttack V1.04R1:
>> http://Synth.Stromeko.net/Downloads.html#WaldorfSDada
>> 
>> 
> 


-- 
The graveyard is filled with indispensible people. -- Julia Sweeney's mother

Re: orgtbl-mode and markdown

[Bastien]

Hi Vegard and Carsten,

Carsten Dominik <carsten.dominik@gmail.com> writes:

> OK, I withdraw my opposition to fixing this issue in org - by my
> criticism toward the way markdown.el defines this hook remains.

I fixed this by saving match data for `org-delete-backward-char'
and `org-delete-char'.

I also sent a patch for Emacs -- let's see.

Thanks!

-- 
 Bastien