orgmode icon indicating copy to clipboard operation
orgmode copied to clipboard

Add org-attach feature

Open broken-pen opened this issue 10 months ago • 8 comments

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. :slightly_smiling_face:

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. :wink:

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!

broken-pen avatar Jan 30 '25 16:01 broken-pen

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.

kristijanhusak avatar Jan 30 '25 17:01 kristijanhusak

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

broken-pen avatar Jan 30 '25 22:01 broken-pen

@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 avatar Feb 02 '25 09:02 kristijanhusak

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

kristijanhusak avatar Feb 15 '25 14:02 kristijanhusak

Hi @kristijanhusak ! Thanks for following up, I've been racking my brain how to split the remaining changes up so that they make sense to a reviewer.

The current set of commits is the best I could do. It would be nice if you could at least look over the very first commit. I consider it an MVP, but it very obviously is not a complete implementation (misses e.g. deleting/opening/linking attachments). Let me know how you would handle this PR going forward, eg whether to split it further up.

I'm traveling the next two weeks, so I've got a bit less time, so response times should be a bit longer, but otherwise can make any changes that are necessary or desirable.

broken-pen avatar Feb 17 '25 16:02 broken-pen

I appreciate the effort to split this into smaller chunks for reviewing, it helps tremendously. I also understand that there's probably the point where splitting is really hard to achieve. If the whole PR is completed, I'll look at it, and leave feedback. If it's mostly good (which looks like it at the first glance), I'll probably merge it and address smaller things myself, and any bigger things we can discuss as an improvement in the next PRs. If that sounds good, let me know, and I'll look into this in the following days. Thanks!

kristijanhusak avatar Feb 17 '25 18:02 kristijanhusak

Sounds good to me!

I've also gone over the commits once more and added notes to the commit messages, which Emacs functions are ported in which commit. This makes the reasoning behind each addition (hopefully!) a bit clearer.

broken-pen avatar Feb 17 '25 23:02 broken-pen

Hi! Sorry for the delay, I've been pretty busy transitioning these past few months :sweat_smile: 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.

broken-pen avatar May 24 '25 23:05 broken-pen