Subject: bug#76120: This feature is not about "sharing", or a "native" anything.

From: Alvaro Ramirez <[email-masked]>
Date: Thu, 13 Mar 2025 13:46:39 +0000

Hi Eli and Stefan

Alvaro Ramirez <[email-masked]> writes:

> Alvaro Ramirez <[email-masked]> 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@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]...