-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix: update orgmode db on roam node creation #64
Conversation
@seflue help me understand. Is this because we have to maintain a separate If so, can you add a comment above the line to explain that? Pretty sure I'll forget why we're doing this. |
lua/org-roam/database/loader.lua
Outdated
@@ -348,6 +348,8 @@ function M:load_file(opts) | |||
insert_new_file_into_database(db, file, { | |||
force = opts.force or file.metadata.changedtick ~= changedtick, | |||
}) | |||
|
|||
require("orgmode").files:load(true) |
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.
@seflue line 329 should already add this file.
This might be an issue in orgmode, because we do early return here https://github.com/nvim-orgmode/orgmode/blob/master/lua/orgmode/files/init.lua#L62-L64.
Can you try comment out that early return, remove this line, and see if it works then?
Edit: Ok, I think line 329 points to a different file loader. If that's the case, we need to do this, but this is not the most optimized way. Let me think a bit about this.
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.
@kristijanhusak Thanks for considering a more optimal solution. I also have the feeling, that something can be improved in loading org-files.
While you are already reflecting on this topic I want to bring in another thought: For Telescope-orgmode I always wished for incremental loading capabilities of orgfiles. Like a stream, so user get's immediate feedback as soon as the first org file has been parsed and the results can be updated with each file parsed. Telescope itself is built to handle such cases, but orgmode currently does not support this.
Is this something you would need to factor into your considerations of optimizing file loading or should I create another issue for this?
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.
While you are already reflecting on this topic I want to bring in another thought: For Telescope-orgmode I always wished for incremental loading capabilities of orgfiles. Like a stream, so user get's immediate feedback as soon as the first org file has been parsed and the results can be updated with each file parsed. Telescope itself is built to handle such cases, but orgmode currently does not support this.
I don't plan to add a stream like loading, because then I would have to worry about the pending files when showing the agenda and similar. There is caching in place so loading is slow only once.
Even this load(true)
function should be fairly fast in this scenario because only files that are not already loaded will be loaded, but I'll still see to add a method that will load the file and add it to path, but only if it belongs to path.
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.
If you add this method to load a single file, I think, I could add this stream-based approach myself in Telescope-orgmode. For me it's just a matter of a flexible API to provide plugins the possibility to do, what they need. That you don't want to implement a stream-based loading directly in orgmode makes sense.
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.
@chipsenkbeil do you recall if this add_to_paths
is needed to just load the file to files
table, or we also need it to append it to path?
Currently, add_to_paths
will load the file, put it in OrgFiles.files
table, and also append the filename to OrgFiles.paths
if the file does not belong to the defined path wildcard.
So for example, if my org-roam config is ~/roam/**/*
, orgmode org_agenda_files is ~/org
, and I do :add_to_paths('~/some/other/path/file.org')
in any of those file loaders, this will add the file to the paths
for the session, even though that might not be desired.
If a file that is already part of the path wildcard, for example :add_to_paths('~/roam/myfile.org')
with a roam file loader, we are not adding it to paths
, since it will be loaded anyway.
My question is: Is it ok if we do not append the filename to OrgFiles.paths
in add_to_paths
? Does org-roam always creates files that are defined in the path wildcard (in my example, ~/roam/**/*
)?
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.
Org roam takes a directory path, not a glob, and always creates node files within that directory. Nowhere else.
To be compatible with your plugin, I convert the directory to a glob doing a simple path join: join_path("path/to/roam", "**", "*")
.
I'd have to go back to read why the function was added. I've got time today to do that.
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.
@kristijanhusak I tried to skim through the code to refresh myself.
With OrgFiles
, I directly access the all_files
field, and make use of these functions:
- filenames
- load_file
- add_to_paths
- load
I pass OrgFiles
to things like OrgCapture
. I don't think I access paths
directly.
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.
and always creates node files within that directory
This answers my question, thanks!
@seflue let me do some updates on the orgmode side and I'll let you know what to change here.
@chipsenkbeil If you need some helper function to avoid accessing properties directly let me know, we can add something.
0d4666f
to
73b8474
Compare
I added the comment you requested. @chipsenkbeil Are you ok with merging that, as long as nvim-orgmode does not provide this directly in |
I'm fine with that. Give me today to try to fully answer @kristijanhusak question in the review in case that changes what we need to do. |
Yes, we can do that. @chipsenkbeil just keep in mind recent changes to hyperlinks before we decide to bump the version. |
@seflue @chipsenkbeil I pushed nvim-orgmode/orgmode@2a10172 that adds an optional Since we need to keep things backward compatible, let's do this in this PR: Do @chipsenkbeil once you update the minimum orgmode version to |
@kristijanhusak We already call
@chipsenkbeil Should I create a new PR to do the adjustments including the version bump? Depending on what @kristijanhusak says, we can merge this PR before the second one or close it, if we don't care about the issue in older versions. Or we reuse it and I do the adjustments here right away. |
@kristijanhusak I played around with your suggestions but in the end, only the call to |
@seflue just replace the line that you added with this: require('orgmode').files:add_to_paths(file.filename) And give it a test. It should also work fine. |
@kristijanhusak As I said, I tested all possible variations of what you suggested and none of them worked. But bringing back my old line |
@seflue I'll give it a test. Are you using the same path for org roam and org agenda files? |
My paths for roam files is a subset of my agenda paths in orgmode. Or to be more precise: It is included by the globbing of one of my agenda file paths. For orgmode I have something like this: opts = {
-- ...
org_agenda_files = {
"~/my/agenda/path1/**/*.org",
"~/my/agenda/path2/**/*.org",
"~/my/agenda/path3/**/*.org",
},
-- ...
}, And for org-roam: opts = {
-- ...
directory = "~/my/agenda/path3/org-roam-files"
-- ...
}, |
@seflue what action are you doing in org roam? This is what I tried: Setup: require('orgmode').setup({
org_agenda_files = { '~/orgtest/**/*.org' },
org_default_notes_file = '~/orgtest/refile.org',
})
require('org-roam').setup({
directory = '~/orgtest/roam',
})
This works when I add Are you doing something else? |
@kristijanhusak: Yes, I do. My particular use case is, that I want to use Telescope-orgmode.nvim for refiling and searching. Here is, what I do, because this is exactly my use case:
So |
@seflue Ok, I found it. I mistyped the argument. Fixed in nvim-orgmode/orgmode@08d763d. It should work with the suggested orgmode version from org-roam (0.3.4). This was issue just on master. |
To make a new roam node accessible for refiling, searching and agenda the org files need to be loaded again.
73b8474
to
7efcffd
Compare
@kristijanhusak I can confirm now, that with orgmode on master it now works for me, if I use @chipsenkbeil: We can merge this PR now, if that's ok for you. I already have an adjusted version, which is compatible with orgmode master ready on another branch. I would rebase it after we merged this and create a new PR. |
@kristijanhusak I saw that you created a tag 0.3.61 and a release 0.3.7 pointing to this tag. Is there a reason for this inconsistency? I would prefer, if we just count up the last number for bugfix releases - no matter if a release includes just one commit or dozens. And bound release numbers strictly to tags. |
@seflue that was a typo. I just released a regular 0.3.7 version with another bug fix, that also includes this one. |
@seflue i want to release one more minor version before we bump the required orgmode version. If we merge this, we're still compatible with 0.3.4? |
Yes. |
To make a new roam node accessible for refiling, searching and agenda the org files need to be loaded again.
See also this discussion.