Documentation is here:

http://www.netbeans.org/download/dev/javadoc/org-netbeans-modules-options-api/

Not sure if this architecture change is trivial and non-controversial, but it's
definitely significant enough to deserve a full review.

As this code is being planned to be handed over to the Core team in the future,
I request a full review with clear documentation of the existing API design and
a discussion about any outstanding issues.

I have no problem with standard review, just another meeting. But I thought that
we will switch to full review when we will find some controversial theme. I have
already had about 5 reviews of this module, so its starts to be boring.

Documentation is there...

The only outstanding issue, I know about, is dependency on FormLayut, ant it is
discussed...

Due to current archicture the review of editor/options is desirable as well.
Dependencies of implementation in editor/options are also interesting in this
context as it might be really hard to use this infrastructure in another product
(platform use). The implementation also suffers from performance problems.

Documentation is poor IMO - almost no doc in arch document for example
http://www.netbeans.org/download/dev/javadoc/org-netbeans-modules-options-api/org/netbeans/spi/options/OptionsCategory.PanelController.html#isValid()
seems to be wrong

No tests that can guarantee that someone can take ownership and start further
development.

L&F designed for Windows users using default colors schemes.

1) editor/options is different module, and there will be separate DevRev for it.
There are 4 new modules, I should prepare documentation for them, etc... So lets
start with core/options, and let start with concrete issues.

2) "The implementation also suffers from performance problems." This is always
true. NetBeans suffers too. Lets start to be concrete. I have one issue there -
first start takes 2 seconds (second start is OK). I do not see it as some
stopper problem. Progress is there. Moreover this is editor/options module issue.

3) "Documentation is poor IMO". Can you be more concrete? You can always say it
about every module.

4) Tests should be there, you are right. This is valid issue.

5) "L&F designed for Windows users using default colors schemes." Simply not
true. I personally do not use default window color scheme, and new OD follows my
color scheme. Have you tested it? There is again some concrete issue which will
be fixed. I am working on UI it with usability team, DevRev should solve
different problems, as I know...

I hope that DevRev will be focused on concrete issues.

Re 1) IMO, it would be simplier (and more efficient) for everybody to do one
review for all the options modules (so I propose to do only one review for all).

A few comments on the core/options:
1. The SPI is based on abstract classes. I propose to make them interfaces.
2. I propose to make OptionsCategory.PanelController a top-level interface (or
class if necessary).

(These are TCAs for 5.0 from my point of view (as the SPI is under development).)

?!?!?!
WHY?

When I write some API with interfaces, I hear many TCRs from core guys (Jarda,
Mr. Kubacki) that its "completetely wrong" and that I should use abstract
classes. And now you are arguing the oposite way. Am I stupid, or DevRev is some
discussion club olny?

I am not following this case closely so just general comments and one example.

You cannot decide abstract classes vs interfaces w/o talking about WHY to use
one or another. So I guess that makes us a discussion club in your eyes :-)

I suppose the abstract classes recommendation could be to allow extensibility.
Here is what I did in j2ee/ejbapi, just as an example: There is an API that
every module can call and a corresponsing SPI that is implemented by ejbproject
and ejbfreeform. The API delegates to the SPI. The API uses final classes so
that it is easily extensible. The SPI uses interfaces. This was designed in 4.1.
When I needed to add functionality in 5.0 I added a method into the final class
and I added a new interface into the SPI. So far so good.
OTOH, designing the SPI as interfaces had one problem -- that anybody can use
directly the SPI. That's why in the end Jarda suggested to use abstract classes
with protected methods to hide the methods implementation and force clients to
use the API instead.. (that was a _suggestion_, not implemented yet).

I have created the opinions document (empty for now):
http://openide.netbeans.org/tutorial/reviews/opinions_64774.html

Re: interface/abstract class: I will add some comments to the opinions document.

Here are just 2 small comments on API:

pb01 - a11y
You have name, icon and title in category, display name and tooltip in advanced.
Do we need accessible names, etc.?
Keyboard navigation does now allow to open advanced option, but that's probably
just a bug.

pb02 - don't refer to button names in javadoc
Method PanelController.applyChanges() says it is triggered by Ok button. This is
true now, but it seems the API is designed so that "Apply" could be added later
(I know, the old options won't work, but maybe in some bright distant future).
Refering to OK button suggests that the component will be closed immediately
after the changes are applied.

Here are my comments:

AB1. The use cases are not sufficiently documented in the answer to the
arch-usecases question of the arch document. For example there is no mention of
the AdvancedOption class, although it is used to cover one of the two use cases.
The answer to this question should at least tell the user which classes to use
and point him to the Javadoc.

AB2. The answers to some arch document questions (especially in the Execution
Evironment and Performance and Scalability sections) are completely unuseful and
their style makes them not eligible to be part of the NetBeans API documentation.

AB3. The Javadoc is insufficient:

- all method parameters should be documented. For example the meaning of the
masterLookup parameter of OptionsCategory.PanelController.getComponent() is not
obvious just by looking at that method's Javadoc, but it should be. 

- all fields should be meaningfully documented. For example the "Property name
constant" Javadoc of OptionsCategory.PanelController.PROP_CHANGED doesn't
contain any useful information.

- the order in which the methods are called should be documented. For example
suppose an implementation of PanelController needs access to the master lookup
in its implementation of the update() method. Is is guaranteed that
getComponent() will be called before update()? If yes, the Javadoc should state
it. Also, is it guaranteed that getLookup() will be called before
getComponent()? If yes, document it. If not, why not?

- the purpose of the PanelController.addPropertyChangeListener() method is not
clear. Which properties should the PCL's be notified about? Only the changed,
valid and helpCtx properties, or every property of the JComponent returned by
PanelController.getComponent() method, as the implementation of the Ant and GUI
Builder advanced panels suggest? If the latter is true, why is it necessary?

- the name of OptionsCategory.getIconBase() suggests some suffixes are appended
to its return value. If yes, document which ones? If not, why the "base" name?

- should OptionsCategory.getIconBase() return an icon base with extension, as
AbstractNode.setIconBaseWithExtension() expects? If so, document this. If not,
document the extension which will be added to the return value of getIconBase().

- the PanelController applyChanges() and cancel() methods are not called in the
EDT, but this isn't documented

AB4. What good brings calling update(), applyChanges and update() in a
RequestProcessor to the SPI user? The implementation will likely have to make
Swing calls, therefore it will need to reschedule to the event dispatcher thread
anyway. Calling these methods in another thread will only cause the users to
forget they have to reschedule, as it happens in the implementation of the Ant
and GUI Builder advanced panels.

AB5. The create() method name suggests it does something else. Usually when you
have a create() method in a class Xyz, you expect it to return an instance of
either Xyz or a subclass of Xyz. Consider renaming the method to something more
meaningful, such as createPanelController().

AB6. The fonts and colors for all languages in the Fonts & Colors category
(apart from SQL) are still in ide/defaults instead of their respective modules.
I was told this is a temporary solution. Will the fonts and colors be moved to
their modules in time for 5.0?

I also have a few comments to the implementation:

AB7. The AdvancedOption.create() and the PanelController getComponent() and
update() methods are called when the Options dialog is opened, although the
Miscellaneous panel is not active. What prevents the creation of the advanced
panels from being lazy?

AB8. Both the option categories and the advanced panels seem to be created only
once (when the Options dialog is opened for the first time) and cached, probably
in an attempt to improve the performance when the dialog is subsequently opened.
But then you have to listen for changes on the Lookup.Result's used to create
the OptionCategory and AdvancedOption instances. The installation of a new
module, for example, could add a new option category.

AB9. The PanelController.getLookup() method doesn't seem to be called and the
masterLookup parameter is null when getComponent() is called for a
PanelController representing an advanced panel.

AB10. The tooltips for the advanced panels don't seem to be displayed anywhere.

AB11. The PanelController.update() method seems to be called in a
RequestProcessor only on the first invocation of the Options dialog. On
subsequent invocations it is called in the EDT.

MM01:
Are the registrations of the OptionsCategory and AdvancedOption impls the only
things that the options module reads from the XML layer? I'm wondering if there
isn't any hidden API in the layer that should be further documented in the arch doc.

fixed in trunk:


IDE:-------------------------------------------------
IDE: [25.11.05 14:27] Committing "Options Module - Editor" started
Checking in keymap/EditorBridge.java;
/cvs/editor/options/src/org/netbeans/modules/options/keymap/EditorBridge.java,v
 <--  EditorBridge.java
new revision: 1.5; previous revision: 1.4
done
Checking in keymap/KeymapViewModel.java;
/cvs/editor/options/src/org/netbeans/modules/options/keymap/KeymapViewModel.java,v
 <--  KeymapViewModel.java
new revision: 1.27; previous revision: 1.26
done
Checking in macros/MacrosModel.java;
/cvs/editor/options/src/org/netbeans/modules/options/macros/MacrosModel.java,v 
<--  MacrosModel.java
new revision: 1.9; previous revision: 1.8
done
IDE: [25.11.05 14:27] Committing "Options Module - Editor" finished

sorry for previous message - it was mistake.
But anyway, review has passed.