Please use the Apache issue tracking system for new NetBeans issues (https://issues.apache.org/jira/projects/NETBEANS0/issues) !!
Bug 35266 - Bug in WizardDescriptor - problem with bugfix #25820
Bug in WizardDescriptor - problem with bugfix #25820
Status: VERIFIED FIXED
Product: platform
Classification: Unclassified
Component: Dialogs&Wizards
3.x
All All
: P1 (vote)
: TBD
Assigned To: Jiri Rechtacek
issues@platform
zebra
: REGRESSION
Depends on: 26552
Blocks: 34405
  Show dependency treegraph
 
Reported: 2003-08-04 18:32 UTC by Todd Fast
Modified: 2008-12-23 11:03 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
demo (4.22 KB, text/plain)
2003-08-08 11:55 UTC, Jiri Rechtacek
Details
a proposed change (allow change w/o firePCh) (3.29 KB, patch)
2003-08-08 14:27 UTC, Jiri Rechtacek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Todd Fast 2003-08-04 18:32:05 UTC
We've encountered and verified a significant problem with 
the org.openide.WizardDescriptor class in Sun ONE Studio 
5/OpenAPI 3.5.  This problem appears to break the API for 
the Netbeans wizard infrastructure.  We also believe that 
this problem affects OpenAPIs version 3.5.1.  We 
encountered this problem when trying to upgrade the S1AF 
module from Sun ONE Studio 4.1 (~OpenAPI 3.4) to Sun ONE 
Studio 5 (OpenAPI 3.5).

The problem stems from a bugfix added to address bug 
#25820.  The code in question resides in the 
WizardDescriptor.setValue() method:

    public void setValue(Object value) {
        //Bugfix #25820: Call resetWizard to make sure that 
storeSettings
        //is called before propertyChange.
        Object convertedValue = backConvertOption(value);
        if (convertedValue == OK_OPTION) {
            resetWizard();
        }
        super.setValue(backConvertOption(value));

        // #17360: Reset wizard on CLOSED_OPTION too.
        if(value == CLOSED_OPTION) {
            resetWizard();
        }
    }

When the user presses a button on the wizard (Next, 
Previous, Finish, or Cancel), a listener attached to these 
buttons invokes the setValue() method on the 
WizardDescriptor.  The value in question is the option 
value selected by the user, typically NEXT_OPTION, 
PREVIOUS_OPTION, FINISH_OPTION, or CANCEL_OPTION.  The 
WizardDescriptor API includes a method called getValue(), 
which is used by module wizard code to determine which 
button on the wizard the user pressed last.

In addition to setting the last-invoked option, the current 
panel's storeSettings() method should be called to allow 
the panel to store its current data.  The intent of the 
bugfix code is to call the current WizardIterator.Panel 
instance's storeSettings() method before a property change 
event is fired by the call to super.setValue().  The 
resetWizard() call invokes the storeSettings() method on 
the current panel.  Note that this code path is only 
executed when the user presses the Finish button.

The bug is caused by the fact that the getValue() method is 
commonly used within a panel's storeSettings() method to 
conditionalize state-saving logic.  For example, if the 
user presses the Cancel button (the CANCEL_OPTION option), 
the current panel's storeSettings() method should avoid 
validating panel data.  If it doesn't, it will either lock 
the user into the wizard, or show an error when it 
shouldn't.  Similarly, validation should be avoided when 
the user opts to move to a prior panel in the wizard.

However, as you can see in the code snippet above, the 
value property is not set to the current value when 
resetWizard(), hence storeSettings(), is invoked.  The 
result is that the value available to module wizard code 
via getValue() is the *previous* value set via the call to 
setValue().  In other words, the available value is stale.  
For example, when the user presses the Finish button on a 
wizard panel, the module wizard code might actually receive 
the NEXT_OPTION when it calls getValue(), because the 
FINISH_OPTION value has not yet been set via the setValue() 
method.  As you can imagine, this behavior leads to very 
unpredictable results in which the module code may receive 
nearly any of the options in response to a button press on 
the wizard.  As a result, the getValue() method becomes 
completely unreliable.

Because the WizardDescriptor used by the IDE is a 
singleton, this stale data persists between wizard 
invocations.  In addition, if the first time a wizard is 
invoked in the IDE the user selects the Finish button on 
the first panel, the value the getValue() method returns is 
null, which can cause NullPointerExceptions in storeSettings
() methods trying to use the getValue() method.

Unfortunately, the intended bugfix and the proper API 
behavior seem to be irreconcilable, as there is now way to 
populate the superclass's value member except by calling 
its setValue() method.  But, calling this method will incur 
a property change event, which is precisely what the bugfix 
is trying to avoid.  A real fix to this problem will 
require changes in the superclass to allow subclasses to 
call setValue() or an equivalent method without 
(immediately) firing a property change event.

Our current concern with this bug is that it makes it 
difficult, if not impossible, to port existing wizard code 
to Sun ONE Studio 5/OpenAPI 3.5, at least without a 
significant rewrite to try and eliminate any use of the 
WizardDescriptor.getValue() method.  However, our module's 
wizards have extensive validation needs, and it's not clear 
to us how we can satisfy those requirements without using 
the getValue() method.
Comment 1 Jesse Glick 2003-08-04 18:57:59 UTC
In the future we should be much more careful about documenting and
unit-testing behavioral aspects of an API, such as which methods will
be called when, and so on. For more complex cases such as the Wizards
API, UML state diagrams may well be appropriate.
Comment 2 Jiri Rechtacek 2003-08-08 09:32:39 UTC
Evaluation: Todd, you are right, there are two opposite view how to
order firing PROP_VALUE and calling storeSettings(). The documentation
don't say about, the order is not specified.

The current state was established by bugfix issue 25820, some Wizards
API's users count with them, it's danger change this (unutterable)
contract now. It must be solve expressively in framework Cleanup
Wizard API (issue 26552).

There is a possible workaround (crooked hack): extending
WizardDescriptor and override setValue() method and add two new
methods: get/setFutureValue() with value which will be fired later. (I
attach a demo to demonstrate it). Todd, is it acceptable for you?
Thanks for uncover this problem, it'll be keep in view in changes
Wizard API.
Comment 3 Todd Fast 2003-08-08 10:20:22 UTC
Jiri, thanks for confirming the problem.  IMHO, the 
problematic change made to WizardDescriptor.setValue() is 
more than just a matter of documentation or difference of 
perspective; it's a definite bug in its own right, since it 
has made the getValue() method in the wizard panel useless.

Wouldn't factoring out the property change notification 
from the superclass's setValue() method work?  For example, 
make a protected method on the superclass called _setValue
() that didn't fire a change event, and wrap it with a 
public setValue() method.  Have setValue() in the 
superclass call _setValue() then fire the change event.  
But, in the subclass (WizardDescriptor), override setValue
() but avoid calling super.setValue(). Instead, call 
_setValue() and fire the change notification in the proper 
sequence with storeSettings().

In any case, your excellent suggestion of extending 
WizardDescriptor would work when our module initiates a 
wizard.  However, is it not true that this would have no 
effect on wizards invoked via the File -> New action?  In 
other words, it seems that this workaround would not work 
for the IDE-managed WizardDescriptor instance.

Unless you can implement a fix like the one I describe 
above and provide it as a patch to the already released 
Studio/Netbeans, our only hope seems to be to eliminate any 
and all reliance on the getValue() method in 
WizardDescriptor.  Can you please tell us if you could 
implement the fix I outlined?  I don't want to begin 
changes to our code without a definite answer from your 
team on whether you can provide a hotfix.
Comment 4 Jiri Rechtacek 2003-08-08 11:55:10 UTC
Created attachment 11251 [details]
demo
Comment 5 Jiri Rechtacek 2003-08-08 14:27:35 UTC
Created attachment 11253 [details]
a proposed change (allow change w/o firePCh)
Comment 6 Jiri Rechtacek 2003-08-08 14:37:49 UTC
I attached the patch based on your proposal, it works for me in my
test wizard and in chosen wizards. I don't ask any API change I might
apply w/o notification other users. Could you look on it and try if it
help you? Thanks
Comment 7 Todd Fast 2003-08-09 00:44:17 UTC
Jiri--

Unfortunately, the patch will not work for this issue 
because the WizardDescriptor.Listener class is not the one 
that calls storeSettings() when the Finish button is 
pressed.  Instead, a listener attached directly to the 
button calls setValue() directly--perhaps this is also a 
bug.  In any case, WizardDescriptor.setVale() is where I 
imagined you would put the fix, as that is the code that 
changed according to bugfix #25820.  Please see the 
following stack trace for more info.

java.lang.Exception: Tracing from inside of storeSettings
    at 
com.sun.jato.tools.sunone.ui.command.CreateCommandSelectionP
anel.storeSettings(CreateCommandSelectionPanel.java:474)
    at org.openide.WizardDescriptor.resetWizard
(WizardDescriptor.java:751)
    at org.openide.WizardDescriptor.setValue
(WizardDescriptor.java:738)
    at 
org.netbeans.core.NbPresenter$ButtonListener.actionPerformed
(NbPresenter.java:932)
    at javax.swing.AbstractButton.fireActionPerformed
(AbstractButton.java:1786)
    at 
javax.swing.AbstractButton$ForwardActionEvents.actionPerform
ed(AbstractButton.java:1839)
    at javax.swing.DefaultButtonModel.fireActionPerformed
(DefaultButtonModel.java:420)
    at javax.swing.DefaultButtonModel.setPressed
(DefaultButtonModel.java:258)
    at 
javax.swing.plaf.basic.BasicButtonListener.mouseReleased
(BasicButtonListener.java:245)
    at java.awt.Component.processMouseEvent
(Component.java:5100)
    at java.awt.Component.processEvent(Component.java:4897)
    at java.awt.Container.processEvent(Container.java:1569)
    at java.awt.Component.dispatchEventImpl
(Component.java:3615)
    at java.awt.Container.dispatchEventImpl
(Container.java:1627)
    at java.awt.Component.dispatchEvent(Component.java:3477)
    at java.awt.LightweightDispatcher.retargetMouseEvent
(Container.java:3483)
    at java.awt.LightweightDispatcher.processMouseEvent
(Container.java:3198)
    at java.awt.LightweightDispatcher.dispatchEvent
(Container.java:3128)
    at java.awt.Container.dispatchEventImpl
(Container.java:1613)
    at java.awt.Window.dispatchEventImpl(Window.java:1606)
    at java.awt.Component.dispatchEvent(Component.java:3477)
    at java.awt.EventQueue.dispatchEvent
(EventQueue.java:456)
    at java.awt.EventDispatchThread.pumpOneEventForHierarchy
(EventDispatchThread.java:201)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy
(EventDispatchThread.java:151)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy
(EventDispatchThread.java:141)
    at java.awt.Dialog$1.run(Dialog.java:540)
    at java.awt.Dialog.show(Dialog.java:561)
    at org.netbeans.core.NbPresenter.superShow
(NbPresenter.java:690)
    at org.netbeans.core.NbPresenter.doShow
(NbPresenter.java:733)
    at org.netbeans.core.NbPresenter.run
(NbPresenter.java:721)
    at org.openide.util.Mutex.doEventAccess(Mutex.java:932)
    at org.openide.util.Mutex.readAccess(Mutex.java:157)
    at org.netbeans.core.NbPresenter.show
(NbPresenter.java:706)
    at org.openide.loaders.TemplateWizard.instantiateImpl
(TemplateWizard.java:425)
    at org.openide.loaders.TemplateWizard.instantiate
(TemplateWizard.java:358)
    at 
com.sun.jato.tools.sunone.ui.common.WizardUtil.executeWizard
(WizardUtil.java:429)
...
Comment 8 _ ttran 2003-08-19 07:34:46 UTC
added "zebra" to the status whiteboard field
Comment 9 Jiri Rechtacek 2003-08-21 13:46:31 UTC
fixed in maintrunk. I added a package-private method
setValueWithoutPropertyChange which allows change a value w/o notify
directly all listeners. But, caller must firePropertyChange itself to
keep the consistent state. Else, I changed WizardDescriptor.setValue()
implementation to call storeSettings after new value is set and more
other changes. I guess it will be solve described problem.
I added WizardDescTest to openide/unit's stable testbag, it should
indicate when a contract might be broken.
I didn't any API => removed API keyword.

Todd, thank you for feedback and support within fixing. Because, your
team found a workaround I didn't assume this fix should be integrated
in any patch-branch. The issues about the clear contract will keep in
mind in next work on wizard framework, especially if will Wizard API
redesigned. Best regards.
Comment 10 Jesse Glick 2003-08-21 14:45:35 UTC
Is this SIMPLEFIX? IMHO if we do some 3.5.2 bugfix release, this
should definitely be included.
Comment 11 Lukas Hasik 2004-02-24 11:15:21 UTC
x


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