Community
Participate
Working Groups
Once https://bugs.eclipse.org/bugs/show_bug.cgi?id=527675 will be merged in master, I could contribute with CodeMiningManager which consumes the "inlined" annotations support. This issue is to provide just the CodeMiningManager which manages provider/resolver of CodeMining and display CodeMining with the "inlined" annotations support. This issue will not modify existing code of SourceViewer and will be in anothat er gerrit patch at https://bugs.eclipse.org/bugs/show_bug.cgi?id=527515
New Gerrit change created: https://git.eclipse.org/r/112601
Created attachment 271702 [details] CodeMining Demo Here a demo with CodeMining manager. In this demo, CodeMining displays references, implementation of class. When you name class with 'X' integer value, the CodeMining resolver waits 'X' ms. It emulates the case of resolver takes time. I'm waiting for org.eclipse.jface.text.examples project exists to push this demo. For the moment you can play with this demo with https://github.com/angelozerr/codelens-eclipse-photon/blob/master/org.eclipse.jface.text.examples/src/org/eclipse/jface/text/examples/codemining/CodeMiningDemo.java and you can see how to CodeMiningManager is used: 1) CodeMiningManager install requires viewer, and new API InlinedAnnotationSupport and list of CodeMining providers. 2) CodeMiningManager run must be called to collect and displays minings. Any feedback are welcome, thanks!
@Mickael, please note after accepting my gerrit patch, I would like to move CodeMiningManager in an internal package, because CodeMining feature should be consumed with ISourceViewerExtension5: ---------------------------------------------------- public interface ISourceViewerExtension5 { /** * Update minings */ void updateCodeMinings() /** * Register the code mining providers. * * @param codeMiningProviders the code mining providers */ void setCodeMiningProviders(ICodeMiningProvider[] codeMiningProviders); } ----------------------------------------------------
(In reply to Angelo ZERR from comment #3) > @Mickael, please note after accepting my gerrit patch, I would like to move > CodeMiningManager in an internal package Why not putting it right now in an internal package then?
> Why not putting it right now in an internal package then? Because I would like to create a gerrit patch in this bug with demo which needs CodeMiningManager. When I will introduce ISourceViewerExtension5 in https://bugs.eclipse.org/bugs/show_bug.cgi?id=527515, I will update too the demo. I think it's the best mean to facillitate review code, no?
(In reply to Angelo ZERR from comment #5) > Because I would like to create a gerrit patch in this bug with demo which > needs CodeMiningManager. > When I will introduce ISourceViewerExtension5 in > https://bugs.eclipse.org/bugs/show_bug.cgi?id=527515, I will update too the > demo. I think it's the best mean to facillitate review code, no? The best to facilitate reviews is to minimize the amount of public classes/methods and so on. So it would be easier if this class could be internal from the beginning. In any case, Platform bundle usually export their internal bundle (with the x-internal annotation on the Export-Package directive) and it's fine to let example, tests or whatever depends on internal code that's part of the same Git repo. So in this case, please make everything that will eventually be internal be internal from now on.
@Mickael, after reading org.eclipse.jface.text.hyperlink.HyperlinkManager again, this class is not in internal package, but this class must not used directly but with ITextViewerExtension6. I would like to follow the same idea, so I will keep CodeMiningManager in a public package and consume CodeMining with ISourceViewerExtension5. So I keep my code like today.
(In reply to Angelo ZERR from comment #7) > @Mickael, after reading org.eclipse.jface.text.hyperlink.HyperlinkManager > again, this class is not in internal package, but this class must not used > directly but with ITextViewerExtension6. This seems more like an issue in HyperlinkManager that didn't manage to properly deal with visibilty and internal flags. It would be nice to avoid making the same issues from the beginning, and to start by placing it in an internal package if you don't want it to be public API; and maybe later, we'll make it public. Starting internal and going public is much easier than the other way round.
> It would be nice to avoid making the same issues from the beginning, and to start by placing it in an internal package if you don't want it to be public API; Thanks @Mickael for your feedback. I will do a new gerrit patch with that. I would like too to improve in another gerrit patches CodeMing with: * custom renderer for CodeMining (like display icon instead of simple text). * click on CodeMining to execute some action. In this case, hover of CodeMining should be supported (like display text as hyperlink when CodeMining is hovered). In other words, perhaps ICodeMining API will change a little to support those feature. I hope it will not a problem.
@Mickael, please don't accept my gerrit patch. I'm improving the performance of CodeMiningManager when there are a lot of CodeMinings. The main issue is to resolve the mining only when it must be drawn (when it is visible, and not every time minings are computed). I will do a new changes ASAP.
It's up to you. If this performance improvement doesn't change public API, it's imo better to merge this iteration and track the perf improvement in another bug. The more we delay merge of public APIs, the less chance we have to deliver the full stack in Photon.
@Mickael, please see my last gerrit patch which improve a lot performances, because resolve is done only when annotation are visible.
Gerrit change https://git.eclipse.org/r/112601 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=aa8c7fbeb626401b6ffd80dfa3b6331dbd77dc92
New Gerrit change created: https://git.eclipse.org/r/113134
@Mickael please review my new gerrit patch https://git.eclipse.org/r/113134 with codemining demo
Gerrit change https://git.eclipse.org/r/113134 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=f1942faa93537121ccad39ea194f1b5c04583065
Anything left to be done on this topic? Do you think it's worth making it part of N&N right now or it's better to include it in N&N together with bug 527515 ?
> Anything left to be done on this topic? Perhaps add JUnit tests, but to be honnest with you, I have not worked on this topic. As you, I would like to provide quickly JDT CodeMining support. Perhaps we could do that in another issues (if you have some ideas how to test that, you are welcome!) > Do you think it's worth making it part of N&N right now or it's better to include it in N&N together with bug 527515 ? I think it's better to add N&N in bug 527515 which will provide a public API with ISourceViewerExtension5 (I will update the CodeMining demo with this public API)
Resolved then, thank you! About unit tests, I understand they're hard to write so far. I hope that further usage of code mining (and bugs discovery) will give good opportunities to identify some good tests to write.
