Skip to content
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

Add org-attach feature #878

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

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

@troiganto
Copy link
Contributor Author

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

@troiganto troiganto marked this pull request as draft January 30, 2025 22:11
@troiganto troiganto force-pushed the feat/attach branch 2 times, most recently from 299f922 to 57ca4e8 Compare January 30, 2025 23:11
@troiganto troiganto 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.

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