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 #379 received at 76120 <at> debbugs.gnu.org (full text, mbox):
From: Alvaro Ramirez <alvaro <at> xenodium.com> To: stefankangas <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org> Cc: Rudolf Adamkovič <rudolf <at> adamkovic.org>, Juri Linkov <juri <at> linkov.net>, rms <at> gnu.org, 76120 <at> debbugs.gnu.org, Visuwesh <visuweshm <at> gmail.com> Subject: Re: bug#76120: This feature is not about "sharing", or a "native" anything. Date: Thu, 13 Mar 2025 13:46:39 +0000
Hi Eli and Stefan
Alvaro Ramirez <alvaro <at> xenodium.com> writes:
> Alvaro Ramirez <alvaro <at> xenodium.com> writes:
>
>> 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.
Anything else I should address in the patch
(0005-Add-Send-to-context-menu-item-to-mouse-el.patch)?
>>
>> 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.
>
> Sorry. Wrong screenshots before. Please ignore.
>
> Attaching send-gnu-linux.jpg and send-macos.jpg, which show the
> use of
> "Send to..." instead of "Share..." terminology.
>>
>> 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
>>
>> [2. text/x-patch;
>> 0005-Add-Send-to-context-menu-item-to-mouse-el.patch]...
>>
>> [3. image/jpeg; macos.jpg]...
>>
>> [4. image/jpeg; linux.jpg]...
>
> [2. image/jpeg; send-macos.jpg]...
>
> [3. image/jpeg; send-gnu-linux.jpg]...
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.