Community
Participate
Working Groups
There is a required field in wizard A, so there is an error message shown in this wizard A like "*** can't be empty"; to enter this field, there is a "New" button next to this field, so click New button to pop up another wizard B, enter an invalid value in wizard B, it will display an error message in wizard B, then the error message can be disappeared after the correct valued entered, click Finish button in wizard B. Now it goes back to wizard A, the value which entered in wizard B is displayed in wizard A, so the error message "*** can't be empty" should be disapperared from wizard A, but the error message is still there. The problem may caused by org.eclipse.jface.util.Policy.java, there is a static field animator, which shares by others.
Could you please identify 'Wizard A'? This will give us a starting point to track this down.
Over to you Shaun...
Wizard A is our compoent which is not in Eclipse,I tried to find a same one in Eclipse, but I can't. This problem can be easly re-produced: 1.create a wizard A with a New button and a required field,this required field should be entered through opening another wizard B, 2.In wizard A, an error message displayed because of the required field missed 3.click New button in wizard A to bring up wizard B, enter an invalid value to display an error message in wizard B, then enter a valid value to Finish in wizard B 4. Since the value entered in step3 has been passed to wizard A, the error message displayed in step2 should be disappeared, but it is still there.
Created attachment 37803 [details] Changes to multiple classes in JFace and workbench to fix bug This patch changes the way control animators are allocated. There used to be a static animator to handle all animations, now there is an animator factory that can be used to create multiple animators. Note: This is a breaking API change but it should not be a big issue since I don't believe anyone other than myself has begun using the new control animation API.
Any API breakage at this point is a big issue, but after looking through this I think it may be necessary. The existing API is flawed because it only allows a single global animator instance for all of JFace. If you want multiple animators active at any given time, there is no hope. This fix looks wrong though - it seems to repeat the same mistake by having a static ControlAnimator instance within the animator factory. There must be NO global mutable state for multiple concurrent animations to be possible. Each dialog instance should instead hold onto the animator instance corresponding to its controls. I also don't see why the get/setAnimationState is necessary. It seems a client should just be able to call ControlAnimator.setVisible(true/false), and have the animator internally track its own state without exposing it as API.
Thanks John for raising some very good points. The Factory should not have held onto a static instance of an animator. Also, while the animation state was initially being used by the dialogs to determine when to set the bounds, after further review it seems that other flags can be used to accomplish the same thing and that the animators can in fact internally track their own states. Based on these changes, some API has been modified and some has been removed; the following are the comments and method signatures for the proposed changes: AnimatorFactory: -added /** * Factory for control animators used by JFace to animate the display of an SWT * Control. Through the use of the method * {@link org.eclipse.jface.util.Policy#setAnimatorFactory(AnimatorFactory)} * a new type of animator factory can be plugged into JFace. * * @see Policy#setAnimatorFactory(AnimatorFactory) * @since 3.2 * */ public class AnimatorFactory { /** * Creates a new ControlAnimator for use by JFace in animating * the display of an SWT Control. * * @param control the SWT Control to de displayed * @return the ControlAnimator. * @since 3.2 */ public ControlAnimator createAnimator(Control control) { return new ControlAnimator(control); } } WorkbenchAnimatorFactory: -added /** * Factory for workbench control animators to be used by JFace in animating * the display of an SWT Control. * * @since 3.2 * */ public class WorkbenchAnimatorFactory extends AnimatorFactory { /** * Creates a new WorkbenchControlAnimator for use by JFace in animating * the display of an SWT Control. * * @param controlthe SWT Control to de animated * @return animator the WorkbenchControlAnimator */ public ControlAnimator createAnimator(Control control); ControlAnimator: -added /** * Constructs a new ControlAnimator instance and passes along the * control that will be displayed or hidden. * * @param control the control that will be displayed or hidden. */ public ControlAnimator(Control control); -changed /** * Displays or hides a control at the bottom of the parent composite. * * @param visible <code>true</code> if the control should be shown, * and <code>false</code> otherwise. */ public void setVisible(boolean visible); -changed all public state variables to protected -changed get/setAnimationState to protected WorkbenchControlAnimator -added /** * Constructs a new WorbenchControlAnimator instance and passes along the * control that will be animated. * * @param control the control that will be animated. */ public WorkbenchControlAnimator(Control control); -changed /* (non-Javadoc) * @see org.eclipse.jface.dialogs.ControlAnimator#setVisible(boolean) */ public void setVisible(boolean visible); Policy: -changed /** * Sets the animator factory used to create control animator * instances. * * @param factory the AnimatorFactory to use. * @since 3.2 */ public static void setAnimatorFactory(AnimatorFactory factory); -changed /** * Returns the animator factory used to create control animator * instances. * * @return the animator factory used to create control animator instances. * @since 3.2 */ public static AnimatorFactory getAnimatorFactory();
Created attachment 37999 [details] UPDATED Changes to JFace and workbench animators This patch changes some API to fix the static animator issue.
A few comments: AnimatorFactory - The class level Javadoc should specify that clients may subclass. The Javadoc for createAnimator() should specify that subclasses should override. ControlAnimator - The Javadoc says "This class is not intended to be extended by clients" but the Workbench is a client who extends this class. Unless we have good reasons for this, I don't think we should treat the Workbench specially. - getAnimationState/setAnimationState: These methods are protected and therefore only useful for subclasses. Since clients cannot call these methods, and the base class ControlAnimator does not really use them (it can call control.isVisible() to query the current state), they seem to be an implementation detail of WorkbenchControlAnimator. Could these methods (and the constants) be moved down to that class (and made private)? - setVisible: The spec should say that this method may be overridden by subclasses and that they should not call the super implementation. It should also say that when the method returns, the value returned by the control's isVisible() method matches the argument.
Created attachment 38201 [details] UPDATED Changes to JFace and workbench animators Changes based on ideas expressed in comment #8.
What is the status of this?
The patch looks good now.
fixed in HEAD
+1. ok to leave the patch in. :-)
