Community
Participate
Working Groups
1. Wipe your deltas. 2. Select a Java file in the 'Package Explorer'. 3. Ctrl+Shift+G 4. The 'Search' view will appear 5. Hide it. 6. Window > Open Perspective > Debug 7. Ctrl+3 > Package Explorer 8. Select the same file. Though it should already be selected. 9. Ctrl+Shift+G 10. The 'Search' view will appear 11. Window > Open Perspective > Debug 12. Select the same file. Again, it should already be selected. 13. Ctrl+Shift+G 14. You will switch to the 'Debug' perspective. All other future attempts to show the view will result in either a) nothing happening or b) switching to the 'Debug' perspective. Based on analysis of the actual model, a physical MPart has been added into the 'Debug' perspective instead of an MPlaceholder.
Created attachment 174932 [details] Reference handling patch v1 This is another unfortunate manifestation of bug 315133 that have plagued me for some time but that I have never been able to identify the cause of until today. The gist of it is that since view references are never destroyed, anyone can try to create the underlying part by invoking getPart(true) on it. The problem here is that since the part has been closed in the original perspective, it is no longer pointing at any particular placeholder. Since the part has no placeholders, the part service will just render it as-is and place it into whichever stack is appropriate. This is why the physical part is placed into the 'Debug' perspective instead of a placeholder of that part. The fix is to create a placeholder for the part before rendering it.
Susan, I feel we need to fix this as the user will get into a state where the view just will not show up in any perspective that's not the one where the physical part resides.
+1 for fixing with the attached patch in RC3.
I can reproduce the bug without the patch, except that step 11 should be >11. Window > Open Perspective > Java (not Debug). I can also see that it's fixed by the patch. Good. I have to say, though, that I don't feel knowledgeable enough about the whole part/reference/placeholder model to really say the code has no problems, or to feel that the problem is general based on the steps given. however, I have had those pesky "for some reason, the view is stuck in some other perspective" problems in daily usage and could never explain how I got there. So I feel that: - if Boris (or another committer) checks the code and is OK and feels we understand the scenarios it fixes and risks, that is good. - I'm wondering why is this an issue only for view references? Basically my +1 is conditional on someone more knowledgeable than me +1'ing the patch, not just the fact that it works and would be good to fix (I can't tell if Boris reviewed the code or not).
I did review the code and had a short discussion on IRC with Remy. Basically, I am for fixing the problem with the attached patch, but after 4.0 releases we should refactor the code. Here's why: After applying the patch, ViewReference.renderModel() creates a model that is not well-formed, in the sense that one model object points to another that has no parent and hence is not in the same resource. It then calls PartServiceImpl.showPart which calls addPart which will make the model well-formed again by setting the parent of the newly created placeholder. Our code should be structured such that you always ensure model well-formedness before calling a method, and before returning from a method, such that one can reason about this by looking at individual pieces of code. This means that if intermediate states exist that are not well-formed, these states should only occur locally, in a single method, and be fixed in the same method. In the concrete example, we should change our API for EPartService to accept an additional flag that tells addPart to add a placeholder if necessary, so that the caller of showPart does not have to do it. Again, given how close we are to the release, and that refactoring our code is not a realistic option, we should apply Remy's patch for 4.0 and open a new bug to track the architectural issue.
(In reply to comment #4) > - I'm wondering why is this an issue only for view references? Editor MParts do not have placeholders. They are physical parts placed inside a _shared_ editor area (an MPartSashContainer). This is why editors can just be rendered as is without worrying about whether it has a placeholder or not (technically speaking, they should not have a placeholder).
+1 based on Remy and Boris' comments. I agree with Boris that ensuring model integrity after any manipulation is the future direction.
Fix released to CVS HEAD.
Verified with I20100726-2152 on Windows XP.
(In reply to comment #1) > Created an attachment (id=174932) [details] > Reference handling patch v1 This patch has been effectively rolled back as part of the work for bug 320578.
