Bug 148944

Summary:

need to render resource folders in JARs

Product:

[Eclipse Project] JDT

Reporter:

Martin Aeschlimann <martinae>

Component:

Core

Assignee:

Jerome Lanneluc <jerome_lanneluc>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

daniel_megert, markus.kell.r

Version:

3.2

Target Milestone:

3.3 M6

Hardware:

OS:

Windows XP

Whiteboard:

Bug Depends on:

Bug Blocks:

142048, 148657, 151112, 176210

Attachments:

Description	Flags
Proposed fix	none
Fix with the possibility of the parent to be an IPackageFragmentRoot	none

Description Martin Aeschlimann

2006-06-28 04:23:21 EDT

3.2

Before fixing bug 142530, resources in JAR files were shown in package fragments also for folders that weren't legal package fragments.

We had discussion here about the fix, and found that rendering the resources as flat list is not a good solution.

We suggest to turn back the fix and accept the problematic package fragments, or better, add a new element for folders in JARs.

To be backward compatible I suggest to add a new interface that still implements IStorage:

interface IJARFileResourceEntry extends IStorage {
   isFile(): boolean    // true if file, false if folder
   getParent(): Object  // either a IJARFileResourceEntry or a IJavaElement
   getChildren(): IJARFileResourceEntry[]  // empty if entry is a file
}

or if you don't mind adding two interfaces:
IJARFileResourceFile and IJARFileResourceFolder

I think this the best solution and also fixes our problem of not being able to reveal such elements (bug 142048)

Comment 1 Dani Megert

2006-06-28 05:08:46 EDT

+1 to revert the fix for bug 142530 and bug 148657.

Comment 2 Markus Keller

2006-07-21 05:05:41 EDT

+1 for a clean fix: Invalid package names in jars should not be package fragments (causes problems such as bug 151112) but some other kind of containers.

Comment 3 Jerome Lanneluc

2007-03-02 12:34:07 EST

Created attachment 60183 [details]
Proposed fix

Comment 4 Jerome Lanneluc

2007-03-02 12:36:56 EST

Martin, please let me know if this works for you. If it is ok, I'll ask the PMC for approval on this new API.
Note we will have to coordinate the releasing of this patch, since without your changes, we will show only the first level directory in the Package Explorer.

Comment 5 Martin Aeschlimann

2007-03-02 13:02:34 EST

I tried the patch and it works.

In getParent, shouldn't it also state that it can return a IPackageFragmentRoot. Or is this not possible?

Comment 6 Jerome Lanneluc

2007-03-05 04:09:45 EST

(In reply to comment #5)
> In getParent, shouldn't it also state that it can return a
> IPackageFragmentRoot. Or is this not possible?
You're right. Thanks for noticing it. Please see new patch for a fix.

Comment 7 Jerome Lanneluc

2007-03-05 04:10:42 EST

Created attachment 60250 [details]
Fix with the possibility of the parent to be an IPackageFragmentRoot

Comment 8 Philipe Mulet

2007-03-05 16:02:22 EST

+1 for API change

Comment 9 Markus Keller

2007-03-06 12:33:33 EST

I think JarEntryDirectory#getContents() should throw a CoreException rather than returning an empty stream.

IStorage#getContents() already tells that subtypes will refine the exception cases, so IJarEntryResource#getContents() could specify that an exception is thrown if isFile() is false.

Anyway, all usage sites of IStorage (esp. 'instanceof IStorage') should be checked for problems like bug 176210 comment 2.

Comment 10 Jerome Lanneluc

2007-03-07 06:19:29 EST

Unfortunately existing clients see the non-Java resources as IStorages, which doesn't refine the exception case. Thus they are not prepared for handling the CoreException.

Returning an empty stream is not ideal, but I think it is more acceptable to see an empty editor rather than an internal error dialog.

Comment 11 Markus Keller

2007-03-07 06:47:09 EST

> Thus they are not prepared for handling the CoreException.
If they're no prepared, then it's a bug in their implementation.

From IStorage:
/**
 * [..]
 * @exception CoreException if the contents of this storage could 
 *		not be accessed.   See any refinements for more information.
 */
public InputStream getContents() throws CoreException;

E.g. the case from bug 176210 comment 2 is handled correctly because StorageDocumentProvider catches the CoreException and nicely shows the error message in the editor. I find that more informative and less confusing than an empty editor.

Comment 12 Martin Aeschlimann

2007-03-07 13:18:41 EST

Although I see Markus point, I'd suggest to keep the behaviour as before (return empty stream). The correct solution would have been to not use IStorage to represent folders. But that's legacy and also not really a major issue.

Comment 13 Markus Keller

2007-03-07 14:21:23 EST

This change will also affect PDE/UI, see bug 176635.

Comment 14 Jerome Lanneluc

2007-03-08 05:46:39 EST

Patch released for 3.3M6 in HEAD.

Comment 15 Jerome Lanneluc

2007-03-09 06:18:59 EST

*** Bug 22376 has been marked as a duplicate of this bug. ***

Comment 16 Maxime Daniel

2007-03-20 03:56:20 EDT

Verified for 3.3 M6 using v_743.