bug#64205: Fix missing border cell when using orgtbl-to-table.el function

bug#64205: Fix missing border cell when using orgtbl-to-table.el function

[Jakub Ječmínek]

[-- Attachment #1.1: Type: text/plain, Size: 980 bytes --]

Hello,
I would like to take this opportunity to express my deep appreciation for
the valuable work the Emacs community has contributed to the development
and maintenance of this remarkable editor.

I've found a bug in the `orgtbl-to-table.el' function. When using
`orgtbl-to-table.el' during `org-table-export', last border cell ("|") was
omitted in the output.

Steps to reproduce:

(require 'org-table)
(insert "\n" (orgtbl-to-table.el '(("first colum" "second column") hline
("12" "34")) nil))

Previous implementation output:

| first colum | second column |
+--------------+---------------------+
|          12     |            34           < missing border cell

Current output:

| first colum | second column |
+--------------+---------------------+
|          12    |             34          |


In attachment, you'll find the smallest patch you've ever seen.

Thank you for everything you do for my beloved Emacs.

Best, Jakub Ječmínek

[-- Attachment #1.2: Type: text/html, Size: 1401 bytes --]

[-- Attachment #2: 0001-Fix-orgtbl-to-table.el-function-to-include-last-cell.patch --]
[-- Type: application/octet-stream, Size: 987 bytes --]

From 688bd0104f1e63df8f757de825481a69c4620f81 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakub=20Je=C4=8Dm=C3=ADnek?= <jecminek.k@gmail.com>
Date: Tue, 20 Jun 2023 23:30:49 +0200
Subject: [PATCH] Fix orgtbl-to-table.el function to include last cell border

Previously used buffer-size function is (1- (point-max)) which means
that last cell border was omitted.
---
 lisp/org/org-table.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
index 42f234790c5..9d3507e34b7 100644
--- a/lisp/org/org-table.el
+++ b/lisp/org/org-table.el
@@ -6134,7 +6134,7 @@ supported."
     (org-table-align)
     (replace-regexp-in-string
      "-|" "-+"
-     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (buffer-size))))))
+     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (point-max))))))
 
 (defun orgtbl-to-unicode (table params)
   "Convert the `orgtbl-mode' TABLE into a table with unicode characters.
-- 
2.39.1

bug#64205: Fix missing border cell when using orgtbl-to-table.el function

[Robert Pluim]

>>>>> On Tue, 20 Jun 2023 23:57:35 +0200, Jakub Ječmínek <jecminek.k@gmail.com> said:

    Jakub> Previously used buffer-size function is (1- (point-max)) which means
    Jakub> that last cell border was omitted.
    Jakub> ---
    Jakub>  lisp/org/org-table.el | 2 +-
    Jakub>  1 file changed, 1 insertion(+), 1 deletion(-)

    Jakub> diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
    Jakub> index 42f234790c5..9d3507e34b7 100644
    Jakub> --- a/lisp/org/org-table.el
    Jakub> +++ b/lisp/org/org-table.el
    Jakub> @@ -6134,7 +6134,7 @@ supported."
    Jakub>      (org-table-align)
    Jakub>      (replace-regexp-in-string
    Jakub>       "-|" "-+"
    Jakub> -     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (buffer-size))))))
    Jakub> +     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (point-max))))))

Thatʼs just (buffer-string).

For bonus points, rewrite it to do the replacement in the temp buffer,
and then return (buffer-string), itʼs likely to be much faster (and
will generate less garbage).

Robert
-- 

bug#64205: Fix missing cell border when using orgtbl-to-table.el function

[Jakub Ječmínek]

[-- Attachment #1.1: Type: text/plain, Size: 417 bytes --]

Hello Robert, thank you for your response.

I'm sending a new patch in the attachment. I've also instrumented the
function using the `elp' package, here are the results:

Function Name           Call Count  Elapsed Time  Average Time
orgtbl-to-table.el-old  1001        2.6707780000  0.0026681098
orgtbl-to-table.el-new  1001        2.4684200000  0.0024659540

New version is indeed a little bit faster.

Best, Jakub

[-- Attachment #1.2: Type: text/html, Size: 576 bytes --]

[-- Attachment #2: 0001-Fix-orgtbl-to-table.el-function-to-include-last-cell.patch --]
[-- Type: application/octet-stream, Size: 1245 bytes --]

From dcb7dfb20157390dd4b234c929ce45d058da48d5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakub=20Je=C4=8Dm=C3=ADnek?= <jecminek.k@gmail.com>
Date: Wed, 21 Jun 2023 15:50:31 +0200
Subject: [PATCH] Fix orgtbl-to-table.el function to include last cell border

* lisp/org/org-table.el (orgtbl-to-table.el): Perform character
replacement in the temp buffer and fix missing cell border. (Bug #64205)
---
 lisp/org/org-table.el | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
index 42f234790c5..6810fd8dc5a 100644
--- a/lisp/org/org-table.el
+++ b/lisp/org/org-table.el
@@ -6132,9 +6132,13 @@ supported."
   (with-temp-buffer
     (insert (orgtbl-to-orgtbl table params))
     (org-table-align)
-    (replace-regexp-in-string
-     "-|" "-+"
-     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (buffer-size))))))
+    (goto-char (point-min))
+    (while (re-search-forward "-|" nil t)
+      (replace-match "-+"))
+    (goto-char (point-min))
+    (while (re-search-forward "|-" nil t)
+      (replace-match "+-"))
+    (buffer-string)))
 
 (defun orgtbl-to-unicode (table params)
   "Convert the `orgtbl-mode' TABLE into a table with unicode characters.
-- 
2.39.1

bug#64205: Fix missing cell border when using orgtbl-to-table.el function

[Robert Pluim]

>>>>> On Wed, 21 Jun 2023 16:08:13 +0200, Jakub Ječmínek <jecminek.k@gmail.com> said:
    Jakub> diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
    Jakub> index 42f234790c5..6810fd8dc5a 100644
    Jakub> --- a/lisp/org/org-table.el
    Jakub> +++ b/lisp/org/org-table.el
    Jakub> @@ -6132,9 +6132,13 @@ supported."
    Jakub>    (with-temp-buffer
    Jakub>      (insert (orgtbl-to-orgtbl table params))
    Jakub>      (org-table-align)
    Jakub> -    (replace-regexp-in-string
    Jakub> -     "-|" "-+"
    Jakub> -     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (buffer-size))))))
    Jakub> +    (goto-char (point-min))
    Jakub> +    (while (re-search-forward "-|" nil t)
    Jakub> +      (replace-match "-+"))
    Jakub> +    (goto-char (point-min))
    Jakub> +    (while (re-search-forward "|-" nil t)
    Jakub> +      (replace-match "+-"))
    Jakub> +    (buffer-string)))

Youʼre replacing fixed strings, so you could use `search-forward'
instead of `re-search-forward'.

Robert
-- 

bug#64205: Fix missing cell border when using orgtbl-to-table.el function

[Jakub Ječmínek]

[-- Attachment #1.1: Type: text/plain, Size: 1315 bytes --]

You're right. There are more of these mistakes in the `org-table' file, for
example on line 1061, 1126 and 4666.

Jakub

st 21. 6. 2023 v 17:24 odesílatel Robert Pluim <rpluim@gmail.com> napsal:

> >>>>> On Wed, 21 Jun 2023 16:08:13 +0200, Jakub Ječmínek <
> jecminek.k@gmail.com> said:
>     Jakub> diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
>     Jakub> index 42f234790c5..6810fd8dc5a 100644
>     Jakub> --- a/lisp/org/org-table.el
>     Jakub> +++ b/lisp/org/org-table.el
>     Jakub> @@ -6132,9 +6132,13 @@ supported."
>     Jakub>    (with-temp-buffer
>     Jakub>      (insert (orgtbl-to-orgtbl table params))
>     Jakub>      (org-table-align)
>     Jakub> -    (replace-regexp-in-string
>     Jakub> -     "-|" "-+"
>     Jakub> -     (replace-regexp-in-string "|-" "+-" (buffer-substring 1
> (buffer-size))))))
>     Jakub> +    (goto-char (point-min))
>     Jakub> +    (while (re-search-forward "-|" nil t)
>     Jakub> +      (replace-match "-+"))
>     Jakub> +    (goto-char (point-min))
>     Jakub> +    (while (re-search-forward "|-" nil t)
>     Jakub> +      (replace-match "+-"))
>     Jakub> +    (buffer-string)))
>
> Youʼre replacing fixed strings, so you could use `search-forward'
> instead of `re-search-forward'.
>
> Robert
> --
>

[-- Attachment #1.2: Type: text/html, Size: 1996 bytes --]

[-- Attachment #2: 0001-Fix-orgtbl-to-table.el-function-to-include-last-cell.patch --]
[-- Type: application/octet-stream, Size: 1239 bytes --]

From 61024954e75c4e71ed48c0566e02fb6e67b3e688 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jakub=20Je=C4=8Dm=C3=ADnek?= <jecminek.k@gmail.com>
Date: Wed, 21 Jun 2023 15:50:31 +0200
Subject: [PATCH] Fix orgtbl-to-table.el function to include last cell border

* lisp/org/org-table.el (orgtbl-to-table.el): Perform character
replacement in the temp buffer and fix missing cell border. (Bug #64205)
---
 lisp/org/org-table.el | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
index 42f234790c5..9a72eb5f314 100644
--- a/lisp/org/org-table.el
+++ b/lisp/org/org-table.el
@@ -6132,9 +6132,13 @@ supported."
   (with-temp-buffer
     (insert (orgtbl-to-orgtbl table params))
     (org-table-align)
-    (replace-regexp-in-string
-     "-|" "-+"
-     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (buffer-size))))))
+    (goto-char (point-min))
+    (while (search-forward "-|" nil t)
+      (replace-match "-+"))
+    (goto-char (point-min))
+    (while (search-forward "|-" nil t)
+      (replace-match "+-"))
+    (buffer-string)))
 
 (defun orgtbl-to-unicode (table params)
   "Convert the `orgtbl-mode' TABLE into a table with unicode characters.
-- 
2.39.1

bug#64205: Fix missing cell border when using orgtbl-to-table.el function

[Robert Pluim]

>>>>> On Thu, 22 Jun 2023 16:33:02 +0200, Jakub Ječmínek <jecminek.k@gmail.com> said:

    Jakub> Tags: patch
    Jakub> I forgot to mention that I´ve fixed what you asked for, thank you for
    Jakub> pointing that out.

Thanks for that. Eli, is this small enough to go into master without
paperwork?

Thanks

Robert

    Jakub> See attached.

    Jakub> Jakub
    Jakub> From 61024954e75c4e71ed48c0566e02fb6e67b3e688 Mon Sep 17 00:00:00 2001
    Jakub> From: =?UTF-8?q?Jakub=20Je=C4=8Dm=C3=ADnek?= <jecminek.k@gmail.com>
    Jakub> Date: Wed, 21 Jun 2023 15:50:31 +0200
    Jakub> Subject: [PATCH] Fix orgtbl-to-table.el function to include last cell border

    Jakub> * lisp/org/org-table.el (orgtbl-to-table.el): Perform character
    Jakub> replacement in the temp buffer and fix missing cell border. (Bug #64205)
    Jakub> ---
    Jakub>  lisp/org/org-table.el | 10 +++++++---
    Jakub>  1 file changed, 7 insertions(+), 3 deletions(-)

    Jakub> diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
    Jakub> index 42f234790c5..9a72eb5f314 100644
    Jakub> --- a/lisp/org/org-table.el
    Jakub> +++ b/lisp/org/org-table.el
    Jakub> @@ -6132,9 +6132,13 @@ supported."
    Jakub>    (with-temp-buffer
    Jakub>      (insert (orgtbl-to-orgtbl table params))
    Jakub>      (org-table-align)
    Jakub> -    (replace-regexp-in-string
    Jakub> -     "-|" "-+"
    Jakub> -     (replace-regexp-in-string "|-" "+-" (buffer-substring 1 (buffer-size))))))
    Jakub> +    (goto-char (point-min))
    Jakub> +    (while (search-forward "-|" nil t)
    Jakub> +      (replace-match "-+"))
    Jakub> +    (goto-char (point-min))
    Jakub> +    (while (search-forward "|-" nil t)
    Jakub> +      (replace-match "+-"))
    Jakub> +    (buffer-string)))
 
    Jakub>  (defun orgtbl-to-unicode (table params)
    Jakub>    "Convert the `orgtbl-mode' TABLE into a table with unicode characters.
    Jakub> -- 
    Jakub> 2.39.1



Robert
-- 

bug#64205: Fix missing cell border when using orgtbl-to-table.el function

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 64205@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 22 Jun 2023 16:38:48 +0200
> 
> >>>>> On Thu, 22 Jun 2023 16:33:02 +0200, Jakub Ječmínek <jecminek.k@gmail.com> said:
> 
>     Jakub> Tags: patch
>     Jakub> I forgot to mention that I´ve fixed what you asked for, thank you for
>     Jakub> pointing that out.
> 
> Thanks for that. Eli, is this small enough to go into master without
> paperwork?

Yes.  Just don't forget the Copyright-paperwork-exempt thingy.

Thanks.

bug#64205: Fix missing cell border when using orgtbl-to-table.el function

[Robert Pluim]

tags 64205 fixed
close 64205 30.1
quit

>>>>> On Thu, 22 Jun 2023 18:15:17 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: 64205@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
    >> Date: Thu, 22 Jun 2023 16:38:48 +0200
    >> 
    >> >>>>> On Thu, 22 Jun 2023 16:33:02 +0200, Jakub Ječmínek <jecminek.k@gmail.com> said:
    >> 
    Jakub> Tags: patch
    Jakub> I forgot to mention that I´ve fixed what you asked for, thank you for
    Jakub> pointing that out.
    >> 
    >> Thanks for that. Eli, is this small enough to go into master without
    >> paperwork?

    Eli> Yes.  Just don't forget the Copyright-paperwork-exempt thingy.

Closing.
Committed as 4c01b0deee1

For future changes, I suspect youʼll need to assign copyright to the
changes to the FSF, if youʼre willing to do so (the limit is approx 10
lines of code).

Thanks

Robert
--