Alvaro Ramirez <alvaro@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.
>
> 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]...