Bugzilla – Full Text Bug Listing
| Summary: | EMF.Edit assigns spurious default values to optional (minOccurs="0") simple type elements | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Trev <456> |
| Component: | Edit | Assignee: | Ed Merks <Ed.Merks> |
| Status: | RESOLVED WONTFIX | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | ||
| Version: | 2.6.0 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Trev
Created attachment 193892 [details]
differences between tutorial's library.xsd and this bug's library.xsd
Created attachment 193893 [details]
this bug's library.xsd
Created attachment 193894 [details]
the library instance serialization from the tutorial
Created attachment 193895 [details]
differences between tutorial's library.xsd and this bug's library.xsd
This is another repeat of the discussion about the fact that EMF treats EEnums as primitives. I.e., just as it's not possible for an EInt value to be null, it's not possible to make an EEnum's value null. Note that it has nothing to do with the editor, it's the getter of the feature that returns this value. I.e., it's a basic behavior of the generated model. You might argue it shouldn't be doing that, but it's been doing that for a long time, and we're not going to change it now. Oops, I meant to return it as won't fix. But the generated model includes a WriterImpl.isSetSex() method which does, correctly, return false when the XML instance did not include that element. Doesn't this mean that the generated model is not really at fault, and that it is actually the generated editor that has caused the damage, by failing to call WriterImpl.isSetSex() before calling WriterImpl.getSex()? Try the same thing with an xsd:int. You'll see the same behavior. The property sheet kind of sucks and does help with showing isSet being true or false. Yes, I tried adding Birth Year as an optional xsd:int, and everyone shows up as 2,011 years old. Which is less confusing than defaulting everyone's sex to female, or defaulting everyone's nationality to Aruban... but it certainly isn't desirable. I tried a few other experiments, and I found that the property sheet works MUCH better for integers if I set nillable="true" in the schema, and that it works MUCH better for both integers and enumerations if I set maxOccurs="2" in the schema. I can't change the schema in real life, so I think that I need to find a line of code in EMF that handles maxOccurs="1" as a special case, and chop it out, so the property sheet will end up using the same user interface for maxOccurs="1" that it currently uses for maxOccurs="2". Can you give me an idea where I should look, to find that line of code and chop it out? When you use nillable, you end up with IntegerObject (java.lang.Integer) rather than int. Similarly for the EEnum case, i.e., a wrapper type that allows null. If you use upperBound > 1 you end up with a list; I doubt you want that in your model. You can look at the EMF recipes to see how you can specialize the property editor; most likely though you can just specialize how the default value is displayed via the property descriptor's label provider. Created attachment 196814 [details]
demonstration of suggested fix: projects, schema, and instance
After enormous effort (as an EMF newbie) I have finally found the real reason for this longstanding problem, and can provide a fix. The real problem is not in the property sheet, nor in the basic behavior of the generated model. The problem is in the EMF.Edit code that adapts between the two. From the WriterImpl's point of view, the sex of the writer is really a compound datum, comprising both a Java value that can never be null, and a boolean that denotes presence or absence. From the property sheet's point of view, the sex of the writer is a non-compound datum, a single Java value that *can* be null. When adapting from the code that provides the non-nillable value and the boolean, to the code that expects the single nillable value, you need to check the boolean, and produce a null if the boolean denotes absence. My suggested fix is to add "if (!isPropertySet(object)) return null;" in org.eclipse.emf.edit.provider.ItemPropertyDescriptor.getValue(). You will probably want to put some guards on it, to do it only when necessary, but in my first cursory testing I have found that things seem to work pretty well with just that one-line fix. I have attached an archive with three projects (model, edit, and editor) that people can import into Eclipse and experiment with; it also includes an XSD schema and an instance document. The schema is exactly like the one in the tutorial, but with "sex" and "birthYear" elements added, as mentioned above. The instance document is exactly like the one in the tutorial. The projects were generated from the schema by following the tutorial, but with these changes: (a) "Compliance Level" set to 6.0; (b) "Provider Root Extends Class" set to org.eclipse.example.library.fix.FixItemProviderAdapter; (c) FixItemProviderAdapter.java added, with the one-line getValue fix mentioned above. You can restore the unfixed behavior of ItemPropertyDescriptor.getValue(), by unsetting "Provider Root Extends Class" and regenerating. I hope you will incorporate this fix into ItemPropertyDescriptor.java, so everyone can benefit from it. The problem with this approach is that the user doesn't know the actual value of the feature. Of course the problem without this approach is that the user doesn't know if the feature is set or not. Either way, it's less than ideal, so better to avoid changes that are potentially disruptive. |