Bug 144060 - API review - UI opened through friend package
API review - UI opened through friend package
Status: RESOLVED FIXED
Product: apisupport
Classification: Unclassified
Component: Project
6.x
All All
: P2 (vote)
: TBD
Assigned To: apireviews
issues@apisupport
http://wiki.netbeans.org/PlatformSuit...
: API_REVIEW_FAST
Depends on:
Blocks: 142283
  Show dependency treegraph
 
Reported: 2008-08-15 13:29 UTC by Andrew Korostelev
Modified: 2008-08-29 13:20 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
inner panel that will be exposed (47.38 KB, image/jpeg)
2008-08-20 12:04 UTC, Andrew Korostelev
Details
patch for apisupport.project (120.65 KB, patch)
2008-08-26 01:18 UTC, Andrew Korostelev
Details | Diff
patch for vmd.componentssupport to use API exposed by previous patch (119.61 KB, patch)
2008-08-26 01:19 UTC, Andrew Korostelev
Details | Diff
updated patch for apisupport.project. Contains test and some updates in implementation. (129.97 KB, patch)
2008-08-26 20:46 UTC, Andrew Korostelev
Details | Diff
patch for apisupport.project with test updated according to jglick comments (128.51 KB, patch)
2008-08-28 09:34 UTC, Andrew Korostelev
Details | Diff
patch for apisupport.project with jtulach commnets (133.39 KB, patch)
2008-08-28 20:38 UTC, Andrew Korostelev
Details | Diff
patch for vmd.componentssupport with jtulach commnets (137.91 KB, patch)
2008-08-28 20:38 UTC, Andrew Korostelev
Details | Diff
patch for apisupport.project with jtulach commnets from Thu Aug 28 21:52:03 (138.37 KB, patch)
2008-08-29 11:25 UTC, Andrew Korostelev
Details | Diff
patch for vmd.componentssupport with jtulach commnets from Thu Aug 28 21:52:03 (137.08 KB, patch)
2008-08-29 11:26 UTC, Andrew Korostelev
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Korostelev 2008-08-15 13:29:13 UTC
The goal is to open UI panel for Platform and Suite selection, used in first step of NetBeans Module project creation
wizard through friend package.
vmd.componentssupport is a friend project which will use exposed UI.

wizard in vmd.componentssupport creates Java ME Visual Designer component. Which is actually a NetBeans Module project
with specific set of descriptor classes and xml updates.

While result is NetBeans Module project, UI Review (issue 136248) required vmd.componentssupport to have the same UI for
module platform and suite selection as apisupport.project.

issue 142283 was filed with request to expose existing implementation through friend package.

related wiki page: http://wiki.netbeans.org/PlatformSuiteUIExposedFromApisupport
Comment 1 Andrew Korostelev 2008-08-15 13:38:34 UTC
please find latest patch version in issue 142283
Comment 2 Jaroslav Tulach 2008-08-15 16:53:36 UTC
Y01 I'd like to see minimalistic API. A single class with factory method public static WizardDescriptor.Panel create() 
would be enough, imho. Maybe plus public static setters like: public static void setProjectFolder(wizardDescriptor, 
folder) and public static FileObject getProjectFolder(wizardDescriptor).

Y02 Replace public static final String with methods like set/getProjectFolder

Y03 No need for updateUI method. Instead the panel shall listen on changes in WizardDescriptor properties via PCL

Y04 As I am suggesting to simplify the API but make the logic more complicated: That means we also need tests to 
verify that the functionality really works. Please cover the "Usage" scenario with automated test.

Btw. I really like the "Usage" section, imho it is the most valuable info about the need for the API change.
Comment 3 Andrew Korostelev 2008-08-20 11:58:52 UTC
jtulach, I can follow your recommendations but have some doubts.

>> A single class with factory method public static WizardDescriptor.Panel create() would be enough, imho. 
It wasn't clear from my description, that plan was to expose only part of wizard panel UI (screenshot is attached).
WizardDescriptor.Panel on "Name and location" step in apisupport has too complex Visual Component, to expose it
completely. It supports NB module project types that vmd.componentssupport will not handle.
Anyway will change to use class with factory method.

>> Y02 Replace public static final String with methods like set/getProjectFolder
Do you mean to replace read(WizardDescriptor) and store(WizardDescriptor) by getter/setter for 5 properties?
Can do, but this will make API less minimalistic. 
And I will have to provide wizardDescriptor using some non-traditional way (commonly panel gets wizardDescriptor on
read/store/validate).

>>Y03 No need for updateUI method. Instead the panel shall listen on changes in WizardDescriptor properties via PCL
agree. Have added add/removePCL and updateUI methods just to make API more clear.
Comment 4 Andrew Korostelev 2008-08-20 12:04:38 UTC
Created attachment 67946 [details]
inner panel that will be exposed
Comment 5 Andrew Korostelev 2008-08-21 14:48:46 UTC
I change API in the following way:
- rename updateUI to setProjectFolder(WizardDescriptor)
- remove store/read methods
updated property values will be stored to WizardDescriptor right after modification
- remove add/removePropertyChangeListener methods
outer panel will listen to changes in WizardDescriptor
- remove validate method
updated property values will be validated and error message will be stored to WizardDescriptor right after modification

5 property names are leaved as "public status final String" attrs:
ACTIVE_PLATFORM
IS_STANDALONE
IS_SUITE_COMPONENT
SUITE_ROOT
IS_NETBEANS_ORG
Comment 6 Jaroslav Tulach 2008-08-22 14:41:41 UTC
OK, I am looking forward to see new patch.
Comment 7 Andrew Korostelev 2008-08-26 01:17:06 UTC
New patch is in the following attachments.
wiki page is updated: http://wiki.netbeans.org/PlatformSuiteUIExposedFromApisupport
Comment 8 Andrew Korostelev 2008-08-26 01:18:22 UTC
Created attachment 68291 [details]
patch for apisupport.project
Comment 9 Andrew Korostelev 2008-08-26 01:19:18 UTC
Created attachment 68292 [details]
patch for vmd.componentssupport to use API exposed by previous patch
Comment 10 Andrew Korostelev 2008-08-26 20:46:07 UTC
Created attachment 68379 [details]
updated patch for apisupport.project. Contains test and some updates in implementation.
Comment 11 Jesse Glick 2008-08-27 19:37:00 UTC
I don't really have time to review in detail. I hope rmichalsky is doing so?


[JG01] Why does template_netbeansorg exist? I don't think there is any need for this template to be available inside the
nb.org source tree.


[JG02] It might be possible to replace TestPCL with MockPropertyChangeListener (o.o.util.test).
Comment 12 Andrew Korostelev 2008-08-27 20:05:01 UTC
Thanks for review
>> I hope rmichalsky is doing so?

>> [JG01] Why does template_netbeansorg exist? I don't think there is any need for this template to be available inside
the nb.org source tree.
It is necessary to support absolutely the same cases as Nb Module project creation wizard does (as was requested in UI
review).

>>[JG02] It might be possible to replace TestPCL with MockPropertyChangeListener (o.o.util.test).
thanks. will do.
Comment 13 Andrew Korostelev 2008-08-28 09:32:12 UTC
>> [JG]I hope rmichalsky is doing so?
He has reviewed before jtulach.
Comment 14 Andrew Korostelev 2008-08-28 09:34:01 UTC
Created attachment 68511 [details]
patch for apisupport.project with test updated according to jglick comments
Comment 15 Jaroslav Tulach 2008-08-28 15:37:04 UTC
Y11 Why there is not void setProjectFolder(WizardDescriptor settings, FileObject folder)? Why the method has so 
complicated Javadoc?

Additional P3 comments:
Y12 I still do not like (Boolean)wizardDescriptor.getProperty(ModuleTypePanel.IS_NETBEANS_ORG) and similar I'd like it 
to be replaced with ModuleTypePanelCreator.isNetBeansOrg(wizardDescriptor).

Y13 I would get rid of the ModuleTypePanel interface completely and instead had:
class ModuleTypePanelCreator {
  public static boolean validate(WizardDescriptor wd);
  public static void setProjectFolder(WizardDescriptor wd, FileObject folder);
  public static JComponent createComponent(WizardDescriptor wd);
}
then the client API would work as follows. Prepare wd, createComponent(...) display it somewhere, use static methods 
to change values in wd and get values from wd.
the implementation would display a component which adds listener to wd and observe all changes in the wd values and 
update its state according to them and vice versa.

Comment 16 Andrew Korostelev 2008-08-28 16:07:31 UTC
>>Y11 Why there is not void setProjectFolder(WizardDescriptor settings, FileObject folder)? Why the method has so
complicated Javadoc?
ok.

>> Y12 I still do not like (Boolean)wizardDescriptor.getProperty(ModuleTypePanel.IS_NETBEANS_ORG) and similar I'd like
it to be replaced with ModuleTypePanelCreator.isNetBeansOrg(wizardDescriptor).
ok. But I have to leave three public static string attributes to filter property change events.

>>Y13 I would get rid of the ModuleTypePanel interface completely and instead had:
ok

Thank you.
Comment 17 Andrew Korostelev 2008-08-28 20:37:21 UTC
P3s took much more time than P1.

the following patches include you comments.
- setProjectFolder is updated
- all wizardDescriptor.getProperty are replaced with getters with (WizardDescriptor) parameter
- all communication with panel is performed through ModuleTypePanel static methods
static JComponent createComponent(WizardDescriptor)
static boolean validate(WizardDescriptor)
static void setProjectFolder(WizardDescriptor)
and PCL for wizard descriptor.

BUT: I had to leave "public static final String" attributes to filter PCL events.
Comment 18 Andrew Korostelev 2008-08-28 20:38:21 UTC
Created attachment 68575 [details]
patch for apisupport.project with jtulach commnets
Comment 19 Andrew Korostelev 2008-08-28 20:38:49 UTC
Created attachment 68576 [details]
patch for vmd.componentssupport with jtulach commnets
Comment 20 Jaroslav Tulach 2008-08-28 22:52:03 UTC
I can imagine that those P3 took more time. That is why I made them P3s. But I like the result. I think we are getting 
more condensed API.

Y21 Missing private constructor of ModuleTypePanel - you do not want people to make instances of that class, I think. 
Easy fix, I guess.
Y22 Same string constants are still public. For example IS_STANDALONE_OR_SUITE_COMPONENT is public, while API users 
shall use the isSuiteComponent(...) method. Please hide those constants from API. A bit of work, I guess.
Y23 A new, not mentioned issue: Versioning. Increase the version of apisupport.project module to X.Y, add @since X.Y 
to the newly added class, make a note in apichanges.xml, if there are some. Simple, I guess.

Good luck and thanks for having patience with my comments and fulfilling them so greatly.
Comment 21 Andrew Korostelev 2008-08-29 10:36:25 UTC
>> Y21 Missing private constructor of ModuleTypePanel - you do not want people to make instances of that class, I think. 
done

>>Y22 Same string constants are still public. For example IS_STANDALONE_OR_SUITE_COMPONENT is public, while API users 
shall use the isSuiteComponent(...) method. Please hide those constants from API.

they were used to filter PropertyChangeEvents. 
new method static boolean isPanelUpdated(PropertyChangeEvent) is added for this purpose.

>>Y23 A new, not mentioned issue: Versioning. Increase the version of apisupport.project module to X.Y, add @since X.Y 
to the newly added class, make a note in apichanges.xml, if there are some.
done

Thanks for comments.
Comment 22 Andrew Korostelev 2008-08-29 11:25:16 UTC
Created attachment 68611 [details]
patch for apisupport.project with jtulach commnets from  Thu Aug 28 21:52:03
Comment 23 Andrew Korostelev 2008-08-29 11:26:11 UTC
Created attachment 68612 [details]
patch for vmd.componentssupport with jtulach commnets from  Thu Aug 28 21:52:03
Comment 24 Andrew Korostelev 2008-08-29 13:20:39 UTC
almost two weeks are passed from issue was filed.
All comments are applied.

I mark issue as fixed.


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