Skip to content

Add org-attach feature #878

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

broken-pen
Copy link
Contributor

Hi, apologies for dropping such a large PR in your lap!

I've been working on-and-off on attachments and it's in a state now where I believe it works as intended and very similarly to the Emacs version. 🙂

The feature seemed simple., but ended up ballooning because:

  1. the Emacs version uses a lot of Emacs machinery that nvim doesn't have (recursive copying, downloads);
  2. I ended up having to modify several utils functions (like orgmode.utils.fs.substitute_path) for my use case.

I've split commits up as much as I could to make accepting/rejecting each individual change easier. I've also split the implementation of org-attach into 6-7 commits to make it easier to review them.

To be clear, I don't expect all changes to go into the main repository. Let me know if there are any design decisions that you'd rather not commit to maintaining. 😉

Some of my more dubitable choices:

  1. I wrapped a lot of vim.uv.fs_* functions in a module orgmode.attach.fs. I simply couldn't figure out how to write an async recursive copy any other way.
  2. I wrote a few new dialogs in orgmode.attach.ui. Some look weird because they replicate Emacs dialogs (yes_or_no_or_cancel_slow() is the equivalent of yes-or-no-p) and might not necessarily make sense in nvim.
  3. The Emacs version wildly interleaves logic and user interaction. Elisp has a lot of macros that make this work, unlike Lua. To keep the code clear, I've put the actual logic into orgmode.attach.core and wrapped it in orgmode.attach. This separation required passing around a few callbacks and effectively writing every function twice. It may be more work than it's worth.

Thanks again for keeping this project going, it's been helping me a lot!

@kristijanhusak
Copy link
Member

Hey! Thanks for the PR!
I still didn't look into it completely, just took a brief look.
I noticed that as part of this PR you added properties inheritance.

To make things simpler, can we extract this into its own PR?
The same applies to anything else that I maybe missed, and is not strictly related to attachments.

@broken-pen
Copy link
Contributor Author

Sure thing 👍 I'll mark this PR as a draft in the meantime, to keep things organized

@broken-pen broken-pen marked this pull request as draft January 30, 2025 22:11
@broken-pen broken-pen force-pushed the feat/attach branch 2 times, most recently from 299f922 to 57ca4e8 Compare January 30, 2025 23:11
@broken-pen broken-pen force-pushed the feat/attach branch 2 times, most recently from 71802a5 to 2ff76bd Compare February 1, 2025 21:05
@kristijanhusak
Copy link
Member

@troiganto I merged the other PR and rebased this one to resolve conflicts.
If there's nothing else to add, move this to "ready to review" and I'll start looking at it.

@kristijanhusak
Copy link
Member

@troiganto can this be reviewed or there are still pending changes?

broken-pen added 12 commits May 25, 2025 01:03
The implementation follows the Emacs implementation in spirit, with some
additional separation of concerns:

1. the actual logic is implemented in `attach/core.lua`, with no
   concerns for UI or configuration;
2. `attach/init.lua` combines the core with UI and configuration and
   provides the public API;
3. `attach/ui.lua` contains any dialogs needed by the module;
4. `attach/fileops.lua` contains file operations that are provided to
   the Emacs implementation by the Emacs core (e.g. recursive
   copy/deletion of directories)
5. `attach/node.lua` provides an abstraction over headlines and whole
   files that is absent in the Emacs implementation
6. `attach/translate_id.lua` corresponds to the Emacs implementation's
   functions `org-attach-id-uuid-folder-format`,
   `org-attach-id-ts-folder-format`, and
   `org-attach-id-fallback-folder-format`. They are separated like this
   because referring to pre-defined functions in Lua is more difficult
   than in Emacs.

To reduce complexity, the following functions are left unimplemented in
this commit:

- `org-attach-file-list`
- `org-attach-expand`
- `org-attach-follow`
- `org-attach-complete-link`
- `org-attach-reveal`
- `org-attach-reveal-in-emacs`
- `org-attach-open`
- `org-attach-open-in-emacs`
- `org-attach-delete-one`
- `org-attach-delete-all`
- `org-attach-sync`
- `org-attach-archive-delete-maybe`
- `org-attach-expand-links`
- `org-attach-url`
- `org-attach-dired-to-subtree`
This adds functions that correspond to these Emacs functions:

- `org-attach-reveal`
- `org-attach-reveal-in-emacs`
- `org-attach-open`
- `org-attach-open-in-emacs`
These correspond to the following Emacs functions:
- `org-attach-delete-one`
- `org-attach-delete-all`
This corresponds to the Emacs function `org-attach-sync`.
This provides functionality provided by the following functions in the
Emacs implementation:

- `org-attach-file-list`
- `org-attach-expand`
- `org-attach-follow`
- `org-attach-complete-link`
These are implemented in Emacs as the hooks
`org-attach-after-change-hook` and `org-attach-open-hook`.
This corresponds to the Emacs function
`org-attach-archive-delete-maybe`.
This corresponds to the Emacs function `org-attach-expand-links`.
This brings the feature in line with capture and agenda, which similarly
have a top-level menu prompt.
This corresponds to the Emacs function `org-attach-url`. It also adds
some functionality from the Emacs core that we don't get for free.
This function has no direct Emacs equivalent. However, it tries to
replicate the functionality provided by `org-attach-dired-to-subtree`,
which attaches files selected in a dired window (roughly equivalent to
Vim's NetRW) to the last-seen Orgmode task.

Our implementation tries to be extremely general because the Neovim
ecosystem has a plethora of file browser plugins.
@broken-pen
Copy link
Contributor Author

Hi! Sorry for the delay, I've been pretty busy transitioning these past few months 😅 but I finally found time to look at this PR again.

I've addressed all your suggestions and fixed the two bugs in the video. (Thanks for that, btw, it made it very easy to locate the issue!)

I think there are a few more TODOs I've left in the code. It'd be cool if you could have a look over them and suggest how to handle/document them before merging, just so they won't get lost.

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I now managed to give it a solid test. I might not cover everything but I tried to go through the most important flows.
Note that I'm not a user of attachments in Orgmode so I might lack some knowledge around it, but I did a comparison with how Emacs works to give a feedback.

These are the things I found:

  1. Do not prompt if there's only a single file attached on the node
  2. Attaching from URL throws this error: [orgmode] .../kristijan/github/orgmode/lua/orgmode/attach/fileops.lua:362: attempt to call field 'exists' (a nil value).
    I tried this url https://placecats.com/millie/300/150
  3. Pressing Esc in "Set directory" prompt shows this error [orgmode] /home/kristijan/github/orgmode/lua/orgmode/attach/ui.lua:24: attempt to index local 'answer' (a nil value).
    We should just print "Quit" in that case.
  4. When setting and unsetting the directory, copying over overwrites the files. In emacs, it throws a "File exists" error. Here are steps to reproduce it:
    1. Have some attachments for a headline
    2. Set the directory, when asked to copy, say "yes", when asked to delete, say "no"
    3. Unset the directory, when asked to copy, say "yes"
    4. In here, it overwrites the files
    5. In emacs, throws "File exists" error
  5. Attaching a buffer content throws the [orgmode] /home/kristijan/github/orgmode/lua/orgmode/attach/core.lua:504: E5560: Vimscript function must not be called in a fast event context.
    It attaches the file, but also throws the error.
    Here's how I reproduce it:
    1. Have 2 org files, todos.org and notes.org
    2. Open todos.org with Neovim nvim todos.org
    3. Open notes.org within Neovim to load it into buffer :e notes.org, go back to todos.org
    4. On some headline, try to attach refile.org as a buffer content
  6. Opening an attachment with o behaves differently. Here we are forcing an external call, where Emacs seems to open it internally most of the time.
    For example, if you attach an org file buffer and then open it, Emacs just opens another buffer and loads the org file, while we open with the external program.
    I assume it was hard to mirror that functionality in Neovim, but please confirm.
  7. I wasn't able to get the completion for the attachment links to work. I'm not sure why though.

Regarding the TODOs:

  1. adding events (hooks) is fine.
  2. Regarding the expanding links, you should be able to capture links with link and link_desc TS node types. It was added in the recent versions.
  3. Filetags can be added separately

Review is a bit all over the place, but it's a big PR so I just wrote things as I found them.

if not opts.exist_ok and M.exists(dest) then
return Promise.reject('EEXIST: ' .. dest)
end
local args = { 'curl' }
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to also check if curl is executable.

Also, this might not work on Windows. The mighty Copilot spit out something like this to support Windows. Haven't tested it though.

local function download_file(url, output)
  local cmd
  if vim.fn.has("win32") == 1 then
    -- Use PowerShell's Invoke-WebRequest on Windows
    cmd = { "powershell", "-Command", string.format("Invoke-WebRequest -Uri '%s' -OutFile '%s'", url, output) }
  else
    -- Use curl on Unix-like systems
    cmd = { "curl", "-fLo", output, "--create-dirs", url }
  end
  local result = vim.system(cmd):wait()
  if result.code ~= 0 then
    vim.notify("Download failed: " .. (result.stderr or ""), vim.log.levels.ERROR)
  else
    vim.notify("Downloaded to " .. output)
  end
end

If it's not a hassle add the Windows part and we will see later how it goes.

end

---@param path string
function AttachNode:_make_absolute(path)
Copy link
Member

Choose a reason for hiding this comment

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

This method name confused me when I saw it in usage. I assumed it makes the directory.
Let's rename it to something that indicates it's only a converter.
Also, lets add a TODO to update this to vim.fs.abspath once we stop supporting Neovim < 0.11.

Suggested change
function AttachNode:_make_absolute(path)
--- TODO: Use `vim.fs.abspath` once we drop support for 0.10.
---@param path string
function AttachNode:_get_absolute_path(path)

local base = vim.fs.dirname(self.file.filename)
path = vim.fs.joinpath(base, path)
end
return vim.fs.normalize(path, { expand_env = false })
Copy link
Member

Choose a reason for hiding this comment

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

Why not expand env?

@@ -87,4 +88,39 @@ function M.trim_common_root(paths)
return result
end

---Return a path to the same file as `filepath` but relative to `base`.
---Starting with nvim 0.11, we can replace this with `vim.fs.relpath()`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
---Starting with nvim 0.11, we can replace this with `vim.fs.relpath()`.
---TODO: Starting with nvim 0.11, we can replace this with `vim.fs.relpath()`.

Comment on lines +172 to +176
dir = self:get_existing_id_dir()
if dir then
return dir
end
return nil
Copy link
Member

Choose a reason for hiding this comment

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

We can just return here, get_existing_id_dir returns dir | nil

Suggested change
dir = self:get_existing_id_dir()
if dir then
return dir
end
return nil
return self:get_existing_id_dir()

---@param file OrgFile
---@param cursor [integer, integer] The (1,0)-indexed cursor position in the buffer
---@return OrgAttachNode
function Attach:get_node(file, cursor)
Copy link
Member

Choose a reason for hiding this comment

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

It seems this is not being used anywhere. Do you think it's worth leaving it for something else in the future?

most recently open org file:

#+begin_src lua
vim.keymap.set('n', '<Leader>o+', function()
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't able to get this working. I get this error:

E5108: Error executing lua: /home/kristijan/github/orgmode/lua/orgmode/attach/core.lua:143: attempt to call a table value                                                                                                                                                                                                                                                                     
stack traceback:                                                                                                                                                                                                                                                                                                                                                                              
        /home/kristijan/github/orgmode/lua/orgmode/attach/core.lua:143: in function 'list_current_nodes'                                                                                                                                                                                                                                                                                      
        /home/kristijan/github/orgmode/lua/orgmode/attach/init.lua:698: in function 'find_other_node'                                                                                                                                                                                                                                                                                         
        /home/kristijan/github/orgmode/lua/orgmode/attach/init.lua:624: in function 'attach_to_other_buffer'                                                                                                                                                                                                                                                                                  
        .../kristijan/.config/nvim/lua/partials/plugins/orgmode.lua:176: in function <.../kristijan/.config/nvim/lua/partials/plugins/orgmode.lua:173> 

I might not understand how it should be used. This is what I did:

  1. Had a todos.org file with a headline and a link to a picture in the headline content. Something like this:
* Test
   [[~/path/to/image.png]]
  1. Opened another notes.org file that has a * Headline inside
  2. Went back to todos.org, hovered over the link, and did <leader>o+

Additionally, I would prefer if we would not suggest using the orgmode instance directly but instead expose this through the API if possible.

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.

2 participants