Unfortunately the problem cannot be fixed in editor module because the editor kit only gets java.io.Reader. Also CloneableEditorSupport.Env only has an InputStream.
First candidate is DataEditorSupport.Env which has knowledge about FileObject.
If doc.putClientProperty(DefaultEditorKit.EndOfLineStringProperty) gets set to the correct line separator after loading a file into the the document then the editor will respect that when saving the document (writing to a given Writer).

How can a filesystem know about line separator?

(In reply to comment #2)
> How can a filesystem know about line separator?
Local file system always has System.getProperty("line.separator")
Remote file system has platform dependent line separator.

OK. Donate a patch and pass API review.

Created attachment 112911 [details]
initial variant

[JG01] A proposed patch should show end-to-end usage whenever possible. Here the SPI end may not be in the NB main repo, but you should be testing against your actual remote FS (show a patch if it is open source). The API usage (in DataEditorSupport?) should be part of the patch.


[JG02] Doing this using the query pattern is OK, but is it necessary? My understanding is that only the remote FileSystem would know anything about this, in which case you could simply document a special FileObject attribute ("line.separator"?) to be used for this purpose.

The query pattern would however be appropriate if you expected various other modules to want to contribute to this decision. For example, we could imagine offering a per-project configurable option for the line separator, in which case the project type would implement the SPI (possibly in Project.lookup via a proxy in Lookup.default); or a VCS might mandate a line ending convention for a particular file pattern in a particular checkout (e.g. based on `hg root`/.hgeol).


[JG03] Is there any chance the query would be needed on a file which has not yet been created? If so, URI should be used rather than FileObject. This would not be the case for DataEditorSupport.Env, which is only created once the file exists; but we might want to ensure that e.g. FileEntry.Format.createFromTemplate and ScriptingCreateFromTemplateHandler and DataEditorSupport.saveAs honor the line separator for newly created files. (Currently all of these actually create the FileObject first and then write its content, but we would be limiting implementation options if the query could only handle FileObject.)

In reply to [JG02]
I believe no additional flexibility is needed, 
so I'm quite OK with getAttribute("line.separator").
But we should introduce a constant in FileObject in this case.

In reply to [JG03]
I think we don't necessarily need to care about nonexistent files in this respect.

Created attachment 114431 [details]
variant #2 (approach JG02)

Basically OK. Just:

Y01 Write a test for DataEditorSupport behavior so I can be clueless when maintaining it.

[JG04] spec.version.base=3.18.1 is not allowed in trunk [1]. You mean to use 3.19.0. And anyway minor="18.1" is syntactically invalid.


[JG05] Revert no-op diff hunks like

-            if (!inited) { // Fill line-separator properties<<EOL>>
+            if (!inited) { // Fill line-separator properties               <<EOL>>

and make sure your text editor is configured properly. For the NB editor, "Removing Trailing Whitespace" should be set to "From Modified Lines Only".


[JG06] The following change

-                lineSeparator = (String) getProperty(BaseDocument.READ_LINE_SEPARATOR_PROP);
+                lineSeparator = (String) getProperty(BaseDocument.DEFAULT_LINE_SEPARATOR_PROP);

is suspicious. If this is intentional then it would seem the Javadoc of WRITE_LINE_SEPARATOR_PROP needs to be changed to match.


[JG07] apichanges.xml line

<a href="@TOP@/architecture-summary.html#answer-arch-usecases">content of system filesystem</a> by

looks bogus; blindly copied?

<class package="org.openide.filesystems" name="Repository"/>

is also clearly wrong.


[JG08] "Default line separator will be used for new files" is a bit misleading. It will be used by the _text editor_ if saving new content to an initially empty file. Any other code which creates file content programmatically (e.g. the various things mentioned in JG03) must manually read this property if it cares.


[1] http://wiki.netbeans.org/DevFaqImplementationDependency

Created attachment 114768 [details]
variant #3 (fixed JG04-JG08, unit test)

If there are no objections, let's integrate Friday.

JG04 was not fixed correctly; you must use 3.19.0, not just 3.19, since impl versions are appended to the base.


[JG09] Repository.getDefault().addFileSystem(...) is deprecated and probably has no useful effect. Similarly in tearDown.


[JG10] Also waitEQ() is odd; did you mean to override runInEQ instead?


[JG11] Can probably delete MyFileObject; just call setAttribute("default-line-separator", "\r") in setUp.

Also consider whether you need a custom loader and data object. DefaultDataObject has an editor support already.

(All this unit test code looks like it was blindly copied from somewhere else. Usually you can write a much smaller and more readable test from scratch - there are plenty of old idioms that have been obsolete for years.)


[JG12] Consider making DEFAULT_LINE_SEPARATOR_PROP a public constant somewhere in openide.text - perhaps NbDocument - for ease of documentation, Find Usages, etc.

(Neither dlight.remote.impl nor editor.lib seems to have a direct dep on it now, but both seem to have indirect deps on it.)

Of course it could also be defined in openide.filesystems; it is unclear if there are other potential use cases outside the editor.

Optional, but it is not nice to have an API which is only visible as a magic string mentioned in an apichanges entry. (apichanges is intended for giving a brief overview of what _changed_ from release to release, but should never be used as the primary documentation for what _is_ in the API.) At a minimum should be in arch.xml.

Created attachment 114819 [details]
variant #4 (fixed JG04, JG09-JG11)

I do not know what to do with JG12.
Probably Miloslav or Jaroslav can suggest class to add public constant?
IMHO BaseDocument is the best place for it (already done).

(In reply to comment #14)
> BaseDocument is the best place for it

BaseDocument is inaccessible to the existing user of the FileObject attribute name (dlight.remote.impl) and also to most potential users (as outlined in JG03), and this attribute is not inherently specific to the editor at all (as mentioned in JG08). It is currently mentioned in apichanges for openide.filesystems, which I think is the right place, but is not otherwise mentioned anywhere in openide.filesystems documentation; a mention in arch.xml would be the minimum. (A Java constant is nicer, for discoverability and compile-time safety and so on, but that is a lower priority.)

There is also a document property interpreted by BaseDocument which happens to have the same name. DataEditorSupport is responsible for bridging the two constants, and it cannot refer to BaseDocument, so the document property must be documented in openide.text/arch.xml (or a public constant) as well.

Let's introduce something like:
public enum FileObjectAttribute {
   DEFAULT_LINE_SEPARATOR("default-line-separator");

   private final String attr;
   private FileObjectAttribute(String name) {
    this.attr = name;
   }

   public String name() {
     return this.attr;
   }

   public String toString() {
     return name();
   }
}

then all other attributes can migrate here compatibly.

FileObjectAttribute is expected to be in the same package org.openide.filesystems as FileSystem/FileObject

Using an enum seems weird, since the actual argument is of type String, and there is no limitation on which attribute names you can read or write. Plain String constants would be better I think.

(Comment #7 suggested placing it in FileObject, which makes as much sense as anywhere I think. There is already a package-private constant REMOVE_WRITABLES_ATTR which it did not seem important to make public since only core.startup would use it.)

Created attachment 114842 [details]
variant #5 (fixed JG12)

fixed, change set:
http://hg.netbeans.org/cnd-main/rev/ba8e48119dd6

Looks good to me.

BTW there is no need to copy detailed Javadoc into apichanges. It need only mention the existence of the new API item, and anything less than obvious you might need to do in order to migrate to using it.

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/ba8e48119dd6
User: Alexander Simon <alexvsimon@netbeans.org>
Log: fixed Bug #199534 Line Separator is attribute of FileSystem