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

[Anders Lindgren]

Hi!

On Thu, Apr 27, 2017 at 12:13 PM, Jean-Christophe Helary <address@hidden> 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