Bug 90403 - Editor Settings Enhancements Review
Editor Settings Enhancements Review
Product: editor
Classification: Unclassified
Component: Settings
All All
: P3 (vote)
: 6.x
Assigned To: Vitezslav Stejskal
Depends on: 59458 115129 123095
Blocks: 67319 77030 95077 103774 118362 121784 127459 127525 132411 132695 132772 133913 133942 134217 134324 134465 134866 135409 135843 139577
  Show dependency treegraph
Reported: 2006-12-01 03:37 UTC by Vitezslav Stejskal
Modified: 2008-08-21 16:05 UTC (History)
4 users (show)

See Also:
Issue Type: TASK

The phase 1 changes (221.81 KB, text/plain)
2007-03-19 05:36 UTC, Vitezslav Stejskal
Code templates storage (9.52 KB, text/plain)
2007-06-11 03:28 UTC, Vitezslav Stejskal
Signature test report (all API changes) (71.16 KB, text/html)
2008-06-05 17:04 UTC, Vitezslav Stejskal
Commented changes in editor, editor.lib modules (37.85 KB, text/html)
2008-06-05 17:05 UTC, Vitezslav Stejskal

Note You need to log in before you can comment on or make changes to this bug.
Description Vitezslav Stejskal 2006-12-01 03:37:07 UTC
I'd like to ask for a fast track review of the enhancements we are going to
implement in the editor/settings and editor/settings/storage modules.

The ultimate goal of these enhancements is to deprecate and stop using the old
editor settings infrastructure and mainly BaseOptions. This effort started quite
a while ago when the two setting modules were created and the new way for
accessing font & color settings and keybindings was implemented. The
enhancements aim to finish off this effort by providing all the other editor
settings in the similar form (ie. in MimeLookup) and consequently to update all
existing modules in Netbeans clusters to stop using BaseOptions,
Settings.Initializer & co.

The detailed list of proposed changes can be found in this document:
Comment 1 Jesse Glick 2006-12-01 16:08:17 UTC
I'm not sure I understand the purpose of **/Defaults/ folders. Normally in the
NB SFS we have a default version of a file provided by some module layer, and
any user changes overwrite that in the corresponding place (config/) of the
userdir. So why does the editor need to do something different? In order to
permit multiple registrations to be made by modules, with only one user
override? Perhaps this would be more simply addressed in the same way we do
everything else in NB: breaking up the files (e.g. keybindings.xml) into smaller
pieces (not necessarily XML) each of which contains one atomic definition that
can be added by a separate module or overridden by the user.

Simple value settings: why not just use the Preferences API?
Comment 2 Jesse Glick 2006-12-01 16:09:58 UTC
BTW this is a nontrivial change which will have long-lasting impacts on everyone
working with the editor. I am not sure fast-track review is appropriate, but I
am not deeply familiar enough with editor usage to know how significant this is.
Comment 3 Jesse Glick 2006-12-01 18:59:29 UTC
Yarda agrees that a full review may be needed.
Comment 4 Vitezslav Stejskal 2006-12-03 22:17:44 UTC
Re. **/Defaults - I don't know why it was designed this way, but it is what we
have now. The proposal is not changing it, it just adds a possibility of having
more  files. If you think that current implementation is not good and should be
changed we can file a separate issue and redesign the settings system, but I
personally don't see any point in redesigning it. I'm CCing jjancura who might
know better than me the reasons that lead to the current implementation.

Re. Preferences API - That's an option and very viable one actually. There is a
few issues though. Mainly, we would like to provide support for loading/saving
settings that are simple classes, but not primitive types (e.g. java.awt.Color
and java.awt.Insets). This is not supported by the Preferences API, but we could
probably get around it somehow. Also, current API in editor/settings emphasizes
that setting classes found in MimeLookup are immutable and if somebody needs to
get notified about changes, they should listen on MimeLookup. This is probably
not very consistent with what Preferences API is like, but again we could sort
this out as well (probably by describing the behavior in javadoc). 

Re. fasttrack review - I know that the propsal is trying to address several
issues and I don't believe that all and every one of them are big issues that
need a full review, so perhaps I should split them apart and solve them one by
one. The simple ones first with hopefully just a fasttrack review and the more
complex afterwards with a full review. Would that sound acceptable?

On the other hand if the general agreement is that the current settings system
in editor needs complete redesigning instead of just addressing a few issues
collected over the time the scale of the changes will obviously be much bigger
and we will have to prepare and follow the full review process.
Comment 5 Jan Jancura 2006-12-04 09:41:28 UTC
Jesse + **/Default/ folder:
First I am not author of this solution, it was there before the new Options
Dialog. Mila can probably explain why its there.
I use it for "revert user changes" feature. 
Comment 6 Miloslav Metelka 2006-12-04 10:19:38 UTC
To Hanz:
 The "Defaults" folder itself is ok but the new options prepend the profile name
which may cause problems if someone would create a user profile with a name that
would clash with any mime-lookup registered functionality folders e.g.
Comment 7 Vitezslav Stejskal 2006-12-04 11:39:52 UTC
Re. **/Defaults - I obviously can't be certain, but I think that some of the
reasons for separating default values provided by modules to their own special
folder might have been the facts that editor needed to support reverting user
customizations and that
had not been implemented yet at that time.

Anyway, the **/Defaults folders have been around for quite a while and nobody
seems to have any major problems with them. So, it's probably not that bad solution.
Comment 8 Vitezslav Stejskal 2006-12-19 03:20:29 UTC
I'd like to proceed further with this issue. I've created an opinion document,
which can be accessed using the link below. I've also updated the proposal
document adding the 'Implementation Phases' section with the description of how
we plan to implement the changes.

Basically, we'll do all the changes on a branch in several phases and will ask
for review of each phase separately. The results of each phase when reviewed and
if approved will be merged to the main trunk before starting the next phase.
This should hopefully be easier for both implementors and reviewers. 

Would it be possible to get us going with an approval for prototyping first and
then start reviewing the phase 1?

Thank you!

Opinions document: http://openide.netbeans.org/tutorial/reviews/opinions_90403.html
Proposal document:
Comment 9 Jesse Glick 2007-01-24 10:35:54 UTC
SimpleValueSettings sure looks a lot like java.util.prefs.Preferences. And do we
really need *.xml files for all these settings, or would *.properties do?
Comment 10 Vitezslav Stejskal 2007-01-24 11:34:31 UTC
I think *.properties should do for SimpleValueSettings (or j.u.p.Preferences if
we use this). I'd like to use xml with proper DTD and validation for more
complex settings.
Comment 11 Jaroslav Tulach 2007-01-30 09:40:45 UTC
After today's review the submitter and the reviewers agreed that it is 
valuable and ok to go and implement phase I. This should solve S1, S2, S3, S4. 
Go on and commit phase I to trunk.

The use of Preferences (outside of phase I) has also been discussed during 
today's review and there is a potential inconsistency that needs to be solved: 
The whole editor settings API is immutable - there are no setters and as soon 
as one gets a reference to some settings object (like FontAndColorSettings) it 
is safe to assume the values will never change and that they are consistent. 
This would not be true with use of Preferences API. It has not been decided 
what is the right approach here - the submitter is asked to bring this up in 
another review before phase II is about to be realized.
Comment 12 Vitezslav Stejskal 2007-03-05 03:16:20 UTC
Reassigning to myself to implement phase 1.
Comment 13 Vitezslav Stejskal 2007-03-05 03:18:57 UTC
The development of these changes is going to be done on editor_settings_90403
branch. You can cvs -q up -P -d -r editor_settings_90403 editor/settings to get
the sources and then clean & build editor/settings and editor/settings/storage
modules. The rest should be the same...
Comment 14 Vitezslav Stejskal 2007-03-19 05:36:26 UTC
Created attachment 39614 [details]
The phase 1 changes
Comment 15 Vitezslav Stejskal 2007-03-19 05:41:03 UTC
I've attached diff'ed changes from the phase 1. I'd like to intergate this on
Wed 21st March, if there are no objections. Thanks.
Comment 16 Vitezslav Stejskal 2007-03-19 06:37:25 UTC
I forgot to mention one important thing. Since all the changes in phase 1 are
backwards compatible I haven't updated any modules yet, but as a follow-up on my
integration I am going to update existing Netbeans modules included in a dev
build to place their editor setting files in the new locations. This update
should be pretty straightforard and can be done anytime later.
Comment 17 Vitezslav Stejskal 2007-03-26 01:25:18 UTC
The phase 1 has been integrated to trunk.

Checking in editor/settings/storage/manifest.mf;
/cvs/editor/settings/storage/manifest.mf,v  <--  manifest.mf
new revision: 1.11; previous revision: 1.10
Checking in editor/settings/storage/arch/apichanges.xml;
/cvs/editor/settings/storage/arch/apichanges.xml,v  <--  apichanges.xml
new revision: 1.4; previous revision: 1.3
Checking in editor/settings/storage/arch/arch-editor-settings-storage.xml;
/cvs/editor/settings/storage/arch/arch-editor-settings-storage.xml,v  <-- 
new revision: 1.10; previous revision: 1.9
Checking in editor/settings/storage/nbproject/project.xml;
/cvs/editor/settings/storage/nbproject/project.xml,v  <--  project.xml
new revision: 1.11; previous revision: 1.10
Checking in
 <--  Bundle.properties
new revision: 1.4; previous revision: 1.3
Checking in
 <--  ColoringStorage.java
new revision: 1.26; previous revision: 1.25
Checking in
 <--  EditorKeyBindings-1_1.dtd
new revision: 1.3; previous revision: 1.2
Checking in
 <--  EditorSettingsImpl.java
new revision: 1.37; previous revision: 1.36
Checking in
 <--  FontColorSettingsImpl.java
new revision: 1.42; previous revision: 1.41
Checking in
 <--  KeyBindingSettingsImpl.java
new revision: 1.35; previous revision: 1.34
Checking in
 <--  KeyMapsStorage.javanew revision: 1.20; previous revision: 1.19
Checking in
 <--  MimeTypesTracker.java
new revision: 1.2; previous revision: 1.1
Checking in
 <--  ProfilesTracker.java
new revision: 1.2; previous revision: 1.1
Checking in
 <--  SettingsProvider.java
new revision: 1.7; previous revision: 1.6
Checking in
 <--  SettingsType.java
new revision: 1.2; previous revision: 1.1
Checking in
 <--  Utils.java
new revision: 1.18; previous revision: 1.17
Checking in
 <--  layer.xml
new revision: 1.3; previous revision: 1.2
Checking in
 <--  mime-resolvers.xml
new revision: 1.2; previous revision: 1.1
Checking in
 <--  Bundle.properties
new revision: 1.2; previous revision: 1.1
Checking in
 <--  EditorSettingsStorageTest.java
new revision: 1.12; previous revision: 1.11
Checking in
 <--  EditorTestLookup.java
new revision: 1.5; previous revision: 1.4
Checking in
 <--  FontColorSettingsImplTest.java
new revision: 1.4; previous revision: 1.3
Checking in
 <--  LocatorTest.java
new revision: 1.2; previous revision: 1.1
Checking in
 <--  MimeTypesTrackerTest.java
new revision: 1.2; previous revision: 1.1
Checking in
 <--  ProfilesTrackerTest.java
new revision: 1.2; previous revision: 1.1
Checking in
 <--  SettingsProviderTest.java
new revision: 1.6; previous revision: 1.5
Checking in
 <--  TestUtilities.java
new revision: 1.2; previous revision: 1.1
Comment 18 Vitezslav Stejskal 2007-06-11 03:27:52 UTC
Reassigning to myself for further work.
Comment 19 Vitezslav Stejskal 2007-06-11 03:28:38 UTC
Created attachment 43491 [details]
Code templates storage
Comment 20 Vitezslav Stejskal 2007-07-23 11:50:57 UTC
This is not going to be finished for nb6.
Comment 22 Jesse Glick 2007-12-12 06:26:53 UTC
As a hotfix, I moved MacrosModelTest to editor/macros where it apparently belongs, though it does not seem to compile
against the changed APIs anyway, so I commented out its body for now. Please fix.

Removing options/test/unit/src/org/netbeans/modules/options/macros/MacrosModelTest.java;
<--  MacrosModelTest.java
new revision: delete; previous revision: 1.2
RCS file:
Checking in macros/test/unit/src/org/netbeans/modules/editor/macros/storage/ui/MacrosModelTest.java;
 <--  MacrosModelTest.java
initial revision: 1.1
Comment 23 Vitezslav Stejskal 2007-12-12 08:48:08 UTC
Thanks Jesse, MacroModel is quite different now, which is why the old tests don't compile. I'll fix them soon. Sorry for
breaking the test-compilation.
Comment 24 Vitezslav Stejskal 2007-12-12 11:02:46 UTC
The tests should be fixed now.

Checking in unit/src/org/netbeans/modules/editor/macros/storage/ui/MacrosModelTest.java;
/cvs/editor/macros/test/unit/src/org/netbeans/modules/editor/macros/storage/ui/MacrosModelTest.java,v  <-- 
new revision: 1.2; previous revision: 1.1
RCS file: /cvs/editor/macros/test/.cvsignore,v
Checking in .cvsignore;
/cvs/editor/macros/test/.cvsignore,v  <--  .cvsignore
initial revision: 1.1
RCS file: /cvs/editor/macros/test/build.xml,v
Checking in build.xml;
/cvs/editor/macros/test/build.xml,v  <--  build.xml
initial revision: 1.1
RCS file: /cvs/editor/macros/test/build-unit.xml,v
Checking in build-unit.xml;
/cvs/editor/macros/test/build-unit.xml,v  <--  build-unit.xml
initial revision: 1.1
RCS file: /cvs/editor/macros/test/cfg-unit.xml,v
Checking in cfg-unit.xml;
/cvs/editor/macros/test/cfg-unit.xml,v  <--  cfg-unit.xml
initial revision: 1.1
Comment 25 Vitezslav Stejskal 2008-06-05 17:04:02 UTC
Created attachment 62425 [details]
Signature test report (all API changes)
Comment 26 Vitezslav Stejskal 2008-06-05 17:05:49 UTC
Created attachment 62426 [details]
Commented changes in editor, editor.lib modules
Comment 27 Vitezslav Stejskal 2008-06-05 17:25:26 UTC
I'd like to ask for reviewing the final stage of the editor settings infrastructure changes. The goal of these changes
was to isolate and deprecate the old editor settings infrastructure, namely BaseOptions and Settings classes. As part of
the effort we also deprecated the old code completion API that has not been used for quite a long time (issue #122089).
The APIs were moved to editor.deprecated.pre61settings and editor.deprecated.pre61completion autoload modules. The
modules are marked as deprecated.

We updated all modules from the basic IDE cluster (plus GSF and Schliemann) and removed their dependency on
editor.deprecated.pre61settings, which also allowed us to remove their dependency on openide.options (Settings Options
API) that has been deprecated since Nb6.0 (issue #77030). We also updated most of the modules and removed their
dependency on editor.deprecated.pre61completion, however, there are still some modules that are using classes from this
long time dead API. Here is the list of modules in the basic IDE cluster that could not have been updated.

     [exec] WARNING [org.netbeans.core.modules]: the module org.netbeans.modules.beans uses org.openide.options which is
deprecated: Use org.openide.util.NbPreferences instead.
     [exec] WARNING [org.netbeans.core.modules]: the module org.netbeans.modules.xml.text uses
org.netbeans.modules.editor.deprecated.pre61completion which is deprecated.
     [exec] WARNING [org.netbeans.core.modules]: the module org.netbeans.modules.db uses org.openide.options which is
deprecated: Use org.openide.util.NbPreferences instead.
     [exec] WARNING [org.netbeans.core.modules]: the module org.netbeans.modules.j2ee.persistence uses
org.netbeans.modules.editor.deprecated.pre61completion which is deprecated.

In order to fully deprecate the old editor settings infrastructure we had to do several other API changes that were
filed under separate issues. Here are links to IZ.

- issue #114747 - BaseDocument should be created rather for mimeType ... - not reviewed
- issue #115129 - Allow pluggable filters for loading/saving settings - third party API
- issue #85442 - Declarative registration of editor actions - reviewed, in trunk

The old APIs originally resided in editor.lib and editor modules, which is where the major changes were done.
Unfortunately, there were several other modules that exposed classes from these APIs in their own API. We had to modify
them too. We used Netbeans signature test infrastructure to report all API differences in all modules caused by our
changes. We ran 'ant check-sigtests' against the API snapshot from hudson's trunk build #2144 (5-Jun-2008) and compared
it with APIs in the repository clone, where we have been implementing the changes. The report can be seen here -
http://www.netbeans.org/nonav/issues/showattachment.cgi/62425/report.html. We also created a commented version of this
report, which explains all the main changes in editor and editor.lib modules. It's here -

The other modules that exposed editor classes in their own API are listed below.

editor.plain - It exposed several settings related classes and PlainKit in a public package. To our best knowledge this
API has never been used by any other module. The likely reason for this is that the API didn't offer anything useful. We
removed the API altogether. This is an incompatible change, but since nobody used the API we think it's acceptable.

java.editor.lib - It also exposed some settings related classes in its API. These classes are still available as part of
the editor.deprecated.pre61settings module. Any module that needs those classes will get its dependencies automatically
adjusted to also include editor.deprecated.pre61settings module.

xml.text - The same as for java.editor.lib module. Plus, we had to remove createCompletionForProvider method from
XMLKit, which is an incompatible change. The method was apparently part of the dead completion API and as such must have
been useless for quite some time now. It didn't seem to be used by any Netbeans modules.

Strictly speaking the changes that we have done are not fully backwards compatible. But we did our best to preserve
compatibility whenever it was practical by keeping old classes available in editor.deprecated.* modules and adding
module-auto-deps.xml to editor and editor.lib modules. Many of our changes are result of bad API design such as
implementing SettingsChangeListener on public API classes for example. Removing this, however, should not affect any
decent API clients, because they were never expected to call settingsChanged(SettingsChangedEvent evt) method directly.
Our experience with modules in Netbeans IDE (all clusters) supports this argument. And so we believe that the changes
should not have any significant adverse effect on modules built for previous versions of Netbeans.

In order to help module owners to migrate their code to the new editor settings infrastructure we wrote 'Editor Settings
Upgrade' guide - http://wiki.netbeans.org/EditorSettingsUpgrade

Finally, we would like to mention several open issues:

1. o.n.editor.Acceptor - Various parts of the editor infrastructure require instances of Acceptor. They used to be
supplied as a setting, which is no longer desirable, because java.util.prefs.Preferences does not support complex value
settings (ie. objects). Currently we use settings factories and keep supporting Acceptor as a setting value. In the
future we will probably have to provide better way (ideally declarative in some form of regex) for registering character

2. Duplicate packages - The editor.deprecated.pre61settings module duplicates packages from several other modules. This
is not ideal, but the ultimate goal (at least for Netbeans IDE) is to remove dependencies on this module and not load it
at all. Therefore the duplication should not be a problem. There is one question though. Is it still neccessary to mark
duplicate packages as a special resource? This code in o.n.core.startup.NbInstaller seems to have gone and things seem
to work fine. So we assume that Netbeans classloaders can now handle duplicate packages transparently.

3. Cumulative diff - I'd like to provide a diff that would show all changes, which we've done. Unfortunately, I don't
seem to be able to do so. The changes are in our repository clone (only accessible from SWAN) and there is a lot of them
in many changesets. Some of them were even migrated from the old CVS branch editor_settings_90403. The clone has been
periodically updated with changes from the main repository. The only thing I was able to generate was an unpleasantly
long and IMO useless list of changesets with diffs for each particular change (ie. output from 'hg out -p -M
http://hg.netbeans.org/main'). I know (now) about mq extension, but we haven't been using it. Does anybody have any idea
how to 'merge' all the diffs in a big cumulative one?
Comment 28 Vitezslav Stejskal 2008-06-05 17:53:02 UTC
I know that this is a full review and we are supposed to have a meeting/confcall, but I'm not sure if and how many
people would be interested and actually who to invite. Maybe Jirka Skrivanek who is currently maintaining
openide.options; Tor who is working on GSF; Hanz who knows most about Schliemann; ... I'm sure Jesse and Jarda will have
comments too. I think I'll see what feedback we get on this and if people feel like discussing this in person we can
have a confcall sometime early next week. If not, I'm happy to do it over email.
Comment 29 Vitezslav Stejskal 2008-06-11 10:18:18 UTC
Hi, I know everybody is busy doing their stuff for the milestone and editor settings is not a terribly interesting
topic, but I would like to make sure that people understand what is going to happen. I'd like to push the changes to
main by the end of this week unless of course I get some serious no-go feedback. I would really want to have this in
6.5M1 in order to get the full exposure to testing and real world users.

So, are people generally happy with this to happen, is the transition guide good enough, are the provided docs and
explanations understandable?
Comment 30 Jan Jancura 2008-06-11 13:14:54 UTC
I have some general comments only:
1) Why we still have some special APIs for storing editor specific options? Can we move MIMELookup to core
infrastructure, and use it in other modules too? Why we have two different implementations of Preferences class? Is
editor really so special that it needs special APIs for storing options?
2) Why we even have two different approaches for storing editor specific options - generic solution based on
Preferences, and specific implementations like FontAndColorsSettings, etc...
3) Is not it possible to use core implementation of Preferences to store all editor settings? Like
CorePreferences.getDefault ().node ("/Editor/" + mimeLookup + "/FontAndColors").get ("backgroundColor", Color.white);
Should we really solve the same problems more than once (load, save, definition of default values, data formats, restore
to defaults, export, import, synchronization, notifications...).

Anyway, I agree with proposed change. My comments are more general.
Comment 31 Jesse Glick 2008-06-12 03:51:47 UTC
Generally your changes look reasonable and carefully prepared to me. I'm not knowledgeable enough about the details to
give much in the way of specific comments, but the description of the work you did to upgrade nb.org modules and how
that went inspires confidence that the incompatibilities are manageable and the result works as expected.

Yes it is now fine to have a package split across modules. The new module class loaders do not require any special
notice of this.

To get a cumulative diff should be easy; just ask for the diff of a recent merge with main. Hg 1.0 by default implements
fetch to first update to the remote version and then merge with your local version, so:

1. Go to an up-to-date clone of your repo.

2. hg fetch main-golden

(and assuming this command actually did a merge and made it the new tip)

3. hg tip -p > /tmp/90403.diff

(Use --config diff.git=1 if you do not have [diff] git=1 in your ~/.hgrc; 'hg tip' does not yet support --git or similar
diff options directly.)

There is also an "rdiff" extension command but I have not used it much and I don't think it is needed here.

Using MQ here would probably not be an improvement. MQ is useful when you plan to create, evaluate, edit, and possibly
retract individual patches. For a long sequence of development like you have, a regular branch is better.

I agree with Hanz that it would be nicer for editor-specific settings stored using the Preferences API to actually use
the regular NbPreferences back-end. But this is perhaps off topic for this review and it is certainly not a blocking issue.
Comment 32 Vitezslav Stejskal 2008-06-16 13:42:10 UTC
Thanks for the review. The changes are now in trunk.
Comment 33 Vitezslav Stejskal 2008-06-16 14:17:53 UTC

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