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

Support org attachments #795

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

Support org attachments #795

wants to merge 16 commits into from

Conversation

xiaoruoruo
Copy link
Contributor

Resolves #1

  • Act as file share target, not just image
  • Have three options for saving attachments.
  • Option 1: Don't copy, just link to the file in the note, this is existing behavior
  • Option 2: Copy file into a single directory specified by a pref
  • Option 3: Copy file into a ID property based directory (org-attach style)
  • Add support for attachment: link

Feedback on UX or code are welcome :)

There is something left that should be addressed

  • Currently only copies the file for DirectoryRepo, ContentRepo. storeFile is not implemented for other Repo yet. Would others who are more familiar with those implement them?

Other nice to have improvements, but should be future work

  • In the edit note page, support adding/updating attachments
  • When refile a note with attachment to a note in another repository, move attachments to new repository

@lytex
Copy link

lytex commented Feb 13, 2021

Great work! Let me share some thoughts on it:

  • First option (link) copies the link okay, but if the file is not supported by orgzly, it fails with an error share type audio/mpeg is not supported; for example ogg, pdf, apk files do not work, but images do.
  • Second option (Copy to the attachment directory root) works ok, with all types of media. I really like the idea of copying the file to the orgzly directory! However, I would like also the option of a regular [[file:<attachment directory>/filename.ext]], maybe as a fourth option, so that orgzly can open it too. Also, surrounding the path with `[[ ]]`` allows for spaces in the filename and/or attachment directory.
  • Third option (Copy to ID property based directory under attachment directory): I'm not a user or org-attachment and therefore I don't know how it's supposed to work, but there is a mismatch: when creating a new headline, its ID is 99683722-CC47-4B70-89AF-6B134942438F, but gets translated to the path 99/683722-CC47-4B70-89AF-6B134942438F/file

@xiaoruoruo
Copy link
Contributor Author

Thanks for your testing and feedback!

First option (link) copies the link okay, but if the file is not supported by orgzly, it fails with an error share type audio/mpeg is not supported; for example ogg, pdf, apk files do not work, but images do.

This is now fixed, by allowing any kind of file type for the link method. It will try to find the file path for the note content, and file name for the note title. But from my testing, real file path almost always could not be found. Instead "content:" path are found, so I think this method is a lot less useful.

Second option (Copy to the attachment directory root) works ok, with all types of media. I really like the idea of copying the file to the orgzly directory! However, I would like also the option of a regular [[file:/filename.ext]], maybe as a fourth option, so that orgzly can open it too. Also, surrounding the path with `[[ ]]`` allows for spaces in the filename and/or attachment directory.

This is also fixed. For the second option, "file:" link prefix should have been used instead of "attachment:" link prefix. Also added the surrounding brackets which fixed for spaces in filenames.

Third option (Copy to ID property based directory under attachment directory): I'm not a user or org-attachment and therefore I don't know how it's supposed to work, but there is a mismatch: when creating a new headline, its ID is 99683722-CC47-4B70-89AF-6B134942438F, but gets translated to the path 99/683722-CC47-4B70-89AF-6B134942438F/file

This path translation is actually expected of the org-attach behavior, see
#1 (comment)

@lytex
Copy link

lytex commented Feb 21, 2021

I've tested again this new version and all work fine.

Regarding the second option, I think it can be interesting to also add the attach path before the filename. In that way, I can open the file I just attached by clicking on it on Android (as opposed by generating a link that only can be followed on desktop by emacs). I don't know a lot of Java/Android but the patch would be something like:

diff --git a/app/src/main/java/com/orgzly/android/ui/share/ShareActivity.java b/app/src/main/java/com/orgzly/android/ui/share/ShareActivity.java
index 7b225ae3..cbdd673a 100644
--- a/app/src/main/java/com/orgzly/android/ui/share/ShareActivity.java
+++ b/app/src/main/java/com/orgzly/android/ui/share/ShareActivity.java
@@ -36,6 +36,7 @@ import com.orgzly.android.usecase.UseCaseResult;
 import com.orgzly.android.util.LogUtils;
 import com.orgzly.android.util.MiscUtils;
 import com.orgzly.org.OrgStringUtils;
+import com.orgzly.android.prefs.AppPreferences;
 
 import java.io.File;
 import java.io.IOException;
@@ -168,7 +169,8 @@ public class ShareActivity extends CommonActivity
                 }
 
             } else if (ATTACH_METHOD_COPY_DIR.equals(AppPreferences.attachMethod(this))) {
-                handleCopyFile(intent, data, "file:");
+                String attachDirPath = AppPreferences.attachDirDefaultPath(this);
+                handleCopyFile(intent, data, "file:"+attachDirPath+"/");
             } else if (ATTACH_METHOD_COPY_ID.equals(AppPreferences.attachMethod(this))) {
                 handleCopyFile(intent, data, "attachment:");
             } else {

E.g. I can open a pdf file using a pdf reader, a voice note using a music app, etc

@Ypot
Copy link

Ypot commented Mar 28, 2021

Option 3: Copy file into a ID property based directory (org-attach style)

Maybe it should work too with a DIR property based directory

  :PROPERTIES:
  :DIR: folder/
  :END:

https://orgmode.org/manual/Attachment-options.html#Attachment-options

For copy method, pass the content Uri to NoteFragment to be saved.

For the link method, keep old behavior.
This is to support storing a file into a directory path in the repo.

Only implemented for ContentRepo and DirectoryRepo for now.
Tested with a webdav server to confirm that attachment and its parent
directories can be created.

Caveat:
- The attachment: link cannot be resolved. Because this link currently
only resolves to local files, it cannot deal with remote files (like
webdav). To handle remote files, need additional changes to download
and cache the remote files.
@xiaoruoruo
Copy link
Contributor Author

xiaoruoruo commented Jun 1, 2021

@lytex Currently file: link is resolved using the setting "Root for relative links". I recognize that some user may not choose the same root for relative link as the "attachment directory" which is another setting. If those two settings point to the same directory, link opening would work. If they point to different places, there is no good way to solve the issue. This is the inherent problem for second option. I think if a user wants to specify an attachment directory, the DIR property based directory could be used, as suggested by @Ypot in the last comment. I might add support for that sometime in the future.

@xiaoruoruo
Copy link
Contributor Author

@nevenz I'd like some feedback on how best to proceed here.

This PR adds the functionality for Orgzly to act as a file share target, so that users can save attachments into Orgzly, but only access them through the links within the content. The options for saving attachment are opinionated.

I have another WIP PR that depends on this PR, but further enables Orgzly to manage files within a note, by adding/deleting/listing multiple files within a note. People may have different taste for the UI.

I haven't received feedback on this PR yet from you, but I need to keep up with the merge conflicts as the main branch develops.

Maybe I can submit both PR to prove the prototype concept, and you can modify as you see fit as that seems to be the most efficient way?

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.

Support org attachments
3 participants