Package: emacs;
Reported by: Eason Huang <aqua0210 <at> foxmail.com>
Date: Wed, 3 Jul 2024 04:47:01 UTC
Severity: wishlist
Found in version 30.0.60
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
Message #109 received at 71909 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Cecilio Pardo <cpardo <at> imayhem.com> Cc: 71909 <at> debbugs.gnu.org Subject: Re: bug#71909: 30.0.60; Date: Tue, 29 Oct 2024 16:25:29 +0200
> Date: Mon, 28 Oct 2024 22:46:36 +0100
> From: Cecilio Pardo <cpardo <at> imayhem.com>
>
> This patch adds support for yank-media on MS-Windows. Media is handled
> in some different ways:
>
> - Clipboard data that is already named as a mime-type needs no work
> besides returning it. For example, Krita provides copied pixels as
> multiple image/xxxx types, and Firefox provides html as text/html.
>
> - Other programs don't use mime types. We try to recognize some names
> and change then to mime types. For example, GIMP uses the name "PNG"
> for copied pixels. We change it to image/png. LibreOffice also uses
> "PNG" for images. It uses "HTML Format" for rich text and also for
> spreadsheet cells, and we change that to text/html.
>
> - Finally, some programs supply image data in DIBV5 format. We offer
> it as image/png, and convert in on the fly when requested. Firefox
> does this when using "Copy image".
>
> This are the tested media types:
>
> - [X] GIMP copy pixels -> image/png
> - [X] LibreOffice vectorial object -> image/png
> - [X] LibreOffice embedded image -> image/png
> - [X] LibreOffice rich text -> text/html
> - [X] LibreOffice Calc cells -> text/html
> - [X] Firefox copy image -> image/png (also text/html as
> embedded image)
> - [X] Firefox page text -> text/html
> - [X] Krita pixels -> image/png (and others)
> - [X] InkScape -> image/svg+xml, image/png
>
> Images can be yanked in at least org-mode, message-mode, html-mode.
> HTML (text/html) can be yanked in at least html-mode.
>
> SVG will not work until bug #74044 is fixed.
Thanks.
> The image conversion is done using GdiPlus functions, which are
> already used on w32image.c, but are static. I have splitted this file
> into .c and .h, to be able to reuse those definitions. The image
> conversion requires that native image functions are activated.
What happens with yank-media if the user disables native image APIs?
Do we signal an error or is there some graceful degradation (like
using another MIME type)?
> Now I think this patch may have been splitted into 2 or 3 for review.
> Let me know if that would be better.
I personally prefer a single patch.
> * lisp/term/w32-win.el (w32--selection-target-translations): New
> variable that holds the name translations for media tytpes.
> (w32--translate-selection-target): New function, translate the name of a
> media type.
> (w32--translate-reverse-selection-target): New function, Reverse
> translation.
> (w32--get-selection): Modified to translate target names when asked for
> targets, and retrieve media types when asked for them.
> * lisp/textmodes/sgml-mode.el (html-mode--image-yank-handler): Fixed the
> image save mechanism, that added line feed characters on MS-Windows,
> breaking binary formats.
> * src/w32image.c (gdiplus_init): Modified to fetch more functions fromm
> gdiplus.
> (get_encoder_clsid): renamed to w32_gdip_get_encoder_clsid and made
> nonstatic.
> * src/w32select.c (stdfmt_name): Made global, was function static.
> (convert_dibv5_to_png): New function to convert DIBV5 clipboard format
> to PNG.
> (get_clipboard_format_name): New function get the name of a format given
> its index.
> (Fw32__get_clipboard_data_media): New function, retrieves and convert
> media content.
> (syms_of_w32select): Export new lisp functions
> * src/w32gdiplus.h: New file, for definitions in w32image.c
Some of these lines are too long, please make sure they don't exceed
70 columns, preferably 63.
> +(defun w32--translate-reverse-selection-target(target)
> + (let ((ret
> + (append
> + (cl-mapcar #'car
> + (cl-remove-if-not
This will load cl-seq in every w32 session, so let's not call cl-*
functions in preloaded files.
> + ((eq type 'CLIPBOARD)
> + (let* ((tmp-file (make-temp-file "emacs-clipboard"))
> + (data-types (w32--translate-reverse-selection-target data-type))
> + (data (w32--get-clipboard-data-media data-types tmp-file))
> + (result (cond
> + ;; data is in the file
> + ((eq data t)
> + (with-temp-buffer
> + (set-buffer-multibyte nil)
> + (insert-file-contents-literally tmp-file)
> + (buffer-string)))
> + ;; data is in data var
> + ((stringp data) data)
> + ;; No data
> + (t nil))))
> + (delete-file tmp-file)
> + result))
This should use unwind-protect and/or condition-case, to make sure the
temporary file is deleted even if the user presses C-g or some code
signals an error.
> +DEFUN ("w32--get-clipboard-data-media", Fw32__get_clipboard_data_media,
> + Sw32__get_clipboard_data_media, 2, 2, 0,
> + doc: /* Gets media (not plain text) clipboard data in one of the given formats.
> +FORMATS is the list of formats.
This should say something about what can be put into FORMATS.
Also, "a list", not "the list".
> +TEMP-FILE-IN is the name of the file to store the data, that must be
> +created by the callee, and also deleted if required.
> +The passed file may be used or not, as indicated by the return value:
Isn't it easier and clearer to say "if the function returns t"?
> +Returns nil it there is no such format, or something failed.
> +If it returns a string, then that is the data (not necessarily textual).
> +If it returns 't, then the file contains the data. */)
^^
t, not 't
> + (Lisp_Object formats, Lisp_Object temp_file_in)
> +{
> + CHECK_CONS (formats);
CHECK_CONS or CHECK_LIST?
> + char *temp_file = SSDATA (ENCODE_FILE (temp_file_in));
Every file name that we pass to C APIs must be run through
Fexpand_file_name, because each buffer in Emacs can have its own
default-directory.
> + if (strcmp (name, "DIBV5") == 0)
> + {
> + if (convert_dibv5_to_png (data, size, temp_file))
> + result = Qt;
> + }
> + else
> + result = make_unibyte_string (data, size);
Why a unibyte string? Is this always binary data or something? If
this could be text (e.g., text/html), then a unibyte string is not the
best choice.
In any case, if the function must return a unibyte string in some
cases, that should be mentioned in the doc string, because callers
will otherwise not expect to get a unibyte string. Also, where will
this unibyte string be decoded?
Finally, this needs some documentation: a NEWS item and some minimal
updates for the "Yanking Media" section of the ELisp manual (AFAICT,
it only needs to say that media can be yanked from the clipboard as
well as from a selection).
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.