Community
Participate
Working Groups
With changes in the the sorting in 3.5M6 related to bug 162330 it revealed some errors in the CNF configuration where JDT was overriding resources not associated with Java. I have attacted the patch provided by Tony to address this. The patch also contains a line that is the suggested patch for bug 215729.
Created attachment 133210 [details] Suggested patch
How does the problem manifest in the UI, i.e. can you please attach a test case.
Created attachment 133310 [details] Sample project (In reply to comment #2) > How does the problem manifest in the UI, i.e. can you please attach a test > case. - Install Eclipse M6 + CDT 6.0 M6. - Import attached project - Expand it in the Project Explorer - Observe the (wrong) sort order: test.c test.txt Binaries Debug Includes src - Disable the Java Elements content extension - Observe the new (correct) sort order Binaries Includes src Debug test.c test.txt
This is a pretty serious issue for CDT and possibly other PE contributors. Please consider for the next release candidate.
We'll have a look for RC1.
Francis, I looked at the patch and it is no good: with this patch the Project Explorer goes empty if I only enable 'Java Elements' content. Also, I'm not 100% sure this is a JDT (only) issue: if I disable 'Resources' and leave 'Java Elements' content then CDT is also sorted correctly. Can you please take a closer look? We can review another patch but won't have time to deeper investigate this for 3.5.
(In reply to comment #6) > Francis, I looked at the patch and it is no good: with this patch the Project > Explorer goes empty if I only enable 'Java Elements' content. That's because <instanceof value="org.eclipse.core.resources.IWorkspaceRoot"/> is missing from the triggerPoints expression. > Also, I'm not 100% sure this is a JDT (only) issue: if I disable 'Resources' > and leave 'Java Elements' content then CDT is also sorted correctly. Interesting. I did not notice this before and I don't have an explanation either.
>That's because > <instanceof value="org.eclipse.core.resources.IWorkspaceRoot"/> >is missing from the triggerPoints expression. Is it at all needed to touch the trigger points to fix the sorting?
(In reply to comment #8) > >That's because > > <instanceof value="org.eclipse.core.resources.IWorkspaceRoot"/> > >is missing from the triggerPoints expression. > Is it at all needed to touch the trigger points to fix the sorting? > I already tried adding only parentExpression to the commonSorter, but it was not sufficient (bug 162330 comment 8).
Francis, can you take a look at this?
(In reply to comment #10) > Francis, can you take a look at this? > Yes, this weekend. Sorry for the delay, been very busy.
(In reply to comment #6) > Francis, I looked at the patch and it is no good: with this patch the Project > Explorer goes empty if I only enable 'Java Elements' content. > > Also, I'm not 100% sure this is a JDT (only) issue: if I disable 'Resources' > and leave 'Java Elements' content then CDT is also sorted correctly. > > Can you please take a closer look? We can review another patch but won't have > time to deeper investigate this for 3.5. > I'm happy with this patch as it is (that is without Tony's suggested additional "workspace root" item). I think it's reasonable to require the resources content extensions to see anything (that's based on resources), so turning off resources will turn off everything (that's based on resources). With this patch, it works the same for CDT and JDT. Previously, there was no difference between Resources + JDT and just JDT alone, so we are not losing any functionality here. So though this is a change in the behaviour, I think it's actually a good change, and I think that people can understand it. The configuration for the JDT UI was having the JDT content provider deal with resources which was really incorrect. I would be willing to write up something for the release notes about this change (since the UI does change). The benefit of this change is that the sorting for CDT now works correctly whether or not JDT is turned on, since JDT is now concerned only with it's own projects, and I think that benefit is huge. Giving this back to you Dani.
OK, I investigated into this and found out that the last two changes made to NavigatorContentServiceContentProvider for fixing some label issues causes the wrong sorting because this was added: pipelinedChildren.setContributor((NavigatorContentDescriptor) theOverridingExtensions[i].getDescriptor()); and this removed: pipelinedChildren.setContributor(null); As a result the last overriding descriptor is set for all children (in our case projects). This results in all projects getting the JDT descriptor because JDT is the last in the loop. Note that if the loop would first handle JDT and then CDT then all projects would get CDT descriptors and we'd probably see other errors. Reverting that class to rev. 1.17 (or even rev. 1.18) perfectly fixes the sorting issue. Note that this results in a broken JDT label (see attached picture) but that is because JDT missed to declare a possible child type (see bug 275847). This finding supports my earlier theory that there's a problem in the framework itself, because if I disable 'Resources' and leave 'Java Elements' content then CDT is also sorted correctly. Also note that when I use I20090209-2000 (which looks like having all the sorter changes - which got previously claimed to be the cause for this bug here - but not the label provider fixes) everything is sorted as expected. It looks like one of the fixes for: bug 255793 [CommonNavigator] [CNF] Chosen label provider should match content provider bug 252293 [CommonNavigator] LabelProviders do not obey override rules introduced the sorting issue as a side-effect. Moving back to CNF for a closer look.
Thanks Dani for your looking into this in such detail. Unfortunately it's too late in the 3.5 cycle to change this code without extensive investigation (and I don't have the time for that investigation at the moment). Even though the changes you suggest could fix the sorting, it would break the other work for the labels which has been stable since M6. Though I agree with you that it is a change in the CNF that caused this problem, I think the Tony's patch is a cleaner JDT UI configuration because then the JDT configuration only affects the JDT-specific resources rather than all of the resources. Of course even with Tony's patch, additional testing of downstream products like DTP and WTP is necessary (not to mention the IBM internal products). The problem with the CNF of course if that this stuff is not well documented (and in some cases the documentation was just wrong), and people got things to work by adjusting their configurations until it worked. And of course we have to live with this and not break the behaviour of current code where possible. I'm not sure of the best solution for 3.5, but for the moment I plan to do nothing; I can't spend any more time on this this week. If further investigation is required for a potential fix (and I would advocate as part of that investigation the consideration of Tony's patch), then someone needs to escalate the matter to the PMC so that a solution will get approval. I'm not planning on doing this, but others are welcome to if they feel it's important enough.
The patch for JDT is a no go, especially because it alters the trigger points and the actionProviders which have proven to work over years and it's definitely too late to change this now. My fear when looking at the code that breaks things is that the sorting issue is just one of many other problems that this could cause and I strongly urge to take a closer look and try to address this properly for 3.5.
(In reply to comment #13) > OK, I investigated into this and found out that the last two changes made to > NavigatorContentServiceContentProvider for fixing some label issues causes the > wrong sorting because this was added: > > pipelinedChildren.setContributor((NavigatorContentDescriptor) > theOverridingExtensions[i].getDescriptor()); > > and this removed: > pipelinedChildren.setContributor(null); It looks like the logic of pipelineChildren() is wrong as soon as there is more than one overridingExtension. Esp. since pipelinedChildren.setContributor() now changes the contributor association of already added elements. The implementation of pipelineChildren() also seems to be inconsistent with pipelineInterceptAdd() and pipelineInterceptRemove(). I suggest to restore the original behavior when there a are more than one overrdingExtensions.
(In reply to comment #16) > It looks like the logic of pipelineChildren() is wrong as soon as there is more > than one overridingExtension. > Esp. since pipelinedChildren.setContributor() now changes the contributor > association of already added elements. > The implementation of pipelineChildren() also seems to be inconsistent with > pipelineInterceptAdd() and pipelineInterceptRemove(). > > I suggest to restore the original behavior when there a are more than one > overrdingExtensions. > Thanks Tony, I will try to spend some time looking at this today. I think there is a similar comment that I have not gotten to in one of the label related bugs that Dani mentioned in a previous comment.
I have looked at this some today, but am not able to come to a resolution, and I don't feel comfortable with any change in this code for the RC1 timeframe. I think the issue with sorting however could be related to changes that I made in CommonViewerSorter. Originally getSource() called findDescriptorsWithPossibleChild(), but then it was changed (in an attempt to deal with allowing sorting overrides) to first check the hash (was like the old cache), which got the source based on the item that contributed it, and then check for descriptors for *trigger point*. I don't remember why I changed it that way, and don't have time today to figure that out. I'm very recluctant to change the pipeline code because that could easily break other downstream products (WST, DTP). And it's not yet clear to me (though it may be to others) what's exactly wrong with it. This will take considerable study and testing to make sure it's right, and I can't spend that kind of time on this right now. So I don't know if this should be considered for RC2, or post 3.5. I leave it to others to make that call. For the moment, I'm going to remove the milestone.
(In reply to comment #4) > This is a pretty serious issue for CDT and possibly other PE contributors. > Please consider for the next release candidate. Anton, it looks like this bug will not be fixed for 3.5. How bad will this be for CDT? Is there a workaround? If you are willing to look into this, feel free to attach a patch - I would be happy to review patches. Adding Martin to the cc list so that he (as an Eclipse PMC member) is aware of this bug.
Dani, can you explain to me why the JDT UI needs to have its Navigator Content Extensions invoked for non-Java resources (this is the way it's currently configured)?
Created attachment 135436 [details] screenshot (In reply to comment #4) > This is a pretty serious issue for CDT and possibly other PE contributors. > Please consider for the next release candidate. I agree with Tony that it is pretty serious. Look at attached screenshot, looks as some crappy product. It gets really bad on a big project. Does not reflect on CDT or Eclipse well.
Should probably add McQ to this as well. I agree that this bug is quite noticeable.
So, unless someone has a brainstorm of some kind, it sounds like this is the way 3.5 will be shipped. If so, I would very much like to see it resolved for 3.5.1, if at all possible.
(In reply to comment #23) > So, unless someone has a brainstorm of some kind, it sounds like this is the > way 3.5 will be shipped. If so, I would very much like to see it resolved for > 3.5.1, if at all possible. > I believe that the JDT UI patch is the best way to proceed with this, it's the least risk and it fixes the problem.
(In reply to comment #16) > It looks like the logic of pipelineChildren() is wrong as soon as there is more > than one overridingExtension. > Esp. since pipelinedChildren.setContributor() now changes the contributor > association of already added elements. > The implementation of pipelineChildren() also seems to be inconsistent with > pipelineInterceptAdd() and pipelineInterceptRemove(). I agree. Something is not quite right here. > I suggest to restore the original behavior when there a are more than one > overrdingExtensions. It seems to me that all this code is there to remember, for each item in the tree, which content extension contributed it. Is this correct, Francis? Here are some of the open questions that I have: 1) Why is finalChildrenSet in internalGetChildren and finalElementsSet in getElements a ContributorTrackingSet and not just a HashSet? As far as I can see, setContributor() is never called on these sets. 2) If there are no overriding extensions, pipelineChildren() will not be called, and thus setContributor() will not be called on the "localSet" objects in those methods. Why is the ContributorTrackingSet "localSet" created eagerly in internalGetChildren and finalElementsSet, and not just in the if statement where we know that there are overriding extensions? 3) If we went back to the original logic regarding setContributor in pipelineChildren and ContributorTrackingSet (call setContributor() inside the if, uncomment the setContributor(null), ), for which of the bugs that were fixed in 3.5 would this cause a regression? Btw, I agree with Francis that at this point it is safer to either fix this bug through changes in JDT, or ship 3.5 with the bug. The discussion in bug 252293 shows that this is a very sensitive area and that any change is likely to affect many clients. I agree with McQ that we should try to find a safe fix for 3.5.1. It is probably best to start thinking about it now while the memory is still fresh. If Francis does not have the time to work on this now, is anybody else willing to help?
I agree that any code change at this time is not a good idea. If JDT does not accept a change of triggerPoints, a more conservative fix is to swap priorities of JDT's and CDT's content extensions. That way, CDT's sorting is not influenced by JDT in CDT projects and CDT does not override the sorting in Java projects because it does limit it's extension to cnature projects only. I.e. JDT switches priority from "high" to "normal" CDT switches priority from "normal" to "high"
>Anton, it looks like this bug will not be fixed for 3.5. How bad will this be >for CDT? Is there a workaround? Yes, document in the README that they can either disable 'Resources' content or 'Java Elements' content. >I agree that any code change at this time is not a good idea Changing the plugin.xml can have same effects. Changing the priority from 'high' to 'normal' can also destroy other contributions, e.g. in WTP or upstream products that worked well for years with JDTs contribution. >I believe that the JDT UI patch is the best way to proceed with this, it's the >least risk and it fixes the problem. >Dani, can you explain to me why the JDT UI needs to have its Navigator Content >Extensions invoked for non-Java resources (this is the way it's currently >configured)? Same arguments like you bring them: the JDT side as it is now works and is well tested over years. Changing it at this point is a risk I am not willing to take on the JDT side, given it is not a JDT issue and it has not been caused by any JDT changes. And, sorry it is boring for you if I repeat myself: I am pretty sure that the change to the pipeline will cause other collateral damage, not just for the sorting we see here. Do we know that the value of the label fixes are worth that risk or can you maybe simple revert them and defer them to 3.5.1?
(In reply to comment #27) > Yes, document in the README that they can either disable 'Resources' content or > 'Java Elements' content. I think this helps my argument that the JDT UI plugin.xml changes to have Java stay out of resources that don't belong to it is the correct fix. > >I agree that any code change at this time is not a good idea > Changing the plugin.xml can have same effects. Changing the priority from > 'high' to 'normal' can also destroy other contributions, e.g. in WTP or > upstream products that worked well for years with JDTs contribution. I agree that changing the JDT priority at this stage is a bad idea as downstream plugins depend on this. > > And, sorry it is boring for you if I repeat myself: I am pretty sure that the > change to the pipeline will cause other collateral damage, not just for the > sorting we see here. Do we know that the value of the label fixes are worth > that risk or can you maybe simple revert them and defer them to 3.5.1? The changes to the pipeline mechanism were added to support both the recent label and sorting changes in M6. None of this code has changed since then and there has been much testing with the downstream CNF users since M6. In fact some of the pipeline changes were corrected at the very last moment in M6 because of major regressions in WST (Boris remembers this well). CNF changes in this area have been widely communicated and downstream projects have been encouraged to test these changes since M5; this area has evolved considerably, and as far as I know there are no outstanding defects (except this one) related to these changes. I still think the issue of having the JDT UI provide content for resources that it does not own is a problem, even though it has been working for years. And that fixing this problem seems to make things better. Of course there needs to be testing of the downstream products (particularly WST), but I think this can be quickly verified. One of the issues with the CNF configuration is that there have been significant errors in the documentation and that people just tried their configurations until they "worked". And of course we need to respect that as much as possible to preserve compatibility. But in this particular case of the JDT UI I think there is an opportunity here to improve things. Though the label provider changes might seem unimportant and could be backed out, I think this is far from the case. This has been an area of flakyness with the CNF since the beginning (try using the 3.4.2 Project Explorer instead of the Package Explorer and you will see what I mean). I think the label provider support is far better in 3.5.
(In reply to comment #28) > > Changing the plugin.xml can have same effects. Changing the priority from > > 'high' to 'normal' can also destroy other contributions, e.g. in WTP or > > upstream products that worked well for years with JDTs contribution. > I agree that changing the JDT priority at this stage is a bad idea as > downstream plugins depend on this. I can see how increasing priority e.g. from "normal" to "high" could be a problem for downstream consumers, since their contributions whichare are meant to override the previously "normal" one would not do that any more. On the other hand, I do not quite understand how lowering the priority could be an issue for downstream consumers. What other contributions does JDT potentially have to override with its "high" priority? Or is this a silly question?
>What other contributions does JDT >potentially have to override with its "high" priority? Or is this a silly >question? The point is we do not know. We just know it (i.e. other projects and upstream products) used to work before and we are in RC phase now.
(In reply to comment #25) > 1) Why is finalChildrenSet in internalGetChildren and finalElementsSet in > getElements a ContributorTrackingSet and not just a HashSet? As far as I can > see, setContributor() is never called on these sets. Don't know, it was that way when I got the code. However I don't think it's a problem as without having setContributor() call it will just function as a HashSet. Maybe Michael knows the answer to this. Since the contributor is recorded in the correponding localSet I think that part is working correctly. > > 2) If there are no overriding extensions, pipelineChildren() will not be > called, and thus setContributor() will not be called on the "localSet" objects > in those methods. Why is the ContributorTrackingSet "localSet" created eagerly > in internalGetChildren and finalElementsSet, and not just in the if statement > where we know that there are overriding extensions? I think the reason for this (though I'm not sure) is that the overriding extensions want to have the cumulative view of the child objects (even as provided by upstream extensions), however each extension may decide to remove some, so we also want the union of all of them, which is the point of the finalElementsSet. > > 3) If we went back to the original logic regarding setContributor in > pipelineChildren and ContributorTrackingSet (call setContributor() inside the > if, uncomment the setContributor(null), ), for which of the bugs that were > fixed in 3.5 would this cause a regression? Not sure, but you can try it and run the CNF tests, it's likely some of them will fail and that will tell you something.
>> Yes, document in the README that they can either disable 'Resources' content or >> 'Java Elements' content. >I think this helps my argument that the JDT UI plugin.xml changes to have Java >stay out of resources that don't belong to it is the correct fix. Well, disabling 'Resources' also fixes it and this doesn't imply that the resource plug-in needs to do something. Anyway, the safest thing so close to the release is really to document this in the README and make sure this gets addressed for 3.5.1 with high prio.
Created attachment 135560 [details] Experimental Patch Tony, can you try this patch? It reverts the sorting behaviour to back out the changes associated with the attempt to provide overriding in sorting. Another thing you can try is to set NEW_WAY to true, but remove the part where it checks the contribution memory (that's the part that's set by the pipeline mechanism). I don't think we can go with this for the RC, but I think it will provide some more data. Note that applying this patch will cause the CNF sorting tests to fail.
(In reply to comment #33) > Created an attachment (id=135560) [details] > Experimental Patch > > Tony, can you try this patch? It reverts the sorting behaviour to back out the > changes associated with the attempt to provide overriding in sorting. Another > thing you can try is to set NEW_WAY to true, but remove the part where it > checks the contribution memory (that's the part that's set by the pipeline > mechanism). This patch makes things even worse. It always invokes the WorkingSetSorter on all elements (because of the higher priority of the Working Set extension). I also tried with NEW_WAY=true and commented out NavigatorContentDescriptor ncd = contentService.getSourceOfContribution(o); if (ncd != null) return ncd; It does not help either. In this case JDT wins the race again. I also investigated the question why disabling the Resources extension also fixes the sorting. In this case pipelineChildren() is not called and although the JDT extension is also asked to contribute children to CDT projects (returning an empty array), it does not get associated with the CDT elements.
Just in case this wasn't clear from my earlier posts: I'm not saying that the JDT contribution needs to be the way it is now and that changing it is completely wrong. It's definitely something we should look at early 3.6 but for 3.5 it is way too dangerous to make such a move.
Hm... I'm afraid that 3.6 is too late. Some README workaround MIGHT work for 3.5 since many adopters (including Wind River) will only pick up 3.5.1 in their products. But I think that we'll definitely need this fixed one way or the other for 3.5.1 so I appreciate the idea posted earlier of investigating potential solutions now or at least real soon. Is a fix to the CNF pipelining code even realistic for 3.5.1 ? If I may reiterate on the question posted by Francis, I'd really like to understand why there is a need for the JDT to operate on non-Java Resources.
>If I may reiterate on the question posted by Francis, I'd really like to >understand why there is a need for the JDT to operate on non-Java Resources. I never said it is needed but it worked for many years like that and it is well tested. Just look at the first suggested patch: it failed. Did the patch submitter test every possible element in a Java project whether it correctly expands and whether all expected actions are there like in 3.4? I doubt. It's just too late for such a radical change. Also note that plugin.xml is like code and if we say it is too late and risky to change CNF code then the same argument logically also applies to plugin.xml. An acceptable solution for JDT would be to change the sorter contribution only like Anton also tried as the most obvious fix to the sorter problem, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=162330#c8. However, as noted this seems not to work as expected i.e. the sorter is still called by CNF. Hence this will also need a fix on the CNF side to correctly honor this but it might be a smaller and less risky fix than fixing the real CNF bug.
At the request of Steve and Jeff on the PMC call we just had, I'd like to highlight again that this is indeed a severe issue for products based on the CDT and probably others leveraging the CNF. As you can see in the screenshot (attachment 135436 [details], added by Andrew in comment 21), messing up the CDT sort order makes the "Includes" item move down below some source files. But the "Includes" item is special and important for anybody browsing the hierarchy. Disabling JDT or Resources contributions is the only workaround know to date, just refreshing the view does not help. Disabling those contributions, however, is not an obvious action for end users, and does have the effect that JDT elements (such as package representation etc) are not visible in the Project Explorer any more. In other words, users who need to work on C and Java stuff at the same time (such as JNI) can't do that properly any more. Thanks for continuing investigations towards finding an acceptable (low-risk) solution!
(In reply to comment #37) > >If I may reiterate on the question posted by Francis, I'd really like to > >understand why there is a need for the JDT to operate on non-Java Resources. > I never said it is needed but it worked for many years like that and it is well > tested. Just look at the first suggested patch: it failed. I'm not clear about this, the first suggested patch does change the behaviour to cause everything to go away if resources is not checked, but I consider this acceptable (as I have stated). I know of no other problems with this patch and can't see that you have stated any. This is the patch I have recommended we go with. > Also note that plugin.xml is like code and if we say it is too late and risky > to change CNF code then the same argument logically also applies to plugin.xml. I am not suggesting we use the patch because it's in the plugin.xml vs the code, I'm suggesting we use the patch because I feel it's a more correct CNF configuration, it removes the JDT UI from resources that don't concern it. And I think this patch is localized in that it impacts only JDT contributions to the CNF and possibly other downstream projects (e.g. WST) that depend on JDT. Any other changes to the CNF could affect anything.
The issue here with all of this is testing. With whatever solution we come up with, in whatever timeframe we come up with, we need to have the consumers of the CNF test to make sure everything still works. Personally (if I were in charge [God forbid]) I try the JDT UI patch and produce an experimental build (perhaps put it in for an I-build) and have the known downstream projects (WST, DTP, CDT, etc) test with this immediately and report any problems. As a bonus, I would have them document their testing procedures so that we (meaning me) can reproduce them, and perhaps evevn in the future add them to the CNF unit tests. If all of that testing went well, then I would try to get it into RC2. We would need to do this exact work for 3.5.1, so I don't see much point in delaying it for 3.5.1.
Created attachment 135600 [details] Restore CNF behaviour for multiple overriding extensions This patch restores the pipelining behaviour for the case overrdingExtensions.length > 1. If there is only one overriding extension it might be reasonable or desirable (although questionable) to associate this extension with all elements of the overridden extension (to allow overriding labels). But with 2 or more overriding extensions it does not make sense to associate the higher priority extension with elements contributed by another extension. Francis, could you test this patch wrt. bug 255793 and bug 252293? Thanks.
(In reply to comment #41) > Francis, could you test this patch wrt. bug 255793 and bug 252293? Thanks. > Thanks Tony, really I would love to, but honestly I don't have time for any CNF work right now, and won't for about a week or so. Can anyone else look at this and try it?
Thanks for the patch, Anton. Would it make sense to schedule a call to work through the CNF code in question together, in order to understand exactly what's going on, and to plan the next steps?
(In reply to comment #37) > An acceptable solution for JDT would be to change the sorter contribution only > like Anton also tried as the most obvious fix to the sorter problem, see > https://bugs.eclipse.org/bugs/show_bug.cgi?id=162330#c8. However, as noted this > seems not to work as expected i.e. the sorter is still called by CNF. Hence > this will also need a fix on the CNF side to correctly honor this but it might > be a > smaller and less risky fix than fixing the real CNF bug. Sounds interesting. If this works out, we may be able to do something for 3.5. Marking this bug for consideration in RC2.
(In reply to comment #44) > (In reply to comment #37) > > An acceptable solution for JDT would be to change the sorter contribution only > > like Anton also tried as the most obvious fix to the sorter problem, see > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=162330#c8. However, as noted this > > seems not to work as expected i.e. the sorter is still called by CNF. Hence > > this will also need a fix on the CNF side to correctly honor this but it might > > be a > > smaller and less risky fix than fixing the real CNF bug. > > Sounds interesting. If this works out, we may be able to do something for 3.5. > Marking this bug for consideration in RC2. > Unfortunately that's not so easy. The sorter is selected based on the source (associated extension) of the elements being sorted. Due to the bug in the pipelining, the source is computed wrong and therefore the sorter is taken from the wrong extension. Actually, the parentExpression for the sorter makes only sense if an extension provides more than one sorter. So, to make this work, the sorter would need to be selected independent of the source which is a non-trivial change and might also break a few clients. If you are willing to fix this in 3.5, I'd suggest to take a look at my patch from comment 41. It is a compromise between - Fixing the regression wrt. associating an extension with an element which might have other yet unknown side effects as Dani pointed out. - Keeping the intended behavior to be able to override labels, etc. when there is only one overriding extension. The most important thing IMHO is to get rid of the regression. The patch does that in a way as easy and safe as it can be at this time. I am not claiming that it is the final solution.
(In reply to comment #45) > The most important thing IMHO is to get rid of the regression. The patch does > that in a way as easy and safe as it can be at this time. I am not claiming > that it is the final solution. After talking about the patch in detail with Anton, and thinking about the options we have right now, I believe the best course of action is to defer this to 3.5.1 and find a real fix. Even with the latest suggested patch, it is unclear to me whether downstream clients would be affected in unwanted ways. Note that those who download CDT from www.eclipse.org/downloads (Eclipse IDE for C/C++ Developers) will not be affected by this bug because the download does not include JDT. I believe the majority of CDT users will be using this download, or one of the products built on top of JDT, which again are unlikely to contain JDT.
In addition to previous comment: CDT should also add an entry to their README: in case of JDT and CDT, the user can simply disable the resource view to get both JDT and CDT (but not simple projects) in the Project Explorer.
I posted it in bug 273660 but maybe should mention it here. Disabling 'Resources' results in duplicated items Copy, Paste and Delete in project context menu.
Andrew, why did you remove me from the cc-list?
No I did not. I did not even know even that it was possible. If I did it was not intentional and I am sorry Dani.
No problem. Bugzilla probably tricked you.
Use use CDT and JDT together when doing JNI programming. I do it all the time so am probably one of the most affected by this. Let me know how I can help.
Here (at BMW) we use CDT and PDT (Plugin DT, not PHP DT) together. We do embedded C and at the same time we extend CDT to support our peculiar processes and tools. I would also like to see this sorted out soon, so if there is anything I can do, please let me know.
By the way, WTP is affected in the same way as CDT. Dynamic and static web projects have the same sorting issue in Project Explorer View.
Please see bug #285344 which is closely related to this discussion. I agree with other posters here that the 3.5 change that corrupts the source contribution memory (see comment #13) must be fixed, there is no way the framework will work predictably until then.
Created attachment 145198 [details] CDT tests in the CNF that make the problem happen This also needs a zip file, in the next attachment.
Created attachment 145199 [details] Zip file required for tests Put this in the org.eclipse.ui.tests.navigator/testdata folder
Tests have been checked into both R3_5maintenace and HEAD, see bug 287138
Created attachment 145227 [details] Proposed patch that fixes the problem The problem was caused by incorrect changes I made to the pipeline mechanism in 3.5 to address the label provider override bug 252293. The patch restores the pipeline behavior to its original correct state and also adds some code to correctly handle overriding a label provider if desired, along the lines of Anton's suggestion. I think the patch is low risk to break any compatibility especially now that the alternate override handling for label provider overrides is treated narrowly. The documentation for the Override element of the NCE was inconsistent with itself, that's also fixed in this patch. The documentation did correctly state (in 3.4) that if you use an Override you *must* implement the IPipelinedTreeContentProvider. Failure to do that would give no errors or warnings, but it would have your overriding NCE silently ignored. For 3.5, you can legitimately use Override without using pipelining (which is used for example to override the label provider). I have added a CDT test to the CNF to verify this, and have verified it with the project that Anton provided. The patch also adds debug tracing for sorting so it can be clearly seen which NCEs are used to contribute to the sorting to make these problems easier to diagnose. One thing that I'm not happy about (discovered in my testing) is that there is a difference between the behavior with this patch and if you turn of the JDT NCE. The difference is indicated in the code of the CDT/CNF tests. It is in the label provider handling. If you turn off the JDT NCE, the CDT NCE will be called to handle the labels on plain resources in the CDT project. However with this patch and the JDT being turned on the CDT is not called to handle labels for these resources (It's not clear yet if the Resoruce or JDT NCE is handling them). CDT folks, please let me know quickly if this is a serious issue that needs to be addressed for 3.5.1.
*** Bug 285344 has been marked as a duplicate of this bug. ***
Prakash, how do you feel about reviewing this patch for 3.5.1? I need another committer and Boris. Let me know if you can't do it or if you have any questions. Thanks! (it complains when I add you as a reviewer, says that your user name needs the editbugs capability; should I use a different email address than grprakash@gmail.com?)
My official id is grprakash@in.ibm.com
After some extensive discussions with Franck on this patch, we decided that the part about resetting the contribution memory was a bad idea (in the case of non-pipelined overrides), so I plan to remove that from the patch before 3.5.1. However the patch as it is fixes the main problem that CDT and others experience and passes all of the tests. Removing that bit of code now will make some tests fail. The plan is in the next couple of days to have a fix for bug 287103 which will be the more correct fix for label provider overrides and with that fix the resetContributionMemory portion of this patch will go away. We would still like to proceed with getting this patch checked in as soon as possible. I'm hoping I can get a +1 from the folks near to CET time and IST (IDT?) on Friday :)
Francis, it would help me review these changes if you could comment each line (or code block) that was changed with the rationale of why it was changed, perhaps in form of another patch that has the changes plus the comments. This is such a sensitive area, and a lot of changed lines of code in your patch, that we need to be extra careful. I hope it's not too much of a burden for you.
(In reply to comment #64) > Francis, it would help me review these changes if you could comment each line > (or code block) that was changed with the rationale of why it was changed, > perhaps in form of another patch that has the changes plus the comments. Let me do it this way: 1) There are only two files that matter, the rest are the addition/modification of tracing or documentation (inspection will verify that to be the case). They should not affect the behaviour at all. The ones that matter are below: 2) ContributorTrackingSet - remove mechanism of changing the contributor at setContributor time (making it how it used to be in 3.4) - add an identical mechanism of resetting the contibutor explictly* 3) NavigatorContentServiceContentProvider - Restore the interaction of the pipeline mechanism and the tracking set to what it was before I messed it up (so it's like 3.40 - In the special case of a non-pipelined override (used only for override of label providers), use the resetContributor stuff to change the contribution memory to be the last overriding NCE for label overrides* Note that the items marked * are to support the override of label providers, a new 3.5 feature, and after discussions with Franck, we agreed to a cleaner mechanism for this support which is captured in the patch to bug 287103 (see comment 63 on this bug). The patch for 287103 is not ready yet, but this patch will keep the tests running and fix the major problem that's the subject of this bug. With the fix to 287103, the * items will be removed. I plan to have that fix ready later tonight, Pacific time. So in essence this patch just backs out the bug introduced by messing up the pipelining in 3.5.
> The patch for 287103 is not ready yet, but this patch > will keep the tests running and fix the major problem that's the subject of > this bug. With the fix to 287103, the * items will be removed. I plan to have > that fix ready later tonight, Pacific time. I posted the label provider patch in bug 287103. Franck
I've looked at the patch and the general approach looks fine. However, I agree with Boris that we want a minimal patch for 3.5.1 i.e. only changes that are absolutely needed to fix the problem. If you think additional logging is required for 3.5.1 then this should be tracked by a separate bug that can be approved by Boris. In addition I'd like to see a patch that just reverts the label provider changes and does not have the additional tweaks (marked * in comment 65). We can then review that patch and commit it and in the next step look at how we bring back the label provider override code.
(In reply to comment #67) > I've looked at the patch and the general approach looks fine. However, I agree > with Boris that we want a minimal patch for 3.5.1 i.e. only changes that are > absolutely needed to fix the problem. If you think additional logging is > required for 3.5.1 then this should be tracked by a separate bug that can be > approved by Boris. In addition I'd like to see a patch that just reverts the > label provider changes and does not have the additional tweaks (marked * in > comment 65). We can then review that patch and commit it and in the next step > look at how we bring back the label provider override code. > I think the additional tracing is very valuable and it's easy to see from the patch that it's benign, so I'm not inclined to split it out. We only have one more day to get this done. I'm unwilling to commit something without fixing the pipelining issue and also keeping the label provider override working as well (even for a day). It's important that our tests continue to pass and that we support clients that have hooked to the 3.5 label provider override. Franck's patch in bug 287103 will more properly address this; if we can get that reviewed in time to do both submissions at the same time, that will be fine. In fact the fix in 287103 effectively supercedes this patch (except for the tracing). So we could take just that one fix instead of this one (then I would need to move the tracing somewhere else). Franck just now finished his work on bug 287103, I need to review that which I will do soon.
(In reply to comment #68) > I think the additional tracing is very valuable and it's easy to see from the > patch that it's benign, so I'm not inclined to split it out. We only have one > more day to get this done. I'm ok with putting the additional tracing code into 3.5.1.
Let's do this, just focus on the patch in 287103. That patch is really the one we want. I will open a separate bug for the tracing, and the patch in bug 287103 completely supercedes this patch (it contains the good parts of the patch for this bug). I'm going to mark this patch obsolete so we can focus on the one thing.
Comment on attachment 145227 [details] Proposed patch that fixes the problem Obsolete, replaced by the patch for bug 287103.
>(From update of attachment 145227 [details]) >Obsolete, replaced by the patch for bug 287103. So let's mark it as dup since we don't plan to work on separate patches. *** This bug has been marked as a duplicate of bug 287103 ***
