Orgmode + Treesitter error on ORG_TOGGLE_CHECKBOX

[kevinroleke]

Describe the bug

I have an item like

* TODO [ ] Do things
  SCHEDULED: <2023-09-08 Fri>

If I want to mark the checkbox, I would normally do <Leader>Space. Now that generates this error

[orgmode] ...ck/packer/start/orgmode/lua/orgmode/utils/treesitter.lua:99: attempt to index local 'node' (a nil value)

Occurs regardless if org is configured for treesitter.

Steps to reproduce

orgmode config

require('orgmode').setup_ts_grammar()
require('orgmode').setup({
  org_agenda_files = {'~/org/*'},
  org_default_notes_file = '~/org/refile.org',
  mappings = {
    note = {
        org_note_finalize = '<Leader>w',
    }
  },
})

treesitter config

require'nvim-treesitter.configs'.setup {
  ensure_installed = { "org", "vimdoc", "javascript", "typescript", "c", "lua", "rust" },
  sync_install = false,
  auto_install = true,
  highlight = {
    enable = true,
    additional_vim_regex_highlighting = {'org'},
  },
}

Expected behavior

Box gets checked

Emacs functionality

No response

Minimal init.lua

-- Enter your minimal_init.lua here

Screenshots and recordings

No response

OS / Distro

Debian 11

Neovim version/commit

0.9.1

Additional context

No response

[jgollenz]

That's not a checkbox, that's an empty priority. Only plain list items can have a checkbox.

[seflue]

@jgollenz Nevertheless the error message should not occur in this form, that's a bug. Either we throw an error "checkboxes in titles are not allowed" or we just swallow the attempt to toggle the checkbox silently. What do you think? Actually I would prefer the silent solution and as an enhance version some context specific mapping of org-toggle-checkbox to something, which makes sense, e.g. refreshing a [/] box (which currently is not supported in nvim-orgmode, but I think this is the behavior in Emacs). Actually I would like to implement this (because I also miss it in my workflow), but currently I'm still occupied with the hyperlinks.

[jgollenz]

Yes, I agree that the error message is ugly and not helpful to users :+1: I'm wondering what the use case for manually refreshing the [/] is, because this should™ happen automagically whenever changes that justify a cookie-update occur. The only thing I can come up with is "something about my progress-cookies is broken, so let's refresh them manually". That certainly may be the case sometimes, but I would rather people report that the progress-cookies are broken (so we can go fix them) than just update them manually out of convenience (which means we might not learn about the bug).

edit: but of course, if emacs has this behavior, we should try to replicate it

[seflue]

It happens (in case of headlines) if you add the cookie afterwards. It doesn't appear automatically in Emacs if I recall correctly.

Am 11. September 2023 17:27:29 schrieb Jonas Gollenz @.***>:

Yes, I agree that the error message is ugly and not helpful to users 👍 I'm wondering what the use case for manually refreshing the [/] is, because this should™ happen automagically whenever changes that justify a cookie-update occur. The only thing I can come up with is "something about my progress-cookies is broken, so let's refresh them manually". That certainly may be the case sometimes, but I would rather people report that the progress-cookies are broken (so we can go fix them) than just update them manually out of convenience (which means we might not learn about the bug). — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

[jgollenz]

Indeed, that's a valid use-case. According to this, toggling a checkbox and updating a statistics cookie is both done with <C-c> <C-c>. So we actually should just map it to <C-Space>.

I would like to implement this

Are you talking about statistics cookies for headlines? If yes, see #572

[seflue]

Are you talking about statistics cookies for headlines? If yes, see https://github.com/nvim-orgmode/orgmode/pull/572

Yes I do and I actually recall this abandoned PR.

So we actually should just map it to <C-Space>.

You mean updating statistic cookies during org-toggle-checkbox. I don't like hardcoded mappings (the user must be able to adjust them), but I'm confident, that you just used <C-Space> as a synonym for the underlying function, didn't you?

[jgollenz]

You mean updating statistic cookies during org-toggle-checkbox

No. I meant doing it like we do for org-meta-return. Let's call it org-meta-space for now. It would do both org-toggle-checkbox and the update of the cookie, depending on the context where the keymap is executed.

I don't like hardcoded mappings

Understandable. Having both a mapping for org-meta-space, which does multiple things, and allowing to override one of those multiple things sounds a bit complicated (although maybe doable?). I see these options:

1. have a dedicated mapping for updating the statistics cookie (which also means that it is adjustable by default)
2. introduce some sort of org-meta-space (since we already use <C-Space> for checkbox toggling) similar to org-meta-return that handles the many things that are handled by <C-c> <C-c> in emacs orgmode. Of course, this would not enable users to override a specific functionality, only the mapping of org-meta-space.
3. we add both a dedicated mapping and org-meta-space. We do what we do in 2., but before actually dispatching to (in this case) the logic that updates the cookie, we check if users have overridden the mapping to something else than the default of <C-Space>. But from the top of my head I'm quite unsure if this is even possible. If we have a mapping for org-meta-space and org-toggle-checkbox that both map to <C-Space>, I would expect things to fall apart.

From the docs it seems that emacs orgmode offers both the <C-c> <C-c> that, based on the context, does a cookie update and a dedicated mapping

edit: 4. we could do 3., but don't have them be the same. A dedicated mapping that is adjustable by the user AND a org-meta-space that does cookie updates based on context

edit 2: I'm not sure if org-update-statistics-cookies only updates the one you are currently on

edit 3: yeah, it updates all statistics cookies of the current heading

[jgollenz]

Yes I do and I actually recall this abandoned PR.

Before you start, please drop me a message. I started working on this myself a while ago and I'd like to discuss some things first :+1: thanks for you contributions :slightly_smiling_face:

[seflue]

@jgollenz I think option 2 seems to be viable while not being too complicated. I'm referring to org-toggle-checkbox because this is the action I would use for updating the statistic cookie manually and which would make the most sense. What currently also isn't implemented is the statistic cookie for subsection todos in general. For check-boxes (and headlines with checkboxes) it seems to be working quite fine - although the manual update option would be convenient if you add it as an afterthought, so you don't have to artificial toggle a checkbox in the sub-list twice to update it.

Do you have an entry-point to your implementation attempt - some branch on your fork or something?

[jgollenz]

option 2 seems to be viable

I started implementing it :+1: we can add a dedicated mapping for org-update-statistics-cookies later.

entry-point to your implementation attempt

Yes, somewhere :grin: Are you in the nvim-orgmode matrix channel by any chance?

[seflue]

Yes, somewhere 😁 Are you in the nvim-orgmode matrix channel by any chance?

No, I wasn't aware of that. Actually I have to admit, that I haven't used Matrix in the past, but if you can help me joining the channel I would be happy to do so.

[jgollenz]

can help me joining the channel

Oh, apparently the badge is broken atm:

Will fix that asap. Anyway, clicking on Chat on the main page (or follow the link in the README.md) should take you there. You will need a matrix account. Let me know if it's not working for you :slightly_smiling_face: