Bugzilla – Full Text Bug Listing
| Summary: | EMFPropertyListener.EMFMapPropertyListener doesn't handle all the different types of notifications that can occur | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Michal Tkacz <Michal.Tkacz> | ||||||||||
| Component: | Edit | Assignee: | Dave Steinberg <davidms> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | Thomas Schindl <tom.schindl> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | Ed.Merks, Kenn.Hussey | ||||||||||
| Version: | 4.0 | ||||||||||||
| Target Milestone: | --- | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Michal Tkacz
Created attachment 184268 [details]
The project containing a snippet illustrating the problem
Created attachment 186545 [details]
First stab at improving the implementation
Tom, please have a look at these changes. Interestingly I started to work on a patch today as well. I'm happy to see that my implementation is similar to yours, Ed :) What I also noticed already is that the listener is not fully functional, because it doesn't respond to changes made to existing entries (changedKeys is always empty). So if I call put(key, value) for a key that already exists in the map, no event will be fired. The only way I can see to solve it is to add listener to every entry in the map. For this addTo and removeTo need to be overriden (to add listener to already existing entries and remove it from all entries when dispose is called), an additional if branch is needed in notifyChanged (to listen for changes of value in entries) and finally the listener needs to be added to every entry being created and removed from every entry being removed. What do you think? Yes, we'd need to listen to the entries themselves to see changes to the "value" feature. We could attach and detatch listeners (perhaps a specialized one that knows it's only for the entries) for these children using the same type of logic as is in EContentAdapter. Created attachment 186795 [details]
An extended version of Ed's patch that takes into account changes to entries' values
Here's a patch that works me in case you want to use it. It's based on the patch posted above but takes into account changes made to the entries' "value" feature and fires events accordingly.
I don't have time at the moment (1-2 weeks) to take a look but if Ed feels comfortable with it I'm quite sure I'm fine too. Would it be possible to add a JUnit-Test for this? Should I create patch for EMFPropertiesTest and org.eclipse.emf.test.databinding or is there a better place where I can find more complicated models (the best would be those with maps) ready to be used in a test? The test suite is very very minimal so you should add it there. My plan is/was that we at least get tests for bugs we fix in the code base. So yes please add it to EMFPropertiesTest. Thanks. Created attachment 186877 [details]
Test case
Here's a patch for org.eclipse.emf.test.databinding. Adjust it to your liking or let me know if you think it could be improved.
Do you think it could be included in M5? I think we are too late for m5, ed? I think Kenn spun and M5 build over the weekend. (In reply to comment #14) > I think Kenn spun and M5 build over the weekend. Yes, M5 is already in the bag. We can spin an integration build with the changes as soon as they're committed, though, if that would help. So is head open for commits? (In reply to comment #16) > So is head open for commits? Ed should probably tag EMF and XSD based on the milestone first, but after that it is, yes. The tagging is in progress. So, is everything ready for the commit now? Let's commit it while the patch is still valid :) released changes to HEAD - I'm not sure what i have to set the Target Milestone too An integration containing this fix is now available. |