Community
Participate
Working Groups
Since the changes in commit https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3f86f44d634a9b9c56a3904b041673b498e1e948 for bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=23862 I believe the Eclipse properties view is broken for E4 views. This is a regression from https://bugs.eclipse.org/bugs/show_bug.cgi?id=403930 The problem is around the changes to the properties view introducing a requirement that the old getSite().getSelectionProvider() E3 behaviour is implemented. This is in PropertySheet.doCreatePage(). In an E4 view, we are expecting to use ISelectionService.setSelection() to set selection, which does not interact with the selection provider framework. The result of this change is that selections in E4 views just result in the message “The active part does not provide properties.” Documented in PropertySheet, you can supply an adapter that adapts your view object to an IPropertySheetPage and supply your own implementation. We have attempted to use this capability as a work around, like this: public <T> T getAdapter(Object adaptableObject, Class<T> adapterType) { if (adaptableObject instanceof TableView) { if (IPropertySheetPage.class == adapterType) { return (T) new PropertySheetPage(); } } return null; } However E4 views are all of type E4PartWrapper, so to make this work we have had to introduce code that unwraps the E4PartWrapper and delegates the adaption - this code ought to be implemented by Eclipse because it requires to be in org.eclipse.ui.internal to access the internals of E4PartWrapper: // TODO this ought to be implemented in the Eclipse code-base public <T> T getAdapter(Object adaptableObject, Class<T> adapterType) { if (adaptableObject instanceof E4PartWrapper) { if (IPropertySheetPage.class == adapterType) { MPart wrappedPart = ((E4PartWrapper) adaptableObject).wrappedPart; T psp = (T) Adapters.adapt(wrappedPart.getObject(), IPropertySheetPage.class); return psp; } } return null; } How to fix it…? 1) Adding an E4PartWrapper adapter that unwraps and delegates, along the lines of the code suggested. This requires every client to add their own adapter for every E4 view. 2) Having the properties view understand both the old E3 mechanism and new E4 mechanism would solve the problem properly.
Dave, thanks for the detailed report. Do you want to contribute a gerrit patch with the fix, probably with the option 2? We can discuss how that could be done in detail on the patch?
Thanks Andrey - I'm not sure I understand the architecture well enough to propose a fix. For E3 views, when the part is activated, the code says "does the part have a selection provider". If the answer is yes, then we decided a properties view is useful. If not, then it's not useful. What's the equivalent for an E4 view? The code could check whether a selection has been set on the view, but when the part is brought up the selection won't have been set. So this way, the properties view would only work on the second activation of the view, in E4! I think the dynamic nature of E4 views would make it difficult to know in advance whether a selection might be set. And therefore I think the decision to blank the properties view out when switching to a part that doesn't provide selection might be a difficult one. I'll see if the team have any thoughts. Did you have any immediate ideas about the direction a fix would go in? Frankly we will have to support the Eclipses currently in the field so I guess we will likely need to use the workaround anyway.
I had a first quick look. First, AFAIK, the ISelectionService is an E3 concept, the ESelectionService is the E4 concept. The properties view is still pure E3, I rather not add E4 concepts there, as it will for sure leave other compatibility issues. I think that there are two issues now: First, the properties view does not show properties for parts that use the ISelectionService. This is a regression from Bug 23862. In E3 it could be discovered with 'hasService' if a part instantiated the ISelectionService. This call would only return true after 'getService' was called first. There could be a problem if the service is requested after the properties page is shown, it will show an empty page. The same is true for late added selection providers. However, in E4 this hasService call will alway return 'true', the key is always in the context. So changing this will effectively show the (empty) properties view all the time. Before changes in Bug 23862, the view was always shown too, though it showed the last valid selection of a previous part. So this shouldn't be a problem. The second issue is in the compatibility for wrapping E4 parts. E4PartWrapper should forward the getAdapter call to the E4 framework. This should be a getAdaptor call on the POJO and/or trying to receive the object from the context, with a fallback to the current implementation.
(In reply to Rolf Theunissen from comment #3) > I had a first quick look. > > First, AFAIK, the ISelectionService is an E3 concept, the ESelectionService > is the E4 concept. The properties view is still pure E3, I rather not add E4 > concepts there, as it will for sure leave other compatibility issues. > > I think that there are two issues now: > First, the properties view does not show properties for parts that use the > ISelectionService. This is a regression from Bug 23862. > > In E3 it could be discovered with 'hasService' if a part instantiated the > ISelectionService. This call would only return true after 'getService' was > called first. There could be a problem if the service is requested after the > properties page is shown, it will show an empty page. The same is true for > late added selection providers. > However, in E4 this hasService call will alway return 'true', the key is > always in the context. So changing this will effectively show the (empty) > properties view all the time. Before changes in Bug 23862, the view was > always shown too, though it showed the last valid selection of a previous > part. So this shouldn't be a problem. > > The second issue is in the compatibility for wrapping E4 parts. > E4PartWrapper should forward the getAdapter call to the E4 framework. This > should be a getAdaptor call on the POJO and/or trying to receive the object > from the context, with a fallback to the current implementation. @Rolf, Can I assign this bug to you?
@Rolf, Can you look at this issue?
(In reply to Kalyan Prasad Tatavarthi from comment #5) > @Rolf, Can you look at this issue? I am trying to spend some time on it. I expect to provide 2 patches: 1) to support adaption of e4 parts 2) fix the regression w.r.t. ISelectionService
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/183100
That patch is helpful! With registering ourselves an adapter I'm now able to have the properties view working.
I imagine a potential fix for the ISelectionService regression would mean I don't even have to register an adapter. This would be brilliant.
In Bug 23862 it is assumed that views only provide properties that have a selection provider. AFAIK this is a correct assumption in E3 API. However, in E4 API, selections are provided with the ESelectionService. There are 3 possible solutions: 1. Let the E4PartWrapper register a ISelectionProvider in the IViewSite. This would probably provide best compatibility. However, I consider this risky, it is hard to predict what the impact is on the compatibility already in the ISelectionService implementation. 2. Introduce an adapter factory that adapts E4PartWrapper to IPropertySheetPage (using the default implementation). 3. Remove the check in ProperySheet to only show properties for views that provide properties. The properties view will be shown for all views. I opt for option 3, as this is least complex and no risk for regressions. Moreover, although it is a nice feature to disable to properties view, in practice many views do provide selections but not of (adaptable) type IPropertySource. As a result, the properties view doesn't show any content for these views either, while it is enabled.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/184025
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/183100 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7ad1b8a3b3732b88eb3c4803364e1aae611604a2
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/184025 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=57558beded75f2536e805341818a70f27b18421a
I agree with your assessment on this approach, Rolf. Also I can confirm that in the latest 4.21 milestone the properties works again for E4 views. Thank you!
