Package: emacs;
Reported by: Álvaro Ramírez <alvaro <at> xenodium.com>
Date: Fri, 7 Feb 2025 15:00:02 UTC
Severity: wishlist
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Visuwesh <visuweshm <at> gmail.com> To: Alvaro Ramirez <alvaro <at> xenodium.com> Cc: Rudolf Adamkovič <rudolf <at> adamkovic.org>, rms <at> gnu.org, Juri Linkov <juri <at> linkov.net>, 76120 <at> debbugs.gnu.org, stefankangas <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org> Subject: bug#76120: This feature is not about "sharing", or a "native" anything. Date: Sat, 15 Mar 2025 17:45:17 +0530
[செவ்வாய் மார்ச் 04, 2025] Alvaro Ramirez wrote:
Sorry, I was busy with $LIFE so couldn't get a chance to look at the
latest patch properly.
> Hi folks,
>
> Thanks for all the feedback. Attaching the latest iteration of the
> patch (0005-Add-Send-to-context-menu-item-to-mouse-el.patch), applying
> your
> suggestions.
>
> Patch highlights:
>
> - Introduces buffer-local `send-to-handlers' list var to break out
> concrete handler functionality per platform (also makes it more
> modular/pluggable).
> - The list also enables changing platform handler priority.
> - Now using shell-command-guess-open instead of xdg-open.
> - I'm deferring changes to direx-aux as discussed with Juri.
> - Fixes a bug in ns--share-items (native macOS menu displayed in the
> wrong location, in multi-window layouts).
> - Other tweaks as per feedback (Thank you Juri, Rudy, and Visuwesh).
>
> Re-attaching screenshots although there have been no UX changes
> applied in this patch.
>
> Please read on for relevant discussion of changes or refer to the
> patch itself.
>
> Alvaro
>
>>> +(require 'seq)
>> Visuwesh: This isn't required since seq is preloaded nowadays IIRC.
>
> Removed.
>
>>> +(defcustom send-to-context-items-function
>>> #'send-to--default-context-items
>>> + "A function to collect the items to be sent by `send-to'.
>>> +
>>> +Defaults to `send-to--default-context-items'.
>>> +
>>> +The function returns a list of items, where each item can be a
>>> either
>>> +a filename or plain text."
>>> + :type '(function :tag "Function")
>>> + :group 'send-to
>>> + :version "31.1")
>>
>> Visuwesh: We should document how a filename is distinguished from
>> plain text
>> item here again.
>
> Now documented in send-to-handlers.
>
>>> +(defcustom send-to-context-items-function
>>> #'send-to--default-context-items
>>> + "A function to collect the items to be sent by `send-to'.
>>> +
>>> +Defaults to `send-to--default-context-items'.
>>> +
>>> +The function returns a list of items, where each item can be a
>>> either
>>> +a filename or plain text."
>>> + :type '(function :tag "Function")
>>> + :group 'send-to
>>> + :version "31.1")
>>
>> Visuwesh: Also should this be buffer-local so different major-mode
>> can provide different providers?
>
> send-to-handlers is now buffer local.
>
>>> + (error "Don't know how to sende %s (adjust
>>> `send-to-handler-function')"
>>
>> Visuwesh: ^^^^^ Typo here!
>
> Fixed.
>
>>> + (cond ((derived-mode-p 'dired-mode)
>>> + (or
>>> + (send-to--dired-filenames-in-region)
>>> + (dired-get-marked-files)))
>>> + ((use-region-p)
>>> + (list (buffer-substring-no-properties
>>> + (region-beginning)
>>> + (region-end))))
>>> + ((buffer-file-name)
>>> + (list (buffer-file-name)))))
>>
>> Visuwesh: Why not add (thing-at-point 'existing-filename)?
>
> Added.
>
>>> Yup. I do this frequently too. The current implementation (using
>>> file-exists-p) should handle sending URLs (ie. https://gnu.org) as
>>> text.
>>
>> Visuwesh: I think this test might fail if someone uses
>> url-handler-mode.
>
> Check now for local files only.
>
>> Visuwesh: Thinking about it a bit more, the job of providing a
>> "items" function is
>> really the package/major-mode author's job. Perhaps, it would be
>> better
>> to have an abnormal hook or somesuch where the first non-nil item
>> returned would be sent.
>>
>> Having separate functions would make it easier for
>> package/major-mode
>> authors to provide glue code rather than having a single big
>> function
>> that does everything.
>
> I've added send-to-handlers buffer-local var, which makes things more
> modular/pluggable.
>
>>> +(defun send-to--default-support-checker-p ()
>>> + "Return non-nil for platforms supporting send capability."
>>> + (or (and (featurep 'ns) (fboundp 'ns-send-items))
>>> + (executable-find "xdg-open")))
>>> [...]
>>> + ((executable-find "xdg-open")
>>> + (when (y-or-n-p (format "Open externally: %s ?"
>>> + (send-to--format-items items)))
>>> + (dolist (item items)
>>> + (with-temp-buffer
>>> + (unless (zerop (call-process
>>> + "xdg-open" nil (current-buffer) t
>>> + (send-to--convert-item-to-filename
>>> + item)))
>>
>> Juri: No need to duplicate (executable-find "xdg-open")
>> since it already exists in shell-command-guess-open
>> used by shell-command-do-open.
>
> Now using shell-command-guess-open, but conditionally loading via:
>
> (unless (boundp 'shell-command-guess-open)
> (require 'dired-aux))
>
>>> Can we defer reorganizing dired-aux.el to a separate patch, please?
>>
>> Juri: Indeed, this would be a separate task.
>
> Until dired-aux bits are moved, I'm requiring conditionally (see
> above) as it caused issues (happy to take an alternate approach):
>
> alvaro <at> linux $ make
>
> ...
>
> Error: error ("Eager macro-expansion failure: (void-function
> easy-menu-define)")
> signal(error ("Eager macro-expansion failure: (void-function
> easy-menu-define)"))
> error("Eager macro-expansion failure: %S" (void-function
> easy-menu-define))
> (condition-case err (let ((macroexp--pending-eager-loads (cons
> load-file-name macroexp--pending-eager-loads))) (if full-p
> (macroexpand--all-toplevel form) (macroexpand form))) ((debug
> error) (error "Eager macro-expansion failure: %S" err) form))
> (cond ((eq 'skip (car macroexp--pending-eager-loads)) form) ((and
> load-file-name (member load-file-name
> macroexp--pending-eager-loads)) (let* ((bt (delq nil (mapcar
> #'macroexp--trim-backtrace-frame (macroexp--backtrace)))) (elem
> (list 'load (file-name-nondirectory load-file-name))) (tail (member
> elem (cdr (member elem bt))))) (if tail (setcdr tail (list '…)))
> (if (eq (car-safe (car bt)) 'macroexpand-all) (setq bt (cdr bt)))
> (if macroexp--debug-eager (debug 'eager-macroexp-cycle) (error
> "Eager macro-expansion skipped due to cycle:\n %s" (mapconcat
> #'prin1-to-string (nreverse bt) " => "))) (setq
> macroexp--pending-eager-loads (cons 'skip
> macroexp--pending-eager-loads)) form)) (t (condition-case err (let
> ((macroexp--pending-eager-loads (cons load-file-name
> macroexp--pending-eager-loads))) (if full-p
> (macroexpand--all-toplevel form) (macroexpand form))) ((debug
> error) (error "Eager macro-expansion failure: %S" err) form))))
> internal-macroexpand-for-load((eval-when-compile (require
> 'send-to)) nil)
> eval-buffer(#<buffer *load*> nil
> "/home/alvaro/Downloads/emacs/lisp/mouse.el" nil t)
>
> ...
>
>> Visuwesh: Sure. It could be equally well be a defvar.
>
> Switched to defvar-local.
>
>> Richard: It's good enough to use, and I don't have a better
>> suggestion. If we
>> come across better choice, we can switch to that.
>
> Sounds good. Using "send" as agreed.
>
>>> * lisp/send-to.el: New package implements sending to apps or
>>> services.
>> Rudy: The verb sending needs an object, i.e. sending what?
>
> Now says: "New package implements sending files/region."
>
>>> +(non-Emacs) apps or services. See send-to.el for customisations.
>>
>> Rudy: Two spaces should separate sentences.
>
> Done
>
>>> + "Send item (region, buffer file, or dired files) to app or
>>> service")))
>> Rudy: Dired is a proper noun and so should be capitalized.
>
> Done
>
>>> +(defgroup send-to nil
>>> + "Send files or text to external applications or services."
>>> + :group 'external
>>> + :version "31.1")
>>
>> Rudy: Inconsistent terminology: the patch says "apps" everywhere
>> else.
>>
>> (Personally, I am for saying "applications" everywhere.)
>
> Switched to "applications" everywhere.
>
>>> + (error "Don't know how to sende %s (adjust
>>> `send-to-handler-function')"
>> Rudy: A typo in the word send.
>
> Fixed
>
> From ef2debe64a9c909b40d669e779eacc581821e81b Mon Sep 17 00:00:00 2001
> From: Alvaro Ramirez <me <at> xenodium.com>
> From: xenodium <8107219+xenodium <at> users.noreply.github.com>
> Date: Thu, 13 Feb 2025 17:30:01 +0000
> Subject: [PATCH] Add "Send to..." context menu item to mouse.el
>
> * lisp/send-to.el: New package implements sending files/region.
>
> * lisp/mouse.el (context-menu-send-to): Add "Send to..." context menu.
>
> * lisp/term/ns-win.el (ns-send-items): Expose native macOS send API.
>
> * src/nsfns.m (ns-send-items): Implement native macOS sending.
>
> * etc/NEWS: Announce the new feature.
> ---
> etc/NEWS | 7 ++
> lisp/mouse.el | 17 +++-
> lisp/send-to.el | 233 ++++++++++++++++++++++++++++++++++++++++++++
> lisp/term/ns-win.el | 1 +
> src/nsfns.m | 54 ++++++++++
> 5 files changed, 311 insertions(+), 1 deletion(-)
> create mode 100644 lisp/send-to.el
>
> diff --git a/etc/NEWS b/etc/NEWS
> index dea24adb3c9..3b79d3c24d0 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -280,6 +280,13 @@ customize help text for tabs displayed on the tab-bar. Help text is
> normally shown in the echo area or via tooltips. See the variable's
> docstring for arguments passed to a help-text function.
>
> +** Mouse
> +
> +---
> +*** context-menu-mode now includes a "Send to..." menu item.
> +The menu item enables sending current file(s) or region text to external
> +(non-Emacs) apps or services. See send-to.el for customisations.
> +
> ** Project
>
> ---
> diff --git a/lisp/mouse.el b/lisp/mouse.el
> index 1f0ca6a51b6..0d74b1e5ca1 100644
> --- a/lisp/mouse.el
> +++ b/lisp/mouse.el
> @@ -30,6 +30,7 @@
> ;;; Code:
>
> (eval-when-compile (require 'rect))
> +(eval-when-compile (require 'send-to))
>
> ;; Indent track-mouse like progn.
> (put 'track-mouse 'lisp-indent-function 0)
> @@ -393,7 +394,8 @@ context-menu-functions
> context-menu-region
> context-menu-middle-separator
> context-menu-local
> - context-menu-minor)
> + context-menu-minor
> + context-menu-send-to)
> "List of functions that produce the contents of the context menu.
> Each function receives the menu and the mouse click event as its arguments
> and should return the same menu with changes such as added new menu items."
> @@ -536,6 +538,19 @@ context-menu-minor
> (cdr mode))))
> menu)
>
> +(defun context-menu-send-to (menu _click)
> + "Add a \"Send to...\" context MENU entry on supported platforms."
> + (run-hooks 'activate-menubar-hook 'menu-bar-update-hook)
> + (when (send-to-supported-p)
> + (define-key-after menu [separator-send] menu-bar-separator)
> + (define-key-after menu [send]
> + '(menu-item "Send to..." (lambda ()
> + (interactive)
> + (send-to))
> + :help
> + "Send item (region, buffer file, or Dired files) to app or service")))
> + menu)
Sorry for asking this so late in the game... should we allow the backend
itself to return a keymap for the context-menu? This would further
prompting/choosing "where to send" in some backends.
> (defun context-menu-buffers (menu _click)
> "Populate MENU with the buffer submenus to buffer switching."
> (run-hooks 'activate-menubar-hook 'menu-bar-update-hook)
> diff --git a/lisp/send-to.el b/lisp/send-to.el
> new file mode 100644
> index 00000000000..064f5ad6645
> --- /dev/null
> +++ b/lisp/send-to.el
> @@ -0,0 +1,233 @@
> [...]
> +(defvar-local send-to-handlers '(((:supported . send-to--ns-supported-p)
> + (:collect . send-to--collect-items)
> + (:send . send-to--ns-send-items))
> + ((:supported . send-to--open-externally-supported-p)
> + (:collect . send-to--collect-items)
> + (:send . send-to--open-externally)))
I wonder if coupling collect with other items here is a good idea. We
could have a separate var for collect functions akin to
context-menu-functions, that major-mode authors could modify.
BTW, seeing the list makes me wonder if using cl-defgeneric for
support&send functions would be more ergonomic for backend authors. To
select the right backend, we could have something like
xref-backend-functions.
> [...]
> +;;;###autoload
> +(defun send-to-supported-p ()
> + "Return non-nil for platforms where `send-to' is supported."
> + (funcall (map-elt (send-to--resolve-handler) :supported)))
Why not nth? I believe it should be a bit faster.
> [...]
> +(defun send-to--open-externally (items)
> + "Open ITEMS externally (using a non-Emacs application)."
> + (unless (boundp 'shell-command-guess-open)
> + (require 'dired-aux))
> + (when (y-or-n-p (format "Open externally: %s ?"
> + (send-to--format-items items)))
> + (dolist (item items)
> + (with-temp-buffer
> + (unless (zerop (call-process
> + shell-command-guess-open nil (current-buffer) t
> + (send-to--convert-item-to-filename
> + item)))
> + (error "%s" (string-trim (buffer-string))))))))
> +
> +(defun send-to--convert-item-to-filename (item)
> + "Convert ITEM to a filename.
> +
> +Unless ITEM is a verifiable filename, save its content to a file and
> +return its new timestamped filename."
> + (if (and (file-local-name item) ;; Ignore url-handler-mode
> + (file-exists-p item))
> + item
> + (let ((filename (concat temporary-file-directory
> + (format-time-string "%F_%H.%M.%S") ".txt")))
> + (with-temp-file filename
> + (insert item))
> + filename)))
Why this renaming? Wouldn't users want to open the original file
instead of a copy?
> +(defun send-to--collect-items ()
> + "Build a list of items to send based on default context.
> +
> +From a `dired' buffer, chosen items are based on either of these being
> +active:
> +
> + - Marked files
> + - Files in region.
> + - File at point.
> +
> +From any other buffer, either of these two, in order of preference:
> +
> + - Active region text.
> + - Thing at point (via `existing-filename').
> + - Buffer file."
> + (cond ((derived-mode-p 'dired-mode)
> + (or
> + (send-to--dired-filenames-in-region)
> + (dired-get-marked-files)))
> + ((use-region-p)
> + (list (buffer-substring-no-properties
> + (region-beginning)
> + (region-end))))
> + ((thing-at-point 'existing-filename)
> + (thing-at-point 'existing-filename))
> + ((buffer-file-name)
> + (list (buffer-file-name)))))
I must sound like a parrot: but could you please consider making
filenames into file: URLs? This would avoid the number of file-exists-p
checks, which is a worthy goal IMO. Remote files could be checked with
(file-remote-p default-directory).
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.