[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [BUG] [PATCH] Add yank-media and DND handler [9.6.7 (9.6.7-g6eb773 @
From: |
Visuwesh |
Subject: |
Re: [BUG] [PATCH] Add yank-media and DND handler [9.6.7 (9.6.7-g6eb773 @ /home/viz/lib/emacs/straight/build/org/)] |
Date: |
Mon, 25 Sep 2023 19:45:33 +0530 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
[ 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.
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, 2023/09/24