Re: [O] [PATCH] org-table-beginning/end-of-field
Hello, Florian Beck f...@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
Re: [O] [PATCH] org-table-beginning/end-of-field
Hello, Florian Beck f...@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:point1 | Cell2 |Cell3|\n| Cell4 | | Cell5| Regards, -- Nicolas Goaziou
Re: [O] [PATCH] org-table-beginning/end-of-field
Nicolas Goaziou m...@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:point1 | Cell2 |Cell3|\n| Cell4 | | Cell5| Great, thanks! -- Florian Beck
Re: [O] [PATCH] org-table-beginning/end-of-field
Hi Nicolas, thanks for the review. I hope the new version is an improvement. Nicolas Goaziou m...@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 From 2b1a63e70830e7604c7f59dd0110aedf3a9c1e53 Mon Sep 17 00:00:00 2001 From: Florian Beck f...@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 @@ -,12 +,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 From 10f68bf26f82f4cbc0e097bac9a4d3b997c10bc4 Mon Sep 17 00:00:00 2001 From: Florian Beck f...@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 From 64f937fe289e7aca41471ec731aec1590bebe947 Mon Sep 17 00:00:00 2001 From: Florian Beck f...@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 +++
Re: [O] [PATCH] org-table-beginning/end-of-field
Nicolas Goaziou m...@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 From 4fb2bbff2238d15ae7c896e0eb268b74ea4e56dc Mon Sep 17 00:00:00 2001 From: Florian Beck f...@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: [O] [PATCH] org-table-beginning/end-of-field
Florian Beck f...@miszellen.de writes: How about this? Nice. Some comments follow. From 4fb2bbff2238d15ae7c896e0eb268b74ea4e56dc Mon Sep 17 00:00:00 2001 From: Florian Beck f...@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
