Fix stale clocked header references when making edits to headers

[ReeceStevens]

Hi all! Thanks for the development of such a useful tool! I've been a happy user of nvim-orgmode for quite some time now.

I use the clocking functionality frequently in nvim-orgmode, as I've written a custom plugin for exporting the clock reports as timesheets for my work. I noticed that after a substantial refactor change for nvim-orgmode got merged into master, I would frequently experience file corruption when trying to clock in a new header. I figured out after some experimentation that this was happening whenever I was editing a file such that the currently clocked in header moved positions in the buffer. It looks like the currently clocked-in header is stored in memory, but for certain conditions the actual position of the header would get out of sync (e.g., if I moved another header above the currently clocked-in one).

This was a pretty significant issue for my workflow, so I went ahead and patched it locally and put together a unit test case which exercises the issue. My fix for this issue was to force a reload of the relevant files when performing clock-in or clock-out to ensure the right header position was used.

You can apply just the first commit of the test case and see the test fail against master, then apply the fix and see the test pass.

I'm happy to follow up with requests for changes or feedback if necessary. Thanks!

[kristijanhusak]

The solution looks ok, but I think there might be a simpler and less "heavy" solution. Would it work if there would be a BufWritePost *.org autocmd that would reload the clocked headline? Basically, after every write it would do the check. In your example, once you move the headline to another place and write the file, it would automatically update. There was also a plan to run a 1 minute timer to do this automatically, but I think the BufWritePost should cover most of the issues.

[ReeceStevens]

@kristijanhusak thanks for your response. Sorry it took me so long to get back to you, but my son was recently born so I've had my hands full :) I've rebased onto the current master branch. I'll need to do a bit of tweaking to my tests after the removal of the orgmode.parser.files api, so I'll push that up when I have some time.

Would it work if there would be a BufWritePost *.org autocmd that would reload the clocked headline?

The issue I see with this approach is that things would still get out of sync if the buffer wasn't yet written to disk. In particular, this happens when you use the default <leader>oxi keybinding on a header to clock something in-- it will update the logbook with the clock, but it won't write the buffer to save. Even beyond this scenario, I would imagine that generally we want actions on the buffer to be consistent with the current state of the buffer, whether it's been written to disk or not.

One solution would be to update all clock-modifying actions (or any property-editing actions for that matter) to write the buffer to file after the update. However, if we're taking that approach, it seems like this solution would be just as heavy (or even a bit heavier) than the currently proposed solution-- we'd still be updating the clocked header every time, and we'd also be writing to disk every time.

Based on this, I think the proposed approach would probably be the lightest approach that still guarantees the clock actions are consistent with the current state of the buffer. Do you agree with this assessment, or would you like to take a different approach?

[seflue]

@kristijanhusak Any news on this? I actually checked out the branch on my machine and played with it compared with it in comparison to master. It actually resolves a lot of problems with clocking I tracked on my own and makes the feature much more reliable. If you don't have very strong concerns against the current implementation, I would really appreciate to see this merged - improvements can be always made later on.

The tests fail happens in hyperlink tests, which don't seem to be related with the change, but I'm currently not deeply enough involved in the code to actually understand, what the issue is:

Scheduling: tests/plenary/org/hyperlinks/link_spec.lua
Error detected while processing command line:
E5108: Error executing lua ...gmode/tests/.deps/plugins/plenary/lua/plenary/busted.lua:268: ...r/work/orgmode/orgmode/tests/plenary/clock/init_spec.lua:3: module 'orgmode.parser.files' not found:
	no field package.preload['orgmode.parser.files']
	no file './orgmode/parser/files.lua'
	no file '/home/runner/work/neovim/neovim/.deps/usr/share/luajit-2.1/orgmode/parser/files.lua'
	no file '/usr/local/share/lua/5.1/orgmode/parser/files.lua'
	no file '/usr/local/share/lua/5.1/orgmode/parser/files/init.lua'
	no file '/home/runner/work/neovim/neovim/.deps/usr/share/lua/5.1/orgmode/parser/files.lua'
	no file '/home/runner/work/neovim/neovim/.deps/usr/share/lua/5.1/orgmode/parser/files/init.lua'
	no file './orgmode/parser/files.so'
	no file '/usr/local/lib/lua/5.1/orgmode/parser/files.so'
	no file '/home/runner/work/neovim/neovim/.deps/usr/lib/lua/5.1/orgmode/parser/files.so'
	no file '/usr/local/lib/lua/5.1/loadall.so'
	no file './orgmode.so'
	no file '/usr/local/lib/lua/5.1/orgmode.so'
	no file '/home/runner/work/neovim/neovim/.deps/usr/lib/lua/5.1/orgmode.so'
	no file '/usr/local/lib/lua/5.1/loadall.so'
stack traceback:
	[builtin#36]: at 0x559d5f5aa660
	...gmode/tests/.deps/plugins/plenary/lua/plenary/busted.lua:268: in function 'run'
	[string ":lua"]:1: in main chunkVim: Caught deadly signal 'SIGTERM'

Vim: Finished.

========================================	
Testing: 	/home/runner/work/orgmode/orgmode/tests/plenary/clock/init_spec.lua	
Scheduling: tests/plenary/org/hyperlinks/url_spec.lua

[ReeceStevens]

@kristijanhusak I've removed the refresh on the get_statusline function and updated the tests. Seems like the 0.10.2 test check is failing due to an updated clock timestamp (might have been a test execution on a minute boundary that caused that test to fail?), and a lot of stuff is failing on nightly that seems unrelated to these changes. I don't see an option to retry the 0.10.2 test but I suspect it will pass if re-run.

[ReeceStevens]

@kristijanhusak thanks for the feedback. I've moved the new test into the clock UI test module and removed the direct calling of OrgFiles:new. I believe this is ready for your review.

[seflue]

Thanks @ReeceStevens, this fixes a problem, which annoyed me for quite a long time! :+1: