lisp/term/ns-win.el modification

lisp/term/ns-win.el modification

[Jean-Christophe Helary]

I've noticed that the system service provided by NS-Emacs (and Aquamacs) did not allow multiple strings to be selected for opening them in emacs since the function called (dnd-open-file) accepts only one argument.

A few days ago I've written a patch to the aquamacs/ns-win.el that was accepted today by David.

https://github.com/davidswelt/aquamacs-emacs/pull/128/files

Since the issue exists also in NS-Emacs I was wondering what could be done to have the patch (or an adaptation) applied to the original lisp/term/ns-win.el file.

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

Hi Jean-Christophe!

Thanks for submitting the patch!

First a bit of formalia:

* All authors of a patch much assign the copyright to FSF in order for it
to be included in Emacs. This mean that you can't include a random piece of
code from the internet (in this case the `trim-spaces' function).

* You must follow the coding standards. First, every function should be
prefixed with a module name, normally this corresponds the name of the name
of the file they are defined in. However, in this case, ns-win, use "ns-".
Secondly, there are hanging parentheses in the code, indentation isn't
correct, and there are lines longer than 79 characters.

Comments on the patch itself:

* I'm not exactly sure which problem you were trying to solve. I just
tested to select a number of files in the Finder on macOS (10.10.5) and
dragged then to Emacs (25.1), and they open just fine. Please include a
description in the source code for the situation your code handle.

* I miss a description, or link to a description, of the format of
`ns-input-spi-argument'. Without it it's hard to tell what the code is
trying to do, and whether or not it does it correctly.

* The code will be smaller, and more easy to read, if you would use
`dolist' instead of `while'. Also, if you do this change, I see no need to
break out the code into a separate function, as it will only be two or
three lines long.

* File names in macOS may start or end with spaces. It's probably not a
good idea to trim spaces. (Again, it's hard to tell without a description
of the format.)

Again, thanks for submitting the patch.

    -- Anders


On Thu, Apr 27, 2017 at 6:50 AM, Jean-Christophe Helary <
jean.christophe.helary@gmail.com> wrote:

> I've noticed that the system service provided by NS-Emacs (and Aquamacs)
> did not allow multiple strings to be selected for opening them in emacs
> since the function called (dnd-open-file) accepts only one argument.
>
> A few days ago I've written a patch to the aquamacs/ns-win.el that was
> accepted today by David.
>
> https://github.com/davidswelt/aquamacs-emacs/pull/128/files
>
> Since the issue exists also in NS-Emacs I was wondering what could be done
> to have the patch (or an adaptation) applied to the original
> lisp/term/ns-win.el file.
>
> Jean-Christophe
>

[-- Attachment #2: Type: text/html, Size: 3123 bytes --]

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 3960 bytes --]

Anders,

Thank you very much for the feedback.

> * All authors of a patch much assign the copyright to FSF in order for it to be included in Emacs. This mean that you can't include a random piece of code from the internet (in this case the `trim-spaces' function).

Ok, but such a trimming function is trivial and I don't see how my rewriting it to make it different from that code would add to it. What should I do?

> * You must follow the coding standards. First, every function should be prefixed with a module name, normally this corresponds the name of the name of the file they are defined in. However, in this case, ns-win, use "ns-". Secondly, there are hanging parentheses in the code, indentation isn't correct, and there are lines longer than 79 characters.

I'll do. Thank you very much.

> Comments on the patch itself:
> 
> * I'm not exactly sure which problem you were trying to solve. I just tested to select a number of files in the Finder on macOS (10.10.5) and dragged then to Emacs (25.1), and they open just fine. Please include a description in the source code for the situation your code handle.

Services are called from the Services menu, not by drag-and-dropping.

The "Open Selected File" service takes a string as its argument, tries to parse that as a path on the file system and if a file corresponds to that path opens it, otherwise creates a buffer with the file name as its name.

The service is basically used on text files, not in the Finder.

> * I miss a description, or link to a description, of the format of `ns-input-spi-argument'. Without it it's hard to tell what the code is trying to do, and whether or not it does it correctly.

You mean `ns_input_spi_arg', right? The line above (defvar ns-input-spi-name) refers to a `nsterm.m' file where it  seems to be defined the following way:

ns_input_spi_arg = build_string ([arg UTF8String]);

> * The code will be smaller, and more easy to read, if you would use `dolist' instead of `while'. Also, if you do this change, I see no need to break out the code into a separate function, as it will only be two or three lines long.

I don't want to modify the ns-spi-service-call block more than necessary.

> * File names in macOS may start or end with spaces. It's probably not a good idea to trim spaces. (Again, it's hard to tell without a description of the format.)

Yes, but we're not talking about file names but file paths in a text file here.
The problem with not trimming is that when a legit file is tripple-clicked, the whole line is selected, including trailing spaces. The way the service is currently implemented considers that spaces a belonging to the file name and since that file does not exist just opens a blank buffer.

So I guess I could add a test to check wether the trimmed file exist and if not add the selected white space until I get an existing file (but then we'd hit cases where 2 files ending with different sorts of white space could be confused for each other).

> Again, thanks for submitting the patch.

Thank me when I have something that's acceptable :)

Jean-Christophe

>     -- Anders
> 
> 
> On Thu, Apr 27, 2017 at 6:50 AM, Jean-Christophe Helary <jean.christophe.helary@gmail.com <mailto:jean.christophe.helary@gmail.com>> wrote:
> I've noticed that the system service provided by NS-Emacs (and Aquamacs) did not allow multiple strings to be selected for opening them in emacs since the function called (dnd-open-file) accepts only one argument.
> 
> A few days ago I've written a patch to the aquamacs/ns-win.el that was accepted today by David.
> 
> https://github.com/davidswelt/aquamacs-emacs/pull/128/files <https://github.com/davidswelt/aquamacs-emacs/pull/128/files>
> 
> Since the issue exists also in NS-Emacs I was wondering what could be done to have the patch (or an adaptation) applied to the original lisp/term/ns-win.el file.
> 
> Jean-Christophe
> 


[-- Attachment #2: Type: text/html, Size: 6172 bytes --]

Re: lisp/term/ns-win.el modification

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 3879 bytes --]

Hi!

On Thu, Apr 27, 2017 at 12:13 PM, Jean-Christophe Helary <
jean.christophe.helary@gmail.com> wrote:

> * All authors of a patch much assign the copyright to FSF in order for it
> to be included in Emacs. This mean that you can't include a random piece of
> code from the internet (in this case the `trim-spaces' function).
>
>
> Ok, but such a trimming function is trivial and I don't see how my
> rewriting it to make it different from that code would add to it. What
> should I do?
>

Since it's small you could inline it and drop the comment. You could try to
rewrite the code into containing only one match and one replace.


> Comments on the patch itself:
>
> * I'm not exactly sure which problem you were trying to solve. I just
> tested to select a number of files in the Finder on macOS (10.10.5) and
> dragged then to Emacs (25.1), and they open just fine. Please include a
> description in the source code for the situation your code handle.
>
>
> Services are called from the Services menu, not by drag-and-dropping.
>
> The "Open Selected File" service takes a string as its argument, tries to
> parse that as a path on the file system and if a file corresponds to that
> path opens it, otherwise creates a buffer with the file name as its name.
>
> The service is basically used on text files, not in the Finder.
>

 Aha, I have't used Services that much. Can you give me a step-by-step
description on how to use them, where the current system doesn't work.
(Concretely, is this I do from within Emacs, from Finder, or from some
other program?)


> * I miss a description, or link to a description, of the format of
> `ns-input-spi-argument'. Without it it's hard to tell what the code is
> trying to do, and whether or not it does it correctly.
>
>
> You mean `ns_input_spi_arg', right? The line above (defvar
> ns-input-spi-name) refers to a `nsterm.m' file where it  seems to be
> defined the following way:
>
> ns_input_spi_arg = build_string ([arg UTF8String]);
>

I mean what the string contains. Your code splits it on certain characters:
"[\f\t\n\r\v]+". It is always good to be able to go to some documentation,
to verify that these really are the characters that delimiter file names.
However, if the content is an arbitrary text file, then that should be
mentioned.


> * The code will be smaller, and more easy to read, if you would use
> `dolist' instead of `while'. Also, if you do this change, I see no need to
> break out the code into a separate function, as it will only be two or
> three lines long.
>
>
> I don't want to modify the ns-spi-service-call block more than necessary.
>

If all you do is to place the call inside a dolist, you should be ok.
Introducing additional functions (only used in one location) isn't 100%
clean either.


> * File names in macOS may start or end with spaces. It's probably not a
> good idea to trim spaces. (Again, it's hard to tell without a description
> of the format.)
>
>
> Yes, but we're not talking about file names but file paths in a text file
> here.
> The problem with not trimming is that when a legit file is
> tripple-clicked, the whole line is selected, including trailing spaces. The
> way the service is currently implemented considers that spaces a belonging
> to the file name and since that file does not exist just opens a blank
> buffer.
>

Trimming newline should obviously be done. How often does files contain
trailing whitespace? (I guess initial whitespace is more common.) Anyway,
once I have a concrete example to test, it will hopefully clear up for me.


So I guess I could add a test to check wether the trimmed file exist and if
> not add the selected white space until I get an existing file (but then
> we'd hit cases where 2 files ending with different sorts of white space
> could be confused for each other).
>

No, that sounds like overdoing things...

   -- Anders

[-- Attachment #2: Type: text/html, Size: 6676 bytes --]

Re: lisp/term/ns-win.el modification

[Noam Postavsky]

On Thu, Apr 27, 2017 at 6:13 AM, Jean-Christophe Helary
<jean.christophe.helary@gmail.com> wrote:
>
> * All authors of a patch much assign the copyright to FSF in order for it to
> be included in Emacs. This mean that you can't include a random piece of
> code from the internet (in this case the `trim-spaces' function).
>
>
> Ok, but such a trimming function is trivial and I don't see how my rewriting
> it to make it different from that code would add to it. What should I do?

There is a `string-trim' function in subr-x.

Re: lisp/term/ns-win.el modification

[mituharu]

>> * File names in macOS may start or end with spaces. It's probably not a
>> good idea to trim spaces. (Again, it's hard to tell without a
>> description of the format.)
>
> Yes, but we're not talking about file names but file paths in a text file
> here.
> The problem with not trimming is that when a legit file is
> tripple-clicked, the whole line is selected, including trailing spaces.
> The way the service is currently implemented considers that spaces a
> belonging to the file name and since that file does not exist just opens a
> blank buffer.
>
> So I guess I could add a test to check wether the trimmed file exist and
> if not add the selected white space until I get an existing file (but then
> we'd hit cases where 2 files ending with different sorts of white space
> could be confused for each other).

For more consistent behavior with other apps on macOS, you can
let the system parse the selected text to get (possibly multiple)
filenames.  Just inspect the service pasteboard with
NSFilenamesPboardType instead of NSStringPboardType.
Alternatively, creating NSURL objects for pasteboard items would
also work.  (I'm not sure if this works on GNUstep.)

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/PasteboardGuide106/Articles/pbReading.html#//apple_ref/doc/uid/TP40008123-SW5

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 27, 2017, at 20:30, Anders Lindgren <andlind@gmail.com> wrote:
> 
> 
>> Ok, but such a trimming function is trivial and I don't see how my rewriting it to make it different from that code would add to it. What should I do?

> Since it's small you could inline it and drop the comment. You could try to rewrite the code into containing only one match and one replace.

Ok, I'll do.
 
>> Aha, I have't used Services that much. Can you give me a step-by-step description on how to use them, where the current system doesn't work. (Concretely, is this I do from within Emacs, from Finder, or from some other program?)

1) Select a path in any application where text can be selected
2) Go to the application menu and select Services > Open Selected File
3) The file should open in Emacs.app if it exists. If it doesn't, a buffer named after the name part of the path is opened.

Problem:

a) if the selected string is not trimmed (for ex you triple-clicked the line and it contained leading or trailing ws), the file won't open because the ws is considered integral part of the path

b) if the selection contains more than 1 line (for ex. a list of paths on multiple lines), no file will open since the whole string is considered the path

What I wrote solves a) and b) by splitting the selected string (except on spaces), by trimming the resulting paths, and by looping the original function on the thus created list of paths.
 
> I mean what the string contains. Your code splits it on certain characters: "[\f\t\n\r\v]+". It is always good to be able to go to some documentation, to verify that these really are the characters that delimiter file names. However, if the content is an arbitrary text file, then that should be mentioned.

The content is an arbitrary string selected in any application that supports services. I've removed \s from the delimiters *because* spaces can be part of a path on Mac.

> I don't want to modify the ns-spi-service-call block more than necessary.
> 
> If all you do is to place the call inside a dolist, you should be ok. Introducing additional functions (only used in one location) isn't 100% clean either.

Ok, that's something I'll try to do. Thank you.

> Trimming newline should obviously be done. How often does files contain trailing whitespace? (I guess initial whitespace is more common.) Anyway, once I have a concrete example to test, it will hopefully clear up for me.

Create an absolute path like /User/path/to/file or a relative path like ~/path/to/file, select them and call the service on the selection.

The only issue that I see is when a file name *ends* with (normal) spaces, which is unlikely to happen, even if possible.

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 27, 2017, at 21:24, Noam Postavsky <npostavs@users.sourceforge.net> wrote:

>> Ok, but such a trimming function is trivial and I don't see how my rewriting
>> it to make it different from that code would add to it. What should I do?
> 
> There is a `string-trim' function in subr-x.

Thank you very much. I'll check.

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 27, 2017, at 21:51, mituharu@math.s.chiba-u.ac.jp wrote:

> For more consistent behavior with other apps on macOS, you can
> let the system parse the selected text to get (possibly multiple)
> filenames.  Just inspect the service pasteboard with
> NSFilenamesPboardType instead of NSStringPboardType.
> Alternatively, creating NSURL objects for pasteboard items would
> also work.  (I'm not sure if this works on GNUstep.)
> 
> https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/PasteboardGuide106/Articles/pbReading.html#//apple_ref/doc/uid/TP40008123-SW5

But that's not code that would fit in ns-win.el would it?

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Davis Herring]

>> I mean what the string contains. Your code splits it on certain characters: "[\f\t\n\r\v]+". It is always good to be able to go to some documentation, to verify that these really are the characters that delimiter file names. However, if the content is an arbitrary text file, then that should be mentioned.
>
> The content is an arbitrary string selected in any application that supports services. I've removed \s from the delimiters *because* spaces can be part of a path on Mac.

All those characters could appear as well: macOS is Unix, after all, and 
so supports anything except NUL (and reserves / as a directory 
separator, although in some interfaces / and : are interchanged).  That 
said, of course spaces are much more common in names, but it's good to 
remember that this is a human factors decision, not a technical one 
based on OS rules.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or 
too sparse, it is because mass-energy conversion has occurred during 
shipping.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 28, 2017, at 0:09, Davis Herring <herring@lanl.gov> wrote:
> 
>>> I mean what the string contains. Your code splits it on certain characters: "[\f\t\n\r\v]+". It is always good to be able to go to some documentation, to verify that these really are the characters that delimiter file names. However, if the content is an arbitrary text file, then that should be mentioned.
>> 
>> The content is an arbitrary string selected in any application that supports services. I've removed \s from the delimiters *because* spaces can be part of a path on Mac.
> 
> All those characters could appear as well: macOS is Unix

They probably could, but how realistic do you think it is to have a file path that *includes* a page break, a carriage return, a tabulation, a new line, or a vertical tab?

> , after all, and so supports anything except NUL (and reserves / as a directory separator, although in some interfaces / and : are interchanged).  That said, of course spaces are much more common in names, but it's good to remember that this is a human factors decision, not a technical one based on OS rules.

I understand.

The current use case is the following:

The user selects lines that all include 1 path on each line, and depending on the way the lines are selected (triple-clicking or selection with the keyboard) some leading and trailing white spaces can be included.

So we are not really talking about a very general case of any possible path that's possible under Unix. I think it is reasonable to make assumptions about what the user *sees* and what the user considers a path to be selected and opened with that service.

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 27, 2017, at 21:51, mituharu@math.s.chiba-u.ac.jp wrote:

> For more consistent behavior with other apps on macOS, you can
> let the system parse the selected text to get (possibly multiple)
> filenames.  Just inspect the service pasteboard with
> NSFilenamesPboardType instead of NSStringPboardType.

I've just checked the clipboard contents when I select a path in a text file and I only get NSStringPboardType. NSFilenamesPboardType is only available when I select a file in the Finder, which is not the use case here.

Or maybe I've misunderstood what you wrote.

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[mituharu]

>> For more consistent behavior with other apps on macOS, you can
>> let the system parse the selected text to get (possibly multiple)
>> filenames.  Just inspect the service pasteboard with
>> NSFilenamesPboardType instead of NSStringPboardType.
>
> I've just checked the clipboard contents when I select a path in a text
> file and I only get NSStringPboardType. NSFilenamesPboardType is only
> available when I select a file in the Finder, which is not the use case
> here.
>
> Or maybe I've misunderstood what you wrote.

If "the clipboard" means NSGeneralPboard, that's not what I meant
by "the service pasteboard".  It's the one passed to the service
method as the first arg.

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/SysServices/Articles/providing.html#//apple_ref/doc/uid/20000853-98262

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

Thank you.

I'm not sure how I could do that but I'll investigate.
In the meanwhile I'll just try to improve my patch based on the other elisp related comments.

Jean-Christophe

> On Apr 28, 2017, at 3:28, mituharu@math.s.chiba-u.ac.jp wrote:
> 
>>> For more consistent behavior with other apps on macOS, you can
>>> let the system parse the selected text to get (possibly multiple)
>>> filenames.  Just inspect the service pasteboard with
>>> NSFilenamesPboardType instead of NSStringPboardType.
>> 
>> I've just checked the clipboard contents when I select a path in a text
>> file and I only get NSStringPboardType. NSFilenamesPboardType is only
>> available when I select a file in the Finder, which is not the use case
>> here.
>> 
>> Or maybe I've misunderstood what you wrote.
> 
> If "the clipboard" means NSGeneralPboard, that's not what I meant
> by "the service pasteboard".  It's the one passed to the service
> method as the first arg.
> 
> https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/SysServices/Articles/providing.html#//apple_ref/doc/uid/20000853-98262

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 28, 2017, at 0:09, Davis Herring <herring@lanl.gov> wrote:
> 
>>> I mean what the string contains. Your code splits it on certain characters: "[\f\t\n\r\v]+". It is always good to be able to go to some documentation, to verify that these really are the characters that delimiter file names. However, if the content is an arbitrary text file, then that should be mentioned.
>> 
>> The content is an arbitrary string selected in any application that supports services. I've removed \s from the delimiters *because* spaces can be part of a path on Mac.
> 
> All those characters could appear as well: macOS is Unix, after all, and so supports anything except NUL (and reserves / as a directory separator, although in some interfaces / and : are interchanged).  That said, of course spaces are much more common in names, but it's good to remember that this is a human factors decision, not a technical one based on OS rules.

After a night of sleep, I guess I could split only on \n and \r since they are the one that the user will use as a visual cue to select paths put on multiple lines. Would that be satisfying ?

Jean-Christophe

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

Thank you everybody for the advice. Following yesterday's discussion, I have modified the patch the following way:

1) The code from ergomacs was removed and replaced with the string-trim function from subr-x
2) The splitting on \f, \t, \v was removed (I've kept the splitting on \n and \r since they are "natural" splitters when selecting multiple lines)
3) The ns- prefix was added to the function
4) while was replaced by dolist

Regarding 3) and Anders' comment "If all you do is to place the call inside a dolist, you should be ok. Introducing additional functions (only used in one location) isn't 100% clean either." I'm not sure I want to change the cond block because it is very elegant as it is. Please advise.

I have yet to investigate how to implement Mitsuharu's suggestion but it seems to be involving Objective-C programming which I am not sure I can handle.

The new patch is now waiting to be considered for a merge in Aquamacs and is available here:

https://github.com/davidswelt/aquamacs-emacs/pull/129/files

Jean-Christophe

> On Apr 28, 2017, at 8:32, Jean-Christophe Helary <jean.christophe.helary@gmail.com> wrote:
> 
>> 
>> On Apr 28, 2017, at 0:09, Davis Herring <herring-YOWKrPYUwWM@public.gmane.org> wrote:
>> 
>>>> I mean what the string contains. Your code splits it on certain characters: "[\f\t\n\r\v]+". It is always good to be able to go to some documentation, to verify that these really are the characters that delimiter file names. However, if the content is an arbitrary text file, then that should be mentioned.
>>> 
>>> The content is an arbitrary string selected in any application that supports services. I've removed \s from the delimiters *because* spaces can be part of a path on Mac.
>> 
>> All those characters could appear as well: macOS is Unix, after all, and so supports anything except NUL (and reserves / as a directory separator, although in some interfaces / and : are interchanged).  That said, of course spaces are much more common in names, but it's good to remember that this is a human factors decision, not a technical one based on OS rules.
> 
> After a night of sleep, I guess I could split only on \n and \r since they are the one that the user will use as a visual cue to select paths put on multiple lines. Would that be satisfying ?
> 
> Jean-Christophe

-- 
You received this message because you are subscribed to the Google Groups "aquamacs-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to aquamacs-devel+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org

Re: lisp/term/ns-win.el modification

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hi!

Finally, I start to understand what you are trying to do. Here is a recipe
on how to enable it:

- In System Preferences > Keyboard > Shortcuts > Services, select Text >
Open Selected File

- Restart Emacs. (I didn't, which caused Aquamacs to be launched)

I did test your patch from within gmail in Chrome. One this that I realized
was that when typing a space, it sometimes came out as a plain space,
sometimes as a non-breakable space. I'm not sure this is specific to the
edit field in gmail, but I think it would ba good idea to trim both kind of
spaces.


Regarding 3) and Anders' comment "If all you do is to place the call inside
> a dolist, you should be ok. Introducing additional functions (only used in
> one location) isn't 100% clean either." I'm not sure I want to change the
> cond block because it is very elegant as it is. Please advise.
>

I'm find with how the code looks now. It's less cluttered than when two
functions were used.

However, I'm not 100% comfortable to use `subr-x' since it is somewhat
experimental (e.g. the functions in the file is not documented in the Emacs
lisp manual). Besides, it doesn't trim non-breakable spaces. You could use
something like the following:

(save-match-data
  (string-match "\\`[ \u00A0]*\\(.*?\\)[ \u00A0]*\\'" path-string)
  (match-string 1 path-string))

Some small things:

- When sending a patch, it's better to do it against the latest Emacs
archive, rather than against the Aquamacs archive. In this case, I had to
manually edit the files to apply the patch. Also, for future reference,
it's better to attach it than to supply a link.

- In a doc string, the first line should be a full sentence with a summary
of what the function does. The reason is that when using "apropos", only
the first line of the doc string is shown.

    -- Anders

[-- Attachment #2: Type: text/html, Size: 2973 bytes --]

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

Thank you Anders,

> However, I'm not 100% comfortable to use `subr-x' since it is somewhat experimental (e.g. the functions in the file is not documented in the Emacs lisp manual). Besides, it doesn't trim non-breakable spaces. You could use something like the following:

Regarding subr-x, maybe it would be good to modify it so that it trims non-breakable spaces too. Also, not using it is a guaranty that it will stay experimental forever...

What is the condition to move something from experimental to "production" ?

> - When sending a patch, it's better to do it against the latest Emacs archive, rather than against the Aquamacs archive.

You are correct, of course, and my original intent was actually to ask about that procedure :)

> In this case, I had to manually edit the files to apply the patch. Also, for future reference, it's better to attach it than to supply a link.

I will.

> - In a doc string, the first line should be a full sentence with a summary of what the function does. The reason is that when using "apropos", only the first line of the doc string is shown.

Ok. I'll change that.

Thank you again for the comments, hopefully the third version will be the good one !

Jean-Christophe 

-- 
You received this message because you are subscribed to the Google Groups "aquamacs-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to aquamacs-devel+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org

Re: lisp/term/ns-win.el modification

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

Hi!


> Regarding subr-x, maybe it would be good to modify it so that it trims
> non-breakable spaces too. Also, not using it is a guaranty that it will
> stay experimental forever...
>

Yes, that sounds like a good idea. You can supply a feature request for
that, this thread is not the right place for it, as most people ignore
NS-related discussions.


> What is the condition to move something from experimental to "production" ?
>

No idea, really. As far as I know "subr-x.el" is the only file with that
status.

Also, there is another reason why I'am reluctant to use it. If ns-win.el
requires it, packages written on a mac may use functions from it without
realising it. Those packages would fail when used on other platforms.  (The
fix is easy, just add a `(require surb-x)', but it's easy to overlook it.)

Thank you again for the comments, hopefully the third version will be the
> good one !
>

We're not in a hurry. It's quite normal for a patch to go through a number
of iterations. It's better to make sure a patch is in a good state before
applying it than to fix problems afterwards.

Before a patch is accepted, you have to assign copyright of it (or,
preferably, all your work on Emacs) to FSF. If you haven't done so yet, you
can ask for papers to sign from assign@gnu.org and ask for papers to assign
copyright. First you they will ask a few basic questions, then they will
send you papers to sign and mail and fax, if I remember correctly.

    -- Anders

[-- Attachment #2: Type: text/html, Size: 2494 bytes --]

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 29, 2017, at 21:24, Anders Lindgren <andlind@gmail.com> wrote:

> However, I'm not 100% comfortable to use `subr-x' since it is somewhat experimental (e.g. the functions in the file is not documented in the Emacs lisp manual).

I've checked the emacs sources and there are currently 22 files that require it.

> Besides, it doesn't trim non-breakable spaces.

That's a pity. Is there a reason why non-breakable spaces would not be considered for trimming purposes?

Jean-Christophe

Re: lisp/term/ns-win.el modification

[Noam Postavsky]

On Sun, Apr 30, 2017 at 1:36 AM, Anders Lindgren <andlind@gmail.com> wrote:
>
> No idea, really. As far as I know "subr-x.el" is the only file with that
> status.
>
> Also, there is another reason why I'am reluctant to use it. If ns-win.el
> requires it, packages written on a mac may use functions from it without
> realising it. Those packages would fail when used on other platforms.

All the definitions in subr-x are defmacro or defsubst, so you can
(eval-when-compile (require 'subr-x)) which means that it would not be
loaded at runtime, and then other packages would not be able to use it
without requiring it.

(I don't know what the right answer for the non-breakable space issue is though)

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Sun, 30 Apr 2017 15:41:45 +0900
> 
> > Besides, it doesn't trim non-breakable spaces.
> 
> That's a pity. Is there a reason why non-breakable spaces would not be considered for trimming purposes?

What's the rationale for adding NBSP?  Also, is it only the NBSP
character that's missing, or there are others?

Re: lisp/term/ns-win.el modification

[Clément Pit-Claudel]

On 2017-04-30 10:12, Eli Zaretskii wrote:
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> Date: Sun, 30 Apr 2017 15:41:45 +0900
>>
>>> Besides, it doesn't trim non-breakable spaces.
>>
>> That's a pity. Is there a reason why non-breakable spaces would not be considered for trimming purposes?
> 
> What's the rationale for adding NBSP?  Also, is it only the NBSP
> character that's missing, or there are others?

Possibly for consistency with other languages.  Here's what Python 3 does:

import sys
import unicodedata

for codepoint in range(sys.maxunicode + 1):
    c = chr(codepoint)
    if c.strip() == "":
        name = unicodedata.name(c, "U+{:04X}".format(codepoint))
        print("Python 3 trims {} ({})".format(name, unicodedata.category(c)))


Python 3 trims U+0009 (Cc)
Python 3 trims U+000A (Cc)
Python 3 trims U+000B (Cc)
Python 3 trims U+000C (Cc)
Python 3 trims U+000D (Cc)
Python 3 trims U+001C (Cc)
Python 3 trims U+001D (Cc)
Python 3 trims U+001E (Cc)
Python 3 trims U+001F (Cc)
Python 3 trims SPACE (Zs)
Python 3 trims U+0085 (Cc)
Python 3 trims NO-BREAK SPACE (Zs)
Python 3 trims OGHAM SPACE MARK (Zs)
Python 3 trims EN QUAD (Zs)
Python 3 trims EM QUAD (Zs)
Python 3 trims EN SPACE (Zs)
Python 3 trims EM SPACE (Zs)
Python 3 trims THREE-PER-EM SPACE (Zs)
Python 3 trims FOUR-PER-EM SPACE (Zs)
Python 3 trims SIX-PER-EM SPACE (Zs)
Python 3 trims FIGURE SPACE (Zs)
Python 3 trims PUNCTUATION SPACE (Zs)
Python 3 trims THIN SPACE (Zs)
Python 3 trims HAIR SPACE (Zs)
Python 3 trims LINE SEPARATOR (Zl)
Python 3 trims PARAGRAPH SEPARATOR (Zp)
Python 3 trims NARROW NO-BREAK SPACE (Zs)
Python 3 trims MEDIUM MATHEMATICAL SPACE (Zs)
Python 3 trims IDEOGRAPHIC SPACE (Zs)

Clément.

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Sun, 30 Apr 2017 11:06:08 -0400
> 
> > What's the rationale for adding NBSP?  Also, is it only the NBSP
> > character that's missing, or there are others?
> 
> Possibly for consistency with other languages.  Here's what Python 3 does:

Thanks, but I asked about the rationale in the particular context of
the proposed change.  Sorry if that was unclear.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On Apr 30, 2017, at 23:12, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>>> Besides, it doesn't trim non-breakable spaces.
>> 
>> That's a pity. Is there a reason why non-breakable spaces would not be considered for trimming purposes?
> 
> What's the rationale for adding NBSP?  Also, is it only the NBSP
> character that's missing, or there are others?

It depends on what the definition of "trimming a string" is.

The regex used in subr-x is:  "\\`[ \t\n\r]+"

If we use the Unicode definition of white space, we need to add a lot more strings:
https://en.wikipedia.org/wiki/Whitespace_character#Unicode

Jean-Christophe

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On May 1, 2017, at 0:24, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> Date: Sun, 30 Apr 2017 11:06:08 -0400
>> 
>>> What's the rationale for adding NBSP?  Also, is it only the NBSP
>>> character that's missing, or there are others?
>> 
>> Possibly for consistency with other languages.  Here's what Python 3 does:
> 
> Thanks, but I asked about the rationale in the particular context of
> the proposed change.  Sorry if that was unclear.

It looks like Anders found that in some cases a a non breakable space could be added to a string in Gmail:

"I did test your patch from within gmail in Chrome. One this that I realized was that when typing a space, it sometimes came out as a plain space, sometimes as a non-breakable space. I'm not sure this is specific to the edit field in gmail, but I think it would ba good idea to trim both kind of spaces."

That's the rationale.

But as I pointed out earlier, it would also be nice to have a clear definition of what "trimming" a string means. It is means remove the ws at the beginning and end of the string then the Unicode characters for ws should be used.

Jean-Christophe

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Mon, 1 May 2017 07:23:55 +0900
> 
> > What's the rationale for adding NBSP?  Also, is it only the NBSP
> > character that's missing, or there are others?
> 
> It depends on what the definition of "trimming a string" is.

No, I was asking about this particular context.  You are trimming
whitespace from file names, is that right?  Can NBSP appear in file
names on macOS?  Can it be appended/prepended to file names by some
other software, without being part of the file names, in the usage
scenario you want to support?

> If we use the Unicode definition of white space, we need to add a lot more strings:
> https://en.wikipedia.org/wiki/Whitespace_character#Unicode

Emacs's [:blank:] already supports the full Unicode definition, so
that's not the issue.  The issue is what is TRT in this particular
context.  I don't use macOS X, so I simply don't know the answers to
those questions, and I'm asking you and those who do know to please
provide enough background for us to make the right decision in this
case.

Thanks.

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Mon, 1 May 2017 07:28:04 +0900
> 
> It looks like Anders found that in some cases a a non breakable space could be added to a string in Gmail:
> 
> "I did test your patch from within gmail in Chrome. One this that I realized was that when typing a space, it sometimes came out as a plain space, sometimes as a non-breakable space. I'm not sure this is specific to the edit field in gmail, but I think it would ba good idea to trim both kind of spaces."
> 
> That's the rationale.

What about other whitespace characters? can they also be added in
these scenarios?

> But as I pointed out earlier, it would also be nice to have a clear definition of what "trimming" a string means. It is means remove the ws at the beginning and end of the string then the Unicode characters for ws should be used.

But if these characters can appear in the string you want to have,
after "trimming", then trimming them wouldn't be correct, would it?
That's why I'm asking these questions.  We shouldn't remove anything
that could be legitimately part of the payload, we should only remove
stuff that might be prepended/appended to the payload by the software
involved in the transaction.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On May 1, 2017, at 15:33, Eli Zaretskii <eliz@gnu.org> wrote:
>> It looks like Anders found that in some cases a a non breakable space could be added to a string in Gmail:
>> 
>> "I did test your patch from within gmail in Chrome. One this that I realized was that when typing a space, it sometimes came out as a plain space, sometimes as a non-breakable space. I'm not sure this is specific to the edit field in gmail, but I think it would ba good idea to trim both kind of spaces."

> What about other whitespace characters? can they also be added in
> these scenarios?

The only scenario that we have is the user who types and selects a string, or selects an already existing string and calls that NS function on it.

>> But as I pointed out earlier, it would also be nice to have a clear definition of what "trimming" a string means. It is means remove the ws at the beginning and end of the string then the Unicode characters for ws should be used.
> 
> But if these characters can appear in the string you want to have,
> after "trimming", then trimming them wouldn't be correct, would it?
> That's why I'm asking these questions.  We shouldn't remove anything
> that could be legitimately part of the payload, we should only remove
> stuff that might be prepended/appended to the payload by the software
> involved in the transaction.

I understand, and that was part of the discussion we had with Anders at the beginning of the thread.

The function only understand paths that start from / or from ~, so the path can't start with ws. As for paths ending with ws, that's a very marginal case. Currently, in a triple-click all the ws ending the string is selected and the function will thus fail to open the selected path. It should be possible to test incrementally whether the trimmed path points to a file, or if the path ending with the first ws does, etc until the end of the trailing ws list, but that's not practical.

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Mon, 1 May 2017 17:23:19 +0900
> 
> The function only understand paths that start from / or from ~, so the path can't start with ws. As for paths ending with ws, that's a very marginal case. Currently, in a triple-click all the ws ending the string is selected and the function will thus fail to open the selected path. It should be possible to test incrementally whether the trimmed path points to a file, or if the path ending with the first ws does, etc until the end of the trailing ws list, but that's not practical.

OK, thanks.

Then I'd suggest to use split-string, which can trim any regexp
specified by its 4th argument.  That way, you can include NBSP in the
characters to trim.  I don't think we should include any additional
characters that have the Zs Unicode category need to be trimmed, as I
believe they are unlikely to be encountered in this scenario.  Perhaps
a comment to that effect is in order.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On May 1, 2017, at 17:59, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> Then I'd suggest to use split-string, which can trim any regexp
> specified by its 4th argument.

Ok, I'll do that. Thank you.

On a side note, what's the point of having string-trim in subr-x.el when a more powerful solution is provided in subr.el?

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Mon, 1 May 2017 19:53:50 +0900
> 
> On a side note, what's the point of having string-trim in subr-x.el when a more powerful solution is provided in subr.el?

Good question.

Perhaps string-trim could be extended to allow an optional argument
that would replace the built-in default regexp.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]


> On May 1, 2017, at 17:59, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> Date: Mon, 1 May 2017 17:23:19 +0900
>> 
>> The function only understand paths that start from / or from ~, so the path can't start with ws. As for paths ending with ws, that's a very marginal case. Currently, in a triple-click all the ws ending the string is selected and the function will thus fail to open the selected path. It should be possible to test incrementally whether the trimmed path points to a file, or if the path ending with the first ws does, etc until the end of the trailing ws list, but that's not practical.
> 
> OK, thanks.
> 
> Then I'd suggest to use split-string, which can trim any regexp
> specified by its 4th argument.  That way, you can include NBSP in the
> characters to trim.  I don't think we should include any additional
> characters that have the Zs Unicode category need to be trimmed, as I
> believe they are unlikely to be encountered in this scenario.  Perhaps
> a comment to that effect is in order.

Ok, I have something that works here. I'm attaching a diff. Let me know if that is satisfactory.

Jean-Christophe 


[-- Attachment #2: ns-win.el.diff --]
[-- Type: application/octet-stream, Size: 1280 bytes --]

diff --git a/lisp/term/ns-win.el b/lisp/term/ns-win.el
index 70bd817d93..d0a95dd701 100644
--- a/lisp/term/ns-win.el
+++ b/lisp/term/ns-win.el
@@ -229,6 +229,19 @@ The properties returned may include `top', `left', `height', and `width'."
 
 (declare-function dnd-open-file "dnd" (uri action))
 
+;; Handles multi line strings that are passed to the "open-file" service.
+(defun ns-open-file-service (filepaths)
+  "Opens multiple files at once when a multiline string is selected."
+  (let ((path_list (split-string filepaths "[\n\r]+")))
+    (dolist (path_string path_list)
+      (if (not (equal "" path_string))
+	  (dnd-open-file
+           ;; The path string is timmed for spaces, nbsp and tabs.
+           (car (split-string path_string nil nil  "[ \u00A0\t]+")) nil)))))
+
+
+
+
 (defun ns-spi-service-call ()
   "Respond to a service request."
   (interactive)
@@ -236,7 +249,7 @@ The properties returned may include `top', `left', `height', and `width'."
 	 (switch-to-buffer (generate-new-buffer "*untitled*"))
 	 (insert ns-input-spi-arg))
 	((string-equal ns-input-spi-name "open-file")
-	 (dnd-open-file ns-input-spi-arg nil))
+	 (ns-open-file-service ns-input-spi-arg))
 	((string-equal ns-input-spi-name "mail-selection")
 	 (compose-mail)
 	 (rfc822-goto-eoh)

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Tue, 2 May 2017 00:12:41 +0900
> 
> Ok, I have something that works here. I'm attaching a diff. Let me know if that is satisfactory.

Thanks.  A few nits:

> +(defun ns-open-file-service (filepaths)
> +  "Opens multiple files at once when a multiline string is selected."

"Open", not "Opens", to be consistent with our style of writing the
first sentence of a function's doc string.

Also, the first sentence of the doc string should reference the
argument(s).  (If that makes the first sentence too long, tell only
the most important summary in it, and leave the rest for the following
sentences.)

> +  (let ((path_list (split-string filepaths "[\n\r]+")))
> +    (dolist (path_string path_list)
> +      (if (not (equal "" path_string))

The GNU project frowns on using "path" for anything except PATH-style
lists of directories.  So please use "filename" or "file-name" etc. in
this function, instead of "path" in "filepaths", "path_list", and
"path_string".

> +	  (dnd-open-file
> +           ;; The path string is timmed for spaces, nbsp and tabs.
                                    ^^^^^^
A typo.

Also, please include a ChangeLog-style commit log message with the
patch (detailed instructions are in CONTRIBUTE).

(I cannot test the patch, but I trust that you and others already
did.)

Thanks again for working on this.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On May 2, 2017, at 0:27, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> Date: Tue, 2 May 2017 00:12:41 +0900
>> 
>> Ok, I have something that works here. I'm attaching a diff. Let me know if that is satisfactory.
> 
> Thanks.  A few nits:

Ok, everything fixed here. I'll resend the patch when I am done with the copyright paperwork and when I understand how to write a proper commit log message (hopefully it will take me less time that it took to write this patch... :)

Jean-Christophe

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Tue, 2 May 2017 00:58:44 +0900
> 
> Ok, everything fixed here. I'll resend the patch when I am done with the copyright paperwork and when I understand how to write a proper commit log message (hopefully it will take me less time that it took to write this patch... :)

This contribution is short enough for us to accept it even without the
paperwork.  (I do encourage you to go on with paperwork, so that your
future contributions will be accepted as well.)  So just send the
fixed patch with the log message, and someone will commit it for you.

Thanks.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On May 1, 2017, at 20:27, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> 
>> On a side note, what's the point of having string-trim in subr-x.el when a more powerful solution is provided in subr.el?
> 
> Good question.
> 
> Perhaps string-trim could be extended to allow an optional argument that would replace the built-in default regexp.

I take the question back. It looks like there is no elegant way to *only* trim a string passed to split-string:

split-string string &optional separators omit-nulls trim

For that, "separators" should be a regex that doesn't match anything in the string since "nil" defaults to split-string-default-separators (instead of, for ex. leaving the string as is and only defaulting when the parameter is omitted).

There are a number of regex that fit the bill according to stackoverflow, like:

(split-string string "[*]" t trim-regex)

but it's not elegant.

It would have been much nicer to implement from the start a "nil" option that does that... So I guess improving string-trim in subr-x.el is still relevant... Or am I missing something?

Jean-Christophe

Re: lisp/term/ns-win.el modification

[Eli Zaretskii]

> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> Date: Tue, 2 May 2017 13:00:29 +0900
> 
> I take the question back. It looks like there is no elegant way to *only* trim a string passed to split-string:

No need to take the question back: you actually supplied the answer.
Thanks.

> So I guess improving string-trim in subr-x.el is still relevant...

Yes, I think so.

So I think your patch should use string-trim, and a separate patch
should extend that to accept a non-default regexp.

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On May 2, 2017, at 15:44, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> So I guess improving string-trim in subr-x.el is still relevant...
> 
> Yes, I think so.
> 
> So I think your patch should use string-trim, and a separate patch
> should extend that to accept a non-default regexp.

That's what I have in mind (both items) ! Thank you for the guidance.

Jean-Christophe 

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]


> On May 2, 2017, at 1:25, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
>> Date: Tue, 2 May 2017 00:58:44 +0900
>> 
>> Ok, everything fixed here. I'll resend the patch when I am done with the copyright paperwork and when I understand how to write a proper commit log message (hopefully it will take me less time that it took to write this patch... :)
> 
> This contribution is short enough for us to accept it even without the
> paperwork.  (I do encourage you to go on with paperwork, so that your
> future contributions will be accepted as well.)  So just send the
> fixed patch with the log message, and someone will commit it for you.

Ok, I'm not sure how to send the log message so I just copied it here. And I've further simplified the patch so I'm attaching the newest and hopefully last version here.

Jean-Christophe 

Log message:

===========================
Multiline support in NS "open-file" service

* lisp/term/ns-win.el (ns-open-file-service): new function. Wraps the original call in a (split-string) to create as many calls as there are lines. (ns-spi-service-call): use it.
===========================


[-- Attachment #2: ns-win.el.diff --]
[-- Type: application/octet-stream, Size: 1224 bytes --]

diff --git a/lisp/term/ns-win.el b/lisp/term/ns-win.el
index 70bd817d93..2d173b2d80 100644
--- a/lisp/term/ns-win.el
+++ b/lisp/term/ns-win.el
@@ -229,6 +229,16 @@ The properties returned may include `top', `left', `height', and `width'."
 
 (declare-function dnd-open-file "dnd" (uri action))
 
+;; Handles multiline strings that are passed to the "open-file" service.
+(defun ns-open-file-service (filenames)
+  "Open multiple files when selecting a multiline string FILENAMES."
+  (let ((filelist (split-string filenames "[\n\r]+" t "[ \u00A0\t]+")))
+    ;; The path strings are timmed for spaces, nbsp and tabs.
+    (dolist (filestring filelist)
+      (if (not (equal "" filestring))
+	  (dnd-open-file filestring nil)))))
+
+
 (defun ns-spi-service-call ()
   "Respond to a service request."
   (interactive)
@@ -236,7 +246,7 @@ The properties returned may include `top', `left', `height', and `width'."
 	 (switch-to-buffer (generate-new-buffer "*untitled*"))
 	 (insert ns-input-spi-arg))
 	((string-equal ns-input-spi-name "open-file")
-	 (dnd-open-file ns-input-spi-arg nil))
+	 (ns-open-file-service ns-input-spi-arg))
 	((string-equal ns-input-spi-name "mail-selection")
 	 (compose-mail)
 	 (rfc822-goto-eoh)

Re: lisp/term/ns-win.el modification

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

Hi!

I just tested your latest patch. The code looks much cleaner now when you
use `split-string'. (I didn't know it could all those tricks -- it's always
good to learn something new!)

The patch seems to work perfectly!

Some minor details:

* You no longer need the `(if (not (equal "" filestring"))' test -- when
the OMIT-NULLS argument is non-nil, `split-string' ensures that the list
doesn't contain empty strings.

* You misspelled "trimmed" as "timmed" in the comment in
`ns-open-file-service'.

    -- Anders

On Tue, May 2, 2017 at 2:57 PM, Jean-Christophe Helary <
jean.christophe.helary@gmail.com> wrote:

>
> > On May 2, 2017, at 1:25, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> >> From: Jean-Christophe Helary <jean.christophe.helary@gmail.com>
> >> Date: Tue, 2 May 2017 00:58:44 +0900
> >>
> >> Ok, everything fixed here. I'll resend the patch when I am done with
> the copyright paperwork and when I understand how to write a proper commit
> log message (hopefully it will take me less time that it took to write this
> patch... :)
> >
> > This contribution is short enough for us to accept it even without the
> > paperwork.  (I do encourage you to go on with paperwork, so that your
> > future contributions will be accepted as well.)  So just send the
> > fixed patch with the log message, and someone will commit it for you.
>
> Ok, I'm not sure how to send the log message so I just copied it here. And
> I've further simplified the patch so I'm attaching the newest and hopefully
> last version here.
>
> Jean-Christophe
>
> Log message:
>
> ===========================
> Multiline support in NS "open-file" service
>
> * lisp/term/ns-win.el (ns-open-file-service): new function. Wraps the
> original call in a (split-string) to create as many calls as there are
> lines. (ns-spi-service-call): use it.
> ===========================
>
>

[-- Attachment #2: Type: text/html, Size: 2639 bytes --]

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

Anders,

> I just tested your latest patch. The code looks much cleaner now when you use `split-string'. (I didn't know it could all those tricks -- it's always good to learn something new!)

Well, I did not know I would be able to even do that in the first place before reading everybody's comments...

> Some minor details:

Details are never minor. They're what makes it look like somebody actually cared for the code :)

> * You no longer need the `(if (not (equal "" filestring"))' test -- when the OMIT-NULLS argument is non-nil, `split-string' ensures that the list doesn't contain empty strings.

You're totally right (and I thought I had read that function dozens of times already).

> * You misspelled "trimmed" as "timmed" in the comment in `ns-open-file-service'.

Oversight.

I'll send something again later. Sorry for the hassle.

Jean-Christophe

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

Anders,

Thank you for the verification. Would you mind checking that this patch is correct before I propose it for a commit?

The log message would be something like:

===========================
Fix Multiline support in NS "open-file" service previous patch

* lisp/term/ns-win.el (ns-open-file-service): removed a useless check and fixed a typo.
===========================

Jean-Christophe 


[-- Attachment #2: ns-win.el.diff --]
[-- Type: application/octet-stream, Size: 739 bytes --]

diff --git a/lisp/term/ns-win.el b/lisp/term/ns-win.el
index 2d173b2d80..4df5f0abe2 100644
--- a/lisp/term/ns-win.el
+++ b/lisp/term/ns-win.el
@@ -233,10 +233,9 @@ The properties returned may include `top', `left', `height', and `width'."
 (defun ns-open-file-service (filenames)
   "Open multiple files when selecting a multiline string FILENAMES."
   (let ((filelist (split-string filenames "[\n\r]+" t "[ \u00A0\t]+")))
-    ;; The path strings are timmed for spaces, nbsp and tabs.
+    ;; The path strings are trimmed for spaces, nbsp and tabs.
     (dolist (filestring filelist)
-      (if (not (equal "" filestring))
-	  (dnd-open-file filestring nil)))))
+      (dnd-open-file filestring nil))))
 
 
 (defun ns-spi-service-call ()

[-- Attachment #3: Type: text/plain, Size: 632 bytes --]


> On May 4, 2017, at 5:40, Anders Lindgren <andlind@gmail.com> wrote:
> 
> Hi!
> 
> I just tested your latest patch. The code looks much cleaner now when you use `split-string'. (I didn't know it could all those tricks -- it's always good to learn something new!)
> 
> The patch seems to work perfectly!
> 
> Some minor details:
> 
> * You no longer need the `(if (not (equal "" filestring"))' test -- when the OMIT-NULLS argument is non-nil, `split-string' ensures that the list doesn't contain empty strings.
> 
> * You misspelled "trimmed" as "timmed" in the comment in `ns-open-file-service'.
> 
>    -- Anders

Re: lisp/term/ns-win.el modification

[Anders Lindgren]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

Hi!

I just pushed this to the master brach.

Congratulations to your first Emacs contribution!

    -- Anders

On Thu, May 4, 2017 at 10:50 AM, Jean-Christophe Helary <
jean.christophe.helary@gmail.com> wrote:

> Anders,
>
> Thank you for the verification. Would you mind checking that this patch is
> correct before I propose it for a commit?
>
> The log message would be something like:
>
> ===========================
> Fix Multiline support in NS "open-file" service previous patch
>
> * lisp/term/ns-win.el (ns-open-file-service): removed a useless check and
> fixed a typo.
> ===========================
>
> Jean-Christophe
>
>
>
> > On May 4, 2017, at 5:40, Anders Lindgren <andlind@gmail.com> wrote:
> >
> > Hi!
> >
> > I just tested your latest patch. The code looks much cleaner now when
> you use `split-string'. (I didn't know it could all those tricks -- it's
> always good to learn something new!)
> >
> > The patch seems to work perfectly!
> >
> > Some minor details:
> >
> > * You no longer need the `(if (not (equal "" filestring"))' test -- when
> the OMIT-NULLS argument is non-nil, `split-string' ensures that the list
> doesn't contain empty strings.
> >
> > * You misspelled "trimmed" as "timmed" in the comment in
> `ns-open-file-service'.
> >
> >    -- Anders
>
>

[-- Attachment #2: Type: text/html, Size: 1976 bytes --]

Re: lisp/term/ns-win.el modification

[Jean-Christophe Helary]

> On May 5, 2017, at 3:37, Anders Lindgren <andlind@gmail.com> wrote:
> 
> Hi!
> 
> I just pushed this to the master brach.
> 
> Congratulations to your first Emacs contribution!

Thank you for the guidance.

Jean-Christophe