Bug 102261 - Possible generification problem in WizardDescriptor constructor
Possible generification problem in WizardDescriptor constructor
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Dialogs&Wizards
6.x
All All
: P2 (vote)
: 6.x
Assigned To: Jaroslav Tulach
issues@platform
: API_REVIEW_FAST
Depends on: 153672
Blocks: 92762
  Show dependency treegraph
 
Reported: 2007-04-24 02:31 UTC by Jesse Glick
Modified: 2008-12-22 11:51 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
:


Attachments
Possible patch (12.88 KB, patch)
2007-04-24 02:33 UTC, Jesse Glick
Details | Diff
Using ... instead of [] (5.01 KB, patch)
2007-04-26 16:05 UTC, Jaroslav Tulach
Details | Diff
new version of the api for jesse. do you like this? (6.97 KB, patch)
2007-04-27 21:54 UTC, Jaroslav Tulach
Details | Diff
Ok, so I'll integrate on Monday (8.62 KB, patch)
2007-04-28 21:21 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2007-04-24 02:31:31 UTC
Marking P2 just because it is an API issue so we should get it right for 6.0.

I have always been a bit confused by the generification of
WizardDescriptor.{Iterator,Panel} and in fact I have not seen any examples of it
being used successfully (i.e. no warnings, no @SuppressWarnings). Today I tried
to fix the warning in the ant module, but found that I could not, unless I
accepted a pointless cast from WizardDescriptor to ShortcutWizard in
CustomizeScriptWizardPanel.storeSettings. The reason is that I would like my
Iterator and Panel's to be of type <ShortcutWizard> (that is in fact the wizard
I am showing), yet the WizardDescriptor constructor accepts only
Iterator<WizardDescriptor>.

I am attaching a possible patch which shows a change to accept Iterator<?
extends WizardDescriptor>, and its usage in the ant module. But I am not sure if
it is correct. Really I would like to express that for a subclass T of
WizardDescriptor, the superclass constructor should accept Iterator<T> (I guess
- really <? super T extends WizardDescriptor>, if that were legal syntax). But
this is probably impossible? The patch compiles without warnings, but you are
using @SuppressWarnings("unchecked") in various places so that does not mean
much. There is nothing preventing the following mistake:

class W1 extends WizardDescriptor {
  W1() {super(new Iterator<W1>() {...});}
}
class W2 extends WizardDescriptor {
  W2() {super(new Iterator<W1>() {...});}
}

where read/storeSettings on W2's iterator will be passed a W2 although it
expects a W1. Is there some way to prevent this from happening?
Comment 1 Jesse Glick 2007-04-24 02:33:37 UTC
Created attachment 41512 [details]
Possible patch
Comment 2 Jaroslav Tulach 2007-04-26 16:05:40 UTC
Created attachment 41741 [details]
Using ... instead of []
Comment 3 Jaroslav Tulach 2007-04-26 16:06:40 UTC
I guess there is a need for just a cosmetic fix. Jesse, is this ok for you?
Comment 4 Jesse Glick 2007-04-26 18:20:13 UTC
I'm not sure what your patch has to do with the problem I reported. You are not
even looking at the same constructor. ??
Comment 5 Jaroslav Tulach 2007-04-27 14:22:18 UTC
#1 I do not think that your patch with ? extends WizardDescriptor is correct.
#2 Somewhere where you instantiate the ShortcutWizard, just call super() and 
then call setPanelsAndSettings(new ShortcutIterator(), this);
Comment 6 Jesse Glick 2007-04-27 17:46:52 UTC
There is no zero-arg supertype constructor. The available constructors are:

1. (Iterator<WizardDescriptor>). Forces unchecked warnings, as described here,
since I have an Iterator<ShortcutWizard>.

2. (Panel<WizardDescriptor>[]). Incorrect (I have a nontrivial iterator), and
anyway an array of a generic type cannot be constructed without unchecked
warnings. (Which makes your proposed [] -> ... replacement useless, since
varargs do not avoid this warning.)

3. (Panel<Data>[],Data). Same problems as #2.

4. (Iterator<Data>,Data). Fine as a method, but cannot be used for a
constructor, since a this-reference cannot be passed to a super constructor.

It would probably suffice for a no-arg protected constructor to be added as then
I could call setPanelsAndSettings as you suggest. However then there is nothing
preventing someone from forgetting to set an iterator. (There are some places
where data.getIterator(this) != null is checked, implying that sometimes it is
null, but it is not obvious when.) Or the default constructor could just be

@SuppressWarnings("unchecked")
/** Initially no panels, call {@link #setPanelsAndSettings} to use. */
protected WizardDescriptor() {
  this(new Panel<WizardDescriptor>[0]);
}
Comment 7 Jaroslav Tulach 2007-04-27 21:54:11 UTC
Created attachment 41891 [details]
new version of the api for jesse. do you like this?
Comment 8 Jesse Glick 2007-04-27 22:09:07 UTC
Yes, I think that can work.

Probably a good idea to mention in the Javadoc of the new method that the
purpose of calling setPanelsAndSettings separately is to avoid unchecked warnings.

Forgot '#' in @link.

(Check your imports. Looks like I am not the only one for whom Retouche adds
unnecessary imports of nested classes. I need to file a bug about this at some
point. I will add you to CC.)
Comment 9 Jaroslav Tulach 2007-04-28 21:21:33 UTC
Created attachment 41932 [details]
Ok, so I'll integrate on Monday
Comment 10 Jaroslav Tulach 2007-04-30 10:36:09 UTC
new revision: 1.14; previous revision: 1.13
done
Checking in dialogs/manifest.mf;
/cvs/openide/dialogs/manifest.mf,v  <--  manifest.mf
new revision: 1.12; previous revision: 1.11
done
Checking in dialogs/src/org/openide/WizardDescriptor.java;
/cvs/openide/dialogs/src/org/openide/WizardDescriptor.java,v  <--  
WizardDescriptor.java
new revision: 1.47; previous revision: 1.46
done
RCS 
file: /cvs/openide/dialogs/test/unit/src/org/openide/WizardSetDataAndIteratorTest.java,v
done
Checking in 
dialogs/test/unit/src/org/openide/WizardSetDataAndIteratorTest.java;
/cvs/openide/dialogs/test/unit/src/org/openide/WizardSetDataAndIteratorTest.java,v  
<--  WizardSetDataAndIteratorTest.java
initial revision: 1.1
Comment 11 Jesse Glick 2007-04-30 21:25:26 UTC
FYI:

Checking in IntroPanel.java;
/shared/data/ccvs/repository/ant/src/org/apache/tools/ant/module/wizards/shortcut/IntroPanel.java,v
 <--  IntroPanel.java
new revision: 1.18; previous revision: 1.17
done
Checking in SelectFolderPanel.java;
/shared/data/ccvs/repository/ant/src/org/apache/tools/ant/module/wizards/shortcut/SelectFolderPanel.java,v
 <--  SelectFolderPanel.java
new revision: 1.16; previous revision: 1.15
done
Checking in CustomizeScriptPanel.java;
/shared/data/ccvs/repository/ant/src/org/apache/tools/ant/module/wizards/shortcut/CustomizeScriptPanel.java,v
 <--  CustomizeScriptPanel.java
new revision: 1.15; previous revision: 1.14
done
Checking in ShortcutIterator.java;
/shared/data/ccvs/repository/ant/src/org/apache/tools/ant/module/wizards/shortcut/ShortcutIterator.java,v
 <--  ShortcutIterator.java
new revision: 1.19; previous revision: 1.18
done
Checking in SelectKeyboardShortcutPanel.java;
/shared/data/ccvs/repository/ant/src/org/apache/tools/ant/module/wizards/shortcut/SelectKeyboardShortcutPanel.java,v
 <--  SelectKeyboardShortcutPanel.java
new revision: 1.15; previous revision: 1.14
done
Checking in ShortcutWizard.java;
/shared/data/ccvs/repository/ant/src/org/apache/tools/ant/module/wizards/shortcut/ShortcutWizard.java,v
 <--  ShortcutWizard.java
new revision: 1.7; previous revision: 1.6
done


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