Bug 199534 - Line Separator is attribute of FileSystem
Line Separator is attribute of FileSystem
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Data Systems
7.0.1
All All
: P3 (vote)
: 7.2
Assigned To: Alexander Simon
issues@platform
: API, API_REVIEW
Depends on:
Blocks: 198072
  Show dependency treegraph
 
Reported: 2011-06-20 07:02 UTC by Alexander Simon
Modified: 2012-01-13 21:53 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
initial variant (6.73 KB, patch)
2011-11-07 10:02 UTC, Alexander Simon
Details | Diff
variant #2 (approach JG02) (11.77 KB, patch)
2011-12-23 14:31 UTC, Alexander Simon
Details | Diff
variant #3 (fixed JG04-JG08, unit test) (29.52 KB, patch)
2012-01-10 15:55 UTC, Alexander Simon
Details | Diff
variant #4 (fixed JG04, JG09-JG11) (17.61 KB, patch)
2012-01-12 10:10 UTC, Alexander Simon
Details | Diff
variant #5 (fixed JG12) (17.04 KB, patch)
2012-01-12 17:34 UTC, Alexander Simon
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Simon 2011-06-20 07:02:43 UTC
Editor library has two classes that have an assumption about default line separator (classes Analyzer & LineSeparatorConversion).
Classes use System.getProperty("line.separator") for line separator.
It is not true for remote file system.
Editor library should get line separator from FileSystem.
Comment 1 Miloslav Metelka 2011-09-19 12:29:24 UTC
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).
Comment 2 Jaroslav Tulach 2011-10-12 08:14:37 UTC
How can a filesystem know about line separator?
Comment 3 Alexander Simon 2011-10-12 08:31:39 UTC
(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.
Comment 4 Jaroslav Tulach 2011-11-04 14:26:04 UTC
OK. Donate a patch and pass API review.
Comment 5 Alexander Simon 2011-11-07 10:02:44 UTC
Created attachment 112911 [details]
initial variant
Comment 6 Jesse Glick 2011-12-22 14:04:17 UTC
[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.)
Comment 7 Vladimir Kvashin 2011-12-23 10:46:18 UTC
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.
Comment 8 Alexander Simon 2011-12-23 14:31:18 UTC
Created attachment 114431 [details]
variant #2 (approach JG02)
Comment 9 Jaroslav Tulach 2011-12-25 15:17:10 UTC
Basically OK. Just:

Y01 Write a test for DataEditorSupport behavior so I can be clueless when maintaining it.
Comment 10 Jesse Glick 2011-12-29 15:11:44 UTC
[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
Comment 11 Alexander Simon 2012-01-10 15:55:15 UTC
Created attachment 114768 [details]
variant #3 (fixed JG04-JG08, unit test)
Comment 12 Alexander Simon 2012-01-11 15:21:06 UTC
If there are no objections, let's integrate Friday.
Comment 13 Jesse Glick 2012-01-11 17:04:00 UTC
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.
Comment 14 Alexander Simon 2012-01-12 10:10:10 UTC
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).
Comment 15 Jesse Glick 2012-01-12 14:37:24 UTC
(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.
Comment 16 Vladimir Voskresensky 2012-01-12 16:03:17 UTC
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.
Comment 17 Vladimir Voskresensky 2012-01-12 16:07:22 UTC
FileObjectAttribute is expected to be in the same package org.openide.filesystems as FileSystem/FileObject
Comment 18 Jesse Glick 2012-01-12 17:05:35 UTC
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.)
Comment 19 Alexander Simon 2012-01-12 17:34:25 UTC
Created attachment 114842 [details]
variant #5 (fixed JG12)
Comment 20 Alexander Simon 2012-01-13 09:38:39 UTC
fixed, change set:
http://hg.netbeans.org/cnd-main/rev/ba8e48119dd6
Comment 21 Jesse Glick 2012-01-13 10:43:03 UTC
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.
Comment 22 Quality Engineering 2012-01-13 21:53:04 UTC
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


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo