feat: Headline:update_cookie() now works for both list and headline children

[oncomouse]

Closes #305

[jgollenz]

Thanks a lot for the contribution :) Please have a look at the tests in tests/plenary/ui/mappings_spec.lua (it starts at L1754) and add some for the functionality you added 👍

[jgollenz]

Why is this necessary? And if we update_cookie() here, is there even a need to call it in L208?

[oncomouse]

The L208 call is necessary because of the return statement at L209. The code was written that way when I added the calls to update_cookie(). Prior to my PR, I think the function worked as follows: if the headline is a todo item already, set the node text to keyword and then return; if not, insert the TODO keyword into the node. self:update_cookie() needs to be called after both changes, and there's probably a way to refactor the function to only call it once, but I didn't feel qualified to rewrite the original logic of the function in this PR.

As to the other question about self.refresh(): the short answer is "because it doesn't work without it." I don't 100% know why the call to self:refresh() is necessary at L215, but if it isn't there, the value of the headline is wrong when self:update_cookie() is called. I suspect it's because tree_utils.set_node_text() uses vim.api.nvim_buf_set_text() to update the node and something in self:refresh() causes treesitter to get back in sync so that the calls to the treesitter API in self:update_cookie() return the correctly updated value. It took my over an hour of debugging to settle on self:refresh() here and I tried several other solutions (including vim.schedule_wrap() and vim.defer_fn()), so I'm pretty confident it has to be called here (unless there's some other solution I didn't try). But I agree with the original question: that call to self:refresh() looks wrong.

[jgollenz]

because of the return statement at L209

🤦‍♂️ of course, my bad

because it doesn't work without it

Let's figure out what is going on and then document the code

[jgollenz]

Am I doing something wrong here?

[oncomouse]

Am I doing something wrong here?

No. That's weird. I thought checked that scenario before I submitted the PR, but I'm seeing it, too. I'll figure out why.

[oncomouse]

Okay, that problem should be fixed. The way target headlines was selected wasn't working. I'm going to need to write tests before I'm completely sure this is working.

[oncomouse]

So I have a philosophical question before I get further into this: how important is feature parity with emacs org-mode, even when emacs is doing something that looks like a bug?

I don't normally use emacs org-mode (I've been using this plugin and Neovim to learn org-mode), and I just checked how emacs handles mixed checkboxes and todo items and, basically, it doesn't. So, for instance:

* foo [/]
- [ ] bar
- [ ] baz
** TODO duh

If I trigger org-todo on ** TODO duh the cookie gets set to [1/1]. If I then trigger org-toggle-checkbox on one of the checkboxes, the cookie gets reset to [1/2]. If I trigger org-todo on ** DONE duh the cookie changes again to [0/0].

This strikes me as incorrect (or, at least, inconsistent), but if the goal of the project is to reproduce emacs org-mode, I guess that's how it should work.

Thoughts?

[jgollenz]

This strikes me as incorrect

I agree. What version of emacs org-mode are you using? I once encountered and reported a bug, only to find out that it had already been fixed in a later version.

If this behavior is the same in the latest version, we need to figure out what the expected behavior is. The orgmode reddit is open or we ask on their IRC.

[oncomouse]

This strikes me as incorrect

I agree. What version of emacs org-mode are you using? I once encountered and reported a bug, only to find out that it had already been fixed in a later version.

According to the org-mode manual, "If a heading has both checkboxes and TODO children below it, the meaning of the statistics cookie become ambiguous. Set the property COOKIE_DATA to either checkbox or todo to resolve this issue."

So, this PR isn't correct as written. There are some other things in the org spec that are going to present problems with the way I implemented this PR. I'm going to close this and go back to the drawing board on solving this issue.