Skip to content
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

feat(core): support org-enforce-todo-dependencies #741

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PannenetsF
Copy link
Contributor

Hi guys, I just impl the option for org-enforce-todo-dependencies, which will prevent parent TODOs from toggling when it has some unfinished children.

I am not sure if you are still interested in it, as the related issue #274 was updated 2 years ago.

Here comes the reference.
https://orgmode.org/manual/TODO-dependencies.html

:)

@seflue
Copy link
Contributor

seflue commented Jun 2, 2024

Hi guys, I just impl the option for org-enforce-todo-dependencies, which will prevent parent TODOs from toggling when it has some unfinished children.

I am not sure if you are still interested in it, as the related issue #274 was updated 2 years ago.

Many thanks for the contribution. 👍 Old issues don't mean, there is no interest in a working implementation. It just means, that we need more active contributors. 😄

From a very quick overlook your code looks good - I will try it out as soon as I can.

Here comes the reference. https://orgmode.org/manual/TODO-dependencies.html

:)

I wasn't even aware of this, but Orgmode is soo big ... you always learn something new ... 😅

@PannenetsF PannenetsF force-pushed the dev-org-enforce-todo-dependencies branch from 08c4490 to 7413717 Compare June 3, 2024 05:25
@PannenetsF
Copy link
Contributor Author

Haha, glad to hear that!

I am migrating my org from emacs to nvim, and will try to implement some daily functions into nvim-orgmode.

@Dardanos-Aeolus
Copy link

Hi there,

I really dont want to be pushy.
I would love this PR to be accepted because it would be a good Feature.

Thanks for your great work here.

@seflue
Copy link
Contributor

seflue commented Oct 19, 2024

@PannenetsF I finally found the time to play around with your contribution. First I was not sure if I misunderstood how the feature is supposed to work, but what was immediately noticeable is that changing the TODO state of a parent headline is completely blocked. And it is actually blocked on pressing "t", so if I want to set it to "Progress" and press "tp" I get a warning and it pastes my current register. On the other hand it does not change the state of the parent headline, when I set a child into "Progress", so I am not able to fix inconsistent states.

I then actually took the time to see, how the feature behaves in Emacs. There you can change the state of the parent headline as long as it is not a state considered as "closed" (a state defined on the right hand side of the "|" in the configuration list).

To get this merged, we should mimic the behavior of Emacs org mode.


local M = {}
-- @param headline OrgApiHeadline
-- local M.vis_head = function (headline, indent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove dead code.

function OrgMappings:_has_unfinished_children(headline)
for _, h in ipairs(headline:get_child_headlines()) do
local was_done = h:is_done()
if not was_done then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this local variable? It is not used later on, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not was_done then
if not h:is_done() then
return true
end

if not was_done then
return true
end
if OrgMappings:_has_unfinished_children(h) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stumbled over this recursion and after some moments I recognized, that not every headline is a todo. I think this is worth a comment.

return false
end

describe('Todo mappings unfer force dependency', function()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?

}, vim.api.nvim_buf_get_lines(0, 2, 4, false))
end)

it('should change todo state of a headline backward (org_todo_prev)', function()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate test name? Can you reflect the difference to the former test in the name?

@@ -0,0 +1,329 @@
local config = require('orgmode.config')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate, that you wrote tests. I saw that some of them are actually testing multiple things. I'm my experience this can make a regression harder to track, although it's much better than having no test. Would you mind to split the tests up a little bit?

@PannenetsF
Copy link
Contributor Author

@PannenetsF I finally found the time to play around with your contribution. First I was not sure if I misunderstood how the feature is supposed to work, but what was immediately noticeable is that changing the TODO state of a parent headline is completely blocked. And it is actually blocked on pressing "t", so if I want to set it to "Progress" and press "tp" I get a warning and it pastes my current register. On the other hand it does not change the state of the parent headline, when I set a child into "Progress", so I am not able to fix inconsistent states.

I then actually took the time to see, how the feature behaves in Emacs. There you can change the state of the parent headline as long as it is not a state considered as "closed" (a state defined on the right hand side of the "|" in the configuration list).

To get this merged, we should mimic the behavior of Emacs org mode.

Hi Seflue, thanks for your review. I would read the original Emacs orgmode for better mimic behavior. I am kind of busy these weeks. Will try to fetch time to do that
:-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants