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

[Jean-Christophe Helary <jean.christophe.helary@gmail.com>]

[-- 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 --]