Bugzilla – Full Text Bug Listing
| Summary: | [patch] [launching] Introduce launcher hooks for bundle based launch configurations | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] PDE | Reporter: | Holger Staudacher <holger.staudacher> | ||||||
| Component: | UI | Assignee: | PDE-UI-Inbox <pde-ui-inbox> | ||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | ankur_sharma, beyhan.veliev, curtis.windatt.public | ||||||
| Version: | 3.7 | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | stalebug | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 347087 | ||||||||
| Attachments: |
|
||||||||
|
Description
Holger Staudacher
Created attachment 199329 [details]
launcher hooks patch
Attached patch provides following features:
* defines an extension point which can be used to hook launcher types for feature/product creation.
* extends PDE UI to use this extension point.
* defines extensions for the OSGI and the Eclipse launcher too hook them.
* provides tests
Sounds reasonable to me. However, I am not sure when we will have enough bandwidth for the review. Also, we need to see if it can go in 3.8. May be 4.2 would be better idea. Hi Ankur, thanks for commenting. Thats why we wanted to provide this patch as early as possible after Indigo. Anyway, let us know if we can help with this. Thanks in advance. Thank you for the patch. I will try to review it as early as possible. However, expect some delay as we are currently busy with Git migration and planning work. The limitation and the proposed solution are both reasonable. Looking at the patch, I see that the extension point needs its metadata filled in (description/example). More importantly, I think we need to change the name of the extension point and classes. 'Launcher hook' doesn't give a developer any idea what you are providing. BundleListProvider, LauncherBundles, LaunchBundleList I also noticed the following pattern: boolean isOsgiLaunchConfigurationType = LauncherHookUtil.isOsgiLaunchConfigurationType(id); models = BundleLauncherHelper.getMergedBundles(fLaunchConfig, isOsgiLaunchConfigurationType); The separate call to the launch hook util seems necessary. I would have expected a single call into BundleLauncherHelper would return the appropriate list or null if the configuration did not have a bundle list. A new method could also be added to provide the list of all configurations that can provide a list of bundles rather than having the details of the extension used in PluginListPage.java. I was expecting an API to be defined for the launch configuration to provide the list of bundles (IBundleListProvider.getBundles()), but in the patch the implementation is hidden inside the helper/util classes. I'm not sure if this is the right approach. A new API would allow a defined way to access the expected list of bundles a launch would use, but I'm not certain if providing that would be beneficial to the community or limit future launch configuration changes. Hi Curtis, thanks for your feedback. We will try to rework the patch within this week and attach it again. Cheers Holger Created attachment 199780 [details] updated launcher hook patch Attached patch is updated based on the feedback in #comment 2 (In reply to comment #5) Hi Curtis, thank you for the fast feedback. See my comments below. > The limitation and the proposed solution are both reasonable. Looking at the > patch, I see that the extension point needs its metadata filled in > (description/example). More importantly, I think we need to change the name of > the extension point and classes. 'Launcher hook' doesn't give a developer any > idea what you are providing. > > BundleListProvider, LauncherBundles, LaunchBundleList > I updated the extension point's metada. I took the nanme "launchBundleList" for it and renamed the related classes. > I also noticed the following pattern: > boolean isOsgiLaunchConfigurationType = > LauncherHookUtil.isOsgiLaunchConfigurationType(id); > models = BundleLauncherHelper.getMergedBundles(fLaunchConfig, > isOsgiLaunchConfigurationType); > > The separate call to the launch hook util seems necessary. I would have > expected a single call into BundleLauncherHelper would return the appropriate > list or null if the configuration did not have a bundle list. A new method > could also be added to provide the list of all configurations that can provide > a list of bundles rather than having the details of the extension used in > PluginListPage.java. I extended the "BundleLauncherHelper" with the suggested new methods. For this I also moved the extension point definition into bundle "o.e.pde.launching". > I was expecting an API to be defined for the launch configuration to provide > the list of bundles (IBundleListProvider.getBundles()), but in the patch the > implementation is hidden inside the helper/util classes. I'm not sure if this > is the right approach. A new API would allow a defined way to access the > expected list of bundles a launch would use, but I'm not certain if providing > that would be beneficial to the community or limit future launch configuration > changes. Our intention for this bug was to provide a possibility to register launch configurations for feature and product creation operations. I think we need to open another bug in case PDE wants to provide new APIs to get the bundle list of a launch configuration. May be it will be enough to provide "BundleLauncherHelper" as API. I did a more indepth review and now do not feel confident putting this in for 3.8. As I mentioned before, the provided API is not complete. For a client to use this extension point, they must have a launch configuration type that uses all of the same attributes as the PDE ones. While the attribute ids are public in IPDELauncherConstants, there is no central place describing when and how they are used. There needs to be a cleaner way to access this information. Adding the extension point fixes what I see as a very small issue while making it harder for pde launching to change things in the future. A compromise would be for the extension point to provide a Bundle list factory (or have the launch config adapt to the factory). The bundle list factory would take a launch configuration as input and return a list of plug-ins or features that configuration includes (perhaps using NameVersionDescriptor). This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. If the bug is still relevant, please remove the "stalebug" whiteboard tag. |