Bug 339857 - It is possible to import package from fragment, but classes are invisible
Summary: It is possible to import package from fragment, but classes are invisible
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6.2+   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2011-03-14 06:03 EDT by Krzysztof Daniel CLA
Modified: 2011-05-17 02:30 EDT (History)
3 users (show)

See Also:


Attachments
Fix proposition (2.41 KB, patch)
2011-03-14 06:57 EDT, Krzysztof Daniel CLA
no flags Details | Diff
Proposed Fix (2.98 KB, patch)
2011-03-25 11:57 EDT, Curtis Windatt CLA
no flags Details | Diff
Patch with altered comments (3.08 KB, text/plain)
2011-03-31 03:24 EDT, Krzysztof Daniel CLA
curtis.windatt.public: iplog+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Daniel CLA 2011-03-14 06:03:01 EDT
This is inconsistency. Either the package should be invisible or classes should be visible.
Comment 1 Krzysztof Daniel CLA 2011-03-14 06:57:36 EDT
Created attachment 191101 [details]
Fix proposition
Comment 2 Krzysztof Daniel CLA 2011-03-14 07:00:50 EDT
Here is a log of what packages and fragments are not allowed to be in the package selection dialog (fragments without parent with extensible API):

fragment org.eclipse.core.filesystem.win32.x86
 has no parent with extensible API
fragment org.eclipse.core.net.win32.x86
 has no parent with extensible API
fragment org.eclipse.core.resources.win32.x86
 has no parent with extensible API
fragment org.eclipse.core.runtime.compatibility.registry
 org.eclipse.equinox.registry has extensible API
fragment org.eclipse.ecf.provider.filetransfer.httpclient.ssl 
 has no parent with extensible API
fragment org.eclipse.ecf.provider.filetransfer.ssl
 has no parent with extensible API
fragment org.eclipse.ecf.ssl 
 has no parent with extensible API
fragment org.eclipse.equinox.launcher.win32.win32.x86
 has no parent with extensible API
fragment org.eclipse.equinox.security.win32.x86 
 has no parent with extensible API
fragment org.eclipse.jdt.compiler.apt
 org.eclipse.jdt.core has extensible API
fragment org.eclipse.jdt.compiler.tool
 org.eclipse.jdt.core has extensible API
fragment org.eclipse.swt.win32.win32.x86
 org.eclipse.swt has extensible API
fragment org.eclipse.ui.win32 
 has no parent with extensible API
fragment org.eclipse.ui.workbench.compatibility
 has no parent with extensible API
fragment org.eclipse.update.core.win32
 has no parent with extensible API
fragment Fragment
 has no parent with extensible API
fragment org.eclipse.core.filesystem.win32.x86
 has no parent with extensible API
fragment org.eclipse.core.net.win32.x86
 has no parent with extensible API
fragment org.eclipse.core.resources.win32.x86
 has no parent with extensible API
fragment org.eclipse.core.runtime.compatibility.registry
 org.eclipse.equinox.registry has extensible API
fragment org.eclipse.ecf.provider.filetransfer.httpclient.ssl
 has no parent with extensible API
fragment org.eclipse.ecf.provider.filetransfer.ssl
 has no parent with extensible API
fragment org.eclipse.ecf.ssl
 has no parent with extensible API
fragment org.eclipse.equinox.launcher.win32.win32.x86
 has no parent with extensible API
fragment org.eclipse.equinox.security.win32.x86
 has no parent with extensible API
fragment org.eclipse.jdt.compiler.apt
 org.eclipse.jdt.core has extensible API
fragment org.eclipse.jdt.compiler.tool
 org.eclipse.jdt.core has extensible API
fragment org.eclipse.swt.win32.win32.x86
 org.eclipse.swt has extensible API
fragment org.eclipse.ui.win32
 has no parent with extensible API
fragment org.eclipse.ui.workbench.compatibility
 has no parent with extensible API
fragment org.eclipse.update.core.win32
 has no parent with extensible API
Comment 3 Curtis Windatt CLA 2011-03-15 14:57:07 EDT
At first glance the patch looks good, marking for M7.
Comment 4 Curtis Windatt CLA 2011-03-25 11:57:26 EDT
Created attachment 191917 [details]
Proposed Fix

Christopher, please confirm that this fix works correctly for you.  It takes the same approach as your patch, just rewords/reorders some of it.
Comment 5 Krzysztof Daniel CLA 2011-03-28 04:57:37 EDT
I am not sure about the comment in line 485. 
I thought it is a host who has to have an extensible api directive set.

I made a typo in line 581. 
It should be written "their packages" not "they packages".

I am a little bit afraid of the inversion you have made in the loop in line 583.

for (int i = 0; i < hosts.length; i++) {
 if (ClasspathUtilCore.hasExtensibleAPI(PluginRegistry.findModel(hosts[i])))
    return false;
}

is not an equivalent of negated
for (int i = 0; i < hosts.length; i++) {
  if (ClasspathUtilCore.hasExtensibleAPI(PluginRegistry.findModel(hosts[i])))
     return true;
}

In the latter case we need to find at least one host with an extensibleAPI, the first approach says that all hosts need to have extensibleAPI. I am not sure what is expected here.
Comment 6 Krzysztof Daniel CLA 2011-03-28 05:15:44 EDT
The part about the inversion in my last comment is wrong - and the new code is right.

The code hides successfully packages from the fragment. I went one step further, and checked how actually Eclipse behaves when the directive is set - and it looks like only the existing packages from the host can be extended, and not those declared only in the fragment. Is this expected behavior?
Comment 7 Krzysztof Daniel CLA 2011-03-28 05:18:20 EDT
(In reply to comment #6)

I forgot to export the package. Everything works as expected. Only comments in the patch require some attention.
Comment 8 Curtis Windatt CLA 2011-03-29 11:41:13 EDT
(In reply to comment #7)
> I forgot to export the package. Everything works as expected. Only comments in
> the patch require some attention.

Which comments would you like to see changed?
Comment 9 Krzysztof Daniel CLA 2011-03-31 03:24:07 EDT
Created attachment 192251 [details]
Patch with altered comments
Comment 10 Curtis Windatt CLA 2011-04-01 12:09:09 EDT
Excellent, applied the patch to HEAD.
Comment 11 Curtis Windatt CLA 2011-04-01 12:23:35 EDT
Patch released to 3.6.x stream for backporting.