From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Beck Subject: Re: [PATCH] org-table-beginning/end-of-field Date: Tue, 09 Sep 2014 13:09:27 +0200 Message-ID: <87ppf59hjs.fsf@sophokles.streitblatt.de> References: <87mwaaekp8.fsf@sophokles.streitblatt.de> <871trmbe92.fsf@nicolasgoaziou.fr> <87egvme27x.fsf_-_@sophokles.streitblatt.de> <87wq9e9lkn.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:43004) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRJuw-00016L-3k for emacs-orgmode@gnu.org; Tue, 09 Sep 2014 07:49:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRJuq-0001W3-HW for emacs-orgmode@gnu.org; Tue, 09 Sep 2014 07:49:02 -0400 Received: from plane.gmane.org ([80.91.229.3]:34992) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRJuq-0001Vq-4M for emacs-orgmode@gnu.org; Tue, 09 Sep 2014 07:48:56 -0400 Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1XRJuo-00084D-Lz for emacs-orgmode@gnu.org; Tue, 09 Sep 2014 13:48:54 +0200 Received: from ip-77-24-113-128.web.vodafone.de ([77.24.113.128]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 09 Sep 2014 13:48:54 +0200 Received: from fb by ip-77-24-113-128.web.vodafone.de with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 09 Sep 2014 13:48:54 +0200 List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: emacs-orgmode@gnu.org --=-=-= Content-Type: text/plain Content-Disposition: inline Hi Nicolas, thanks for the review. I hope the new version is an improvement. Nicolas Goaziou 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-org-element-Adjust-content-boundaries-in-empty-cells.patch >From 2b1a63e70830e7604c7f59dd0110aedf3a9c1e53 Mon Sep 17 00:00:00 2001 From: Florian Beck 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0002-org-element-Implement-org-element-get.patch >From 10f68bf26f82f4cbc0e097bac9a4d3b997c10bc4 Mon Sep 17 00:00:00 2001 From: Florian Beck 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0003-org-table-Handle-optional-arguments-and-cleanup.patch >From 64f937fe289e7aca41471ec731aec1590bebe947 Mon Sep 17 00:00:00 2001 From: Florian Beck 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0004-test-org-table-Add-tests.patch >From a6a00af0f3f291fb0b4eb19012b80d28d3419a38 Mon Sep 17 00:00:00 2001 From: Florian Beck 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 --=-=-=--