-
-
Notifications
You must be signed in to change notification settings - Fork 167
feat(cookie): implement virtual cookies #992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9f8b4d8
to
2de91e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
It works, but I expected it to work differently. I assumed it will always show a virtual text, not only if you add [/]
or [%]
. And also expected to use a default "eol" virtual text that will show on the right of text, like diagnostics do.
I understand it would look strange if we would have both "regular text cookie" and "virtual text cookie" one beside another, but concealing the current cookie looks a bit odd. A slight improvement would be to not conceal the text and use overlay
virtual text.
I'm not really a user of the cookies, so I'd like to understand your usage of it. Do you need to have the actual cookie text for later use (exporting or something else), or it's only visual? Are there situations where you want cookies somewhere but not on other places?
Underlying "text cookie" and "virtual text cookie" are not in sync, as we discussed on the issue. Underlying cookie updates when you toggle the todo state, but not when you manually edit the TODO
to DONE
. This might cause confusion.
To summarize: I'd prefer if we would have an always visible virtual text on the headlines/checkboxes where applicable, when this setting is turned on, or when enabled through global variable (nice addition btw), and if headline/checkbox have both, just show them both.
Let me know what you think.
2de91e0
to
58a501e
Compare
And also expected to use a default "eol" virtual text that will show on the right of text, like diagnostics do.
I avoided that purely because it can be difficult to see a cookie when we have a bunch of tags on a given headline.
I understand it would look strange if we would have both "regular text cookie" and "virtual text cookie" one beside another, but concealing the current cookie looks a bit odd. A slight improvement would be to not conceal the text and use overlay virtual text.
Without the conceal it can be difficult to actually delete the underlying cookie as it's unclear what is actually being selected/modified if the cookie is overlayed and a user wants to edit the underlying cookie text. In my mind we only want to show the virtual cookie and suppress the "real" cookie when possible as the virtual cookie gets live updates and will be in sync with the actual state of the headlines.
I'm not really a user of the cookies, so I'd like to understand your usage of it. Do you need to have the actual cookie text for later use (exporting or something else), or it's only visual? Are there situations where you want cookies somewhere but not on other places?
It's purely visual for me for quickly tracking progress on a larger TODO
item. I figured that if a user wants to see progress then they'd add a cookie to track progress. I'm totally open to putting virtual cookies on all headlines regardless of if an underlying cookie has been created.
Again, an issue that can crop up is if we overlay the cookie with a virtual cookie, it can be difficult to modify the underlying cookie text. That was why I have this setup as it currently is.
Here's what I'll do:
- Make virtual cookies appear on all headlines when applicable (when there's
TODO
or checkbox subitems basically) - I'll combine the progress on both the
TODO
items and the checkboxes beneath the current headline into the cookie.
Here's what I need answered before I can continue on some of this:
- Let me know if you still desire an
EOL
orInline
cookie (considering that tags can cause a cookie to go off screen) - How do you want me to handle the scenario when we have a "physical/real" cookie and a virtual cookie on the same headline?
58a501e
to
d431fb3
Compare
Latest push covers
- Make virtual cookies appear on all headlines when applicable (when there's TODO or checkbox subitems basically)
- I'll combine the progress on both the TODO items and the checkboxes beneath the current headline into the cookie.
As well as
- Allowing users to specify the default "cookie type", whether or not to show a
/
or a%
for the sign in the cookie if there isn't a "real" cookie specified for the given headline. - Support for toggling the type of default virtual cookie for headlines that do not specify the "real" cookie (via
Org cookie_type
) - Removed the
[???]
"unknown" cookie type, no longer applicable seeing as we're now drawing cookies for all headlines that have subitems - Small improvement to deleting extmarks
- Support placing the virtual cookie at the end of a headline title (before the tags if they exist) when there's no "real" cookie in the headline
d431fb3
to
4579116
Compare
Updated documentation to reflect the new changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
I tested this locally and found a single bug. The behavior of the "real" cookie being hidden in favor of the virtual one looks reasonable to me.
Thank you for the effort on this. I appreciate the feature.
Virtual text remains rendered when parent heading is deleted
Kazam_screencast_00021.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of things came to mind on this docs entry.
- We could use a
#+begin_src lua
code block to show an example for this config, same as done with the other config options. Something like this:
diff --git a/docs/configuration.org b/docs/configuration.org index 899c3df..91f01f7 100644 --- a/docs/configuration.org +++ b/docs/configuration.org @@ -3018,9 +3018,22 @@ require('orgmode').setup({ :CUSTOM_ID: virt_cookies :END: +Virtual cookies display the progress of TODO headings with children by indicating the proportion of DONE children. + You can toggle Virtual cookies on the fly by executing command =:Org cookie_mode= when in a org buffer. This additionally sets the buffer variable =vim.b.org_cookie_mode= to =true= or =false=, depending on the current state. +#+begin_src lua +require('orgmode').setup({ + ui = { + virt_cookies = { + enabled = true, + type = '/', + }, + } +}) +#+end_src + Currently this only applies Virtual cookies to headlines. Uses the following highlights:
- We are missing an entry in
doc/orgmode.txt
for this config option - The individual config properties (
type
andenabled
) should probably not be described in individual child headings (****
), but instead in list items, similar to what is done here for theorg_capture_templates
config options:
orgmode/docs/configuration.org
Lines 832 to 842 in 32ef9e9
- =description= (=string=) --- description of the template that isdisplayed in the template selection menu- =template= (=string|string[]=) --- body of the template that will beused when creating capture- =target= (=string?=) --- name of the file to which the capture contentwill be added. If the target is not specified, the content will beadded to the [[#org_default_notes_file][org_default_notes_file]] file- =headline= (=string?=) --- title of the headline after which thecapture content will be added. If no headline is specified, thecontent will be appended to the end of the file- =datetree (boolean | { time_prompt?: boolean, reversed?: boolean, tree_type: 'day' | 'month' | 'week' | 'custom' })=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question
Is there a particular reason for the watchers not to hang from the OrgVirtCookie
class itself?
I've not done much with lua, so not sure if there is much of a difference in the approaches.
I have in mind something like this:
diff --git a/ftplugin/org.lua b/ftplugin/org.lua index 6bc4213..b5c82b5 100644 --- a/ftplugin/org.lua +++ b/ftplugin/org.lua @@ -19,7 +19,7 @@ if config.org_startup_indented then end if config.ui.virt_cookies.enabled then - require('orgmode.ui.virtcookie').new(bufnr, config.ui.virt_cookies.type):attach() + require('orgmode.ui.virtcookie'):new(bufnr, config.ui.virt_cookies.type):attach() end vim.bo.modeline = false diff --git a/lua/orgmode/org/global.lua b/lua/orgmode/org/global.lua index f2879f0..5f0c260 100644 --- a/lua/orgmode/org/global.lua +++ b/lua/orgmode/org/global.lua @@ -99,13 +99,13 @@ local build = function(orgmode) require('orgmode.ui.virtual_indent').toggle_buffer_indent_mode() end, cookie_mode = function() - local virtcookie = require('orgmode.ui.virtcookie').get() + local virtcookie = require('orgmode.ui.virtcookie'):get() if virtcookie then virtcookie:toggle() end end, cookie_type = function() - local virtcookie = require('orgmode.ui.virtcookie').get() + local virtcookie = require('orgmode.ui.virtcookie'):get() if virtcookie then virtcookie:toggle_cookie_type() end diff --git a/lua/orgmode/ui/virtcookie.lua b/lua/orgmode/ui/virtcookie.lua index c9928fd..ab08531 100644 --- a/lua/orgmode/ui/virtcookie.lua +++ b/lua/orgmode/ui/virtcookie.lua @@ -1,6 +1,7 @@ local org = require('orgmode') ---@alias OrgVirtCookieType '/' | '%' The type of cookie to default to when no "real" cookie exists +---@alias OrgCookieWatchers table<integer, OrgVirtCookie> A mapping of buffer ids to watchers ---@class OrgVirtCookie Updates Headline Cookies for Progress using Virtual Text ---@field private bufnr integer Buffer Watcher is attached to @@ -9,34 +10,31 @@ local org = require('orgmode') ---@field private ns_id integer local OrgVirtCookie = { ns_id = vim.api.nvim_create_namespace('orgmode.ui.cookie'), + ---@type OrgCookieWatchers + watchers = {}, } ----@alias OrgCookieWatchers table<integer, OrgVirtCookie> A mapping of buffer ids to watchers - ----@type OrgCookieWatchers -local watchers = {} - ---Get all currently registered cookie watchers ---@return OrgCookieWatchers -function OrgVirtCookie.watchers() - return watchers +function OrgVirtCookie:getWatchers() + return self.watchers end ---Gets an existing OrgCookieWatcher for the given buffer if it exists ---@param bufnr? integer Buffer to get the watcher for ---@return OrgVirtCookie? -function OrgVirtCookie.get(bufnr) +function OrgVirtCookie:get(bufnr) bufnr = bufnr or vim.api.nvim_get_current_buf() - local watcher = OrgVirtCookie.watchers()[bufnr] + local watcher = OrgVirtCookie:getWatchers()[bufnr] return watcher end ---Creates a new headline watcher ---@param bufnr? integer Buffer to watch, if unspecified then uses the current buffer ---@param cookie_type? OrgVirtCookieType -function OrgVirtCookie.new(bufnr, cookie_type) +function OrgVirtCookie:new(bufnr, cookie_type) bufnr = bufnr or vim.api.nvim_get_current_buf() - local watcher = OrgVirtCookie.get(bufnr) + local watcher = OrgVirtCookie:get(bufnr) if watcher then return watcher end @@ -45,7 +43,7 @@ function OrgVirtCookie.new(bufnr, cookie_type) attached = false, cookie_type = cookie_type or '/', }, { __index = OrgVirtCookie }) - watchers[this.bufnr] = this + self.watchers[this.bufnr] = this vim.api.nvim_create_autocmd('BufDelete', { buffer = this.bufnr, @@ -55,7 +53,7 @@ function OrgVirtCookie.new(bufnr, cookie_type) end, }) - return watchers[this.bufnr] + return self.watchers[this.bufnr] end ---@param new_cookie_type OrgVirtCookieType @@ -251,7 +249,7 @@ function OrgVirtCookie:attach() vim.b[self.bufnr].org_cookie_mode = true vim.b[self.bufnr].org_cookie_type = self.cookie_type - watchers[self.bufnr] = self + self.watchers[self.bufnr] = self self:_update_cookies_in_range(0, vim.api.nvim_buf_line_count(self.bufnr) - 1) -- We cant use `nvim_set_decoration_provider()` here because of a @@ -314,7 +312,7 @@ end ---Completely removes the watcher, including from tracked watchers function OrgVirtCookie:delete() self:detach() - watchers[self.bufnr] = nil + self.watchers[self.bufnr] = nil end return OrgVirtCookie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a scenario where this would go infinite, but I believe you can avoid while true
with a do while
(repeat until
in lua)
diff --git a/lua/orgmode/ui/virtcookie.lua b/lua/orgmode/ui/virtcookie.lua index c9928fd..c2b239b 100644 --- a/lua/orgmode/ui/virtcookie.lua +++ b/lua/orgmode/ui/virtcookie.lua @@ -82,7 +82,8 @@ end function OrgVirtCookie._parent_headlines(headline) local located_headlines = {} local count = 0 - while true do + + repeat count = count + 1 local parent = headline:get_parent_headline() @@ -92,7 +93,8 @@ function OrgVirtCookie._parent_headlines(headline) table.insert(located_headlines, parent) headline = parent - end + until not parent or not parent.headline + return located_headlines end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These do look good, but the highlights make it seem like it is real text rendered in the buffer.
The UX may be more consistent if regular virtual text highlights are used instead.
diff --git a/lua/orgmode/colors/highlights.lua b/lua/orgmode/colors/highlights.lua index 3601119..2885cdb 100644 --- a/lua/orgmode/colors/highlights.lua +++ b/lua/orgmode/colors/highlights.lua @@ -73,9 +73,9 @@ function M.link_highlights() ['@org.edit_src'] = 'Visual', -- For cookie extmarks (applicable if enabled) - ['@org.cookie.delimiter'] = 'Delimiter', - ['@org.cookie.sign'] = 'Special', - ['@org.cookie.number'] = 'Number', + ['@org.cookie.delimiter'] = 'VirtualTextInfo', + ['@org.cookie.sign'] = 'VirtualTextInfo', + ['@org.cookie.number'] = 'VirtualTextInfo', } for src, def in pairs(links) do
before:
Screenshot from 2025年06月04日 00-34-15
after:
Screenshot from 2025年06月04日 00-34-03
ae38aa8
to
de701e8
Compare
Uh oh!
There was an error while loading. Please reload this page.
Summary
This PR adds virtual cookies, a way of using extmarks to get live updating cookies for progress.
Currently it is only implemented for headlines, does not support list items.
This slightly modifies the headlines class to expose progress amounts from the headlines.
The virtual cookies prioritize list checkboxes over todo checkboxes for determining progress. Would it be preferred to flip that OR combine the totals from both as our progress?
Virtual cookies are gated behind
ui.cookies_use_extmarks
and are disabled by default.I have not added tests for them, I consider them largely experimental (though pretty robust from my experience) and didn't allocate the time to write tests for them.
See more details from my earlier comment (#745 (comment)) for additional information.
Related Issues
Closes #745
Changes
Headline
classconfiguration.org
Org cookie_mode
command for toggling cookies on & off in the current bufferChecklist
I confirm that I have:
Conventional Commits
specification (e.g.,
feat: add new feature
,fix: correct bug
,docs: update documentation
).make test
. (Ensured existing tests still pass)