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