feat: add a way to cache state between restarts of Neovim #624

PriceHiller · 2023-10-31T20:34:40Z

This is useful for features such as org-clock-in-last that has the ability to persist its "knowledge" of the last clocked in task. Org mode does this by saving to a cache file on system. This commit gives nvim-orgmode the same capability.

I'm creating this PR in what I consider a partially finished state. I'm primarily looking for feedback as to the design of this module. In the scenario we're all happy with the design then I can get some simple tests written and then promote this to a non draft PR.

Some quick notes:

This state management system uses the in-built promises library that nvim-orgmode bundles. This means there's some additional validation and steps taken than what might exist in fully synchronous code.
I use a metatable to ensure any accesses against the State table in which we assign new values are saved into the subtable data. This is so later on it is easy to determine what to save into the cache. Without it, I'd have to write additional logic explicitly excluding specific keys from the State table.
Seeing as the State object is a singleton is may seem weird to bother with a State:new() function. This actually is quite important, this sets up the metatable after all other functions are read in the file, ensuring they are not bound to the data subtable within the State object.
Reading the cache should work on all platforms as I'm "normalizing" the paths with vim.fs.normalize.

Any feedback (especially critical feedback) is desired on this. I'm looking to iron out kinks and find out if this is even desired 😃.

This is useful for features such as `org-clock-in-last` that has the ability to persist its "knowledge" of the last clocked in task. Org mode does this by saving to a cache file on system. This commit gives nvim-orgmode the same capability.

PriceHiller · 2023-10-31T20:37:42Z

@kristijanhusak I'm primarily interested in what you think. If the above all sounds good and the code looks solid, I intend to get this merged and then turn around and implement at least org-clock-in-last with persistent state between Neovim restarts.

kristijanhusak · 2023-11-02T14:01:55Z

Thanks for the PR. I didn't give it a deeper look yet. I have one question regarding this:

Org mode does this by saving to a cache file on system

Can you point me where I can read more about this? I wasn't aware it is done in Emacs, at least not in this way. I assumed Emacs has a more advanced persistence.

PriceHiller · 2023-11-02T14:10:27Z

So emacs is a little bit fancier than the method done above to persist state. That index file contains:

((:container
  ((elisp org-element--headline-cache)
   (elisp org-element--cache))
  :persist-file "26/0e7168-1132-40fb-9c4a-f9527c58b0eb" :associated
  (:hash "db811d99c98367fd949e030078d6c3dc" :file "/home/sam/org/sprints.org" :inode 1798784)
  :expiry 30 :last-access 1698760936.4788425 :last-access-hr "2023-10-31T09:02:16-0500")
 (:container
  ((elisp org-element--headline-cache)
   (elisp org-element--cache))
  :persist-file "16/77b350-d23c-435f-a1af-79fab01dc901" :associated
  (:hash "9b28fb51f68a22e72bfe80dd6eea5713" :file "/home/sam/.notes" :inode 1798482)
  :expiry 30 :last-access 1698754134.958719 :last-access-hr "2023-10-31T07:08:54-0500")
 (:container
  ((elisp org-element--headline-cache)
   (elisp org-element--cache))
  :persist-file "32/2a423d-7966-474a-b285-bec583e15678" :associated
  (:hash "f821ab3b4dde77f42b322d56138e8b6f" :file "/home/sam/Notes/Whitelist-API.org" :inode 1853863)
  :expiry 30 :last-access 1698847955.2776916 :last-access-hr "2023-11-01T09:12:35-0500")
 (:container
  ((elisp org-element--headline-cache)
   (elisp org-element--cache))
  :persist-file "7a/71cd55-9320-4ced-8290-64f32f790869" :associated
  (:hash "0c752e61eadaa71b7e3408264c6a2349" :file "/home/sam/Notes/sprints.org" :inode 1709198)
  :expiry 30 :last-access 1698649783.8752766 :last-access-hr "2023-10-30T02:09:43-0500")
 (:container
  ((index "3.1"))
  :persist-file "c0/b60f88-9bc6-45e0-a7f9-711167b5b71d" :associated nil :expiry never :last-access 1698847955.2797225 :last-access-hr "2023-11-01T09:12:35-0500"))

Which definitely is more complex than what I'm implementing here.

See here for some details in the manual as to clocking in with persistence. Trying to find a more specific manual currently.

PriceHiller · 2023-11-02T14:21:59Z

@kristijanhusak

Found it, source code for org-persist can be found in a mirrored Github repository here.

Or you can directly get it by cloning from Savannah, git clone git://git.sv.gnu.org/emacs/org-mode.git && cd org-mode && $EDITOR lisp/org-persist.el . It has a fair bit of its own documentation in the top of the file. I was unable to access said docs in Emacs 🤷.

kristijanhusak

I didn't give it a test, but I did review the code.
Generally, it looks ok but I need to investigate a bit more to see what and how we should persist data.

Also, it would be great to write some tests around this to ensure it works as expected.

lua/orgmode/state/state.lua

lua/orgmode/utils/init.lua

PriceHiller · 2023-11-02T15:19:45Z

Also, it would be great to write some tests around this to ensure it works as expected.

Absolutely. I just wanted to check that this looked good in the first place before writing up tests etc. only to be told that it's a no go.

I'll get those tests written after resolving your comments in a sec here.

No need to do `vim.tbl_deep_extend`. kristijanhusak Nov 2, 2023: > Doing this should be enough, since nil is considered a falsy value. Co-authored-by: Kristijan Husak <[email protected]>

Since we instantly attempt to repair the State file by saving it, the `decoded` variable can be set again to the correct value. As such, we need to take a copy of the value to ensure it doesn't get overriden by the next load operation.

Instead of using `Promise.next`, it's preferred to use the `catch` and `finally` functions provided by Orgmode's Promise library. See nvim-orgmode#624 (comment) > Handle the errors through the `:catch` function instead of 2nd param to `:next`. > > ```lua > utils.writefile(cache_path, vim.json.encode(State.data)):catch(function(err_msg) > vim.schedule_wrap(function() > vim.notify('Failed to save current state! Error: ' .. err_msg, vim.log.levels.WARN, { title = 'Orgmode' }) > end) > end) > ```

See nvim-orgmode#624 (comment) > You can just return `Promise.resolve()` here.

This change makes it easier to test. To use the state module a downstream caller must now do "require('orgmode.state.state')()".

PriceHiller · 2023-11-02T17:17:11Z

Still need to write a test for the self-healing.

PriceHiller · 2023-11-06T19:53:14Z

As a heads up @kristijanhusak this should be good to go. If you're happy with this, I'm going to begin implementing some missing orgmode features that can use persistence.

kristijanhusak

We're almost there, just few minor changes and it's good to go.

lua/orgmode/state/state.lua

See nvim-orgmode#624 (comment) > Since this is a singleton we should return the instance of it here `return State.new()`

See nvim-orgmode#624 (comment) > Move this also to a :catch so it is consistent with other error handlers in the file.

See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

PriceHiller · 2023-11-06T22:06:31Z

@kristijanhusak Let me know if you see anything else.

kristijanhusak · 2023-11-07T09:58:50Z

@treatybreaker how did you test this manually?
I tried setting up a keymap like this to ensure it's working but it doesn't seem to do anything:

vim.keymap.set('n', '<leader>u', function()
  local state = require('orgmode.state.state')
  return state:load():finally(function()
    print('LOADED', state.data)
    state.field = 'value'
    state:save()
  end):next(function()
    print('SAVED')
    vim.print('UPDATED STATE', state.data)
  end)
end)

Prints are never shown, and org-cache.json file is never created.
Also, tests are not actually checking anything. For example, if you change this line https://github.com/nvim-orgmode/orgmode/pull/624/files#diff-23e5eb566ad6dac12d0c71f0d549d5e273bd9ed2c79b837688a0ad21e0946dd7R52 to expect something that it should not get, tests still pass:

-- This passes, even though it shouldn't
assert.are.equal(state.my_var, 'hello world foo')

Something seems to be off. Can you check? Thanks.

All tests should work as expected now.

PriceHiller · 2023-11-08T01:25:44Z

@kristijanhusak

This was uhhh, painful. Async code in lua is the very definition of not fun.

I think this should be good now, I have the tests responding correctly now by abusing vim.wait. And I did some minor fixes to the State module. One of those is tracking the State:saved context which particularly useful for tests.

I am really hoping this is good at this point. If it's not then I'm going to keep on grinding until it's in a good state.

Take a look and let me know your findings.

PriceHiller · 2023-11-08T01:26:40Z

As a note, Neovim didn't seemingly create those stdpaths at startup with the minimal init... I had to create them during the minimal init setup.

kristijanhusak · 2023-11-08T10:37:03Z

Awesome, now it works :)

vim.wait is a way to go. I would even suggest introducing load_sync() and save_sync() as part of the State class, where it would do the same thing you do in tests. Then you can leverage that in tests and we also have it as an option if needed.
Feel free to increase timeout to 1000 and use an interval of 5. I did similar thing to ensure that org files are loaded.

PriceHiller · 2023-11-08T22:41:01Z

@kristijanhusak

Added those functions as well as a State:wipe function. In the State module I had to do some fancy stuff with State:save as I realized there can be multiple saves running as once. Tracking this in the context now. For the sync functions I went with an interval of 20 with a default timeout of 500 ms. If that's too low, let me know and I can do a quick commit to change that.

Man... async code and multiple execution is hell on earth. Very useful, but my god.

Looking forward to more hellish findings 😉.

It's possible to have multiple save operations happening at once. We only want to declare the context as "saved" when all savers are finished.

PriceHiller · 2023-11-08T22:56:36Z

Working on replicating the failure in 0.9.1.

PriceHiller · 2023-11-08T23:01:43Z

Fixed.

kristijanhusak

Looks good! Just some minor tweaks to types and tests and it's good to go.

lua/orgmode/state/state.lua

tests/plenary/state/state_spec.lua

See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

See nvim-orgmode#624 (comment) See nvim-orgmode#624 (comment) See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

PriceHiller · 2023-11-09T09:28:56Z

Let me know if you see anything else 🙂

kristijanhusak

Ok, everything looks great now, thanks!

PriceHiller · 2023-11-09T09:35:29Z

I'll look into implementing some features that take advantage of this come this weekend or next week as a heads up.

…de#624) Co-authored-by: Kristijan Husak <[email protected]>

PriceHiller force-pushed the feat/state-management branch from 9b2b087 to 7106e2e Compare October 31, 2023 20:35

kristijanhusak reviewed Nov 2, 2023

View reviewed changes

lua/orgmode/state/state.lua Outdated Show resolved Hide resolved

lua/orgmode/state/state.lua Outdated Show resolved Hide resolved

lua/orgmode/utils/init.lua Outdated Show resolved Hide resolved

PriceHiller and others added 6 commits November 2, 2023 10:26

refactor: use or pattern to set default

afdd6a7

No need to do `vim.tbl_deep_extend`. kristijanhusak Nov 2, 2023: > Doing this should be enough, since nil is considered a falsy value. Co-authored-by: Kristijan Husak <[email protected]>

fix: get deep copy of State:load error

3e0c1bf

Since we instantly attempt to repair the State file by saving it, the `decoded` variable can be set again to the correct value. As such, we need to take a copy of the value to ensure it doesn't get overriden by the next load operation.

refactor: use more idiomatic empty Promise creation in State:load

6161297

See nvim-orgmode#624 (comment) > You can just return `Promise.resolve()` here.

refactor: make the State module more ergonomic for initialization

281925c

This change makes it easier to test. To use the state module a downstream caller must now do "require('orgmode.state.state')()".

test: add initial tests for the State module

4ebb84b

PriceHiller force-pushed the feat/state-management branch from 6ab1217 to 4ebb84b Compare November 2, 2023 17:09

test: validate self-healing capability of State module

c594f2b

PriceHiller marked this pull request as ready for review November 2, 2023 17:37

PriceHiller requested a review from kristijanhusak November 2, 2023 17:42

kristijanhusak requested changes Nov 6, 2023

View reviewed changes

lua/orgmode/state/state.lua Outdated Show resolved Hide resolved

lua/orgmode/state/state.lua Show resolved Hide resolved

lua/orgmode/state/state.lua Outdated Show resolved Hide resolved

kristijanhusak force-pushed the master branch 2 times, most recently from 9094379 to abff0dc Compare November 6, 2023 20:52

PriceHiller and others added 4 commits November 6, 2023 15:59

refactor: return new instance of state from state module

0320f93

See nvim-orgmode#624 (comment) > Since this is a singleton we should return the instance of it here `return State.new()`

refactor: use catch for more consistency with other error handlers

c299d57

See nvim-orgmode#624 (comment) > Move this also to a :catch so it is consistent with other error handlers in the file.

refactor: early return to better raise file error in state load

312d519

See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

style: format state module

2e01ff1

PriceHiller requested a review from kristijanhusak November 6, 2023 22:06

PriceHiller added 5 commits November 7, 2023 16:45

feat: track State:save context in State._ctx

37b5274

test: ensure needed vim stdpaths are created on startup

4130207

test: correctly handle async testing of the State module

76beae3

All tests should work as expected now.

refactor: use echo_warning for notifications in the State module

d5c7f8f

fix: make chaining off State:save correctly wait until file save

056191f

PriceHiller force-pushed the feat/state-management branch from 0f8b807 to 056191f Compare November 8, 2023 01:21

PriceHiller added 2 commits November 8, 2023 15:58

feat: add synchronous wrappers for State:load & State:sync

de1ad28

docs: update annotation for State.new

d533595

PriceHiller force-pushed the feat/state-management branch from 8a601b6 to 414e8d0 Compare November 8, 2023 22:37

PriceHiller added 3 commits November 8, 2023 16:52

fix: ensure save state is correctly tracked

389ba27

It's possible to have multiple save operations happening at once. We only want to declare the context as "saved" when all savers are finished.

feat: add State:wipe function to State module

751b5dd

test: update state_spec to use new State functions

d74f13b

PriceHiller force-pushed the feat/state-management branch from 923416e to d74f13b Compare November 8, 2023 22:52

test: wait for cache to be fully saved in self-healing test

4f5d9c2

kristijanhusak requested changes Nov 9, 2023

View reviewed changes

PriceHiller and others added 3 commits November 9, 2023 03:20

refactor: rename State -> OrgState with annotation

a0e8d3e

See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

test: remove unnecessary logic in State testing

6407b0e

See nvim-orgmode#624 (comment) See nvim-orgmode#624 (comment) See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

test: add annotation for state in state_spec

54bbf88

See nvim-orgmode#624 (comment) Co-authored-by: Kristijan Husak <[email protected]>

kristijanhusak approved these changes Nov 9, 2023

View reviewed changes

kristijanhusak merged commit e9c08d5 into nvim-orgmode:master Nov 9, 2023
6 checks passed

PriceHiller deleted the feat/state-management branch November 9, 2023 09:33

SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024

feat: add a way to cache state between restarts of Neovim (nvim-orgmo…

fa8c4e0

…de#624) Co-authored-by: Kristijan Husak <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a way to cache state between restarts of Neovim #624

feat: add a way to cache state between restarts of Neovim #624

PriceHiller commented Oct 31, 2023 •

edited

Loading

PriceHiller commented Oct 31, 2023 •

edited

Loading

kristijanhusak commented Nov 2, 2023

PriceHiller commented Nov 2, 2023 •

edited

Loading

PriceHiller commented Nov 2, 2023

kristijanhusak left a comment

PriceHiller commented Nov 2, 2023

PriceHiller commented Nov 2, 2023

PriceHiller commented Nov 6, 2023

kristijanhusak left a comment

PriceHiller commented Nov 6, 2023

kristijanhusak commented Nov 7, 2023

PriceHiller commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

kristijanhusak commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

kristijanhusak left a comment

PriceHiller commented Nov 9, 2023

kristijanhusak left a comment

PriceHiller commented Nov 9, 2023

feat: add a way to cache state between restarts of Neovim #624

feat: add a way to cache state between restarts of Neovim #624

Conversation

PriceHiller commented Oct 31, 2023 • edited Loading

PriceHiller commented Oct 31, 2023 • edited Loading

kristijanhusak commented Nov 2, 2023

PriceHiller commented Nov 2, 2023 • edited Loading

PriceHiller commented Nov 2, 2023

kristijanhusak left a comment

Choose a reason for hiding this comment

PriceHiller commented Nov 2, 2023

PriceHiller commented Nov 2, 2023

PriceHiller commented Nov 6, 2023

kristijanhusak left a comment

Choose a reason for hiding this comment

PriceHiller commented Nov 6, 2023

kristijanhusak commented Nov 7, 2023

PriceHiller commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

kristijanhusak commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

PriceHiller commented Nov 8, 2023

kristijanhusak left a comment

Choose a reason for hiding this comment

PriceHiller commented Nov 9, 2023

kristijanhusak left a comment

Choose a reason for hiding this comment

PriceHiller commented Nov 9, 2023

PriceHiller commented Oct 31, 2023 •

edited

Loading

PriceHiller commented Oct 31, 2023 •

edited

Loading

PriceHiller commented Nov 2, 2023 •

edited

Loading