Bugzilla – Full Text Bug Listing
| Summary: | Generated editor does not handle pinning and/or multiple property viewers | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Ales Dolecek <ales_d> |
| Component: | Edit | Assignee: | Dave Steinberg <davidms> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | Ed.Merks, give.a.damus, jawr, Kenn.Hussey, mdt-papyrus-inbox, pwebster |
| Version: | 2.6.0 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 397324 | ||
I didn't even know this was possible. It all seems very problematic for a properties view that doesn't just show information but is actually used for editing: it's not okay for it to last longer than the life of the editor itself. The properties view is also refreshed when there are state changes so the editor has to know to do that. It's not at all clear how to make all this work correctly... The fix (maintain a list of property sheet pages) is committed to master: http://git.eclipse.org/c/emf/org.eclipse.emf.git/commit/?id=4f68e28fcbb3f1a7266d7d4857a789e745427b21 This change breaks the protected API of generated editors. They used to have a protected 'propertySheetPage' field, but now it is replaced by a 'propertySheetPages' field. This breaks the Papyrus project's subclass of the UMLEditor. (granted, it may not be intended that generated editors be subclassed in this way, but there we have it. This is API according to Eclipse convention) In any case, now I have to figure out how to adapt. The obvious approach would be to use the getPropertySheetPage() method instead of accessing the field, but I see that this actually creates a new page and adds it to the list on every invocation. How will these pages be removed from the list? I see that the IAdaptable::getAdapter() implementation for the IPropertySheetPage interface calls this method, and that adapter can be fetched any time for any reason. Are we not now accumulating pages in this list without end (until the editor is closed, of course)? In the Papyrus case, the requirement is to refresh the property sheet page if it is not yet disposed, so I think we can just iterate the propertySheetPages list and refresh each that is still in use. However, this continues a dependency on a protected field that I would rather not access directly ... Moreover, not all of the property sheet pages created by getPropertySheetPage() will have had their part control created, so iterating the list to see whether a page is disposed (as in the editor's command-stack listener) appears to be susceptible to NullPointerExceptions. Yes, it breaks the API. Of course you can add the old field manually, and you could set things up so that the old field is populated with the most recently created property sheet page. That would preserve most of the API and the old behavior (other than getPropertySheetPage creating new pages). You could modify the getAdapter code to call a different method that creates new ones to preserve the getPropertySheetPage behavior. So you can certainly preserve the API, but the problem is, the old API/behavior is fundamentally broken. It's obviously not okay to use getPropertySheet page outside of the getAdapter call because it creates new pages. If you look at the full generated code (you might have @generated not marked the method), you'll see that the stale pages are discarded during the update processing in the command stack listener, so stale pages will remain only as long as the next command execution. So yes, you need to iterate over all the pages and refresh all the non-stale ones (and can discard the stale ones). The getPropertySheetPage should only be called in response to getAdapter calls and in that case, the page is always hooked to a control in the UI. If the control is null, there's a programming error that needs fixing. So in summary, the old API is fundamentally broken. You could set things up manually so that clients see the old API and behavior, but they'll also see manifestations of this bug. Even the example you mention here, i.e., needing to refresh the properties sheet page, is exactly an example where the client needs the new API to refresh all the pages. (In reply to comment #4) > > It's obviously not okay to use getPropertySheet page outside of the > getAdapter call because it creates new pages. If you look at the full > generated code (you might have @generated not marked the method), you'll see > that the stale pages are discarded during the update processing in the > command stack listener, so stale pages will remain only as long as the next > command execution. So yes, you need to iterate over all the pages and > refresh all the non-stale ones (and can discard the stale ones). Thanks, Ed. So, if the contract of the IPropertySheetPage adapter is that all clients that obtain such adapter must create its control, then we'll have neither null-pointer issues nor leaks. I'll raise a bug for Papyrus to update its UMLEditor subclass. The changes are available in Kepler. *** Bug 398521 has been marked as a duplicate of this bug. *** |
Editors generated by EMF.Edit UI have single instance of PropertySheetPage which leads to problems when user uses multiple property views. Steps to reproduce in any EMF generated editor: 1) open new editor and created resource with 2 objects 2) select first of the objects and open properties view 3) use properties view menu (drop down next to view maximize icon in top right corner) - select new properties view => you have two views that share same PropertySheetPage provided by editor First bug: 4) pin one of the views to current selection and change selection - both views change (because they both show same property sheet page) Second bug: 5) close one of the views - the other will go blank and won't be usable any more - application log contains following message java.lang.RuntimeException: WARNING: Prevented recursive attempt to activate part org.eclipse.ui.views.PropertySheet while still in the middle of activating part org.eclipse.ui.views.PropertySheet 6) change active part to force properties view use different PropertySheetPage and then return back - you will get Error Pop-Up with following message: An error has occurred when activating this view Widget is disposed --- As I already wrote the problem is in sharing PropertySheetPage by multiple property views. Maybe this is problem of property view - you do not know which property view is calling editor.adapt(IPropertySheetPage.class). The editor probably should not cache the instance returned from adapt - I belive it is property view responsibility to dispose the page when the editor is closed - it installs part listener (implemented by inner class of org.eclipse.ui.views.properties.PropertySheetPage). Ales