Bug 198544

Comment 1 Tomasz Zarna 2007-08-31 06:21:45 EDT

Lars, are you referring to a method from the IContentTypeManager[1] class? If yes, I'm afraid I'm not able to track it. Could you double the name of the method/class?


[1] org.eclipse.core.runtime.content.IContentTypeManager (org.eclipse.core.contenttype project)

Comment 2 lars gersmann 2007-08-31 09:00:06 EDT

(In reply to comment #1)
> Lars, are you referring to a method from the IContentTypeManager[1] class? 

correct: IContentManager.findContentTypeFor( String filename) 

> If
> yes, I'm afraid I'm not able to track it. Could you double the name of the
> method/class?

the implementation is 

org.eclipse.core.internal.content.ContentTypeManager 

which basically delegates the call to its superclass method 

org.eclipse.core.internal.content.ContentTypeMatcher#findContentTypeFor(String fileName)

the last line takes just the first associated contenttype instead of the best matching:

return associated.length == 0 ? null : new ContentTypeHandler((ContentType) associated[0], currentCatalog.getGeneration());

> 
> 
> [1] org.eclipse.core.runtime.content.IContentTypeManager
> (org.eclipse.core.contenttype project)
> 

does that answer your question ?

Comment 3 Szymon Brandys 2007-09-10 15:09:23 EDT

Created attachment 78011 [details]
Fix

When the patch is applied, both findContentTypeFor methods from ContentTypeMatcher will return a content type with the highest priority.

Comment 4 Szymon Brandys 2007-09-10 16:13:58 EDT

Created attachment 78018 [details]
Fix 02

Comment 5 John Arthorne 2007-09-11 16:08:57 EDT

The patch to ContentTypeMatcher looks good, but can you describe your changes in the test plugin? Is there a test case that exercises the patch (fails without the patch and succeeds with the patch)?

Comment 6 John Arthorne 2007-09-11 16:10:19 EDT

I have released your patch, but I'll leave this open until we have a JUnit test for it.

Comment 7 Szymon Brandys 2007-09-12 06:58:38 EDT

(In reply to comment #5)
Before the patch, the custom xml content type defined in the test plug-in was always before the default one. When we started respect the priority, the default was first (it has the HIGH priority and custom is NORMAL). So, to fix the test I had to change the custom xml content type priority to HIGH as well.

Comment 8 Szymon Brandys 2007-09-12 08:07:29 EDT

John. Please revert the patch. I investigated the problem further and I found that the priority is respected, but as a second criteria.
See ContentTypeCatalog#policyConstantSpecificIsBetter field and its javadoc.

Comment 9 Szymon Brandys 2007-09-12 08:08:22 EDT

Created attachment 78169 [details]
Tests showing that the priority works fine

This patch should be applied when the previous is reverted.

Comment 10 Szymon Brandys 2007-09-12 11:16:07 EDT

Created attachment 78195 [details]
Updated tests

Comment 11 Valentin Baciu 2007-09-12 11:18:53 EDT

I hope you may find my notes in bug 194935 interesting. In there I also
describe a scenario where the incorrect content type is picked up. I did not
study in detail this case but at the first glance they seem related. I'm
thinking the culprit is either the xmlsource content type from WTP which is
intended as a fallback for a few extensions (xsd, wsdl and xsl among them) and
is masking other content types mapped to these extensions or the way the
content type is matched - extensions first.

Comment 12 Szymon Brandys 2007-09-12 11:19:44 EDT

Created attachment 78196 [details]
Updated tests

Comment 13 John Arthorne 2007-09-12 12:22:49 EDT

The previous fix has been reverted in CVS, and the new tests has been released.

Comment 14 Szymon Brandys 2007-09-13 04:06:27 EDT

The priority works. See ContentTypeCatalog#policyConstantSpecificIsBetter field and its javadoc.

Comment 15 Boris Bokowski 2007-09-13 08:17:59 EDT

Valentin, do you agree?

Comment 16 Valentin Baciu 2007-09-13 19:29:10 EDT

Well, I confess my brain is not 100% on this issue at the moment, so please take my comments with a grain of salt. My comments and test cases in bug 194935 are my best take on this type of issue. That bug and bug 146951 are the visible effects and they're not particularly pretty. I know Boris is looking into those two bugs.

With the risk of repeating and not being entirely accurate, here is what I think happens here:
- we're trying to find the content type for a file named test.xsl
- the first pass is to choose content types by matching them against the xsl file extension. This yields two content types, the one from WTP (xmlsource mapped to the xsl extension among other) and the XSL one (also mapped to the xsl extension). xmlsource is at level n in its content type inheritance branch  and xsl is at level n+1 in its own branch.
- these content types are put in an array which is then being sorted by applying the org.eclipse.core.internal.content.ContentTypeCatalog.policyConstantGeneralIsBetter sorting policy (not policyConstantSpecificIsBetter)
- policyConstantGeneralIsBetter first checks the depth of the compared content types 
- the xmlsource content type will always win because it is defined higher in the hierarchy,
- array[0] is declared winner so xmlsource will always be picked for files with xsl extensions

In conclusion, the sorting policy code is likely fine. The question is why is the general is better policy being used? What needs clarifying also is if a content type in a content type inheritance branch can redefine file extensions declared at other levels in a different branch of the content types inheritance hierarchy.

Comment 17 Szymon Brandys 2007-09-14 04:11:22 EDT

Valentin,
As I promised :-) I will take care of the issue. The reason why I marked that bug as INVALID is that the priority works. I agree that we have problems with content types but they are described in bugs mentioned in the comment #16.

Comment 18 Valentin Baciu 2007-09-14 10:01:03 EDT

Szymon, re-reading the bug summary and the original report, I agree, strictly speaking the bug report is invalid. The priority based sorting likely works, if the code makes it that far. I guess I was overly focused on the scenario and explaining why it actually fails. 
And by the way, last night I realized the WTP XML core plug-in also defines an XSL content type, in addition to the xmlsource one.
Over and out.