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: | Bug in WizardDescriptor - problem with bugfix #25820 | ||
|---|---|---|---|
| Product: | platform | Reporter: | Todd Fast <tfast> |
| Component: | Dialogs&Wizards | Assignee: | Jiri Rechtacek <jrechtacek> |
| Status: | VERIFIED FIXED | ||
| Severity: | blocker | CC: | jglick, jkovar, jtulach, ttran |
| Priority: | P1 | Keywords: | REGRESSION |
| Version: | 3.x | ||
| Hardware: | All | ||
| OS: | All | ||
| Issue Type: | DEFECT | Exception Reporter: | |
| Bug Depends on: | 26552 | ||
| Bug Blocks: | 34405 | ||
| Attachments: |
demo
a proposed change (allow change w/o firePCh) |
||
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. 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. 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. Created attachment 11251 [details]
demo
Created attachment 11253 [details]
a proposed change (allow change w/o firePCh)
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 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)
...
added "zebra" to the status whiteboard field 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. Is this SIMPLEFIX? IMHO if we do some 3.5.2 bugfix release, this should definitely be included. x |
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.