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

[Visuwesh <visuweshm@gmail.com>]

[ Please keep me in the CC as I don't follow the list.  ]

[ஞாயிறு செப்டம்பர் 24, 2023] Max Nikulin wrote:

> On 23/09/2023 17:28, Ihor Radchenko wrote:
>> Visuwesh writes:
>> 
>>> +  (let* ((ext (symbol-name (mailcap-mime-type-to-extension mimetype)))
>>> +         (iname (read-string "Insert filename for image: "))
>> It would be nice if we auto-generate the file name here by
>> default. It
>> is what I would expect from yanking an image at least.
>
> Certainly it would be great to provide some default value allowing
> user to override it. However it may be not so trivial. Clipboard
> content may be just copy region from some graphics editor or a
> screenshot tool, so not associated with a file yet. In X11 xprop may
> be used to get clipboard owner application and its title. Wayland may
> be less permissive.
>
> I have tried to copy an image in Firefox. There is a chance to find
> file name (it may be image.php?id=1324 though) in text/html clipboard
> content, but it requires parsing of HTML
>
> xclip -selection clipboard -o -t text/html

I would rather not go down this rabbit hole since there is no way to
cover all the possible cases.

[ஞாயிறு செப்டம்பர் 24, 2023] Max Nikulin wrote:

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

What harm does it cause?

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

I have never used that package, this is what I wrote in a discussion
with Ihor in the Matrix room which we iteratively improved.  It was
mostly written with my workflow in mind.  Without knowledge of how
others work, I cannot improve the implementation.  I also don't
understand what the linked package from the Commentary section (which is
why I never used it).

>> +++ b/lisp/org.el
>
> I am in doubts if the following code is more suitable for org.el or
> for org-attach.el

I have the same doubts.

>> @@ -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'?

It should be easy enough to do it but do we want to go there?

>> +  "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/)

Is there a reliable way to know which directory is "safe"?  If not, then
let the users shoot themselves in the foot.

>> +
>> +(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.

Sorry, I don't understand what you mean here.  I am not really familiar
with attachment facilities of org-mode.

>> +         (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.

This would include too many prompts and make the whole process annoying
again.

>> [...]
>> +(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"?

Okay, will make them both follow what org-attach uses.

>> +
>> +(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

Yes, thanks for the catch.

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

Okay.

>> +                                 "Attach using method"
>> +                                 '((?c "cp" cp)
>> +                                   (?m "mv" mv)
>> +                                   (?l "ln hard link" ln)
>
> "ln" in the label perhaps should be dropped.

I added ln to make the first letter bolded in rmc prompt, but with
org-mks this shouldn't be necessary anymore.

> Should it be hidden if stat reports different file systems for source
> and target or fallback to copy is implicitly used?

It should be handled appropriately by Emacs already.  See (info "(emacs)
Copying and Naming").  I also don't find org-attach doing anything
special.

>> +                                   (?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.

That can be done.

>> +                  "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.

Sorry, I don't understand what you mean here.  Note that my usage of
attachments is sparse.  "Particular image" is hard to determine since
the only reliable data at hand is its mimetype.