[BUG] org-table-beginning/end-of-field

[BUG] org-table-beginning/end-of-field

[Florian Beck]

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

Hi,

The argument of `org-table-beginning-of-field' and
`org-table-end-of-field' is in fact not optional.

-- 
Florian Beck

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-optional-argument-spec.patch --]
[-- Type: text/x-diff, Size: 1339 bytes --]

From d1d12380a1c260bef7a2137831434614f7d9ec1f Mon Sep 17 00:00:00 2001
From: Florian Beck <fb@miszellen.de>
Date: Mon, 8 Sep 2014 07:34:56 +0200
Subject: [PATCH] Remove &optional argument spec

* lisp/org-table.el (org-table-beginning-of-field): fix argument
(org-table-end-of-field): fix argument
---
 lisp/org-table.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 547f933..a17e95a 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -1061,7 +1061,7 @@ Before doing so, re-align the table if necessary."
   (if (looking-at "| ?")
       (goto-char (match-end 0))))
 
-(defun org-table-beginning-of-field (&optional n)
+(defun org-table-beginning-of-field (n)
   "Move to the end of the current table field.
 If already at or after the end, move to the end of the next table field.
 With numeric argument N, move N-1 fields forward first."
@@ -1076,7 +1076,7 @@ With numeric argument N, move N-1 fields forward first."
       (and (looking-at " ") (forward-char 1)))
     (if (>= (point) pos) (org-table-beginning-of-field 2))))
 
-(defun org-table-end-of-field (&optional n)
+(defun org-table-end-of-field (n)
   "Move to the beginning of the current table field.
 If already at or before the beginning, move to the beginning of the
 previous field.
-- 
1.9.1

Re: [BUG] org-table-beginning/end-of-field

[Nicolas Goaziou]

Hello,

Florian Beck <fb@miszellen.de> writes:

> The argument of `org-table-beginning-of-field' and
> `org-table-end-of-field' is in fact not optional.

Thanks for the patch. Though, wouldn't it make more sense to properly
handle a missing argument instead?


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] org-table-beginning/end-of-field

[Florian Beck]

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

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Thanks for the patch. Though, wouldn't it make more sense to properly
> handle a missing argument instead?

How about this?

-- 
Florian Beck

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-table-fix-arguments-of-org-table-beginning-of-fi.patch --]
[-- Type: text/x-diff, Size: 3011 bytes --]

From 4fb2bbff2238d15ae7c896e0eb268b74ea4e56dc Mon Sep 17 00:00:00 2001
From: Florian Beck <fb@miszellen.de>
Date: Mon, 8 Sep 2014 14:08:56 +0200
Subject: [PATCH] org-table: fix arguments of `org-table-beginning-of-field'
 and `org-table-end-of-field'. Also fix docstring and cleanup code.

* lisp/org-table.el (org-table--border-of-field): new function
(org-table-beginning-of-field): call it
(org-table-end-of-field): call it
---
 lisp/org-table.el | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 547f933..31fda57 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -1061,37 +1061,34 @@ Before doing so, re-align the table if necessary."
   (if (looking-at "| ?")
       (goto-char (match-end 0))))
 
-(defun org-table-beginning-of-field (&optional n)
-  "Move to the end of the current table field.
-If already at or after the end, move to the end of the next table field.
-With numeric argument N, move N-1 fields forward first."
-  (interactive "p")
+(defun org-table--border-of-field (n move-fn element cmp-fn)
   (let ((pos (point)))
     (while (> n 1)
       (setq n (1- n))
-      (org-table-previous-field))
-    (if (not (re-search-backward "|" (point-at-bol 0) t))
-	(user-error "No more table fields before the current")
-      (goto-char (match-end 0))
-      (and (looking-at " ") (forward-char 1)))
-    (if (>= (point) pos) (org-table-beginning-of-field 2))))
+      (funcall move-fn))
+    (goto-char (org-element-property element (org-element-context)))
+    (if (and (> n 0) (funcall cmp-fn (point) pos))
+	(org-table--border-of-field 2 move-fn element cmp-fn))))
 
-(defun org-table-end-of-field (&optional n)
+(defun org-table-beginning-of-field (&optional n)
   "Move to the beginning of the current table field.
-If already at or before the beginning, move to the beginning of the
-previous field.
+If already at or before the beginning and N is not 0, move to the 
+beginning of the previous table field.
 With numeric argument N, move N-1 fields backward first."
   (interactive "p")
-  (let ((pos (point)))
-    (while (> n 1)
-      (setq n (1- n))
-      (org-table-next-field))
-    (when (re-search-forward "|" (point-at-eol 1) t)
-      (backward-char 1)
-      (skip-chars-backward " ")
-      (if (and (equal (char-before (point)) ?|) (looking-at " "))
-	  (forward-char 1)))
-    (if (<= (point) pos) (org-table-end-of-field 2))))
+  (org-table--border-of-field (or n 1)
+			      'org-table-previous-field
+			      :contents-begin '>=))
+
+(defun org-table-end-of-field (&optional n)
+  "Move to the end of the current table field.
+If already at or after the end and N is not 0, move to the end of the
+next field.
+With numeric argument N, move N-1 fields forward first."
+  (interactive "p")
+  (org-table--border-of-field (or n 1)
+			      'org-table-next-field
+			      :contents-end '<=))
 
 ;;;###autoload
 (defun org-table-next-row ()
-- 
1.9.1

Re: [PATCH] org-table-beginning/end-of-field

[Nicolas Goaziou]

Florian Beck <fb@miszellen.de> writes:

> How about this?

Nice. Some comments follow.
>
> From 4fb2bbff2238d15ae7c896e0eb268b74ea4e56dc Mon Sep 17 00:00:00 2001
> From: Florian Beck <fb@miszellen.de>
> Date: Mon, 8 Sep 2014 14:08:56 +0200
> Subject: [PATCH] org-table: fix arguments of `org-table-beginning-of-field'

"fix" needs to be capitalized.

>  and `org-table-end-of-field'. Also fix docstring and cleanup code.

You need a shorter summary, e.g.,

  org-table: Fix org-table/beginning/end/-of-field.

The second sentence should go at the end of the commit message.  Also,
there's no full stop on summary lines.

> * lisp/org-table.el (org-table--border-of-field): new function

"new" needs to be capitalized.  Also there's a missing full stop.

> (org-table-beginning-of-field): call it
> (org-table-end-of-field): call it

(org-table-beginning-of-field, org-table-end-of-field): Call it.

> +(defun org-table--border-of-field (n move-fn element cmp-fn)

A docstring would be nice, even for a helper function.

>    (let ((pos (point)))
>      (while (> n 1)
>        (setq n (1- n))
> -      (org-table-previous-field))
> -    (if (not (re-search-backward "|" (point-at-bol 0) t))
> -	(user-error "No more table fields before the current")
> -      (goto-char (match-end 0))
> -      (and (looking-at " ") (forward-char 1)))
> -    (if (>= (point) pos) (org-table-beginning-of-field 2))))
> +      (funcall move-fn))

While you're at it, I suggest

  (dotimes (_ (1- n)) (funcall move-fn))

instead of

  (while (> n 1) ...)

Also, note that you can avoid requiring MOVE-FN, ELEMENT and CMP-FN if
you decide that

  (< n 0)  => (#'org-table-next-field :contents-end #'<=)
  (> n 0)  => (#'org-table-previous-fierd :contents-begin #'>=)

Up to you.

> +    (goto-char (org-element-property element (org-element-context)))

You need to be more careful here.  You might be outside of a table, or
within an object contained in a cell, e.g.,

  | some *bold* object |

where (org-element-context) on bold will return a `bold' type object
with different :contents-begin and :contents-end values.

IOW, you need to let-bind (org-element-context) and find the first
`table', `table-row' or `table-cell' object/element among it and its
parents. Then

  - if no such ancestor is found: return an error (not at a table)

  - if `table' is found but point is not within
    [:contents-begin :contents-end[ interval, return an error (not
    inside the table)

  - if `table' or `table-row' is found, you need to apply
    org-table/previous/next/-field once (and diminish N by one) to make
    sure point will be left on a regular cell, if possible.

> +    (if (and (> n 0) (funcall cmp-fn (point) pos))
> +	(org-table--border-of-field 2 move-fn element cmp-fn))))

I don't think (> n 0) is necessary, is it? Also, I suggest to use `when'
(or `and' if return value matters) over one-armed `if'.

> -(defun org-table-end-of-field (&optional n)
> +(defun org-table-beginning-of-field (&optional n)
>    "Move to the beginning of the current table field.
> -If already at or before the beginning, move to the beginning of the
> -previous field.
> +If already at or before the beginning and N is not 0, move to the 
> +beginning of the previous table field.
>  With numeric argument N, move N-1 fields backward first."

I wouldn't start a new line here.  Also don't forget double spaces:

  If already at or before the beginning and N is not 0, move to the
  beginning of the previous table field.  With numeric argument N, move
  N-1 fields backward first.

> +  (org-table--border-of-field (or n 1)
> +			      'org-table-previous-field
> +			      :contents-begin '>=))

Nitpick: #'org-table-previous-field and #'>=.

> +(defun org-table-end-of-field (&optional n)
> +  "Move to the end of the current table field.
> +If already at or after the end and N is not 0, move to the end of the
> +next field.
> +With numeric argument N, move N-1 fields forward first."
> +  (interactive "p")
> +  (org-table--border-of-field (or n 1)
> +			      'org-table-next-field
> +			      :contents-end '<=))

Ditto.

Thank you for taking care of this. There are bonus points if you can
write tests along with this change.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] org-table-beginning/end-of-field

[Florian Beck]

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

Hi Nicolas,

thanks for the review. I hope the new version is an improvement.

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Also, note that you can avoid requiring MOVE-FN, ELEMENT and CMP-FN if
> you decide that
>
>   (< n 0)  => (#'org-table-next-field :contents-end #'<=)
>   (> n 0)  => (#'org-table-previous-fierd :contents-begin #'>=)
>
> Up to you.

I wanted to use the n=0 case to supress the conditional movement to the
next field. It's probably not worth it and I removed it. Now everything
simplifies to one function.

> IOW, you need to let-bind (org-element-context) and find the first
> `table', `table-row' or `table-cell' object/element among it and its
> parents. Then
>
>   - if no such ancestor is found: return an error (not at a table)
>
>   - if `table' is found but point is not within
>     [:contents-begin :contents-end[ interval, return an error (not
>     inside the table)
>
>   - if `table' or `table-row' is found, you need to apply
>     org-table/previous/next/-field once (and diminish N by one) to make
>     sure point will be left on a regular cell, if possible.

But as long as I have a table cell ancestor, I should be fine. Am I
missing something? I added a function `org-element-get' to help with
that. What do you think? (If `cl-labels' is a no-go, we could split out
a helper function.)

> Thank you for taking care of this. There are bonus points if you can
> write tests along with this change.

I added a couple of tests. Not really succinct, though.

Also, I modified `org-element-table-cell-parser', because otherwise
:contents-begin and :contents-end point to the end of the cell. Not sure
if this breaks anything.

-- 
Florian Beck



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-element-Adjust-content-boundaries-in-empty-cells.patch --]
[-- Type: text/x-diff, Size: 1267 bytes --]

From 2b1a63e70830e7604c7f59dd0110aedf3a9c1e53 Mon Sep 17 00:00:00 2001
From: Florian Beck <fb@miszellen.de>
Date: Tue, 9 Sep 2014 12:29:53 +0200
Subject: [PATCH 1/4] org-element: Adjust content boundaries in empty cells

* lisp/org-element.el (org-element-table-cell-parser): Let
:contents-begin and :contents-end point to the beginning
of an empty table cell.
---
 lisp/org-element.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lisp/org-element.el b/lisp/org-element.el
index f175fbc..47fa3f1 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -3333,12 +3333,15 @@ and `:post-blank' keywords."
   (let* ((begin (match-beginning 0))
 	 (end (match-end 0))
 	 (contents-begin (match-beginning 1))
-	 (contents-end (match-end 1)))
+	 (contents-end (match-end 1))
+	 ;; Avoid having the contents at the end of an empty cell.
+	 (empty-pos (when (= contents-begin contents-end)
+		      (min (1+ begin) end))))
     (list 'table-cell
 	  (list :begin begin
 		:end end
-		:contents-begin contents-begin
-		:contents-end contents-end
+		:contents-begin (or empty-pos contents-begin)
+		:contents-end (or empty-pos contents-end)
 		:post-blank 0))))
 
 (defun org-element-table-cell-interpreter (table-cell contents)
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-org-element-Implement-org-element-get.patch --]
[-- Type: text/x-diff, Size: 1425 bytes --]

From 10f68bf26f82f4cbc0e097bac9a4d3b997c10bc4 Mon Sep 17 00:00:00 2001
From: Florian Beck <fb@miszellen.de>
Date: Tue, 9 Sep 2014 12:31:35 +0200
Subject: [PATCH 2/4] org-element: Implement `org-element-get'

* lisp/org-element.el (org-element-get): New function.
---
 lisp/org-element.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lisp/org-element.el b/lisp/org-element.el
index 47fa3f1..f55dd37 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -5873,6 +5873,26 @@ Providing it allows for quicker computation."
 	   ;; Store results in cache, if applicable.
 	   (org-element--cache-put element cache)))))))
 
+(defun org-element-get (&optional type pom)
+  "Return the nearest object or element of TYPE at POM."
+  (let* ((pom (or pom (point)))
+	 (context (with-current-buffer (if (markerp pom)
+					   (marker-buffer pom)
+					 (current-buffer))
+		    (save-excursion
+		      (goto-char pom)
+		      (org-element-context)))))
+    (cl-labels ((get-type (type context)
+			  (cond ((not context) nil)
+				((not type) context)
+				((eql type (car context))
+				 context)
+				(t (get-type type
+					     (plist-get
+					      (cadr context)
+					      :parent))))))
+      (get-type type context))))
+
 (defun org-element-nested-p (elem-A elem-B)
   "Non-nil when elements ELEM-A and ELEM-B are nested."
   (let ((beg-A (org-element-property :begin elem-A))
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-org-table-Handle-optional-arguments-and-cleanup.patch --]
[-- Type: text/x-diff, Size: 3360 bytes --]

From 64f937fe289e7aca41471ec731aec1590bebe947 Mon Sep 17 00:00:00 2001
From: Florian Beck <fb@miszellen.de>
Date: Tue, 9 Sep 2014 12:35:09 +0200
Subject: [PATCH 3/4] org-table: Handle optional arguments and cleanup

* lisp/org-table.el (org-table-beginning-of-field): Fix
docstring. Call `org-table-end-of-field'.
(org-table-end-of-field): Fix docstring.  Handle missing and
negative args.
---
 lisp/org-table.el | 60 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 547f933..290cdce 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -1062,36 +1062,44 @@ Before doing so, re-align the table if necessary."
       (goto-char (match-end 0))))
 
 (defun org-table-beginning-of-field (&optional n)
-  "Move to the end of the current table field.
-If already at or after the end, move to the end of the next table field.
-With numeric argument N, move N-1 fields forward first."
+  "Move to the beginning of the current table field.
+If already at or before the beginning, move to the 
+beginning of the previous table field.  With numeric 
+argument N, move N-1 fields backward first."
   (interactive "p")
-  (let ((pos (point)))
-    (while (> n 1)
-      (setq n (1- n))
-      (org-table-previous-field))
-    (if (not (re-search-backward "|" (point-at-bol 0) t))
-	(user-error "No more table fields before the current")
-      (goto-char (match-end 0))
-      (and (looking-at " ") (forward-char 1)))
-    (if (>= (point) pos) (org-table-beginning-of-field 2))))
+  (org-table-end-of-field (- (or n 1))))
 
 (defun org-table-end-of-field (&optional n)
-  "Move to the beginning of the current table field.
-If already at or before the beginning, move to the beginning of the
-previous field.
-With numeric argument N, move N-1 fields backward first."
+  "Move to the end of the current table field.
+If already at or after the end, move to the end of the
+next table field.  With numeric argument N, move N-1 fields 
+forward first.  If the numeric argument is negative, move backwards."
   (interactive "p")
-  (let ((pos (point)))
-    (while (> n 1)
-      (setq n (1- n))
-      (org-table-next-field))
-    (when (re-search-forward "|" (point-at-eol 1) t)
-      (backward-char 1)
-      (skip-chars-backward " ")
-      (if (and (equal (char-before (point)) ?|) (looking-at " "))
-	  (forward-char 1)))
-    (if (<= (point) pos) (org-table-end-of-field 2))))
+  (let* ((pos (point))
+	 (n (or n 1)))
+    (dotimes (_ (1- (abs n)))
+      (funcall (if (> n 0)
+		   #'org-table-next-field
+		 #'org-table-previous-field)))
+    (let ((cell (or (org-element-get 'table-cell)
+		    ;; Fuzzy matching when on a table border:
+		    (org-element-get 'table-cell (1+ (point)))
+		    (org-element-get 'table-cell (1- (point))))))
+      (unless cell
+	(user-error "Not in a table cell"))
+      (goto-char (org-element-property
+		  (if (< n 0)
+		      :contents-begin
+		    :contents-end)
+		  cell))
+      ;; Point already is at the end/beginning of the field,
+      ;; so we move to the next/previous field.
+      (cond
+       ((and (> n 0) (>= pos (point)))
+	(org-table-end-of-field 2))
+       ((and (< n 0) (<= pos (point)))
+	(org-table-end-of-field -2))))))
+
 
 ;;;###autoload
 (defun org-table-next-row ()
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-test-org-table-Add-tests.patch --]
[-- Type: text/x-diff, Size: 4035 bytes --]

From a6a00af0f3f291fb0b4eb19012b80d28d3419a38 Mon Sep 17 00:00:00 2001
From: Florian Beck <fb@miszellen.de>
Date: Tue, 9 Sep 2014 12:40:27 +0200
Subject: [PATCH 4/4] test-org-table: Add tests

* testing/lisp/test-org-table.el (test-org-table/org-table-end-of-field,
test-org-table/org-table-beginning-of-field): New tests.
---
 testing/lisp/test-org-table.el | 105 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/testing/lisp/test-org-table.el b/testing/lisp/test-org-table.el
index 40101be..49ed153 100644
--- a/testing/lisp/test-org-table.el
+++ b/testing/lisp/test-org-table.el
@@ -1168,6 +1168,111 @@ See also `test-org-table/copy-field'."
       (should (string= got
 		       expect)))))
 
+(ert-deftest test-org-table/org-table-end-of-field ()
+  "Test `org-table-end-of-field' specifications."
+  ;; Outside a table, return an error.
+  (should-error
+   (org-test-with-temp-text "Paragraph"
+     (goto-char 2)
+     (org-table-end-of-field)))
+  ;; Move to end of cell.
+  (should
+   (org-test-with-temp-text
+    "| Cell:1   | Cell2 |Cell3|\n| Cell4 |     | Cell5|"
+    (search-forward ":")
+    (org-table-end-of-field)
+    (looking-back "Cell:1")))
+  ;; Move to next cell if already at the end.
+  (should
+   (org-test-with-temp-text
+    "| Cell:1   | Cell2 |Cell3|\n| Cell4 |     | Cell5|"
+    (search-forward ":")
+    (org-table-end-of-field)
+    (org-table-end-of-field)
+    (looking-back "Cell2")))
+  ;; Work in unaligned cells.
+  (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |Cell:3|\n| Cell4 |     | Cell5|"
+    (search-forward ":")
+    (org-table-end-of-field)
+    (looking-back "Cell:3")))
+  ;; Move to next row
+  (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |Cell:3|\n| Cell4 |     | Cell5|"
+    (search-forward ":")
+    (org-table-end-of-field)
+    (org-table-end-of-field)
+    (looking-back "Cell4")))
+  ;; With argument, skip N-1 fields forward
+  (should
+   (org-test-with-temp-text
+    "| Cell:1   | Cell2 |Cell3|\n| Cell4 |     | Cell5|"
+    (search-forward ":")
+    (org-table-end-of-field 4)
+    (looking-back "Cell4")))
+  ;; Empty cells
+  (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |Cell3|\n| Cell4: |     | Cell5|"
+    (search-forward ":")
+    (forward-char)
+    (org-table-end-of-field)
+    (looking-back "| ")))
+  (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |Cell3|\n| Cell4: |     | Cell5|"
+    (search-forward ":")
+    (forward-char 6)
+    (org-table-end-of-field)
+    (looking-back "Cell5")))
+ (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |Cell3|\n| Cell4: || Cell5|"
+    (search-forward ":")
+    (forward-char 3)
+    (org-table-end-of-field)
+    (looking-back "Cell5")))
+ ;; On table border
+ (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |:Cell3|\n| Cell4 || Cell5|"
+    (search-forward ":")
+    (forward-char -2)
+    (org-table-end-of-field)
+    (looking-back ":Cell3"))))
+
+(ert-deftest test-org-table/org-table-beginning-of-field ()
+  "Test `org-table-beginning-of-field' specifications."
+  ;; Outside a table, return an error.
+  (should-error
+   (org-test-with-temp-text "Paragraph"
+     (goto-char 2)
+     (org-table-beginning-of-field)))
+  ;; Move to end of cell.
+  (should
+   (org-test-with-temp-text
+    "| Cell:1   | Cell2 |Cell3|\n| Cell4 |     | Cell5|"
+    (search-forward ":")
+    (org-table-beginning-of-field)
+    (looking-at "Cell:1")))
+  ;; With argument, skip N-1 fields backwards
+  (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |Cell3|\n| Cell4 |     | Cell:5|"
+    (search-forward ":")
+    (org-table-beginning-of-field 5)
+    (looking-at "Cell2")))
+  ;; Empty cells
+  (should
+   (org-test-with-temp-text
+    "| Cell1   | Cell2 |Cell3|\n| Cell4: |     | Cell5|"
+    (search-forward ":")
+    (forward-char 6)
+    (org-table-beginning-of-field)
+    (looking-back "| "))))
+
 (provide 'test-org-table)
 
 ;;; test-org-table.el ends here
-- 
1.9.1

Re: [PATCH] org-table-beginning/end-of-field

[Nicolas Goaziou]

Hello,

Florian Beck <fb@miszellen.de> writes:

> I hope the new version is an improvement.

Thanks for the update.

> But as long as I have a table cell ancestor, I should be fine. Am
> I missing something?

Yes. There are some place where `org-element-context' will return
a `table-row' or `table' object even though you can technically move to
the next (or maybe previous) cell. You may want to try
`org-element-context' in the following places, where X denotes cursor:

  | a | b |
  X c | d |

  X a | b |
  | c | d |

X | a | b |
  | c | d |

  | a | b | X
  | c | d |

  | a | b |
X | c | d |

> I added a function `org-element-get' to help with that. What do you
> think? (If `cl-labels' is a no-go, we could split out a helper
> function.)

I don't think `org-element-get' should go into "org-element.el" in its
current implementation. It is tied to a position, which implies to take
care of boring stuff like buffer narrowing. It doesn't allow to re-use
an already computed element or object either, which is sub-optimal.

However, the feature is useful enough that some function could provide
something similar. Maybe the following one:

  (defun org-element-parent (blob &optional types genealogy)
    "Return BLOB's parent, or nil.

  BLOB is an object or element.

  When optional argument TYPES is a list of symbols, return the
  first element or object between BLOB and its ancestors whose type
  belongs to that list.

  When optional argument GENEALOGY is non-nil, return a list of all
  ancestors, from closest to farthest.

  When BLOB is obtained through `org-element-context' or
  `org-element-at-point', only ancestors from its section can be
  found. There is no such limitation when BLOB belongs to a full
  parse tree."
    (if (not (or types genealogy)) (org-element-property :parent blob)
      (let ((up blob) ancestors)
        (while (and up (not (memq (org-element-type up) types)))
          (when genealogy (push up ancestors))
          (setq up (org-element-property :parent up)))
        (if genealogy (nreverse ancestors) up))))

Its name is a bit confusing since it can return BLOB under some optional
argument combination (i.e., if BLOB's type belongs to TYPES). However,
it covers most, if not all, needs about ancestors. WDYT?

> I added a couple of tests. Not really succinct, though.

Thanks.

> Also, I modified `org-element-table-cell-parser', because otherwise
> :contents-begin and :contents-end point to the end of the cell. Not sure
> if this breaks anything.

Usually, when an object has no contents (e.g., a link without
description), both :contents-begin and :contents-end are nil. It is less
confusing than providing an arbitrary buffer position.

> +    (let ((cell (or (org-element-get 'table-cell)
> +		    ;; Fuzzy matching when on a table border:
> +		    (org-element-get 'table-cell (1+ (point)))
> +		    (org-element-get 'table-cell (1- (point))))))

We need to be more careful here, as explained above.

> +  (should
> +   (org-test-with-temp-text
> +    "| Cell:1   | Cell2 |Cell3|\n| Cell4 |     | Cell5|"
> +    (search-forward ":")

Minor note: you can insert "<point>" in the string to avoid finding the
correct position later. E.g.,

  "| Cell:<point>1   | Cell2 |Cell3|\n| Cell4 |     | Cell5|"


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] org-table-beginning/end-of-field

[Florian Beck]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> the next (or maybe previous) cell. You may want to try
> `org-element-context' in the following places, where X denotes cursor:
>
>   | a | b |
>   X c | d |
>
>   X a | b |
>   | c | d |

This I did.


> X | a | b |
>   | c | d |
>
>   | a | b | X
>   | c | d |
>
>   | a | b |
> X | c | d |

This is arguably outside the scope of org-table-beginning/end, isn't it?

Come to think of it, these functions should only move to the
beginning or end of the field, period. Then handle the special case in
org-forward/backward-sentence or create two new functions.

> I don't think `org-element-get' should go into "org-element.el" in its
> current implementation. It is tied to a position, which implies to take
> care of boring stuff like buffer narrowing. It doesn't allow to re-use
> an already computed element or object either, which is sub-optimal.

Yes, it should be split into two functions.

> However, the feature is useful enough that some function could provide
> something similar. Maybe the following one:
>
>   (defun org-element-parent (blob &optional types genealogy)
>     "Return BLOB's parent, or nil.
>
>   BLOB is an object or element.
>
>   When optional argument TYPES is a list of symbols, return the
>   first element or object between BLOB and its ancestors whose type
>   belongs to that list.
>
>   When optional argument GENEALOGY is non-nil, return a list of all
>   ancestors, from closest to farthest.
>
>   When BLOB is obtained through `org-element-context' or
>   `org-element-at-point', only ancestors from its section can be
>   found. There is no such limitation when BLOB belongs to a full
>   parse tree."
>     (if (not (or types genealogy)) (org-element-property :parent blob)
>       (let ((up blob) ancestors)
>         (while (and up (not (memq (org-element-type up) types)))
>           (when genealogy (push up ancestors))
>           (setq up (org-element-property :parent up)))
>         (if genealogy (nreverse ancestors) up))))
>
> Its name is a bit confusing since it can return BLOB under some optional
> argument combination (i.e., if BLOB's type belongs to TYPES). However,
> it covers most, if not all, needs about ancestors. WDYT?

Yes this works. Making TYPES into a list is a nice touch. However:
 - the advertised functionality (returning the parent) is only
 marginally useful and handled in half a line of code; whereas most of
 the functionality is hidden as optional;
 - this leads to confusing behaviour, as you have noted, because
 the type of object I'm looking for, is not necessarily the parent;
 - BLOB on the other hand should be optional, because defaulting it to
 org-element-context would be highly useful.
 - not sure what GENEALOGY is useful for; if the user also specified
 TYPES, this returns the ancestors up to but NOT including those of TYPE.

>> Also, I modified `org-element-table-cell-parser', because otherwise
>> :contents-begin and :contents-end point to the end of the cell. Not sure
>> if this breaks anything.
>
> Usually, when an object has no contents (e.g., a link without
> description), both :contents-begin and :contents-end are nil. It is less
> confusing than providing an arbitrary buffer position.

That's another possibility. Unlike a link without description, one might
argue that an empty cell has a natural contents position.


> Minor note: you can insert "<point>" in the string to avoid finding the
> correct position later. E.g.,
>
>   "| Cell:<point>1   | Cell2 |Cell3|\n| Cell4 |     | Cell5|"

Great, thanks!


-- 
Florian Beck

Re: [PATCH] org-table-beginning/end-of-field

[Nicolas Goaziou]

Hello,

Florian Beck <fb@miszellen.de> writes:

Thanks for your feedback.

> This I did.

Well, 

  (or (org-element-get 'table-cell)
      ;; Fuzzy matching when on a table border:
      (org-element-get 'table-cell (1+ (point)))
      (org-element-get 'table-cell (1- (point))))

is not necessary, since the first object/element returned should give
you enough information.

Also, I forgot the following case:

  | a | b |
  |---+-X-|
  | c | d |

I think a correct analysis of the current object can solve all tricky
cases, without blindly poking around.

>> X | a | b |
>>   | c | d |
>>
>>   | a | b | X
>>   | c | d |
>>
>>   | a | b |
>> X | c | d |
>
> This is arguably outside the scope of org-table-beginning/end, isn't it?

The current implementation handles them (sometimes incorrectly). I find
this sloppiness useful.

> Come to think of it, these functions should only move to the
> beginning or end of the field, period. Then handle the special case in
> org-forward/backward-sentence or create two new functions.

OTOH, is it really useful to isolate beginning/end-of-field feature? I'm
not sure, but feel free to try if you think it can make the code better.

>>   (defun org-element-parent (blob &optional types genealogy)
>>     "Return BLOB's parent, or nil.
>>
>>   BLOB is an object or element.
>>
>>   When optional argument TYPES is a list of symbols, return the
>>   first element or object between BLOB and its ancestors whose type
>>   belongs to that list.
>>
>>   When optional argument GENEALOGY is non-nil, return a list of all
>>   ancestors, from closest to farthest.
>>
>>   When BLOB is obtained through `org-element-context' or
>>   `org-element-at-point', only ancestors from its section can be
>>   found. There is no such limitation when BLOB belongs to a full
>>   parse tree."
>>     (if (not (or types genealogy)) (org-element-property :parent blob)
>>       (let ((up blob) ancestors)
>>         (while (and up (not (memq (org-element-type up) types)))
>>           (when genealogy (push up ancestors))
>>           (setq up (org-element-property :parent up)))
>>         (if genealogy (nreverse ancestors) up))))
>>
>> Its name is a bit confusing since it can return BLOB under some optional
>> argument combination (i.e., if BLOB's type belongs to TYPES). However,
>> it covers most, if not all, needs about ancestors. WDYT?
>
> Yes this works. Making TYPES into a list is a nice touch. However:
>  - the advertised functionality (returning the parent) is only
>  marginally useful and handled in half a line of code; whereas most of
>  the functionality is hidden as optional;

That's my intention. The functionality is very common so there is a high
chance that it will be remembered.

It is possible to separate the features and create, let's say,
`org-element-genealogy'. However, in this case, I lean towards
overloading a common function instead of creating a dedicated niche one.

>  - this leads to confusing behaviour, as you have noted, because
>  the type of object I'm looking for, is not necessarily the parent;

This is why it can return the current object when TYPES is provided.

If we create another function, you will need to check both the current
item and the return value of that function.

>  - BLOB on the other hand should be optional, because defaulting it to
>  org-element-context would be highly useful.

I don't consider "highly useful" the fact that you could call

  (org-element-parent)
  
instead of 

  (org-element-parent (org-element-context))

However I see two drawbacks in this optional argument: it hides the call
to most expensive function and `org-element-context' is not always what
you want (there's no reason to prefer it over `org-element-at-point' and
the other way around).

Eventually, I'd like to keep `org-element-at-point',
`org-element-context' and `org-element-parse-buffer' as the highest
level functions in "org-element.el" (i.e., no other function should call
them there). Using these functions is out of the scope of the parser.

>  - not sure what GENEALOGY is useful for;

You may grep for `org-export-get-genealogy'. There are a couple of
places where this one is used.

Actually, this function would also generalize:

 - `org-export-get-parent-headline'
 - `org-export-get-parent-element'
 - `org-export-get-parent-table'

> if the user also specified TYPES, this returns the ancestors up to but
> NOT including those of TYPE.

I admit I didn't put more thoughts into TYPES + GENEALOGY combination.
Of course, if this function is implemented, I agree it should also
include the first element matching TYPES.

> That's another possibility. Unlike a link without description, one might
> argue that an empty cell has a natural contents position.

For the time being, I prefer all parsing functions to be consistent.


Regards,

-- 
Nicolas Goaziou