ajaxorg / ace

Ace (Ajax.org Cloud9 Editor)
https://ace.c9.io
Other
26.77k stars 5.29k forks source link

please treat newlines as terminating a line instead of being a separator #2083

Closed obfusk closed 2 years ago

obfusk commented 10 years ago

Hi,

Ace currently treats newlines as separators, whereas the POSIX standard defines a line as A sequence of zero or more non- <newline> characters plus a terminating <newline> character.

Most UNIX editors (e.g. vim) adhere to this standard. Most unix utilities (like cat and the read builtin from bash) expect files to adhere to this standard. Unfortunately, ace treats newlines differently and both unexpectedly produces files with no newline at the end and displays files that do have a newline at the end as having an extra, empty, line at the end.

Given that some people may want to retain the existing behaviour and the fact that the new (IMHO correct) behaviour would not be backwards compatible (i.e. it breaks some of the current tests), I would suggest making the behaviour configurable.

Thanks.

- Felix

nightwing commented 10 years ago

Hi, Newer text editors like Sublime Text, Textmate, Notepad++ behave same way as Ace. Also having vim behavior as default is not possible since it loses information edior.setValue("x"); edior.getValue() != "x"

I think option to emulate vim behavior should be handled by the code which loads files into the editor.

obfusk commented 10 years ago

Hi,

I'm aware of the (unfortunate) incorrect (since it both violates the POSIX standard and causes problems with standard UNIX tools) behaviour of Sublime (et al). For example: a file foo containing "foo\nbar\n" and a file bar containing "foo\nbar":

The behaviour of vim (et al) is correct and that of Sublime (et al) is not.

It is of course possible to have the code that saves (and loads) files handle newlines -- although it would then be nicer to have a setting that allows setValue and getValue to do this automatically. For example: getValue() --> treat_newlines_properly && value.length && value[value.length-1] !== '\n' ? value + '\n' : value, or getValue(true) --> ....

Thanks.

- Felix

nightwing commented 10 years ago

Not all text is meant to be written to files on UNIX compatible OS, e.g cloud9 uses ace as findbar input, where adding newlines isn't proper behavior at all. Why do you think having this behavior in setValue/getValue is nicer? i'd say having less magic there is better. Also note that there are other similar behaviors which file handling logic should implement: e.g trimming trailing spaces.

obfusk commented 10 years ago

You have a point. However, I consider proper newline handling (for UNIX systems) a fundamental part of being a (cross-platform) editor. Therefore I would prefer ace to (have an option to) handle it. Otherwise everything that uses ace (and wants to properly handle UNIX text files) will have to deal with it separately.

I regularly see problems (like incompatibility with shell scripts and other UNIX tools) and annoyances (like \ No newline at end of file in diffs) created by editors that do not properly handle newlines and would prefer that ace not be one of them.

I do realize that ace does not deal with files directly. I would consider it debatable whether it makes sense for ace to implement useful features like optionally trimming trailing spaces. There are good arguments for doing so (not needing to implement it multiple times) and not doing so (keeping the complexity of ace to a minimum). But as I said, I consider proper newline handling more fundamental.

- Felix

matthewkastor commented 10 years ago

Why not just type a new line at the end of your file, instead of trying to make ace into the kind of program that alters what the user intends in order to support obscure tools nobody (awesome) uses?

matthewkastor commented 10 years ago

A diff tool that warns you about not having a newline at the end of your file is telling you to type a newline at the end of your file. It is not telling you that your text editor is broken...

I consider spell checking to be indispensable, but to try jamming it down everyone's throat so that ace becomes less useful just to please me... well that would be asinine.

obfusk commented 10 years ago
  1. Whether newline handling (cf trimming spaces or spell checking) belongs in ace is, of course, open to debate -- that's what we're doing after all.
  2. I'm not trying to jam anything down anyone's throat -- I'm just asking whether it's possible for ace to provide optional POSIX-compatible newline-handling so that programs (and users) using ace do not have to.
  3. Since when is UNIX obscure? I'll grant you that not everyone uses wc, cat or bash (directly), but using shell scripts to process text files is something that e.g. servers do quite often.
  4. As to "... It is not telling you that your text editor is broken...": it depends on the context. On UNIX systems, that's exactly what it's doing. Because the file is not a (POSIX) text file.
  5. As to "... that alters what the user intends ...": I'm a user. I intend to create a POSIX-compatible text file. Currently, ace alters what I intend. When I create a file with three lines, I expect it's content to be "line 1\nline 2\nline 3\n", not "line 1\nline 2\nline 3". The fact that ace considers the former to be a file with 4 lines, the last of which is empty is not in line with my expectations (and POSIX standards).
  6. Obviously, you have different requirements and expectations. I have no intention of breaking those. I'm just pointing out that my expectations are not "obscure" or "unreasonable" even if you may not have encountered them before. I would like ace to be configurable as to the user's expectations so we can both get what we want/expect.

- Felix

matthewkastor commented 10 years ago

Please use bold text and all caps when you write the word "optional", it will clear throats and make obvious that this is a feature request.

I apologize if you've been offended by my disregard for ancient tech. I assumed you were cracking jokes because you can't even save files in ace without writing your own code to do some post processing. Ensuring that certain file types always end with a new line, and use the proper line breaks for the given platform, is such a trivial task to do when your server gets the post. It is definitely much more sensible to let implementors implement text transformations, and submit pull requests if they create a decent extension. . . Don't you think?

You seem quite well informed on the POSIX standards for text files. Couldn't you come up with a simple JavaScript extension for ace that takes the editor contents and manipulated them to suit your needs? I'd applaud you for doing so, and it would look good for you showing off your programming skills instead of your strong opinion of what others should be doing for you.

XD hell, I'll even help a bit if you need it.

nightwing commented 10 years ago

@obfusk To implement this behavior from file loading code one needs to do.

- session.setValue(string)
+ session.setValue(string.replace(/\r?\n?$/, ""))

- value = session.getValue()
+ value = session.getValue() + session.doc.getNewLineCharacter() 

if we decide to add option it will become

+ session.setOption("treatNewLinesAsTerminators", true)
 session.setValue(string)

which isn't much simpler, but will add much more code into ace for checking the option and making sure switching it on and off doesn't change document value.

However the main problem is that this behavior is inconsistent . The only way to select newline character is to move cursor to the next line, but since we do not allow that for the last line, last newline becomes unselectable, and also editor.selectAll(); editor.getSelectedText() != editor.getValue()

Another thing to consider is that for simple functionality like this, it is usually easier to reimplement than to find functions provided by ace. e.g. see whitespace.js in ace and same functionality reimplemented then extended in zed. So i vote to leave this behavior to file loading code for now.

obfusk commented 10 years ago

My apologies if my request seemed at all like a demand. That was certainly not my intention.

I agree that the solution is somewhat trivial and that there is a strong case for doing this kind of processing/validation server-side.

Thank you both for taking the time to look at the issue and how it impacts ace.

The reason I requested this feature is not that I would not be able to fix it server-side or that I would not be able to write an extension (if I took some time to find some documentation on how to do that) but that I've come across the "incorrect" behaviour multiple times (e.g. when editing files with gitlab or github) and was hoping that fixing it via ace would be a simple way to get the "correct" behaviour (when wanted/needed) in multiple places where ace is used. Especially since it seems that many people using ace are not aware of this issue and will thus be unaware of the potential problems and/or annoyance "incorrect" newline handling can sometimes cause users.

I'll try to think of a more appropriate solution (like an extension) and maybe contact gitlab and github as well.

- Felix

matthewkastor commented 10 years ago

@nightwing is there a how-to guide showing how to create a simple extension?

obfusk commented 10 years ago

What I'm currently thinking of is an extension that shows an unobtrusive warning if there's no newline at the end of the file and also integrates with existing settings systems (or maybe just localstorage) to allow a user to specify a preference for either ignoring this in future or always making sure there is one.

Not sure how to do that yet though. That how-to might be helpful.

- Felix

matthewkastor commented 10 years ago

Like an icon in the gutter beside the Line numbers? That's unobtrusive and probably wouldn't even need a setting to turn it off. I could see the icon as the symbol on the return key with a "no" (circle slashed out) drawn over it. If the last line isn't "empty" then show the mark. The thing I don't know is whether there is already something in ace for dealing with gutter images, so that if another extension wants to warn on the same line a different image for multiple warnings would show. This should be easy to implement, maybe @nightwing has more information that can help?

nightwing commented 10 years ago

and was hoping that fixing it via ace would be a simple way to get the "correct" behaviour (when wanted/needed) in multiple places where ace is used. Especially since it seems that many people using ace are not aware of this issue and will thus be unaware of the potential problems and/or annoyance "incorrect" newline handling can sometimes cause users.

@obfusk i think fixing this behavior via ace won't work because we can't enable that by default, only people that are aware of this issue, will enable the option, also i doubt that github or gitlab will turn this on by default, since many people who are accustomed to sublime behavior, will complain. (I would complain both about unselectable last newline character and about automatically adding newline at the end when i only edit at the top of the file).

If you want only to display a warning we can show crossed red circle at the end of the line, like github was doing some time ago, also a behavior to automatically add a newline when writing on empty last line might be useful (it will prevent from accidentally removing last new line, but will still allow to delete it with delete key)

there isn't any special documentation about extensions. They either provide some utility functions like ext/whitespace.js or define options

gsf commented 9 years ago

Agreed with the general consensus here (handle it within each implementation) but thought it might be worthwhile to note that GitHub does automatically append a newline. I don't know when that started or even if it was a change from previous behavior. See, for example, this patch for Caret which I submitted without touching the bottom of the file.

github-actions[bot] commented 2 years ago

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

mgeisler commented 2 years ago

Hi all,

Just another data point: I believe I ran into a closely related problem as a user of mdBook: rust-lang/mdBook#1836.

There, a Markdown file with

```rust
let x = 10;
```

is turned into HTML like this:

<pre>
let x = 10;
</pre>

This code block is in turn is turned into an editor with

let editor = window.ace.edit(code_block);

The resulting editor looks like this:

I would have expected the editor to have a single line, not two. The problem is the trailing newline before </pre>.

My changes in rust-lang/mdBook#1836 makes mdBook carefully strip the trailing newline, but I think a better solution would be to let ACE ignore the final newline when creating lines in the editor.