Community
Participate
Working Groups
I am trying to implement properties for a text editor (154781). PropertySheet currently adds itself as a selectionListner to the associated workbenchPage instead of a postSelectionListener. Text editors fire selection events multiple times when the user is highlighting text while a post selection event will only be fired after the user stops moving the cursor. Listening to selection events instead of post selection events is a performance issue for text editors that want to use the Properties view. Fixing this bug might change behavior for existing Properties page contributors. If it does break an existing contributor it is most likely because they aren't following the contract of a Selection Provider and implementing IPostSelectionProvider
Created attachment 52593 [details] Proposed patch to make properties view a post selection listener
+1 for this change (see bug 154781 comment 24 for details) > If it does break an existing contributor it is most likely >because they aren't following the contract of a Selection Provider and >implementing IPostSelectionProviderusing post selection changed event if >possible. I didn't check the patch but the fix should continue to respect normal selection changed listeners in case the provider doesn't support post selection change events.
Just to clarify: the +1 is not for the patch but for the bug to be fixed ;-)
Comment on attachment 52593 [details] Proposed patch to make properties view a post selection listener I am rescinding my patch as I am unsure of how it should really work.
*** Bug 331616 has been marked as a duplicate of this bug. ***
Let me fix this.
Fixed in HEAD. Available in builds >= N20101203-2000.
*** Bug 132262 has been marked as a duplicate of this bug. ***
(In reply to comment #7) > Fixed in HEAD. > Available in builds >= N20101203-2000. Dani, your change does not seem to address comment 2. Or is it that you no longer feel that that should be of concern?
(In reply to comment #9) > (In reply to comment #7) > > Fixed in HEAD. > > Available in builds >= N20101203-2000. > > Dani, your change does not seem to address comment 2. Did you test it?
(In reply to comment #10) > Did you test it? I did now. It seems it was "never" of a concern! :O AbstractSelectionService's partActivated(IWorkbenchPart) method actually compensates for the scenario where the part site's selection provider is not an implementation of IPostSelectionProvider. The code has been like this since the org.eclipse.ui split it seems, so it was a non-issue since at least 2002. You learn something new every day as I like to say. Sorry we couldn't fix this earlier, Karen.
(In reply to comment #11) > (In reply to comment #10) > > Did you test it? > > I did now. It seems it was "never" of a concern! :O > > AbstractSelectionService's partActivated(IWorkbenchPart) method actually > compensates for the scenario where the part site's selection provider is not an > implementation of IPostSelectionProvider. The code has been like this since the > org.eclipse.ui split it seems, so it was a non-issue since at least 2002. You > learn something new every day as I like to say. Exactly!
This change has introduced test failures in N20101203-2000. http://download.eclipse.org/eclipse/downloads/drops/N20101203-2000/
(In reply to comment #13) > This change has introduced test failures in N20101203-2000. > http://download.eclipse.org/eclipse/downloads/drops/N20101203-2000/ FYI: This was not a problem with the fix but with the tests (see bug 331847 for details).
Verified in I20101206-1800.
Adam has expressed the need to get this in 3.6.2. Given the fix is small and low risk, I've opened bug 332041 to track this.
Dani, during the M4 testing today I noticed that the Properties View no longer works at all !! The scenario I use to test with is: - check out the org.eclipse.ui.examples.propertysheet project - start an inner, create a project and add a file 'foo.usr' to it - open the file, you should see the Outline view give a bunch of names - open the Properties view - select anything in the Outline view...nothing happens in the property view Without the changes to using 'post' listeners all is well... I'd suggest (at minimum) rolling back this fix for M4. I'd also recommend that until more complete testing is done we not back port this fix to 3.6.2.
>Dani, during the M4 testing today I noticed that the Properties View no longer >works at all !! I'll take a look. I've tested with Package and Project Explorer and there it worked fine.
The problem here stems from ContentOutlinePage. The AbstractSelectionService I checked in comment 11 allows a first-layer proxy selection handling to "fake" post selection events. However, the ContentOutlinePage itself does not implement IPostSelectionProvider and it is in PageBookView's showPageRec(PageRec) method that this becomes a problem. We ask the page site here for its selection provider (and we get the ContentOutlinePage back) and try to attach our post selection listener to it. However, as noted above, the COP does not implement IPSP by default. Hence, the post selection listener does not get attached and no post selection events are ever fired. Now you might ask why the proxy selection handling code did not kick in for AbstractSelectionService? That is because PageBookViews have an IPSP implementation that it sets as the selection provider of the _view_. This selection provider acts as an internal proxy (let's say a second-layer proxy) for handling selection events between the PBV's pages and propagating it upwards to the workbench window. In the case of this examples bundle, one of its pages isn't an IPSP so no post selection events are fired. Uh, I hope that made sense. Just debug around the showPageRec(PageRec) method and you'll see what I mean.
The PageBooksView's implementation of the IPostSelectionProvider support is wrong: a post-selection provider has to make sure that a selection change eventually is fired. Since the PageBookView declares to be a post-selection provider it has to make sure that it correctly works even if some of its pages aren't post-selection providers. The same fall-back code like in AbstractSelectionService.setActivePart(IWorkbenchPart) has to be added to the PageBookView. I've fixed this in HEAD (PageBookView.java rev. 1.48) and released it to I20101208-0800. I think it's better to put this in M4 so that we get early feedback in case there are other issues with this. Note that the Outline in the SDK was working as expected, only the example was surfacing the problem. Also note, that there is no other fix possible for the Properties view: even updating the selection listener on part switching (instead of adding it to the page) wouldn't help because the Outline view already says that it supports post selection.
(In reply to comment #20) > I've fixed this in HEAD (PageBookView.java rev. 1.48) and released it to > I20101208-0800. I've reviewed this and I agree to put it into M4.
I still have one concern. The patch includes fixes for a couple of tests, replacing a simple call with an obscure loop containing a 'readAndDispatch' call. Is this just an artifact of it being test code ? I just want to make sure that this pattern isn't something we expect our clients to use...
(In reply to comment #22) > I still have one concern. The patch includes fixes for a couple of tests, > replacing a simple call with an obscure loop containing a 'readAndDispatch' > call. > > Is this just an artifact of it being test code ? I just want to make sure that > this pattern isn't something we expect our clients to use... The original test is based on the selection being immediately fired in the UI thread. This is not true for post-selection and hence if you want to test this you need to spin the event loop. Nothing a real client has to deal with.
Beauty...
Verified that the example works on I20101208-1300 with Windows XP.
This change broke two of our editors (that have worked correctly for years) after customers upgraded to 3.6.2. A search found 3 other cases that appear related: The first: http://forums.activiti.org/en/viewtopic.php?f=8&t=1263 http://jira.codehaus.org/browse/ACT-691 The second: http://www.anddev.org/sdk-adt-emulator-problems-f16/clicking-items-in-graphical-layout-not-updating-properties-t51734.html The third that was fixed somehow: https://issuetracker.springsource.com/browse/STS-1649?page=com.atlassian.jira.plugin.system.issuetabpanels%3Achangehistory-tabpanel#issue-tabs Since we have little control over the Eclipse version used by our customers my solution/workaround was to fire *both* a selection event for pre 3.6.2 and a post selection event for 3.6.2 as: selectionProvider.fireSelectionChanged(event); selectionProvider.firePostSelectionChanged(event); Either this change needs more work or I/we need a better understanding of "following the contract of a Selection Provider and implementing IPostSelectionProvider".
(In reply to comment #26) > This change broke two of our editors (that have worked correctly for years) > after customers upgraded to 3.6.2. Are those editors multi-page editors? If so, see 339360.
(In reply to comment #27) > (In reply to comment #26) > > This change broke two of our editors (that have worked correctly for years) > > after customers upgraded to 3.6.2. > Are those editors multi-page editors? If so, see 339360. Ed, are your editors subclasses of MultiPageEditorPart (or FormEditor)?
Yes, our editors are extensions of MultiPageEditorPart and the page editors are swt control editors extending EditorPart. This gives you a MainEditor with attached EditorSite with attached MultiPageSelectionProvider. The nested PageEditors each have an attached MultiPageEditorSite with no attached SelectionProvider. I take it that to properly use the API one needs to supply a SelectionProvider to the nested editor sites that then forward events to the main provider. Is this right? I bypassed all of this (years ago) and had a method in the main editor as: public void command_setSelection(IStructuredSelection arg_ssel) { final MultiPageSelectionProvider selectionProvider = (MultiPageSelectionProvider)this.getSite().getSelectionProvider(); if (selectionProvider != null) { final SelectionChangedEvent event = new SelectionChangedEvent(selectionProvider, arg_ssel); Display.getDefault().syncExec(new Runnable() { public void run() { selectionProvider.fireSelectionChanged(event); selectionProvider.firePostSelectionChanged(event); } }); } } Called by the nested editors. However this is implemented do you need to fire both a selection event and an post selection event in order to insure all event consumers get notified? Is there supposed to be functionality to create a post selection event if none is fired? How exactly should this be implemented?
(In reply to comment #29) > Yes, our editors are extensions of MultiPageEditorPart and the page editors are > swt control editors extending EditorPart. If you pick up the fix for bug 341219 then you shouldn't need to force a post selection event to be fired for your 'Properties' view to work again. That has been released to the 3.7 stream. > I take it that to properly use the API one needs to supply a SelectionProvider > to the nested editor sites that then forward events to the main provider. Is > this right? No, you should only have to set your selection provider to your editor's site. However, because of bug 341219, this didn't actually work properly. > However this is implemented do you need to fire both a selection event and an > post selection event in order to insure all event consumers get notified? That would depend on whether you are forwarding events to a root selection provider or if you yourself are the selection provider. In any case, if you do not implement IPostSelectionProvider, then you have no obligation to fire post selection events. > Is > there supposed to be functionality to create a post selection event if none is > fired? No, how and when you fire the event is up to the implementation, assuming you want to implement that interface.
Not all changes made it into the 4.x stream. Cherry-picked now together with bug 341219: Commit a5e52d50b8b26e06ea3be32dde1536a4c32e7d69
