* [BUG] org-table-beginning/end-of-field @ 2014-09-08 5:37 Florian Beck 2014-09-08 10:25 ` Nicolas Goaziou 0 siblings, 1 reply; 8+ messages in thread From: Florian Beck @ 2014-09-08 5:37 UTC (permalink / raw) To: emacs-orgmode [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [BUG] org-table-beginning/end-of-field 2014-09-08 5:37 [BUG] org-table-beginning/end-of-field Florian Beck @ 2014-09-08 10:25 ` Nicolas Goaziou 2014-09-08 12:17 ` [PATCH] org-table-beginning/end-of-field Florian Beck 0 siblings, 1 reply; 8+ messages in thread From: Nicolas Goaziou @ 2014-09-08 10:25 UTC (permalink / raw) To: Florian Beck; +Cc: emacs-orgmode 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] org-table-beginning/end-of-field 2014-09-08 10:25 ` Nicolas Goaziou @ 2014-09-08 12:17 ` Florian Beck 2014-09-08 15:30 ` Nicolas Goaziou 0 siblings, 1 reply; 8+ messages in thread From: Florian Beck @ 2014-09-08 12:17 UTC (permalink / raw) To: emacs-orgmode [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] org-table-beginning/end-of-field 2014-09-08 12:17 ` [PATCH] org-table-beginning/end-of-field Florian Beck @ 2014-09-08 15:30 ` Nicolas Goaziou 2014-09-09 11:09 ` Florian Beck 0 siblings, 1 reply; 8+ messages in thread From: Nicolas Goaziou @ 2014-09-08 15:30 UTC (permalink / raw) To: Florian Beck; +Cc: emacs-orgmode 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] org-table-beginning/end-of-field 2014-09-08 15:30 ` Nicolas Goaziou @ 2014-09-09 11:09 ` Florian Beck 2014-09-10 13:42 ` Nicolas Goaziou 0 siblings, 1 reply; 8+ messages in thread From: Florian Beck @ 2014-09-09 11:09 UTC (permalink / raw) To: emacs-orgmode [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] org-table-beginning/end-of-field 2014-09-09 11:09 ` Florian Beck @ 2014-09-10 13:42 ` Nicolas Goaziou 2014-09-10 17:08 ` Florian Beck 0 siblings, 1 reply; 8+ messages in thread From: Nicolas Goaziou @ 2014-09-10 13:42 UTC (permalink / raw) To: Florian Beck; +Cc: emacs-orgmode 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] org-table-beginning/end-of-field 2014-09-10 13:42 ` Nicolas Goaziou @ 2014-09-10 17:08 ` Florian Beck 2014-09-12 8:29 ` Nicolas Goaziou 0 siblings, 1 reply; 8+ messages in thread From: Florian Beck @ 2014-09-10 17:08 UTC (permalink / raw) To: emacs-orgmode 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] org-table-beginning/end-of-field 2014-09-10 17:08 ` Florian Beck @ 2014-09-12 8:29 ` Nicolas Goaziou 0 siblings, 0 replies; 8+ messages in thread From: Nicolas Goaziou @ 2014-09-12 8:29 UTC (permalink / raw) To: Florian Beck; +Cc: emacs-orgmode 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-09-12 8:29 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-08 5:37 [BUG] org-table-beginning/end-of-field Florian Beck 2014-09-08 10:25 ` Nicolas Goaziou 2014-09-08 12:17 ` [PATCH] org-table-beginning/end-of-field Florian Beck 2014-09-08 15:30 ` Nicolas Goaziou 2014-09-09 11:09 ` Florian Beck 2014-09-10 13:42 ` Nicolas Goaziou 2014-09-10 17:08 ` Florian Beck 2014-09-12 8:29 ` Nicolas Goaziou
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).