Re: [BUG] [PATCH] Add yank-media and DND handler [9.6.7 (9.6.7-g6eb773 @ /home/viz/lib/emacs/straight/build/org/)]

[Max Nikulin]

On 22/09/2023 21:52, Visuwesh wrote:
> Attached patch adds yank-media and DND handler to attach files in the
> clipboard and dropped onto an Emacs frame respectively.

Please, use `make-temp-file' to create files in the temporary directory 
that may be world writable. Predictable file names there are undesired 
from security point of view.

Other notes are no more than opinion. I may easily miss something and 
verbose reply may be wasting of time. Do not hesitate to ask more 
details if you do not agree.

At first, I expected more common with the following project
https://github.com/abo-abo/org-download/
however even the approach to fetch images from clipboard is different.

> +++ b/lisp/org.el

I am in doubts if the following code is more suitable for org.el or for 
org-attach.el

> @@ -20254,6 +20257,146 @@ it has a `diary' type."
>                     (org-format-timestamp timestamp fmt t))
>           (org-format-timestamp timestamp fmt (eq boundary 'end)))))))
>  
> +;;; Yank media handler and DND
> +(defun org-setup-yank-dnd-handlers ()
> +  "Setup the `yank-media' and DND handlers for buffer."
> +  (setq-local dnd-protocol-alist
> +              (cons '("^file:///" . org--dnd-local-file-handler)
> +                    dnd-protocol-alist))
> +  (yank-media-handler "image/.*" #'org--image-yank-media-handler)
> +  ;; Looks like different DEs go for different handler names,
> +  ;; https://larsee.com/blog/2019/05/clipboard-files/.
> +  (yank-media-handler "x/special-\\(?:gnome\|KDE\|mate\\)-files"
> +                      #'org--copied-files-yank-media-handler))
> +
> +(defcustom org-media-image-save-type 'attach

Could it be extended to any mime type? If so I would avoid "image" in 
its name. `org-yank-media-save-dir'?

> +  "Method to save images yanked from clipboard and dropped to Emacs.
> +It can be the symbol `attach' to add it as an attachment, or a
> +directory name to copy/cut the image to that directory."
> +  :group 'org
> +  :package-version '(Org . "9.7")
> +  :type '(choice (const :tag "Add it as attachment" attach)
> +                 (directory :tag "Save it in directory"))
> +  :safe (lambda (x) (or (stringp x) (eq x 'attach))))

Unsure if every directory may be considered as safe (e.g. ~/.ssh/)

> +
> +(declare-function org-attach-attach "org-attach" (file &optional visit-dir 
> method))
> +
> +(defun org--image-yank-media-handler (mimetype data)
> +  "Save image DATA of mime-type MIMETYPE and insert link at point.
> +It is saved as per `org-media-image-save-type'.  The name for the
> +image is prompted and the extension is automatically added to the
> +end."
> +  (let* ((ext (symbol-name (mailcap-mime-type-to-extension mimetype)))
> +         (iname (read-string "Insert filename for image: "))

Unless 'attach is used, `read-file-name' would allow saving to arbitrary 
directory with path completion.

> +         (filename (file-name-with-extension iname ext))
> +         (absname (expand-file-name
> +                   filename
> +                   (if (eq org-media-image-save-type 'attach)
> +                       temporary-file-directory

`make-temp-file' should be used instead of `temporary-file-directory', 
however it requires changes in code around.

> +                     org-media-image-save-type)))
> +         link)
> +    (when (and (not (eq org-media-image-save-type 'attach))
> +               (not (file-directory-p org-media-image-save-type)))
> +      (make-directory org-media-image-save-type t))
> +    (with-temp-file absname
> +      (insert data))
> +    (if (null (eq org-media-image-save-type 'attach))
> +        (setq link (org-link-make-string
> +                    (concat "file:" (file-relative-name absname))
> +                    filename))

Is there a chance to request for link description? Unsure if 
`org-insert-link' may be easily reused.

> +      (require 'org-attach)
> +      (org-attach-attach absname nil 'mv)
> +      (setq link (org-link-make-string
> +                  (concat "attachment:" filename)
> +                  filename)))
> +    (insert link)))
> +
> +;; I cannot find a spec for this but
> +;; https://indigo.re/posts/2021-12-21-clipboard-data.html and pcmanfm
> +;; suggests that this is the format.
> +(defun org--copied-files-yank-media-handler (_mimetype data)
> +  "Attach copied or cut files from file manager.
> +If the files were cut from the file manager, then the `mv' attach
> +method is used; `cp' otherwise.
> +
> +DATA is a string where the first line is the operation to
> +perform: copy or cut.  Rest of the lines are file: links to the
> +concerned files."
> +  (require 'org-attach)
> +  (let* ((data (split-string data "[\0\n\r]" t "^file://"))
> +         (files (cdr data))
> +         (method (if (equal (car data) "cut")
> +                     'mv
> +                   'cp)))
> +    (dolist (f files)
> +      (setq f (url-unhex-string f))
> +      (if (file-readable-p f)
> +          (org-attach-attach f nil method)
> +        (message "File `%s' is not readable, skipping" f)))))
> +
> +(defcustom org-dnd-default-attach-method nil
> +  "Default attach method to use when DND action is unspecified.
> +This attach method is used when the DND action is `private'.
> +This is also used when `org-media-image-save-type' is nil.
> +When nil, use `org-attach-method'."
> +  :group 'org
> +  :package-version '(Org . "9.7")
> +  :type '(choice (const :tag "Default attach method" nil)
> +                 (const :tag "Copy" cp)
> +                 (const :tag "Move" mv)
> +                 (const :tag "Hard link" ln)
> +                 (const :tag "Symbol link" lns)))

Labels in the menu below are a bit different. "Symbolic"?

> +
> +(declare-function mailcap-file-name-to-mime-type "mailcap" (file-name))
> +(defvar org-attach-method)
> +
> +(defun org--dnd-local-file-handler (url action)
> +  "Attach filename given by URL using method pertaining to ACTION.
> +If ACTION is `move', use `mv' attach method.
> +If `copy', use `cp' attach method.
> +If `ask', ask the user.
> +If `private', use the method denoted in `vz/org-dnd-default-attach-action'.

"vz/" likely should be dropped

> +The action `private' is always returned."
> +  (require 'mailcap)
> +  (let* ((filename (dnd-get-local-file-name url))
> +         (mimetype (mailcap-file-name-to-mime-type filename))
> +         (separatep (and (string-prefix-p "image/" mimetype)
> +                         (not (eq 'attach org-media-image-save-type))))
> +         (method (pcase action
> +                   ('copy 'cp)
> +                   ('move 'mv)
> +                   ('ask (caddr (read-multiple-choice

`org-mks' has some issues, but it is the current way to present a kind 
of menu in Org.

> +                                 "Attach using method"
> +                                 '((?c "cp" cp)
> +                                   (?m "mv" mv)
> +                                   (?l "ln hard link" ln)

"ln" in the label perhaps should be dropped. Should it be hidden if stat 
reports different file systems for source and target or fallback to copy 
is implicitly used?

> +                                   (?s "symbolic link" lns)))))
> +                   ('private (or org-dnd-default-attach-method
> +                                 org-attach-method)))))
> +    (if separatep
> +        (funcall
> +         (pcase method
> +           ('cp #'copy-file)
> +           ('mv #'rename-file)
> +           ('ln #'add-name-to-file)
> +           ('lns #'make-symbolic-link))
> +         filename
> +         (expand-file-name (file-name-nondirectory filename)
> +                           org-media-image-save-type))
> +      (org-attach-attach filename nil method))
> +    (insert
> +     (org-link-make-string
> +      (concat (if separatep

I would consider swapping of `concat' and `if' for the sake of readability.

> +                  "file:"
> +                "attachment:")
> +              (if separatep
> +                  (expand-file-name (file-name-nondirectory filename)
> +                                    org-media-image-save-type)
> +                (file-name-nondirectory filename)))
> +      (file-name-nondirectory filename))
> +     "\n")
> +    'private))
> +
>  ;;; Other stuff
>  
>  (defvar reftex-docstruct-symbol)
> --

I do not know if it can be easily implemented, but it may be useful to 
control attachment/generic directory at the moment when a particular 
image is yanked/dropped instead of purely relying on predefined user option.