Community
Participate
Working Groups
Build Identifier: M20120208-0800 Developing OSGI bundles with import/export package is currently a bit cumbersome. You cannot use "Organize Imports" or Ctrl-Space to add imports, because packages need to be exported (from the supplier) and imported from the consumer first. This slows down development a lot. There is a quickfix available that allows importing packages into a bundle, but AFAICS, this quickfix is only available on an unresolveable import declaration. So one has to manually add the import declaration first and invoke the quickfix on that. I hacked the existing QuickFixProcessor a bit, so that one can import a missing package right on the unresolveable type. Additionally, if no exported package is found, one can export a package right there. There is still room for improvement (e.g. export and import the package in one go), but it is already quite a timesaver. See https://github.com/cpfeiffer/eclipse.pde.ui/commit/2e19266fae14691c3fee374aa876f5d314d6a3c6 for the changes. Reproducible: Always
This is something we would like to improve so thanks for your contribution. We will review it for next release. I'm not certain about a quickfix to change the export settings of a package as this is something the bundle should be managing. This is more reasonable if there is no package export entry for the package at all (i.e. the bundle developer forgot to export it). Certainly a quickfix shouldn't change the permissions on an export entry.
This is exactly what the quickfix does. If the type name in question is found in at least one package, but not in any exported package, then you get a quickfix that will simply export that package.
I am reviewing the fix, since I'm cherry picking it out of github, can you please confirm the following on this bug: Contributor asserts on the corresponding bug or in a comment on the Gerrit push record: 1. I have authored 100% of the content I'm contributing 2. I have the rights to donate the content to Eclipse 3. I contribute the content under the EPL
(In reply to comment #3) Thanks for taking care of this, Curtis. > Contributor asserts on the corresponding bug or in a comment on the Gerrit push > record: > > 1. I have authored 100% of the content I'm contributing > 2. I have the rights to donate the content to Eclipse > 3. I contribute the content under the EPL Yes, yes, yes. Please make sure to use the latest version of the patch, i.e. https://github.com/cpfeiffer/eclipse.pde.ui/tree/import_package_master for 4.2+, as it contains a few improvements compared to the first version.
And FWIW, there is still some room for improvement, that I haven't had a look at: if you use a class that has dependencies on another class (e.g. by inheritance or interface implementation) then you would like to add that package dependency with a QuickFix as well. Currently you have to manually add the import-package to get rid of the compile error.
(In reply to comment #5) > And FWIW, there is still some room for improvement, that I haven't had a look > at: > if you use a class that has dependencies on another class (e.g. by inheritance > or interface implementation) then you would like to add that package dependency > with a QuickFix as well. Currently you have to manually add the import-package > to get rid of the compile error. Can you please create a single patch/commit and attach/link to it? Otherwise for Eclipse IP due diligence, each contribution would have to be reviewed separately. My only concern when reviewing was that the proposal doesn't strongly indicate that you are modifying a different plug-in than you are currently working on. I see that the proposal already existed for fixing access restrictions, so I can accept the change. Since the user must have the project in question in their workspace, the risk is pretty low.
> Can you please create a single patch/commit and attach/link to it? Otherwise > for Eclipse IP due diligence, each contribution would have to be reviewed > separately. There you go: https://github.com/cpfeiffer/eclipse.pde.ui/commit/c63ace8cc0be16d51d0b53406eddfa9ca7de95c4 > My only concern when reviewing was that the proposal doesn't strongly indicate > that you are modifying a different plug-in than you are currently working on. I > see that the proposal already existed for fixing access restrictions, so I can > accept the change. Since the user must have the project in question in their > workspace, the risk is pretty low. There are two scenarios, which are already available in current PDE: import a package and export a package. The former is available on an import statement, and I guess the latter as well (didn't try, but the code for exporting a package from a quickfix is already there in PDE). All I did was make these quickfixes available for more compilation problems.
Using the latest patch, I get two duplicate 'add import-package foo' quickfix proposals for most unresolved imports. Are you seeing that problem as well? To test I was taking a copy of pde.core (which has many packages both API and non-API) and taking out some of the exports. Then I created a new plug-in with no dependencies on pde.core and tried to access various classes, added import statements, etc. The quickfix proposals for changing export settings work fine, but the import proposals are doubled.
I cannot reproduce this on my main development machine (Linux), but I *have* observed the problem on Windows a few times. I will investigate.
Hmm, whatever I do, I cannot reproduce this. Can you provide me two sample projects that show this problem?
The duplicate entries only happen if you have matching projects in the workspace and the target platform. i.e. I have org.eclipse.pde.core in my workspace and the target platform. It also looks like the 'add required bundle' quickfix disappears under these circumstances. If org.eclipse.pde.core is closed in the workspace everything works correctly.
(In reply to comment #11) > The duplicate entries only happen if you have matching projects in the > workspace and the target platform. i.e. I have org.eclipse.pde.core in my > workspace and the target platform. > > It also looks like the 'add required bundle' quickfix disappears under these > circumstances. If org.eclipse.pde.core is closed in the workspace everything > works correctly. The problem is your changes to: org.eclipse.pde.internal.ui.correction.java.QuickFixProcessor.handleAccessRestrictionByImportPackage(IProject, ExportPackageDescription, Collection) Previously, a require bundle proposal was always returned if it would provide the package. Now the method always returns an import package unless that package is already imported (in which case require bundle is added). I think that reverting the code in the method to just create the require bundle proposal as before will solve this issue. However, please let me know if there was a reason to change that code I'm not seeing. The naming of the method is unfortunate. I can fix that and the redundant null check. I also noticed that there is a similar method for fragments that you did not modify.
(In reply to comment #12) > The problem is your changes to: > org.eclipse.pde.internal.ui.correction.java.QuickFixProcessor.handleAccessRestrictionByImportPackage(IProject, > ExportPackageDescription, Collection) > > Previously, a require bundle proposal was always returned if it would provide > the package. Now the method always returns an import package unless that > package is already imported (in which case require bundle is added). > > I think that reverting the code in the method to just create the require bundle > proposal as before will solve this issue. However, please let me know if there > was a reason to change that code I'm not seeing. The naming of the method is > unfortunate. I can fix that and the redundant null check. I also noticed that > there is a similar method for fragments that you did not modify. I wondered why sometimes an import-package is created and sometimes a require-bundle. AFAIU, import-package is the preferred way, and that's what the QuickFix primarily provides. That's why I changed this method to do import-package instead and only fall back to require bundle (if the package is already imported). If you revert that change, you will be back to the situation that some of the scenarios do "require bundle".
(In reply to comment #13) > If you revert that change, you will be back to the situation that some of the > scenarios do "require bundle". I have not experienced this so far. The import package proposal is added via the FindClassResolutionsOperation, whereas the require bundle proposal is added through the QuickFixProcessor.
(In reply to comment #14) > I have not experienced this so far. The import package proposal is added via > the FindClassResolutionsOperation, whereas the require bundle proposal is > added through the QuickFixProcessor. My commit message says: --- Support for import-package on method-invocations and field-invocations E.g. Ctrl-1 on someVariable.getSomething().doit(); or someVariable.getSomething().SOME_FIELD; will propose importing the package containing the type returned by getSomething(). --- Can you try if these still work?
(In reply to comment #15) > My commit message says: > --- > Support for import-package on method-invocations and field-invocations > > E.g. Ctrl-1 on someVariable.getSomething().doit(); > or > someVariable.getSomething().SOME_FIELD; > will propose importing the package containing the type returned by > getSomething(). > --- > > Can you try if these still work? I understand the problem more now. The QuickFixProcessor does nothing to coalesce duplicate proposals and was previously responsible for only adding require bundle proposals. After the code changes, the processor only provides import package proposals (except for the special case where an import exists but the problem remains). Reverting the changes to only create required bundle proposals solves things for types, but is still broken for the new proposals on methods. If only the method requires an import it returns a single import package proposal. This isn't broken, but I think we should still provide both proposals. If you have both a type and a method that require an import, you end up with three identical proposals. One from FindClassResolutionsOperation for the type and two from QuickFixProcessor, one for the type and one for the method. Now for solutions. The proper solution is to figure out why the operation and processor are both adding proposals. I'm not familiar with that code and fundamentally changing it will probably have side effects. A hackish solution would be to revert the processor to only create req bundle proposals and then remove duplicates for methods. That means no import pkg proposals would be provided for methods. We could also remove the method support entirely until a better solution is found.
(In reply to comment #16) > I understand the problem more now. The QuickFixProcessor does nothing to > coalesce duplicate proposals and was previously responsible for only adding > require bundle proposals. After the code changes, the processor only provides > import package proposals (except for the special case where an import exists > but the problem remains). I'm fine with fixing it to support both import-package and require-bundle :-) It shouldn't be too hard to make sure that there are no duplicate import-package proposals. I would favor that a lot over removing the support for import-package on methods. That said, I probably won't have time for this before end of July.
Curtis, I finally found the time to have another go at that. I did the following: - made sure that always both require-bundle and import-package quickfixes are provided - made sure that duplicate import-package proposals will not happen - also updated the method handleAccessRestrictionByImportPackage(IPackageFragment, ...) This time I actually developed the patch with Juno, and I had to add some more IProblems to make the quickfixes work for missing types (e.g. someField.someMissingTypeField.doSomething() or someField.someMissingTypeMethod().doSomething()). Open issues: 1. the calls to JavaResolutionFactory were scattered over a few places; I moved them to the AbstractClassResolutionCollector/subclasses. There were some places were the "relevance" parameter was explicitly set to 17 and 16. I'm not sure if this is really needed. We either need to add that parameter to the AbstractClassResolutionCollector methods like I did with the addRequireBundleModification() method, or we simply hardcode 17 in the collector. 2. even without the code that prevents duplicate import-package proposals, I was unable to find a way to reproduce that. Do you have some example code that definitely triggers it? 3. I'll provide the changes as a patch for now, as creating another "single-commit" branch for every update is not exactly in the spirit of git ;-) When we have a final version, I'll do it, in case it's still needed.
Created attachment 222702 [details] updated patch doing both import-package and require-bundle
Created attachment 223971 [details] Improved patch The patch no longer applies because of the move to a 1.5 BREE. I updated the patch and made a few tweaks and it seems to be working well for me now. At one point I had duplicate require bundle proposals, but I can't reproduce now. I would leave the relevance settings as is. It's not clear where those values came from, but PDE extenders may be making assumptions of those values to situate their own quick fixes above/below the entries. Two concerns: 1) Performance - Occasionally I would have a long pause when opening the proposals. 2) Package/Bundle versioning - Some bundles and packages may be duplicates with different versions. It would be good to handle this case (though it isn't handled well elsewhere in PDE proposals). I think this is ready to test in integration builds so I plan to commit it today.
Fix is in master http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=84b471a2fabf3bc475d09adc7a80cf9c0ed001fe
Trying this in my host and things are working well. Closing as FIXED. Thanks for the contribution
Note that I pushed a fix for a possible NPE related to this change, it is not in last night's integration build. http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=b5440732c3b48ab5e6083693c0765c758ca0023f
*** Bug 237638 has been marked as a duplicate of this bug. ***
