This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | Preferences for Project | ||
---|---|---|---|
Product: | projects | Reporter: | Jan Lahoda <jlahoda> |
Component: | Generic Infrastructure | Assignee: | Jan Lahoda <jlahoda> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | apireviews, dkonecny, jglick |
Priority: | P3 | Keywords: | API, API_REVIEW_FAST |
Version: | 6.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | ENHANCEMENT | Exception Reporter: | |
Bug Depends on: | 191393 | ||
Bug Blocks: | |||
Attachments: |
Proposed patch.
Sample project.xml. Updated patch. Updated patch. Updated patch. |
Description
Jan Lahoda
2008-05-06 10:45:51 UTC
sounds ok to me. Not bad, though I would actually rather see a matching SPI; for example, if the project provided an instance of Preferences (i.e. a root node) in its Lookup, that would be used instead of AuxiliaryConfiguration. This would permit more natural storage of simple key-value preferences in a .properties file. (For example, in project.properties or private.properties with a "prefs." prefix to isolate them from Ant-related properties.) Also note that a full implementation of Preferences is not trivial. You need to implement listening, subnodes, etc. - a significant amount of code, as seen in our impl of NbPreferences. All this may be overkill for our needs. A slim interface like interface ProjectPreferences { // SPI, API would be matching final class String get(String key); void put(String key, String/*|null*/ value); } might suffice (callers would be expected to choose distinctive keys). Re SPI I was thinking about such an SPI, but seemed quite complex to me. We would need to use AuxiliaryConfiguration as the storaqe if the preferences SPI is not available anyway, IMO. And if a project would not implement the Preference SPI for e.g. 6.5, and implemented it after 6.5, we couldn't simply use the Preferences SPI in post-6.5, as the project wouldn't work in 6.5. We would need to wait for project upgrade. So seemed to me much simpler and acceptable for me to use only AuxiliaryConfiguration as the storage (the preferences SPI could be added at any time in the future is necessary). Impl. of Preferences does not seem so difficult to me - it seems to be enough to extend AbstractPreferences and implement a few methods. The use of Preferences allows existing features to be configured on the project-level with minimal changes. Thanks for the comments. Regarding SPI, of course we would need a fallback implementation of Preferences (or whatever the interface is) to use AC in case the project did not provide one. I do not understand your compatibility argument. The client calls something in ProjectUtils and reliably gets an interface to r/w properties. PU automatically uses a project-specific backend if available, otherwise AC. The client need not know the difference. Eventually we could probably deprecate AC. "Impl. of Preferences does not seem so difficult to me" - well talk to Radek Matous who spent quite some time implementing it for NbPreferences. The asynchronous nature of storage, plus change support, makes it tricky. A simplified interface letting you get and set simple string keys (no hierarchical nodes) with no change support would be far easier to implement. For unshared properties, I would recommend simply using NbPreferences. You can just e.g. hash the project directory path and include that in the key, to make a setting apply to just one project. (Does not carry over when the project is moved, but this is usually desirable anyway, and nbproject/private/ gets deleted in a move too.) Sorry for confusion on the compatibility: I meant compatibility from user's point of view. If a project is created in 6.5, opened in post-6.5 (not upgraded) and then opened in 6.5, it should work reasonably. Another possible problem is support for inner nodes - this may be difficult to do inside one .properties file. I would rather start with a simple implementation based solely on the AuxiliaryConfiguration, and introduce some kind of faster/better SPI later if necessary. The reason why I am proposing this API is that features that use "global" Preferences can easily be changed to use project-level Preferences. If there would be two separate API, it would be more difficult for these feature to be ported from "global" to "project" settings level. I think that the benefit/cost ratio of Preferences is quite good. Thanks for the suggestion regarding private properties - will try to do it this way. Support for inner nodes is one of the main reasons I do not suggest using j.u.p.Preferences; inner nodes are a significant complication for which there is no real use case. Re support for inner nodes: e.g. each Java Hint has its own Preferences node and could create its own sub-nodes. Moreover, Preferences are a standard API, so reusing it seems OK to me. I am attaching a patch that implements ProjectUtils.getPreferences. I would like to start an API review on adding this method, unless there are objections. A note on tests: the tests require an implementation of AuxiliaryConfiguration. I used J2SE Project for simplicity - I will try to rewrite the test so that the dependency on J2SE Project is not necessary. Created attachment 62158 [details]
Proposed patch.
Regarding support for inner nodes: you don't really need subnodes. You can just use "foo.bar.baz" syntax. Compare Firefox/Thunderbird settings storage, etc. Regarding project upgrade compatibility: if the fallback SPI impl for shared properties used AC, the API impl would just need to check in both AC and the project's custom SPI for a given key. I.e.: package spi; interface Impl { String get(String key, boolean shared); void put(String key, String value, boolean shared); // optionally also: Iterable<String> keys(boolean shared); } package api; public static String get(Project project, String key, boolean shared) { Impl i = p.lookup.lookup(Impl); if (i != null) { String v = i.get(k, shared); if (v != null) { return v; } } AC ac = project.lookup.lookup(AC); if (ac != null) { String v = ACHelper.get(ac, key, shared); if (v != null) { return v; } } // otherwise ignore shared flag // FileObject.attribute best here; a future masterfs impl could in fact use NbPreferences internally: Object o = project.getProjectDirectory().getAttribute(key); return (o instanceof String) ? (String) o : null; } public static void put(Project project, String key, String value, boolean shared) { Impl i = p.lookup.lookup(Impl); if (i != null) { i.put(key, value, shared); return; } AC ac = project.lookup.lookup(AC); if (ac != null) { ACHelper.put(ac, key, value, shared); return; } project.getProjectDirectory().setAttribute(key, value); } // optionally also: public static Iterable<String> keys(Project project, boolean shared) I would still prefer the option for the project to store simple key/value properties in a more natural way - in project.properties, private.properties, etc. Going through AuxiliaryConfiguration is awkward, inefficient, and harder to debug. Also will look extremely ugly for projects which use FileObject.setAttribute to implement AC. (I plan to submit an API change to provide a fallback impl of AC using FO.attribute. Should be useful for Maven projects, autoproject, etc.) I won't block this implementation which forces everything to go through AC, I just don't like it so much. Anyway, review comments: [JG01] The test dependency on j2seproject is illegal (will cause cycles building tests). Better to make a fake project type with a trivial AC impl which just persists its DOM fragments in memory. [JG02] Please use descriptive parameter names, not "p" etc. [JG03] "(specified as using {@link Class} as in {@link org.openide.util.NbPreferences#forModule(java.lang.Class)})" duplicates information in @param and can be deleted. [JG04] "for which project should be the preferences returned" => "project for which preferences should be returned" [JG05] getPreferences fails to document that it could return null. In such a case you could consider just delegating to getPrivateSettings to fall back gracefully and simplify the caller's API. [JG06] I'm not sure I get the point of the __idXXXX key. Is there some reason you cannot just return prefs.node(projectDir)? [JG07] findCNBForClass could be made more efficient by copying the simple impl from NbPreferences. [JG08] type="module" in the schema should perhaps be type="module-or-node"? maxOccurs=unbounded" is a syntax error. Actually try validating some files (preferably in the unit test). The names of elements in the schema are very confusing. What does a sample document actually look like? [JG09] Does your Preferences impl fire changes? If so, it should be tested. If not, it should be documented. [JG10] If the project does provide an AC, that should be used for both shared and private settings (passing the appropriate boolean flag to AC methods). I have added a simple SPI interface that allows efficient backstore for the properties and an impl provided by AntProjectHelper. Currently stores the data into nbproject/project.properties and nbproject/private/private.properties, but we may want to introduce something like nbproject/auxiliary.properties and nbproject/private/auxiliary.properties. JG01: fixed JG02: done JG03: done JG04: done JG05: added a note on null value. I do not think it is correct to return non-sharable settings when sharable settings are requested. JG06: prefs.node(projectDir) could cause the path would be too long for the system. Preferences also have a limit on the length of the key and node name. Anyway, I have rewritten this to use FileObject.{g|s}etAttribute. JG07: done (although I did not find a simplier way than to introduce a dep on Module System and loop through installed modules). JG08: fixed the problems in the schema. I am attaching a sample document. Didn't have time to write the test yet, but I will try. JG09: It does - the functionality is provided by java.util.prefs.AbstractPreferences. So the test would actually test whether the j.u.p.AbstractPreferences is implemented correctly. JG10: done (although I understood the comment in #desc5 in such a way that the private properties should go through NbPreferences in all cases). Attaching a new patch. Created attachment 62234 [details]
Sample project.xml.
Created attachment 62235 [details]
Updated patch.
I would like to ask API review for addition of: org.netbeans.api.project.ProjectUtils.getPreferences(Project, Class, boolean) org.netbeans.spi.project.AuxiliaryProperties org.netbeans.spi.project.support.ant.AntProjectHelper.createAuxiliaryProperties() Regarding JG05: I still think it is better to fall back to nonsharable settings in this case. At least the configuration will work for the current user - better than nothing. And what is the caller code supposed to do, anyway? Check for null, and if null, .... try using nonsharable settings instead. Better to keep the caller code concise and simple. This is consistent with the general API philosophy of Preferences - hope that the backing store is available but do not burden the caller if there is a problem with it, under the reasonable assumption that preferences are not precious and failure to store them will not cripple the application. Regarding JG06: NbPreferences has no such length restrictions. Regarding JG07: hmm, may be inefficient to loop this way. Do you really need to group things by module anyway? Is there anything wrong with using simple package name, as in Preferences.forPackage? [JG11] Module names should be code.name.base, not code-name-base which is used only in filenames. [JG12] Will you use the APH-based implementation in j2seproject and similar? According to the patch, I guess you will - but this contradicts the sample project.xml, doesn't it? [JG13] ProjectUtils.getPreferences should mention its SPI and when it is used. [JG14] What is the purpose of the CleaningWeakReference? Can't you just have a WeakHashMap<Project,AuxiliaryConfigBasedPreferencesProvider>? [JG15] AuxiliaryPropertiesImpl.java (in project.ant) appears to be missing from the patch. Regarding JG05: if we auto-fallback to non-sharable settings, the client code cannot decide what to do (it could, for example, show a warning to the user). Also, I am not sure if the users would consider their setting "not precious". If a non-null value is required, we need to add a new method like isSharablePreferencesSupport(Project). Regarding JG06: NbPreferences has no such length restrictions: only for property names (which is a clear violation of the Preferences contract, and moreover the impl is incorrect). There is still limitation on node name length. Also the path length issues still may occur. Regarding JG12: I plan to use the APH-based impl. for j2seproject and apisupport project. I attached the project.xml as a sample for only illustration. Regarding JG14: such a map would leak. Regarding JG15: Oops, attaching a patch that contains it. Created attachment 62281 [details]
Updated patch.
JG14: you mean because the value (ACBPP) refers to the key (Project). But your current code will leak, too, since the key is held strongly by HashMap. I might instead suggest using a WeakHashMap as originally suggested but not keeping a project field in ACBPP. (If you need to call saveProject in flush, keep the projectDirectory instead.) JG14: the project should be removed from the map by the CleaningWeakReference (see testReclaimable). To use WeakHashMap, the AuxiliaryConfiguration and AuxiliaryProperties couldn't be stored in the ACBPP (these are allowed to hold the corresponding instance of Project, IMO). So it would be needed to lookup them from the project lookup each time they are accessed (and the returned instances could vary in time). JG14: I see your point, but I think it would be accomplished more easily with a simple WeakHashMap<Project,WeakReference<ACBPP>>: if the value gets collected, make a new one, but don't bother to remove unused keys for Project's which still exist. You are trying to make a "biweak" map, which is impossible, so you have to use one of various workarounds. See Java bug #6389107 for discussion. JG05: I have removed the note about null return value - will rewrite the impl. to use ProjectUtils.getAuxiliaryConfiguration once it is available. JG11: if the cnb used dots, these would need to be escaped before they would be written into the properties file (dots are used to separate nodes in the properties files), so I used dashes. Hopefully this is not a big problem. JG13: done JG14: Ok, I used (Weak)HashMap+WeakReference to replace CleaningWeakReference. I am attaching an updated patch and will integrate on Monday, unless there are objections. Created attachment 62467 [details]
Updated patch.
Integrated. Don't forget to update www/www/ns (in CVS) with the new schema (auxiliary-configuration-preferences.xsd). This is necessary to permit files using it to be validated. Thanks for reminder: lahvac@lahvac-laptop:/tmp/www/www/ns/auxiliary-configuration-preferences$ cvs commit RCS file: /cvs/www/www/ns/auxiliary-configuration-preferences/1.xsd,v done Checking in 1.xsd; /cvs/www/www/ns/auxiliary-configuration-preferences/1.xsd,v <-- 1.xsd initial revision: 1.1 done Do you mind fixing this also for web/j2ee project types: web.project, j2ee.clientproject, j2ee.earproject and j2ee.ejbjarproject? It would be highly appreciated. Is it actually necessary to add something to a project type? You added helper.createAuxiliaryProperties() to J2SE project but web/j2ee projects seem to work without that as ProjectUtils provide a fallback, right? Use of APH.cAP is preferable as it uses the *.properties storage which is more concise, does not trigger build{,-impl.xml} regeneration, etc. [JG16] Please mention in Project.getLookup Javadoc that an impl of AuxiliaryProperties is encouraged, and in the Javadoc for AP mention APH.cAP as an example implementation. Re. "Use of APH.cAP is preferable" - I fixed that for web/j2ee. Next time if you decide not to fix all project types it would useful to at least file issue so that it is not forgotten and and somebody else can do. Thanks, -David. Integrated into 'main-golden', available in NB_Trunk_Production #267 build Changeset: http://hg.netbeans.org/main/rev/b5b11c3cd9d9 User: David Konecny <dkonecny@netbeans.org> Log: #134580 - Preferences for Web/J2EE Projects Integrated into 'main-golden', available in NB_Trunk_Production #271 build Changeset: http://hg.netbeans.org/main/rev/07659e55df39 User: Tomas Mysik <tmysik@netbeans.org> Log: Preferences for PHP Projects Related to #134580 - Preferences for Web/J2EE Projects. |