Re: [O] [patch] extend org-meta-return to keywords

[Rasmus]

Hi,

Thanks for the comments.

This email is long (sorry); fortunately there are many blank lines. . .

Thierry Banel <address@hidden> writes:
> If not, maybe it would be better to keep the oriGiNaL cASe.

Yeah, fixed.

Nicolas Goaziou <address@hidden> writes:

> Rasmus <address@hidden> writes:
>
>>> Moreover, it can get in the way of expected M-RET behaviour, as in the
>>> following example
>>>
>>>   - item
>>>
>>>   | #+caption: test
>>>     untenrsiu
>>
>> I don't know if I should do something about this case.  I guess it would
>> be possible, but I find it awkward when behavior $BUTTON depends not
>> only on not only the adjacent element, but also the previous element.
>>
>> If it's important, I guess it should be toggled explicitly on by a
>> custom variable.
>
> M-RET, is, first and foremost, an important keybinding for editing the
> /structure/ of the document. The behaviour you want to add has nothing
> to do with structure.

I see it differently.  I see M-RET as a function that "magically" adds
more of what is adjacent to point.

I—obviously—think what I propose is better than what we have now.  Let's
go through the current functionality.

* Example 1:

#+LATEX_HEADER: foo

Click M-RET anywhere above (line-beginning-position) and you get
something like

#+LATEX
* _HEADER: foo

This is nonsense.


Click M-RET at (line-beginning-position) and you get

* #+LATEX_HEADER: foo

This is nonsense.

* Example 2

- s

#+LATEX_HEADER: foo % no space between ^ and #.

Same as example 2.

* Next case

- s

 #+LATEX_HEADER: foo % space between ^ and # 

M-RET at a position greater than (line-beginning-position) yields
something like

- s

 #+LATEX

- _HEADER: tes

This is nonsense.


M-RET at (line-beginning-position) will yield

- s

- | ← that's point

 #+LATEX_HEADER: foo % space between ^ and #

Thus far this is the only sensible result that we'd loose with the
patch.  If this is a great loss (rather than a bug in the current
implementation) this can be dealt with, though it adds complexity to the
meaning of M-RET (since it's a function of the previous element rather
than the adjacent one).

* Example 4

- s


#+LATEX_HEADER: foo % three spaces

This works like example 1.

> As a consequence, I'm not sure it should go with M-RET, and if it does,
> I'm pretty sure it should not override the main purpose of the binding.
> Inserting headlines, and possibly items, is much more important than
> duplicating keywords.

IMO the examples above show that M-RET fails at doing this in a sensible
manner is all but one case above.  The case where it does not fail
requires (i) that the keyword is not at the beginning of the line and
(ii) that the use has point before the keyword.  I don't care strongly
about this "feature"—It's just check that point is not at bol in
`org-meta-return'.

In fact I would be happy to go further.  One also get nonsense result
doing M-RET on #+begin_src lines and probably elsewhere.

>> Also, `org-insert-keyword' is not really using org-element anymore.
>
> It should since you make it interactive and can, therefore, be called on
> its own.

I moved the check to a separate function,
`org-looking-at-repeatable-keyword-p'.  There are now the following
issues:

  1. it's called twice: once in org-meta-return and once in
     org-insert-keyword.  I'm not sure if it's possible to emit a signal
     to avoid the second check somehow...  Perhaps the performance-loss
     is such that we should not worry about it.

  2. The name is terrible.  I'm also not sure if this function should
     reside in org.el.  Same for the regexp.  Thoughts?

> Some comments follow:

Thanks for these!

>> +     (indention
>> +      (buffer-substring
>> +       (line-beginning-position)
>> +       (save-excursion
>> +         (beginning-of-line)
>> +         (skip-chars-forward " \t")
>> +         (point))))
>
> Another option:
>
>   (ind (org-get-indentation))

Cool.  I really need to internalize to look for org-prefixed functions!


>> +     (keyword-re "#\\+\\(.+?\\):")
>> +     (key (save-excursion (beginning-of-line)
>> +                          (skip-chars-forward " \t")
>> +                          (looking-at keyword-re)
>> +                          (upcase (org-match-string-no-properties 1))))
>
> As discussed above, this is too fragile.  You need to duplicate the
> checks done in `org-meta-return' or refactor the code.

Yeah, I see your point.  Casual testing suggest it's more solid now
albeit there's now the double check as outlined above.
 
>> +     (end-of-keyword (save-excursion
>> +                       (beginning-of-line)
>> +                       (re-search-forward keyword-re (line-end-position) t)
>> +                       (point))))
>
>   (end-of-keyword (save-excursion (beginning-of-line) (search-forward ":"))
> [...]
> I think end-of-keyword should be bound after KEY is checked to avoid
> errors.

Since there's now a check before entering the function I guess this
should be safe.  Still I added extra checks, that I'm pretty sure are
unnecessary.  I'd be happy to remove these.

—Rasmus

-- 
Don't panic!!!

0001-org.el-Add-keyword-support-to-M-RET.patch
Description: Text Data