Fix stale clocked header references when making edits to headers #759

ReeceStevens · 2024-06-16T18:28:26Z

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 · 2024-06-24T10:57:29Z

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.

Without this update, certain changes to the buffer would not be reflected in the `self.clocked_headline` attribute. This could cause changes such as the auto-clock-out of a previously clocked header to be inserted in the wrong location, corrupting the file.

ReeceStevens · 2024-10-06T19:05:09Z

@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 · 2024-10-19T12:38:06Z

@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

kristijanhusak

I'm ok with leaving this update on all methods except get_statusline. Statusline function is called a lot, and this update is a bit heavy, especially if user has a lot of files/headlines.

kristijanhusak · 2024-10-20T11:04:11Z

tests/plenary/clock/init_spec.lua

@@ -0,0 +1,55 @@
+local OrgFiles = require('orgmode.files')
+local OrgFile = require('orgmode.files.file')
+local Files = require('orgmode.parser.files')


This file no longer exists, it was refactored to a different structure. You can check other tests how to set up your tests.

…ock-fixes

ReeceStevens · 2024-10-26T18:32:07Z

@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.

kristijanhusak

Looks good, but I dont think we need that test.

kristijanhusak · 2024-10-28T18:53:06Z

tests/plenary/clock/init_spec.lua

+    return OrgFile.load(filename):wait()
+  end
+
+  it('should properly close out an existing clock when clocking in a new headline', function()


I think this test is not needed. We already have a similar one. If you want to test some scenario that is not tested you can add a test to that linked file.

@kristijanhusak could you please point me to the existing test that is similar? I am not familiar with all the test files so some guidance would be appreciated. This test fails without the patch proposed here, so it does seem to be testing something the other tests are not covering-- keeping the currently clocked header up-to-date as the buffer changes.

Ah ok, it's not the same. I didn't notice that you are editing file before clocking in/out.
Please move this test into https://github.com/nvim-orgmode/orgmode/blob/master/tests/plenary/ui/clock_spec.lua and adapt the setup around it. For example, you shouldn't do OrgFiles:new directly.

ReeceStevens · 2024-11-05T02:23:27Z

@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.

kristijanhusak

Looks good, thanks!

ReeceStevens added 2 commits October 6, 2024 13:41

Add test case for clock-out bug

9d8fcca

ReeceStevens force-pushed the clock-fixes branch from d2ba9c0 to 8600c50 Compare October 6, 2024 19:01

Merge branch 'master' into clock-fixes

4eca40f

kristijanhusak reviewed Oct 20, 2024

View reviewed changes

ReeceStevens added 4 commits October 26, 2024 13:25

Upgrade tests to use new file loading schema

a9e8084

Merge branch 'master' into clock-fixes

be8bf14

Remove clocking update when refreshing statusline

722b657

Merge branch 'clock-fixes' of github.com:ReeceStevens/orgmode into cl…

4f1f8e1

…ock-fixes

ReeceStevens requested a review from kristijanhusak October 26, 2024 18:29

kristijanhusak requested changes Oct 28, 2024

View reviewed changes

ReeceStevens added 2 commits November 4, 2024 20:21

Move test to clock UI test file

406ceb0

Merge branch 'master' into clock-fixes

1e2365d

ReeceStevens requested a review from kristijanhusak November 5, 2024 02:21

Fix lint error

bd648ee

kristijanhusak approved these changes Nov 5, 2024

View reviewed changes

kristijanhusak merged commit fafb8f1 into nvim-orgmode:master Nov 5, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix stale clocked header references when making edits to headers #759

Fix stale clocked header references when making edits to headers #759

ReeceStevens commented Jun 16, 2024 •

edited

Loading

kristijanhusak commented Jun 24, 2024

ReeceStevens commented Oct 6, 2024

seflue commented Oct 19, 2024

kristijanhusak left a comment

kristijanhusak Oct 20, 2024

ReeceStevens commented Oct 26, 2024 •

edited

Loading

kristijanhusak left a comment

kristijanhusak Oct 28, 2024

ReeceStevens Oct 28, 2024

kristijanhusak Oct 28, 2024

ReeceStevens commented Nov 5, 2024

kristijanhusak left a comment

Fix stale clocked header references when making edits to headers #759

Fix stale clocked header references when making edits to headers #759

Conversation

ReeceStevens commented Jun 16, 2024 • edited Loading

kristijanhusak commented Jun 24, 2024

ReeceStevens commented Oct 6, 2024

seflue commented Oct 19, 2024

kristijanhusak left a comment

Choose a reason for hiding this comment

kristijanhusak Oct 20, 2024

Choose a reason for hiding this comment

ReeceStevens commented Oct 26, 2024 • edited Loading

kristijanhusak left a comment

Choose a reason for hiding this comment

kristijanhusak Oct 28, 2024

Choose a reason for hiding this comment

ReeceStevens Oct 28, 2024

Choose a reason for hiding this comment

kristijanhusak Oct 28, 2024

Choose a reason for hiding this comment

ReeceStevens commented Nov 5, 2024

kristijanhusak left a comment

Choose a reason for hiding this comment

ReeceStevens commented Jun 16, 2024 •

edited

Loading

ReeceStevens commented Oct 26, 2024 •

edited

Loading