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

[Nicolas Goaziou]

Hello,

Florian Beck <address@hidden> 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