Bugzilla – Full Text Bug Listing
| Summary: | [content type] XMLRootElementContentDescriber is overriden by file name or file extension | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Kim Sullivan <alicebot> |
| Component: | Resources | Assignee: | Platform-Resources-Inbox <platform-resources-inbox> |
| Status: | CLOSED WONTFIX | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | david_williams, john.arthorne |
| Version: | 3.3 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | stalebug | ||
|
Description
Kim Sullivan
We'd definitively have to work through several cases, allow lots of time for tests, etc, but I think I agree "INVALID" would be more appropriate for this case. According to "http://www.eclipse.org/eclipse/platform-core/documents/content_types.html" the rule, while a tiny bit vague, says "... If not [VALID] (or if cannot be determined), this method should exit immediately, returning IContentDescription.INVALID or IContentDescription.INDETERMINATE, depending on how strict the file format is." I suspect all file formats of XMLRootElementContentDescriber should be considered pretty strict. If it helps, a contrasting case is "Ant" ... there's not many rules about what an Invalid ANT file is, so "intermediate" might be appropriate, even for <foo></foo>. (well, as defined, it's not quite that loose, but ... it can vary a great deal). What both the AntBuildfileContentDescriber and the XMLRootElementContentDescriber have in common is that they both return INDETERMINATE if the sax parser fails for any reason. This leads to the paradox that they "accept" a file even if (and especially when) it contains binary data which doesn't seem right. While I was trying different return values, I got to a point where the XMLRootElementContentDescriber accepted arbitrary (non XML) data, but rejected an XML file with an incorrect root node. So, both of these describers need to be fixed (and fixed in the same way). What I'm not sure about are empty files. Currently, an empty file is accepted solely on the base of its filename, which might be OK in some situations (create an appropriately named file, and launch the editor that fills in the necessary details) but not so good in others (conflicts between editors that work solely on the extension, esp. if its just "xml"). So, this is probably a broader issue than just fixing the return type (but nevertheless, I think that returning INVALID instead of INDETERMINATE would help a lot). I'm not convinced this is the way to go. I can imagine someone accidentally introducing a syntax error in a file, and then on close/reopen it would open in a different kind of editor. Unless there is another describer that gets an exact match, as a user I would expect plugin.xml to always be opened in the manifest editor regardless of syntax errors. (In reply to comment #3) > I'm not convinced this is the way to go. I can imagine someone accidentally > introducing a syntax error in a file, and then on close/reopen it would open in > a different kind of editor. Unless there is another describer that gets an exact > match, as a user I would expect plugin.xml to always be opened in the manifest > editor regardless of syntax errors. In that case, I think there would be no point in using an XMLRootElementContentDescriber to define the pluginManifest content type, the pluginManifest content type should be based on the file name only, and the describer can still be made stricter. As for accidentally introducing syntax errors - you have a valid point, but the XRECD doesn't actually check the syntax of the whole file (none of the platform ContentDescribers so that). It goes only so far as to try to parse the first tag of a file, and if it looks OK, it goes on to say the file is VALID (leaving the actual validation of the content type to the consumer of the content type, such as an editor or builder). So the only syntax errors that would actually trigger the behavior you describe are introducing random characters at the beginning of the file before the <plugin> tag (making the sax parser fail), or changing the root element name from <plugin> to something else (making the criteria fail). The current situation is that the XRECD looks at a file, reads its first tag, sees that it's a perfectly well formed tag with the name "foo" and says "well, despite all evidence, it could still be a document with a root element called 'bar'", which sort of defeats the whole purpose of using an XMLRootElementContentDescriber. This isn't such a problem with the plugin.xml file, which might have been a bad example but it was easiest to reproduce. Actual problems start to show up once you have several content types, all using the xml file extension, and differentiated only via an XMLRootElementContentDescriber. In that case, the current system more or less correctly selects the right content type for the xml files with the correct root element, but randomly selects one of these content types (and associated editors!) for all other files that have the xml extension (and aren't overridden by a more specific content type, such as plugin.xml). So you have a bunch of xml files, one of which has the correct content type but ALL of them open in your editor. And once a particular editor is already opened on an invalid file, there is no easy way for the editor to back off and say "I don't want that file and I won't open" - you have to throw a PartInitException (besides, this might have triggered unnecessary loading of the editor plugin, which is something that the ContentDescribers are trying to avoid). 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. 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. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. 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. -- The automated Eclipse Genie. |