Package: emacs;
Reported by: Álvaro Ramírez <alvaro <at> xenodium.com>
Date: Fri, 7 Feb 2025 15:00:02 UTC
Severity: wishlist
Tags: patch
Message #373 received at 76120 <at> debbugs.gnu.org (full text, mbox):
From: Alvaro Ramirez <alvaro <at> xenodium.com> To: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com, rms <at> gnu.org, Rudolf Adamkovič <rudolf <at> adamkovic.org>, Visuwesh <visuweshm <at> gmail.com>, Juri Linkov <juri <at> linkov.net> Cc: 76120 <at> debbugs.gnu.org Subject: Re: bug#76120: This feature is not about "sharing", or a "native" anything. Date: Tue, 04 Mar 2025 13:08:48 +0000
[Message part 1 (text/plain, inline)]
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
[0005-Add-Send-to-context-menu-item-to-mouse-el.patch (text/x-patch, attachment)]
[macos.jpg (image/jpeg, attachment)]
[linux.jpg (image/jpeg, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.