org-insert-heading rewritten from scratch

org-insert-heading rewritten from scratch

[Carsten Dominik]

Hi,

I have rewritten org-insert-heading, because it had become an unmaintainable beast.
Please follow up in this thread if you find problems with the new implementation.
Very likely there will be bugs, but now I am at least confident they can be fixed.

- Carsten

Re: org-insert-heading rewritten from scratch

[Eric Abrahamsen]

Carsten Dominik <carsten.dominik@gmail.com> writes:

> Hi,
>
> I have rewritten org-insert-heading, because it had become an unmaintainable beast.
> Please follow up in this thread if you find problems with the new implementation.
> Very likely there will be bugs, but now I am at least confident they can be fixed.
>
> - Carsten

Awesome! Thanks very much for doing this. Will report back with bugs.

E

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

On 8.8.2013, at 09:41, Eric Abrahamsen <eric@ericabrahamsen.net> wrote:

> Carsten Dominik <carsten.dominik@gmail.com> writes:
> 
>> Hi,
>> 
>> I have rewritten org-insert-heading, because it had become an unmaintainable beast.
>> Please follow up in this thread if you find problems with the new implementation.
>> Very likely there will be bugs, but now I am at least confident they can be fixed.
>> 
>> - Carsten
> 
> Awesome! Thanks very much for doing this. Will report back with bugs.

I take it none have been found?

- Carsten

> 
> E
> 
> 

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Hello,

Carsten Dominik <carsten.dominik@gmail.com> writes:

> On 8.8.2013, at 09:41, Eric Abrahamsen <eric@ericabrahamsen.net> wrote:
>
>> Carsten Dominik <carsten.dominik@gmail.com> writes:
>> 
>>> Hi,
>>> 
>>> I have rewritten org-insert-heading, because it had become an unmaintainable beast.
>>> Please follow up in this thread if you find problems with the new implementation.
>>> Very likely there will be bugs, but now I am at least confident they can be fixed.
>>> 
>>> - Carsten
>> 
>> Awesome! Thanks very much for doing this. Will report back with bugs.
>
> I take it none have been found?

Not really a bug, but I find some behaviour surprising: when at a the
beginning of a regular text line, there is no way to create a headline
just above it. In the following example:

  XCursor is at "X"

Neither M-RET, C-u M-RET, C-RET nor C-u C-RET can do it. Is it intended?

Also in this case, I think C-RET should create the new headline _after_
the subtree, since that's its whole point anyway, AFAIU.

Eventually, it seems that behaviour towards empty lines is a bit
unpredictable. In the following example, M-RET will be behave
differently on each blank line between Paragraph and "H2". C-RET will be
consistent. Note: I have `auto' as value for `heading' key in
`org-blank-before-new-entry'.

  * H1

    Paragraph



  * H2

IMO, using M-RET and C-RET should be as smooth an experience as
possible.

I also suggest to write function specifications as tests in test-org.el.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Eric Abrahamsen]

Carsten Dominik <carsten.dominik@gmail.com> writes:

> On 8.8.2013, at 09:41, Eric Abrahamsen <eric@ericabrahamsen.net> wrote:
>
>> Carsten Dominik <carsten.dominik@gmail.com> writes:
>> 
>>> Hi,
>>> 
>>> I have rewritten org-insert-heading, because it had become an unmaintainable beast.
>>> Please follow up in this thread if you find problems with the new implementation.
>>> Very likely there will be bugs, but now I am at least confident they can be fixed.
>>> 
>>> - Carsten
>> 
>> Awesome! Thanks very much for doing this. Will report back with bugs.
>
> I take it none have been found?
>
> - Carsten
>

Nope, I've been using this heavily and haven't seen anything surprising.
I do confirm Nicolas' observations on whitespace, though...

E

Re: org-insert-heading rewritten from scratch

[Samuel Wales]

I've found four, but they are minor.  In capture, you can create a
blank line with m-ret at the top．Repeated invocation creates invalid
headlines.  There is no c-ret way to create a headline above the first
child.  M-ret is slow.

On 8/31/13, Carsten Dominik <carsten.dominik@gmail.com> wrote:
> I take it none have been found?

-- 
The Kafka Pandemic: http://thekafkapandemic.blogspot.com

The disease DOES progress.  MANY people have died from it.  ANYBODY can get it.

Denmark: free Karina Hansen NOW.

Re: org-insert-heading rewritten from scratch

[Samuel Wales]

Clarification: it is no more slow than the original and it fixes bugs.
 Just answering your question about whether there are any bugs.

-- 
The Kafka Pandemic: http://thekafkapandemic.blogspot.com

The disease DOES progress.  MANY people have died from it.  ANYBODY can get it.

Denmark: free Karina Hansen NOW.

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

Hi Nicolas,

On 31.8.2013, at 16:34, Nicolas Goaziou <n.goaziou@gmail.com> wrote:

> Hello,
> 
> Carsten Dominik <carsten.dominik@gmail.com> writes:
> 
>> On 8.8.2013, at 09:41, Eric Abrahamsen <eric@ericabrahamsen.net> wrote:
>> 
>>> Carsten Dominik <carsten.dominik@gmail.com> writes:
>>> 
>>>> Hi,
>>>> 
>>>> I have rewritten org-insert-heading, because it had become an unmaintainable beast.
>>>> Please follow up in this thread if you find problems with the new implementation.
>>>> Very likely there will be bugs, but now I am at least confident they can be fixed.
>>>> 
>>>> - Carsten
>>> 
>>> Awesome! Thanks very much for doing this. Will report back with bugs.
>> 
>> I take it none have been found?
> 
> Not really a bug, but I find some behaviour surprising: when at a the
> beginning of a regular text line, there is no way to create a headline
> just above it. In the following example:
> 
>  XCursor is at "X"
> 
> Neither M-RET, C-u M-RET, C-RET nor C-u C-RET can do it. Is it intended?

Which behavior would you propose?  I guess you mean that, at the beginning of a line,
the result is so different for normal lines versus headlines?

The way I was thinking about the behavior at the beginning of a non-headline
is that it is the same as in the middle of a line:  Take the rest of the line
and turn it into a headline.

To create a headline before a nonempty line, I use `C-o M-RET'

To be sure:  I am happy to change the behavior if that is what people
want and if it makes logically sense.

> Also in this case, I think C-RET should create the new headline _after_
> the subtree, since that's its whole point anyway, AFAIU.

That is what happens for me.  It does not for you?

> 
> Eventually, it seems that behaviour towards empty lines is a bit
> unpredictable. In the following example, M-RET will be behave
> differently on each blank line between Paragraph and "H2". C-RET will be
> consistent.

Because it does what M-RET does in the last line of the subtree.

> Note: I have `auto' as value for `heading' key in
> `org-blank-before-new-entry'.
> 
>  * H1
> 
>    Paragraph
> 
> 
> 
>  * H2


Hmm, I do find this behavior consistent. M-RET does not change the number
of while lines after the current, only before, in order to either have
an empty line or not.

Which behavior would you propose?

> 
> IMO, using M-RET and C-RET should be as smooth an experience as
> possible.

I fully agree.

> 
> I also suggest to write function specifications as tests in test-org.el.

Yes, I have yet to write my first test.  Need to figure out how that works.

- Carsten

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Hello,

Carsten Dominik <carsten.dominik@gmail.com> writes:

> On 31.8.2013, at 16:34, Nicolas Goaziou <n.goaziou@gmail.com> wrote:

>> Not really a bug, but I find some behaviour surprising: when at a the
>> beginning of a regular text line, there is no way to create a headline
>> just above it. In the following example:
>> 
>>  XCursor is at "X"
>> 
>> Neither M-RET, C-u M-RET, C-RET nor C-u C-RET can do it. Is it intended?
>
> Which behavior would you propose?  I guess you mean that, at the beginning of a line,
> the result is so different for normal lines versus headlines?

Yes.

I never, ever, want to turn a regular text line into a headline. OTOH
during note taking, I very often write paragraphs and, afterwards,
decide to split them into sections.

Moreover, AFAICT, there's no more difference between C-u M-RET, which
meant "create headline right here" and M-RET.

> The way I was thinking about the behavior at the beginning of a non-headline
> is that it is the same as in the middle of a line:  Take the rest of the line
> and turn it into a headline.p
>
> To create a headline before a nonempty line, I use `C-o M-RET'

That's what I do. But that's not optimal and it introduces another
problem. In the following example

  * H

  XText

I want to create a headline above "Text" and point is at "X". I use C-o
M-RET and the latter greedily eat the blank line above, resulting in

  * H
  * X
  Text

as if I had typed C-p M-RET instead.

Again, I have `org-blank-before-new-entry' set to `auto' for headlines.
Since there is no information about how many blank lines I usually want
before headlines, I think the algorithm should trust me and do not
remove any blank lines (see `org-list-separating-blank-lines-number').

In the same vein, in the following situation

  * H1

    Text1

  * H2

    XText2

C-o M-RET should leave blank above "Text2" because it has information
about my preferences.

Oddly, in an empty buffer, it will create a blank line above.

>> Also in this case, I think C-RET should create the new headline _after_
>> the subtree, since that's its whole point anyway, AFAIU.
>
> That is what happens for me.  It does not for you?

It does, sorry about the noise.

> Hmm, I do find this behavior consistent. M-RET does not change the number
> of while lines after the current, only before, in order to either have
> an empty line or not.
>
> Which behavior would you propose?

Well same as above: I think it eats blank lines where it shouldn't. It
the following cases:

  * H1

  ** H2

     H
  X

and

  * H1

  * H2

    H

  X  

I don't think there's any reason for M-RET to eat blank line before
point with either `org-blank-before-new-entry' set to `auto' or t. It
should know that a blank line is expected before the new entry and
therefore should create the headline at point.

>> I also suggest to write function specifications as tests in test-org.el.
>
> Yes, I have yet to write my first test.  Need to figure out how that
> works.

That's simple. You can use the following pattern:

  (ert-deftest test-org/insert-heading ()
    "Test specifications for heading insertion."
    ;; In an empty buffer, headline should be created at its beginning,
    ;; notwithstanding value for `org-blank-before-new-entry'.
    (should
     (equal "* "
            (org-test-with-temp-text ""
              (let ((org-blank-before-new-entry '((heading . nil))))
                (org-insert-heading))
              (buffer-string))))
    (should
     (equal "* "
            (org-test-with-temp-text ""
              (let ((org-blank-before-new-entry '((heading . t))))
                (org-insert-heading))
              (buffer-string))))
    (should
     (equal "* "
            (org-test-with-temp-text ""
              (let ((org-blank-before-new-entry '((heading . auto))))
                (org-insert-heading))
              (buffer-string))))
    ;; At the end of a single headline: Create headline below, following
    ;; `org-blank-before-new-entry' specifications.  When it is `auto',
    ;; since there's not enough information to deduce what is expected,
    ;; create it just below.
    (should
     (equal "* H\n* "
            (org-test-with-temp-text "* H"
              (end-of-line)
              (let ((org-blank-before-new-entry '((heading . nil))))
                (org-insert-heading))
              (buffer-string))))
    (should
     (equal "* H\n\n* "
            (org-test-with-temp-text "* H"
              (end-of-line)
              (let ((org-blank-before-new-entry '((heading . t))))
                (org-insert-heading))
              (buffer-string))))
    (should
     (equal "* H\n* "
            (org-test-with-temp-text "* H"
              (end-of-line)
              (let ((org-blank-before-new-entry '((heading . auto))))
                (org-insert-heading))
              (buffer-string))))
    ;; Etc.
    )

I suggest to always put the `should' (or `should-not', `should-error')
outside each test: it makes it easier to inspect results from partial
evaluations.

You run each test individually with C-x C-e at the end of the `should'
sexp. You run all tests with "make test" from "org/" directory.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

Hi Nioclas,

thank you for the feedback.

On 1.9.2013, at 10:19, Nicolas Goaziou <n.goaziou@gmail.com> wrote:

> Hello,
> 
> Carsten Dominik <carsten.dominik@gmail.com> writes:
> 
>> On 31.8.2013, at 16:34, Nicolas Goaziou <n.goaziou@gmail.com> wrote:
> 
>>> Not really a bug, but I find some behaviour surprising: when at a the
>>> beginning of a regular text line, there is no way to create a headline
>>> just above it. In the following example:
>>> 
>>> XCursor is at "X"
>>> 
>>> Neither M-RET, C-u M-RET, C-RET nor C-u C-RET can do it. Is it intended?
>> 
>> Which behavior would you propose?  I guess you mean that, at the beginning of a line,
>> the result is so different for normal lines versus headlines?
> 
> Yes.
> 
> I never, ever, want to turn a regular text line into a headline. OTOH
> during note taking, I very often write paragraphs and, afterwards,
> decide to split them into sections.

OK, I'll go along with this.  `C-c *' if for turning a line into a headline,
M-RET for making a *new* headline.  That is logical.  Fixed now.

> 
> Moreover, AFAICT, there's no more difference between C-u M-RET, which
> meant "create headline right here" and M-RET.

C-u means: Do not ask org-insert-item, just go ahead and insert a heading.
That is supposed to be the only difference.

> 
>> The way I was thinking about the behavior at the beginning of a non-headline
>> is that it is the same as in the middle of a line:  Take the rest of the line
>> and turn it into a headline.p
>> 
>> To create a headline before a nonempty line, I use `C-o M-RET'
> 
> That's what I do. But that's not optimal and it introduces another
> problem. In the following example
> 
>  * H
> 
>  XText
> 
> I want to create a headline above "Text" and point is at "X". I use C-o
> M-RET and the latter greedily eat the blank line above, resulting in
> 
>  * H
>  * X
>  Text
> 
> as if I had typed C-p M-RET instead.
> 
> Again, I have `org-blank-before-new-entry' set to `auto' for headlines.
> Since there is no information about how many blank lines I usually want
> before headlines, I think the algorithm should trust me and do not
> remove any blank lines (see `org-list-separating-blank-lines-number').

I believe this is now better, please check.

> 
> In the same vein, in the following situation
> 
>  * H1
> 
>    Text1
> 
>  * H2
> 
>    XText2
> 
> C-o M-RET should leave blank above "Text2" because it has information
> about my preferences.

I think this is fixed as well.

> 
> Oddly, in an empty buffer, it will create a blank line above.

Fixed.

> 
>>> Also in this case, I think C-RET should create the new headline _after_
>>> the subtree, since that's its whole point anyway, AFAIU.
>> 
>> That is what happens for me.  It does not for you?
> 
> It does, sorry about the noise.
> 
>> Hmm, I do find this behavior consistent. M-RET does not change the number
>> of while lines after the current, only before, in order to either have
>> an empty line or not.
>> 
>> Which behavior would you propose?
> 
> Well same as above: I think it eats blank lines where it shouldn't. It
> the following cases:
> 
>  * H1
> 
>  ** H2
> 
>     H
>  X
> 
> and
> 
>  * H1
> 
>  * H2
> 
>    H
> 
>  X  
> 
> I don't think there's any reason for M-RET to eat blank line before
> point with either `org-blank-before-new-entry' set to `auto' or t. It
> should know that a blank line is expected before the new entry and
> therefore should create the headline at point.

Please take another look.

Thank you!

- Carsten

> 
>>> I also suggest to write function specifications as tests in test-org.el.
>> 
>> Yes, I have yet to write my first test.  Need to figure out how that
>> works.
> 
> That's simple. You can use the following pattern:
> 
>  (ert-deftest test-org/insert-heading ()
>    "Test specifications for heading insertion."
>    ;; In an empty buffer, headline should be created at its beginning,
>    ;; notwithstanding value for `org-blank-before-new-entry'.
>    (should
>     (equal "* "
>            (org-test-with-temp-text ""
>              (let ((org-blank-before-new-entry '((heading . nil))))
>                (org-insert-heading))
>              (buffer-string))))
>    (should
>     (equal "* "
>            (org-test-with-temp-text ""
>              (let ((org-blank-before-new-entry '((heading . t))))
>                (org-insert-heading))
>              (buffer-string))))
>    (should
>     (equal "* "
>            (org-test-with-temp-text ""
>              (let ((org-blank-before-new-entry '((heading . auto))))
>                (org-insert-heading))
>              (buffer-string))))
>    ;; At the end of a single headline: Create headline below, following
>    ;; `org-blank-before-new-entry' specifications.  When it is `auto',
>    ;; since there's not enough information to deduce what is expected,
>    ;; create it just below.
>    (should
>     (equal "* H\n* "
>            (org-test-with-temp-text "* H"
>              (end-of-line)
>              (let ((org-blank-before-new-entry '((heading . nil))))
>                (org-insert-heading))
>              (buffer-string))))
>    (should
>     (equal "* H\n\n* "
>            (org-test-with-temp-text "* H"
>              (end-of-line)
>              (let ((org-blank-before-new-entry '((heading . t))))
>                (org-insert-heading))
>              (buffer-string))))
>    (should
>     (equal "* H\n* "
>            (org-test-with-temp-text "* H"
>              (end-of-line)
>              (let ((org-blank-before-new-entry '((heading . auto))))
>                (org-insert-heading))
>              (buffer-string))))
>    ;; Etc.
>    )
> 
> I suggest to always put the `should' (or `should-not', `should-error')
> outside each test: it makes it easier to inspect results from partial
> evaluations.
> 
> You run each test individually with C-x C-e at the end of the `should'
> sexp. You run all tests with "make test" from "org/" directory.
> 
> 
> Regards,
> 
> -- 
> Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Carsten Dominik <carsten.dominik@gmail.com> writes:

> Please take another look.

There is no new commit in maint or master.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Hello,

Carsten Dominik <carsten.dominik@gmail.com> writes:

>> Moreover, AFAICT, there's no more difference between C-u M-RET, which
>> meant "create headline right here" and M-RET.
>
> C-u means: Do not ask org-insert-item, just go ahead and insert a heading.
> That is supposed to be the only difference.

Point taken. I thought C-u had another meaning too.

> Please take another look.

It looks better. Though it seems to ignore a nil
`org-blank-before-new-entry' value for `heading':

  * H

  X

Here I get,

  * H

  * X

but, with a nil value, it should be:

  * H
  * X


Thanks.

Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

On Sep 3, 2013, at 3:16 PM, Nicolas Goaziou <n.goaziou@gmail.com> wrote:

> Hello,
> 
> Carsten Dominik <carsten.dominik@gmail.com> writes:
> 
>>> Moreover, AFAICT, there's no more difference between C-u M-RET, which
>>> meant "create headline right here" and M-RET.
>> 
>> C-u means: Do not ask org-insert-item, just go ahead and insert a heading.
>> That is supposed to be the only difference.
> 
> Point taken. I thought C-u had another meaning too.
> 
>> Please take another look.
> 
> It looks better. Though it seems to ignore a nil
> `org-blank-before-new-entry' value for `heading':
> 
>  * H
> 
>  X
> 
> Here I get,
> 
>  * H
> 
>  * X
> 
> but, with a nil value, it should be:
> 
>  * H
>  * X


Hmmm, I thought you just asked me to implement exactly what you see, namely that M-RET will not remove empty lines above the cursor - only add them.  Did I misunderstand?

Thanks

- Carsten

> 
> 
> Thanks.
> 
> Regards,
> 
> -- 
> Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

On Sep 3, 2013, at 3:25 PM, Carsten Dominik <carsten.dominik@gmail.com> wrote:

> 
> On Sep 3, 2013, at 3:16 PM, Nicolas Goaziou <n.goaziou@gmail.com> wrote:
> 
>> Hello,
>> 
>> Carsten Dominik <carsten.dominik@gmail.com> writes:
>> 
>>>> Moreover, AFAICT, there's no more difference between C-u M-RET, which
>>>> meant "create headline right here" and M-RET.
>>> 
>>> C-u means: Do not ask org-insert-item, just go ahead and insert a heading.
>>> That is supposed to be the only difference.
>> 
>> Point taken. I thought C-u had another meaning too.
>> 
>>> Please take another look.
>> 
>> It looks better. Though it seems to ignore a nil
>> `org-blank-before-new-entry' value for `heading':
>> 
>> * H
>> 
>> X
>> 
>> Here I get,
>> 
>> * H
>> 
>> * X
>> 
>> but, with a nil value, it should be:
>> 
>> * H
>> * X
> 
> 
> Hmmm, I thought you just asked me to implement exactly what you see, namely that M-RET will not remove empty lines above the cursor - only add them.  Did I misunderstand?


OK, I just re-read your earlier mail, and you had only made this request for auto and t values of org-blank before new entry.

I'll contemplate this.

- Carsten

> 
> Thanks
> 
> - Carsten
> 
>> 
>> 
>> Thanks.
>> 
>> Regards,
>> 
>> -- 
>> Nicolas Goaziou
> 

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Carsten Dominik <carsten.dominik@gmail.com> writes:

> Hmmm, I thought you just asked me to implement exactly what you see,
> namely that M-RET will not remove empty lines above the cursor - only
> add them.  Did I misunderstand?

I think so.

M-RET should not remove (or add) anything when it has no information
whatsoever about what the user want, namely when behaviour is set to
`auto' _and_ number of blank lines (or lack thereof) cannot be deduced
from other headlines.

When `org-blank-before-new-entry' is set to t (respectively nil), the
message is pretty clear. There are no heuristics involved and the
function can add (respectively remove) blank lines to its heart's
contents.

Sorry if I wasn't clear enough.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

I am still not clear about this.  In your earlier mail you made this example:

> Well same as above: I think it eats blank lines where it shouldn't. It
> the following cases:
> 
>  * H1
> 
>  ** H2
> 
>     H
>  X
> 
> and
> 
>  * H1
> 
>  * H2
> 
>    H
> 
>  X  
> 
> I don't think there's any reason for M-RET to eat blank line before
> point with either `org-blank-before-new-entry' set to `auto' or t. It
> should know that a blank line is expected before the new entry and
> therefore should create the headline at point.

WIth `auto', there will be an empty line before the next entry in both cases.  In both cases it looks at the H2 headline and sees the empty line before it.  Are you saying the behaviour should be different in both cases?

Cheers

- Carsten


On Sep 3, 2013, at 3:38 PM, Nicolas Goaziou <n.goaziou@gmail.com> wrote:

> Carsten Dominik <carsten.dominik@gmail.com> writes:
> 
>> Hmmm, I thought you just asked me to implement exactly what you see,
>> namely that M-RET will not remove empty lines above the cursor - only
>> add them.  Did I misunderstand?
> 
> I think so.
> 
> M-RET should not remove (or add) anything when it has no information
> whatsoever about what the user want, namely when behaviour is set to
> `auto' _and_ number of blank lines (or lack thereof) cannot be deduced
> from other headlines.
> 
> When `org-blank-before-new-entry' is set to t (respectively nil), the
> message is pretty clear. There are no heuristics involved and the
> function can add (respectively remove) blank lines to its heart's
> contents.
> 
> Sorry if I wasn't clear enough.
> 
> 
> Regards,
> 
> -- 
> Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Carsten Dominik <carsten.dominik@gmail.com> writes:

> I am still not clear about this.  In your earlier mail you made this example:
>
>> Well same as above: I think it eats blank lines where it shouldn't. It
>> the following cases:
>> 
>>  * H1
>> 
>>  ** H2
>> 
>>     H
>>  X
>> 
>> and
>> 
>>  * H1
>> 
>>  * H2
>> 
>>    H
>> 
>>  X  
>> 
>> I don't think there's any reason for M-RET to eat blank line before
>> point with either `org-blank-before-new-entry' set to `auto' or t. It
>> should know that a blank line is expected before the new entry and
>> therefore should create the headline at point.
>
> WIth `auto', there will be an empty line before the next entry in both
> cases. In both cases it looks at the H2 headline and sees the empty
> line before it. Are you saying the behaviour should be different in
> both cases?

Yes, it should.

As you point out, in both cases the algorithm knows that there should be
a blank line before the new entry (with the assumption that behaviour is
set to `auto'). In the first case, if it inserts the headline at point,
there will be none, so it has to add one. In the second case, there is
no need to add one since creating it at point will fulfill the
requirement (which is "a blank line before new headline").

Re-reading myself, I agree that my quoted explanations are a bit
confusing. I hope this should clarify my point.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

On Sep 3, 2013, at 3:58 PM, Nicolas Goaziou <n.goaziou@gmail.com> wrote:

> Carsten Dominik <carsten.dominik@gmail.com> writes:
> 
>> I am still not clear about this.  In your earlier mail you made this example:
>> 
>>> Well same as above: I think it eats blank lines where it shouldn't. It
>>> the following cases:
>>> 
>>> * H1
>>> 
>>> ** H2
>>> 
>>>    H
>>> X
>>> 
>>> and
>>> 
>>> * H1
>>> 
>>> * H2
>>> 
>>>   H
>>> 
>>> X  
>>> 
>>> I don't think there's any reason for M-RET to eat blank line before
>>> point with either `org-blank-before-new-entry' set to `auto' or t. It
>>> should know that a blank line is expected before the new entry and
>>> therefore should create the headline at point.
>> 
>> WIth `auto', there will be an empty line before the next entry in both
>> cases. In both cases it looks at the H2 headline and sees the empty
>> line before it. Are you saying the behaviour should be different in
>> both cases?
> 
> Yes, it should.
> 
> As you point out, in both cases the algorithm knows that there should be
> a blank line before the new entry (with the assumption that behaviour is
> set to `auto'). In the first case, if it inserts the headline at point,
> there will be none, so it has to add one. In the second case, there is
> no need to add one since creating it at point will fulfill the
> requirement (which is "a blank line before new headline").

Yes.  But you agree that the *result* should be the same, i.e. that there will
be an empty line before the newly inserted headline.

I think/hope we do agree now.

> 
> Re-reading myself, I agree that my quoted explanations are a bit
> confusing. I hope this should clarify my point.

I think so.

Thank you for your patience.

- Carsten

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Carsten Dominik <carsten.dominik@gmail.com> writes:

> Yes.  But you agree that the *result* should be the same, i.e. that there will
> be an empty line before the newly inserted headline.
>
> I think/hope we do agree now.

We do.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Hello,

There seem to be a bug when using M-RET at "X" in the following buffer:

  * H1
  TextX

Try it with different values for `org-blank-before-new-entry'.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> There seem to be a bug when using M-RET at "X" in the following buffer:
>
>   * H1
>   TextX
>
> Try it with different values for `org-blank-before-new-entry'.

Well, please scratch that: I was on a dubious local branch. Sorry for
the noise.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

Hi Carsten

On Thu, Aug 8, 2013 at 8:43 AM, Carsten Dominik
<carsten.dominik@gmail.com> wrote:
> I have rewritten org-insert-heading, because it had become an unmaintainable beast.
> Please follow up in this thread if you find problems with the new implementation.
> Very likely there will be bugs, but now I am at least confident they can be fixed.

On the way of rewiring my muscle memory from C-RET to M-RET for some
cases, I stumbled across this:

#+DRAWERS: MyStructuredDrawer
:MyStructuredDrawer:
- a
:END:

To insert a new item I once changed to use C-RET also on items. How is
one supposed to do this now within a drawer? M-RET just inserts an
empty line. I would like to suggest the attached patches with an ERT.
They change (fix?) org-meta-return to insert a new item in this case.

About the following different issue I don't care as much and only
wanted to report: C-RET before any headline when within a drawer, or
generally before any headline(?), could bark instead of changing to a
headline leading to invalid Org syntax within a drawer.

Michael

[-- Attachment #2: 0001-Add-ERTs-for-org-meta-return.patch.txt --]
[-- Type: text/plain, Size: 1621 bytes --]

From 6bc4c15c4a76a98c841e8a200c75f5a0737ffece Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.ch.brand@gmail.com>
Date: Mon, 9 Sep 2013 18:38:58 +0200
Subject: [PATCH 1/2] Add ERTs for org-meta-return

* testing/lisp/test-org-list.el (test-org-list/insert-item): Adapt
docstring.
* (test-org-list/meta-return): New `ert-deftest' to test
`org-meta-return'.
---
 testing/lisp/test-org-list.el | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/testing/lisp/test-org-list.el b/testing/lisp/test-org-list.el
index ac81d4d..ea19606 100644
--- a/testing/lisp/test-org-list.el
+++ b/testing/lisp/test-org-list.el
@@ -627,7 +627,7 @@
     (should (org-invisible-p2))))
 
 (ert-deftest test-org-list/insert-item ()
-  "Test item insertion."
+  "Test item insertion with `org-insert-item'."
   ;; Blank lines specifications.
   ;;
   ;; Non-nil `org-blank-before-new-entry': insert a blank line, unless
@@ -713,6 +713,22 @@
        (forward-line -1)
        (looking-at "$")))))
 
+(ert-deftest test-org-list/meta-return ()
+  "Test item insertion with `org-meta-return'."
+  (should
+   (org-test-with-temp-text "- a"
+     (org-meta-return)
+     (beginning-of-line)
+     (looking-at "- $")))
+  ;; TODO Insert an item also in a drawer.
+  (should
+   (let ((org-drawers '("MYDRAWER")))
+     (org-test-with-temp-text ":MYDRAWER:\n- a\n:END:"
+       (forward-line)
+       (org-meta-return)
+       (forward-line -1)
+       (looking-at "$")))))
+
 (ert-deftest test-org-list/repair ()
   "Test `org-list-repair' specifications."
   ;; Repair indentation.
-- 
1.7.12.4 (Apple Git-37)


[-- Attachment #3: 0002-org-meta-return-Insert-an-item-also-in-a-drawer.patch.txt --]
[-- Type: text/plain, Size: 1846 bytes --]

From 9896499fb7f497a13857b5b86f33cfbf1b918029 Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.ch.brand@gmail.com>
Date: Mon, 9 Sep 2013 18:40:07 +0200
Subject: [PATCH 2/2] org-meta-return: Insert an item also in a drawer

* lisp/org.el (org-meta-return): Exclude item from cond for drawer.
* testing/lisp/test-org-list.el (test-org-list/meta-return): On an
item in a drawer expect an item to be inserted.
---
 lisp/org.el                   | 3 ++-
 testing/lisp/test-org-list.el | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 59a22a2..edc8725 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20698,7 +20698,8 @@ See the individual commands for more information."
   (org-check-before-invisible-edit 'insert)
   (cond
    ((run-hook-with-args-until-success 'org-metareturn-hook))
-   ((or (org-at-drawer-p) (org-in-drawer-p) (org-at-property-p))
+   ((and (or (org-at-drawer-p) (org-in-drawer-p) (org-at-property-p))
+	 (not (org-in-item-p)))
     (newline-and-indent))
    ((org-at-table-p)
     (call-interactively 'org-table-wrap-region))
diff --git a/testing/lisp/test-org-list.el b/testing/lisp/test-org-list.el
index ea19606..f3ced15 100644
--- a/testing/lisp/test-org-list.el
+++ b/testing/lisp/test-org-list.el
@@ -720,14 +720,14 @@
      (org-meta-return)
      (beginning-of-line)
      (looking-at "- $")))
-  ;; TODO Insert an item also in a drawer.
+  ;; Insert an item also in a drawer.
   (should
    (let ((org-drawers '("MYDRAWER")))
      (org-test-with-temp-text ":MYDRAWER:\n- a\n:END:"
        (forward-line)
        (org-meta-return)
-       (forward-line -1)
-       (looking-at "$")))))
+       (beginning-of-line)
+       (looking-at "- $")))))
 
 (ert-deftest test-org-list/repair ()
   "Test `org-list-repair' specifications."
-- 
1.7.12.4 (Apple Git-37)

Re: org-insert-heading rewritten from scratch

[Nicolas Goaziou]

Hello,

Michael Brand <michael.ch.brand@gmail.com> writes:

> About the following different issue I don't care as much and only
> wanted to report: C-RET before any headline when within a drawer, or
> generally before any headline(?), could bark instead of changing to a
> headline leading to invalid Org syntax within a drawer.

I don't think it should bark, but it should insert the headline after
the drawer, i.e. just before the first headline, if there's one at all,
or at the end of the buffer.

> From 9896499fb7f497a13857b5b86f33cfbf1b918029 Mon Sep 17 00:00:00 2001
> From: Michael Brand <michael.ch.brand@gmail.com>
> Date: Mon, 9 Sep 2013 18:40:07 +0200
> Subject: [PATCH 2/2] org-meta-return: Insert an item also in a drawer
>
> * lisp/org.el (org-meta-return): Exclude item from cond for drawer.
> * testing/lisp/test-org-list.el (test-org-list/meta-return): On an
> item in a drawer expect an item to be inserted.

FWIW, I think the test belongs to test-org.el, not to test-org-list.el.


Regards,

-- 
Nicolas Goaziou

Re: org-insert-heading rewritten from scratch

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

Hi Nicolas

On Mon, Sep 9, 2013 at 9:10 PM, Nicolas Goaziou <n.goaziou@gmail.com> wrote:
> FWIW, I think the test belongs to test-org.el, not to test-org-list.el.

Ok, I assume, then better also with more tests than only for list items.
See the attached patches that replace the previous ones.

Michael

[-- Attachment #2: 0001-Add-ERTs-for-org-meta-return.patch.txt --]
[-- Type: text/plain, Size: 1892 bytes --]

From 66f6d15234bda97fc2e197efd2f9cb7c6439fef1 Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.ch.brand@gmail.com>
Date: Mon, 9 Sep 2013 22:32:36 +0200
Subject: [PATCH 1/2] Add ERTs for org-meta-return

* testing/lisp/test-org.el (test-org/meta-return): New `ert-deftest'
to test `org-meta-return'.
---
 testing/lisp/test-org.el | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 8a1e9f1..4944c24 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -365,6 +365,51 @@
 
 
 \f
+;;; Editing
+
+;;;; Insert elements
+
+(ert-deftest test-org/meta-return ()
+  "Test M-RET (`org-meta-return')."
+  ;; In a table field insert a row above.
+  (should
+   (org-test-with-temp-text "| a |"
+     (forward-char)
+     (org-meta-return)
+     (forward-line -1)
+     (looking-at "|   |$")))
+  ;; In a paragraph change current line into a header.
+  (should
+   (org-test-with-temp-text "a"
+     (org-meta-return)
+     (beginning-of-line)
+     (looking-at "\* a$")))
+  ;; In an item insert an item, in this case above.
+  (should
+   (org-test-with-temp-text "- a"
+     (org-meta-return)
+     (beginning-of-line)
+     (looking-at "- $")))
+  ;; In a drawer and paragraph insert an empty line, in this case above.
+  (should
+   (let ((org-drawers '("MYDRAWER")))
+     (org-test-with-temp-text ":MYDRAWER:\na\n:END:"
+       (forward-line)
+       (org-meta-return)
+       (forward-line -1)
+       (looking-at "$"))))
+  ;; TODO In a drawer and item insert an item, in this case above.
+  (should
+   (let ((org-drawers '("MYDRAWER")))
+     (org-test-with-temp-text ":MYDRAWER:\n- a\n:END:"
+       (forward-line)
+       (org-meta-return)
+       (forward-line -1)
+       (looking-at "$")))))
+
+
+
+\f
 ;;; Links
 
 ;;;; Fuzzy Links
-- 
1.7.12.4 (Apple Git-37)


[-- Attachment #3: 0002-org-meta-return-Insert-an-item-also-in-a-drawer.patch.txt --]
[-- Type: text/plain, Size: 1762 bytes --]

From fddcaaed03630e7acc26ed701937b2ca17184fe5 Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.ch.brand@gmail.com>
Date: Mon, 9 Sep 2013 22:33:47 +0200
Subject: [PATCH 2/2] org-meta-return: Insert an item also in a drawer

* lisp/org.el (org-meta-return): Exclude item from cond for drawer.
* testing/lisp/test-org.el (test-org/meta-return): In a drawer and
item insert an item.
---
 lisp/org.el              | 3 ++-
 testing/lisp/test-org.el | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 59a22a2..edc8725 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20698,7 +20698,8 @@ See the individual commands for more information."
   (org-check-before-invisible-edit 'insert)
   (cond
    ((run-hook-with-args-until-success 'org-metareturn-hook))
-   ((or (org-at-drawer-p) (org-in-drawer-p) (org-at-property-p))
+   ((and (or (org-at-drawer-p) (org-in-drawer-p) (org-at-property-p))
+	 (not (org-in-item-p)))
     (newline-and-indent))
    ((org-at-table-p)
     (call-interactively 'org-table-wrap-region))
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 4944c24..3538242 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -398,14 +398,14 @@
        (org-meta-return)
        (forward-line -1)
        (looking-at "$"))))
-  ;; TODO In a drawer and item insert an item, in this case above.
+  ;; In a drawer and item insert an item, in this case above.
   (should
    (let ((org-drawers '("MYDRAWER")))
      (org-test-with-temp-text ":MYDRAWER:\n- a\n:END:"
        (forward-line)
        (org-meta-return)
-       (forward-line -1)
-       (looking-at "$")))))
+       (beginning-of-line)
+       (looking-at "- $")))))
 
 
 
-- 
1.7.12.4 (Apple Git-37)

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

Hi Michael,

thanks for the patch - I think we will go with Nicolas' patch for fixing org-insert-heading.  May I ask you to check if your tests still to the right thing, and resubmit the test patch?

Many thanks in advance.

- Carsten

On 9.9.2013, at 22:37, Michael Brand <michael.ch.brand@gmail.com> wrote:

> Hi Nicolas
> 
> On Mon, Sep 9, 2013 at 9:10 PM, Nicolas Goaziou <n.goaziou@gmail.com> wrote:
>> FWIW, I think the test belongs to test-org.el, not to test-org-list.el.
> 
> Ok, I assume, then better also with more tests than only for list items.
> See the attached patches that replace the previous ones.
> 
> Michael
> <0001-Add-ERTs-for-org-meta-return.patch.txt><0002-org-meta-return-Insert-an-item-also-in-a-drawer.patch.txt>


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: org-insert-heading rewritten from scratch

[Michael Brand]

[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

Hi Carsten

On Thu, Sep 12, 2013 at 10:20 PM, Carsten Dominik
<carsten.dominik@gmail.com> wrote:
> thanks for the patch - I think we will go with Nicolas' patch for fixing org-insert-heading.  May I ask you to check if your tests still to the right thing, and resubmit the test patch?
>
> Many thanks in advance.

Yes, merged patch attached. No changes in the test cases were necessary.

Michael

[-- Attachment #2: 0001-Add-ERTs-for-org-meta-return.patch.txt --]
[-- Type: text/plain, Size: 1888 bytes --]

From d87dd6c70c594b55abcabdb67ea6852b261a1a7c Mon Sep 17 00:00:00 2001
From: Michael Brand <michael.ch.brand@gmail.com>
Date: Thu, 12 Sep 2013 22:48:16 +0200
Subject: [PATCH] Add ERTs for org-meta-return

* testing/lisp/test-org.el (test-org/meta-return): New `ert-deftest'
to test `org-meta-return'.
---
 testing/lisp/test-org.el | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 8a1e9f1..3538242 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -365,6 +365,51 @@
 
 
 \f
+;;; Editing
+
+;;;; Insert elements
+
+(ert-deftest test-org/meta-return ()
+  "Test M-RET (`org-meta-return')."
+  ;; In a table field insert a row above.
+  (should
+   (org-test-with-temp-text "| a |"
+     (forward-char)
+     (org-meta-return)
+     (forward-line -1)
+     (looking-at "|   |$")))
+  ;; In a paragraph change current line into a header.
+  (should
+   (org-test-with-temp-text "a"
+     (org-meta-return)
+     (beginning-of-line)
+     (looking-at "\* a$")))
+  ;; In an item insert an item, in this case above.
+  (should
+   (org-test-with-temp-text "- a"
+     (org-meta-return)
+     (beginning-of-line)
+     (looking-at "- $")))
+  ;; In a drawer and paragraph insert an empty line, in this case above.
+  (should
+   (let ((org-drawers '("MYDRAWER")))
+     (org-test-with-temp-text ":MYDRAWER:\na\n:END:"
+       (forward-line)
+       (org-meta-return)
+       (forward-line -1)
+       (looking-at "$"))))
+  ;; In a drawer and item insert an item, in this case above.
+  (should
+   (let ((org-drawers '("MYDRAWER")))
+     (org-test-with-temp-text ":MYDRAWER:\n- a\n:END:"
+       (forward-line)
+       (org-meta-return)
+       (beginning-of-line)
+       (looking-at "- $")))))
+
+
+
+\f
 ;;; Links
 
 ;;;; Fuzzy Links
-- 
1.7.12.4 (Apple Git-37)

Re: org-insert-heading rewritten from scratch

[Carsten Dominik]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

Hi Michael,

I have applied your testing patch, thank you!

- Carsten

On 12.9.2013, at 22:52, Michael Brand <michael.ch.brand@gmail.com> wrote:

> Hi Carsten
> 
> On Thu, Sep 12, 2013 at 10:20 PM, Carsten Dominik
> <carsten.dominik@gmail.com> wrote:
>> thanks for the patch - I think we will go with Nicolas' patch for fixing org-insert-heading.  May I ask you to check if your tests still to the right thing, and resubmit the test patch?
>> 
>> Many thanks in advance.
> 
> Yes, merged patch attached. No changes in the test cases were necessary.
> 
> Michael
> <0001-Add-ERTs-for-org-meta-return.patch.txt>


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 455 bytes --]