Community
Participate
Working Groups
Build ID: 3.4 I20071127-0800 At present, PDESourcePage's fSelection, which holds the currently selected item is protected rather than private. Although also exposed via a getSelection() method, this has meant that various other classes are accessing this attribute directly. This is preventing refactoring of the selection mechanism in general as described by bug 211698 comment 4. To resolve this bug, the fSelection attribute should be made private, and all references to it corrected to use the getSelection() method.
One of the reasons (that I can see from the code) to make the field protected is to allow subclasses to assign a value to that variable. If we make the field private, we will need to create a method to allow subclasses to continue to set the value of that field. I figured you already figured that out, but just wanted to make sure :)
Absolutely, and in fairness the refactoring I had in mind to support bug 211698 wasn't just a simple change of storing the selection with storing the selection event and altering getSelection to probe inside the event. I was hoping to be able to have a new SelectionHelper class (inner withing PDESourcePage) that would enable the selection object to be cached from the event (to prevent repeated expensive lookups as a consequence of supporting ITextSelections) and to allow for explicit setting where appropriate. Should have probably made that clearer in the initial description - sorry :-)
Unfortunately, I can't visualize what you are suggesting, but I trust you. When you get a patch go ahead and attach it and we will see what we can do about getting it in.
Created attachment 86385 [details] WIP Patch - minimal refactoring to remove protected fSelection This is a minimal refactoring; i.e. just like for like replacement using proposed SelectionHelper solution - no additional support for ITextSelection, yet. As a minimal refactoring, this addition of the inner SelectionHelper class may seem a little over the top, but try to think about the changes that would be needed once ITextSelection is supported. Since we should be keeping the processing performed during the SelectionChangedEvent down to a minimum due to the frequency with which they might occur, and the additional processing required for processing an ITextSelection over an IStructuredSelection, this inner class protects the source page from that complexity, always making available the selected object when needed.
Created attachment 86388 [details] WIP Patch - minimal refactoring to remove protected fSelection Whoops - I've managed to include a couple of changed to files in .settings in the patch file. Changes removed.
Brian, Since 'a picture is worth a thousand words'... I've attached the above patch to show what I meant. Have a look and let me know what you think.
FYI - don't let this patch hold up the reformatting work we've been discussing in bug 214595. I can easily re-apply these changes again to reformatted code when another patch is wanted for this.
Thanks for your hard work Les. It took me a while to understand, but I think I have a better idea of what is going on :) Let me know if I am on the right track. To support bug 211698, instead of having custom logic in PDESourceInfoProvider to look up the current text selection, you want to build the logic into PDESourcePage. And in order to handle the scenario of ITextSelection, you added the SelectionHelper nested class (where you will add a case to get the proper selection for ITextSelection), right? I could still be off cloud 9. If this is right, I wanted to know why you used a nested class? Might it be more simple to make fSelection private and continue using that? We could still add similar logic to handle ITextSelection and set fSelection accordingly. I figured I would ask because this would not be the first time I overlooked something.
First, to reassure you, you are on the right track - so you're not off on cloud 9 - well, not on this bug ;-) The reason I didn't make fSelection private was because I felt that processing of ITextSelection would no only be more expensive than that of IStructuredSelection, but it would also be more significantly more frequent for those in the source pages. Also, the object that would currently be selected is also not of any interest for most of those events; it would typically only become of interest when the quick outline is selected (anpaged probably in some other similar cases). Therefore, I introduced that notion of delaying the processing of the ISelection to the point at which the selection object was queried. That said, running some further tests it appears that the time to execute the processing we'd probably require for ITextSelection is negligible (at least as far as my PC's clock is concerned) - so this is perhaps a piece of premature performance optimisation. So, if we ignore that last point for the moment and assume we want the delayed processing; the processing of the selection is now a little more involved. The inner class allows this processing to be encapsulated within a class that has a clear responsibility; with the source page delegating to it. This allows for better cohesion and separations of concerns. To me, it's no different from when one chooses to use an anonymous listener inner-class when processing events as opposed to implementing the interface of the main/parent class which then registers itself. If we agree though, that the processing can and should be done on the fly (at message receipt), then the inner class mechanism (although it would still work) is perhaps overkill.
I am glad I asked :) It is now all making sense. I can also see why you made the optimization since the text selection is fired quite often. It was pretty clever. Unfortunately, like you mentioned above as of right now there does not seem to be much of a performance hit in processing a ITextSelection. Therefore, I decided to remove the SelectionHelper class. We have the patch saved in the bug report in case we do see performance problems in the future. Thanks for the quality patch Les! Now if I am correct, we should be able to remove the fix we dropped for bug 211698, right?
Unfortunately not :-( I checked out your changes and reversed out the patch from bug 211698 and it didn't work as I'd wanted. It seems that the ITextSelection events are not firing as I'd assumed they would; although they are incredibly frequent, they are not frequent enough. You can see this for yourself if you also reverse out the change (I'll attach a new 'reversing' patch since there's a conflict when I try to do it automatically from the one in the bug). If you open the manifest editor, go straight to the plugin.xml tab (the caret will be at the top of the file) and press Ctrl-O, you'll see the selected item as "plugin" which is correct. Now cursor down (with the keyboard) into an extension point and press Ctrl-O, you'll see the selected item hasn't changed and is still at "plugin". Now press Shift-Right to select a character and then Ctrl-O; the selection is now correct. So the selection events are not fired whenever the caret moves, which whilst unfortunate, is I guess understandable. Therefore, we will be unable to back out the change from bug 211698 :-(
Created attachment 86544 [details] Patch to backout changes from 211698 - as promised Apply forwards, i.e. it IS a reversing patch, rather than it being a 'normal' patch that should be applied 'reversed'. Hope that makes some sense ;-)
> I checked out your changes and reversed out the patch from bug 211698 and it > didn't work as I'd wanted. Just to make sure, was it something that I changed that caused this? Want to make sure I don't butcher your code :)
No, I don't think so - what you've done seems fine to me. I've also gone back and reapplied my changes, altering them to support ITextSelection as well (just in case) and the results from running some tests are consistent with the results I get when I use what you've produced. I guess this is just down to a false assumption by me about when the events might be fired and how we could use them to get the context for the quick outline we wanted :-( Even so, it's still better that the selection variable is private; just a shame we couldn't make this work for bug 211698 too.
Created attachment 86607 [details] patch to listen to postChange events Les, you might be able to do it depending on how much more time you want to put into this one. I was playing around with it for a few minutes and was finding an event is fired when the user moves the selection with the keyboard. If you place a break point in MultiPageSelectionProvider.fireEventChange (line 162), you will see a postSelection event is fired when the keyboard is used. I attached a patch to register PDESourcePage to listen to postSelection events. My concern with the patch is that we get twice as many notifications when a regular selection event is fired. It would be good to try to optimize that some how. I don't have as much time as I want to inorder to look into this more, but I imagine there is a way to make it work correctly, so if you are interested I can help you try to fix the keyboard issue. Otherwise, I think we are at an acceptable point :)
Created attachment 86656 [details] Patch (ontop of those previously applied to HEAD) I think that given the fix we have in place for bug 211698 that we are in an acceptable position; however, before we drop this have a quick look at this patch. The patch backs out the 211698 change and applies another new one. I'd popped over to the JDT's Java Editor to see what it did, and when I came back to PDESourcePage I stumbled across handleSelectionChangedSourcePage()... suddenly I found that I'd taken my 'hard of thinking' hat off and found what we were looking for. This is called, even for just simple moves of the caret. In addition, it compresses multiple moves into a single event by delaying the event slightly, reducing the impact of processing the event - and is the techique already used when synchronising the outline view. The only down side is caused by the delay... if you move the cursor with the keyboard and very quickly press Ctrl-O (i.e. within a second) you'll get the wrong item highlighted. This is only going to be a problem for those who have extremely fast fingers... so 'may' be acceptable. That said, it is a slight regression of the functionality provided by the current 211698 fix. Thoughts?
I didn't know it was going to be THAT easy :) Thanks for continuing to work on it even though we had an acceptable solution! Made two minor changes before committing. First, I removed the code from bug 211698 instead only commenting it out. Second, added a "super.handleSelectionChangedSourcePage(event);" to BundleSourcePage. It seems BundleSourcePage extended handleSelectionChangedSourcePage without calling super, which caused the selected element to be incorrect for the MANIFEST.MF. You rock Les.
