Community
Participate
Working Groups
JavaSourceViewer overrides updateCodeMinings() method. The body of the method is empty and there is no way to update third party CodeMinings that are based on the push model rather than pull model. Current state of JavaSourceViewer assumes pull model for CodeMinings. For example it'd pull the code minings when editor is opened for the first time or content of the editor has change. However, there might be CodeMinings coming from 3rd party CodeMining providers delivering info about number of method invocations at runtime etc. These code minings are based on the push model. A message from another process arrives and code minings need an update. Having an empty method body for updateCodeMinings() breaks the behavior above. Making doUpdateCodeMinings() public would help. Currently we are forced to call reflection to update push model codeminings.
(Marking it as critical as it seems to be a regression) I think what's implemented basically breaks the general CodeMining framework. We cannot assunme that codemining are done only in case of ICompilationUnit resolution, it's very possible to write a code mining provider that only cares about plain text. The method should be tweaked to prevent from erroneous cases, but not fully cleared as it is. Reading at the JavaDoc, it looks more like JavaEditor.installCodeMiningProviders() and/or JavaSourceViewerConfiguration.setCodeMiningProviders() should be overriden to guard the undesired behavior instead.
Who's going to work on this?
@Alex: what would be great would be that you provide a unit test integrating a dummy code mining provider in a similar way as the one you're using and you see broken now. If we have this unit test (and others), it would really help in writing the right fix and making code minings more sustainable.
New Gerrit change created: https://git.eclipse.org/r/134419
I have attached the unit test for CodeMinings
New Gerrit change created: https://git.eclipse.org/r/134420
Ping! M1 is next week!
I believe there is some sort of flaw in how code minings are computed vs rendered. Currently, the API assumes that code minings are always case of a text reconciler, which is not always true; JDT overrides it to assume that it's an ASTReconciler, which is not always true neither and blocks extensibility. The codemining manager takes care of updating the code minings "model" (ie asking providers for code minings) and of rendering them. I think we need to better split those 2 steps: find a way for codeminingmanager to not necessarily poll new model from a provider (so provider can decide to modify the model whenever it fits them best), and then to react to change when the model change by doing the rendering.
New Gerrit change created: https://git.eclipse.org/r/135336
Some comments about the strategy implemented in the proposed change. * SourceViewer.updateCodeMinings() API *mustn't* be overriden without calling super.updateCodeMining(), or it's breaking the generic code mining contract (cause of this), so we need this method to remain intact and used in JavaSourceViewer. * The change here makes that ** usage of JavasourceViewer.updateCodeMinings() works and updates code mining as expected, so all of them from all providers ** Calling the JavaElementCodeMiningProvider early (ie on editor startup before AST is reconciled) doesn't cause erroneous results nor freeze. To do so we store which viewers are ready for providing code mining or not. And the reconcilier is still used to automatically re-trigger an update. On longer term, we could consider to implement improvement upstream: make the provider have a feature to be marked as "dirty" according to whatever they want. Then consumer could mark one specific provider as dirty and that would automatically call the rendering, that would reuse non-dirty code mining and re-compute new ones from dirty providers. That would allow a finer grain control over what to recompute and when to do it. Currently, as a workaround, this approach can be implemented in each specific implememtation of code mining provider, which could keep their last results in cache and return the cache as long as they're not marked as dirty.
Gerrit change https://git.eclipse.org/r/135336 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=bf4f87715f4a93ff9d332a5404c353a74f830cc2
Thanks Alex and Mickael!! @Mickael, will you create another bug for long term solution or keep this open ?
Thanks for the review Sarika! @Alex: please give a try to next (or 2nd next) I-Build and check whether it actually fixes your issue. If it does, please mark the bug as VERIFIED, otherwise, please reopen with more details. > @Mickael, will you create another bug for long term solution or keep this open ? I've created another bug as the solution would be on the Platform/Text component. It's bug 543885
