Bugzilla – Full Text Bug Listing
| Summary: | Make org.eclipse.wst.common.project.facet.ui.ModifyFacetedProjectWizard.handleSelectedFacetsChangedEvent() protected for extenders | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Common Tools | Reporter: | Kevan Holdaway <kholdaway> | ||||
| Component: | Faceted Project Framework | Assignee: | wst.common <wst.common-inbox> | ||||
| Status: | RESOLVED INVALID | QA Contact: | Konstantin Komissarchik <konstantin> | ||||
| Severity: | normal | ||||||
| Priority: | P1 | CC: | ccc, jcagle, shr31223, thatnitind | ||||
| Version: | 3.2.5 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Kevan Holdaway
Created attachment 205140 [details]
Proposed patch
I attached a patch.
A few additional details. To work around this issue we had to actually clone ModifyFacetedProjectWizard. The problem we found is that some facet install pages actually cast to org.eclipse.wst.common.project.facet.ui.ModifyFacetedProjectWizard. Our cloned version of ModifyFacetedProjectWizard page is not a org.eclipse.wst.common.project.facet.ui.ModifyFacetedProjectWizard. One such example is in org.eclipse.jpt.ui.internal.wizards.JpaFacetActionPage.setUpRuntimeListener(). This is the super class of org.eclipse.jpt.ui.internal.wizards.JpaFacetInstallPage. We cannot display the JpaFacetInstallPage in our wizard. Other clients do so as well. As per the WTP Hotbug policy - http://wiki.eclipse.org/WTP/Hotbug_Policy - an adopter, IBM, needs this to allow for proper functionality in the adopter's product. As such, I am marking this as a P1. We need this fixed for WTP 3.2.5 adoption. Konstantin- if you will allow this change, can you assign this bug to Roberto? (Unless you are inclined to get the necessary PMC approval yourself.) Could you explain why overriding this method is the only solution? If you needing to do something extra on facet change, you can register a working copy listener from you subclass. It's not that we're looking to do something extra, we're looking to do something different *instead* of what it does now. Ok... That's rather unsettling... That method is pretty much the core of this wizard's behavior. I don't see how allowing suppressing of this wizard's key characteristics and allowing it to be turned into a completely different type of wizard while fooling code that potentially relies on this class to behave in a certain way is a safe direction for this API. (In reply to comment #6) > Ok... That's rather unsettling... That method is pretty much the core of this > wizard's behavior. I don't see how allowing suppressing of this wizard's key > characteristics and allowing it to be turned into a completely different type > of wizard while fooling code that potentially relies on this class to behave in > a certain way is a safe direction for this API. How the wizard pages get created isn't the central portion of the wizard. Facets delegates creating the project is the core. Presentation of the UI should be more flexible and that's what we require as an extender. It appears to me is that the core of your issue is tight coupling of facet implementations (their wizard pages, in particular) and ModifyFacetedProjectWizard class. I suggest you focus on loosening that coupling. The ModifyFacetedProjectWizard isn't a general wizard for any sort of facet-related work. It's a wizard that behaves in a certain specific manner and that behavior is API. For instance, in your JpaFacetActionPage.setUpRuntimeListener() example, there is absolutely no reason that this code should be accessing ModifyFacetedProjectWizard class. The facet's ActionConfig class has getFacetedProjectWorkingCopy() API through which it can access the working copy and setup a listener on the runtime. I agree with the suggestion to have WTP's Dali code remove the tight wizard coupling. I can open a bug for this. I need to override this behavior for the following reasons: 1) Clients who intent to manipulate the appearance of the wizard are limited by the requirement SWT places on component creation to provide the parent Composite at the time of create. Because this wizard locks down the page creation in this manner, an extender cannot use a different parent Composite. 2) The method doesn't allow to clients to determine when to create the page. Its event driven. A more efficient approach might be desirable in certain cases (e.g. create all pages in a single operation) 3) The method doesn't clean up after itself. It doesn't dispose of wizard pages (until the entire wizard is closed). Adding and removing a facet and adding it back will cause 2 instances of the page and its models to exist. This can cause potential problems. I am not sure this is a change is direction for the API, its simply improving its extensibility for extenders so WTP can be used in a broader audience. Either way, I will leave the decision with Konstantin as the owner of the code. > I am not sure this is a change is direction for the API, its simply improving > its extensibility for extenders so WTP can be used in a broader audience. This wizard was not designed for what you are trying to do with it. Simply allowing this method to be overridden does not improve extensibility. It just creates support problems by letting adopters walk down the path that will likely lead them off a cliff two steps beyond what we are currently discussing. If you need to build a custom wizard that re-uses facet pages, but manages them in the precise manner that your usecases require, you can do that with the existing API. > Clients who intent to manipulate the appearance of the wizard [snip] It is possible that some of your needs could be expressed via different extender API that I would find less problematic... > The method doesn't clean up after itself. [snip] This doesn't seem relevant to this discussion. If you believe you found a bug, please open a bug report. |