Community
Participate
Working Groups
Goal of this issue is to initialize CodeMining inside Java editor: * register Java Reference/Implementation code minings with "org.eclipse.ui.workbench.texteditor.codeMiningProviders" extension point. * update code minings by adding a IJavaReconcilingListener listener to call ((ISourceViewerExtension5)getSourceViewer()).updateCodeMinings(); as soon as CompilationUnit is reconciled.
New Gerrit change created: https://git.eclipse.org/r/114644
Created attachment 272018 [details] A Java codemining demo with JFace Text project Here a a Java codemining demo with JFace Text project
Created attachment 272019 [details] A Java codemining demo with a simple class Here a Java codemining demo with a simple class
@Dani, @Mickael, @Sarika you can play now with Java codeminings with my gerrit patch https://git.eclipse.org/r/114644 (you need to import org.eclipse.jface.text & org.eclipse.ui.workbench.texteditor from master) Pay attention, this gerrit patch don't give the capability to disable codeminings! I'm working in this issue at https://bugs.eclipse.org/bugs/show_bug.cgi?id=528728 Once I will finish it, I will do a new changes to support enable/disable of minings. Hope you will like Java code minings, any feedback are welcome, thanks!
Will try out this week.
(In reply to Angelo ZERR from comment #4) > @Dani, @Mickael, @Sarika you can play now with Java codeminings with my > gerrit patch https://git.eclipse.org/r/114644 (you need to import > org.eclipse.jface.text & org.eclipse.ui.workbench.texteditor from master) > > Pay attention, this gerrit patch don't give the capability to disable > codeminings! > > I'm working in this issue at > https://bugs.eclipse.org/bugs/show_bug.cgi?id=528728 Once I will finish it, > I will do a new changes to support enable/disable of minings. > > Hope you will like Java code minings, any feedback are welcome, thanks! Apart from these steps, any thing else required to see the code mining in a Java Class in the launched configuration? I don't see it.
> Apart from these steps, any thing else required to see the code mining in a Java Class in the launched configuration? Nothing, it should work? > I don't see it. 1) At first please check that generic editor example with project references codemining is working (you told me that it worked for you). 2) Set a breakpoint at SourceViewerDecorationSupport line 831 to check annotation painter to use to draw codemining is well initialized: ------------------------------------------------------------ ((ISourceViewerExtension5) fSourceViewer).setCodeMiningAnnotationPainter(fAnnotationPainter); ------------------------------------------------------------ 3) Set a breakpoint in SourceViewer in line 1298: ------------------------------------------------------------ fCodeMiningManager= new CodeMiningManager(this, fInlinedAnnotationSupport, fCodeMiningProviders); ------------------------------------------------------------ to check fCodeMiningProviders is filled with JavaCodeMiningProvider instance. 4) Set a breakpoint in CompilationUnitEditor line 1074 to check updateCodeMinings is called. ------------------------------------------------------------ fCodeMiningsReconcilingListener = new IJavaReconcilingListener() { @Override public void reconciled(CompilationUnit ast, boolean forced, IProgressMonitor progressMonitor) { ((ISourceViewerExtension5)getSourceViewer()).updateCodeMinings(); ------------------------------------------------------------ Have you that?
Thanks, the new package in jdt.ui is not exported, and due to errors it was not loading the jdt.ui patch. After fixing I am able to see references for Class and methods. Some questions - 1. References are not available for variables ? only class and methods? 2. If I reference a class and say ClassA a = new ClassA(); it updates ClassA with 2 references. Is this intentional ? This can lead to misconceptions in a huge framework where we want to find the references.
> Thanks, the new package in jdt.ui is not exported, and due to errors it was not loading the jdt.ui patch. After fixing I am able to see references for Class and methods. Sorry -( I have fixed that https://git.eclipse.org/r/#/c/114644/ > Some questions - > 1. References are not available for variables ? only class and methods? Yes. It follow the same idea than CodeLens of VSCode, Visual Studio (I think). I think it's a good idea to do that because it avoid having a lot of mining in the Java class and variables are generally used inside the Java class file. I think references is really interesting when references comes from outside the Java class file. > 2. If I reference a class and say ClassA a = new ClassA(); it updates ClassA with 2 references. Is this intentional ? This can lead to misconceptions in a huge framework where we want to find the references. To be honnest with you, I don't know. I have implemented CodeMining exactly like JDT LS Codelens (used inside vscode-java as language server). The goal of this issue is to intialize Java CodeMining and after we could improve the compute of minings in other issues. What do you think about that?
Rather than having it as a preference, we can also have this data as an action like we have validation in EE, we can have Analyze action which can compute all this. This is based on the thought that a user does not need a live information update always and will need this information for sometime.
@Roland: maybe you could have a look when you have some availability?
> @Roland: maybe you could have a look when you have some availability? @Mickael see my comments at https://bugs.eclipse.org/bugs/show_bug.cgi?id=530825#c6 My idea is to apply the basic patch https://bugs.eclipse.org/bugs/show_bug.cgi?id=530825#c6 in JDT and after I could create an external plugin which provide Java CodeMinings. In other words, even if JDT team doesn't wish to integrate CodeMinings, CodeMinings could be consumed inside the Java Editor for Photon with my external plugin.
Now https://bugs.eclipse.org/bugs/show_bug.cgi?id=530825 was merged in master, I tell me if you wish I update my gerrit patch to: * remove the code which updates codemining since https://bugs.eclipse.org/bugs/show_bug.cgi?id=530825 support that * provide an UI preferences with "Show references", "Show implementations". Do you like I do this gerrit patch?
New Gerrit change created: https://git.eclipse.org/r/119929
Please review my new gerrit patch https://git.eclipse.org/r/119929 which provides: * an UI preferences Java / Editor / Code Minings with 2 checkbox (Show references, Show implementations) * the Java References/ Implementations code minings.
IIRC, the discussions at EclipseCon France with Dani and Sarika is that we first need bug 532924 to be fixed (Dani's comment) or at least worked around from UX persective with things like warning on macOS preference page (Sarika's comment) before we can move forward with codeminings in JDT.
(In reply to Mickael Istria from comment #16) > or at least worked around > from UX persective with things like warning on macOS preference page > (Sarika's comment) before we can move forward with codeminings in JDT. We don't release features that only fully work on certain platforms.
As https://bugs.eclipse.org/bugs/show_bug.cgi?id=532924 is now fixed, I will restart my contribution. I will try to create a new gerrit patch this week.
New Gerrit change created: https://git.eclipse.org/r/126847
Please review my new gerrit patch https://git.eclipse.org/r/#/c/126847/ Thanks!
Some suggestions - 1. We can move the Preference out of Editor scope. This is based on the future plans as I see from github implementation of code mining that we can have Debug, and JUnit options as well. SCCM may not fit under Java. Team might be a better place. 2. We can have a general enable/disable of this whole feature which can be toggled by a checkbox in Preferences(like Enable Project Specific Settings). This can also be provided as a toolbar button like we do for toggling whitespace, breakoints etc. 3. The name of "Show references" and "Show implementations" should have a prefix "Enable" for clarity as they are behaving like toggle. 4. What is the plan for other features like Method parameters under General ? Only Reference and Implementation will be part of 4.9?
> 1. We can move the Preference out of Editor scope. This is based on the future plans as I see from github implementation of code mining that we can have Debug, and JUnit options as well. SCCM may not fit under Java. Team might be a better place. IMHO, I think all "Code Mining" should be host in the same preferences page after "Editor" preference page because: * code mining is for only Editor * dispatching code mining in several preferences page could be hard to discover it. * it looks like "Editor / Content Assist / Advanced", the whole content assist are managed in one preferences page and it is not dispatched in several preference page. in my JDT CodeMining all code mining are displayed in the same page. It's easy for me because I have just one plugin. In JDT contribution we have several plugins like ui, debug, junit. We could imagine to provide a new extension point to contribute to the UI code mining preference page, but I'm not sure that you will be aware with that. So we could use preference page like this: * Java * Editor * Code Mining (references, implementation, method, etc) * Debug * JUnit * Team > 2. We can have a general enable/disable of this whole feature which can be toggled by a checkbox in Preferences(like Enable Project Specific Settings). This can also be provided as a toolbar button like we do for toggling whitespace, breakoints etc. Do you need that in my gerrit patch? > 3. The name of "Show references" and "Show implementations" should have a prefix "Enable" for clarity as they are behaving like toggle. You mean just update the label "Show references" to "Enable show references"? Or update name fo preferences too? > 4. What is the plan for other features like Method parameters under General ? Only Reference and Implementation will be part of 4.9? I would like really to contribute it! But before doing that, I would like to provide just 2 references/implementations code minings. And I would like to contribute in another gerrit patch other code minings. I think this gerrit patch is very big, but other gerrit patches will be little, it will be more easy for you to review it. As soon as you will merge my gerrit patch, I will contribute with other code mining.
(In reply to Angelo ZERR from comment #22) > > 1. We can move the Preference out of Editor scope. This is based on the future plans as I see from github implementation of code mining that we can have Debug, and JUnit options as well. SCCM may not fit under Java. Team might be a better place. > > IMHO, I think all "Code Mining" should be host in the same preferences page > after "Editor" preference page because: > > * code mining is for only Editor > * dispatching code mining in several preferences page could be hard to > discover it. > * it looks like "Editor / Content Assist / Advanced", the whole content > assist are managed in one preferences page and it is not dispatched in > several preference page. > > in my JDT CodeMining all code mining are displayed in the same page. It's > easy for me because I have just one plugin. In JDT contribution we have > several plugins like ui, debug, junit. We could imagine to provide a new > extension point to contribute to the UI code mining preference page, but I'm > not sure that you will be aware with that. So we could use preference page > like this: > > * Java > * Editor > * Code Mining (references, implementation, method, etc) > * Debug > * JUnit > * Team > Ok that sounds logical. Team might have to check if they are installed as they are not part of SDK. > > 2. We can have a general enable/disable of this whole feature which can be toggled by a checkbox in Preferences(like Enable Project Specific Settings). This can also be provided as a toolbar button like we do for toggling whitespace, breakoints etc. > > Do you need that in my gerrit patch? Yes, that will be better. > > > 3. The name of "Show references" and "Show implementations" should have a prefix "Enable" for clarity as they are behaving like toggle. > > You mean just update the label "Show references" to "Enable show > references"? Or update name fo preferences too? Label only. > > > 4. What is the plan for other features like Method parameters under General ? Only Reference and Implementation will be part of 4.9? > > I would like really to contribute it! But before doing that, I would like to > provide just 2 references/implementations code minings. And I would like to > contribute in another gerrit patch other code minings. I think this gerrit > patch is very big, but other gerrit patches will be little, it will be more > easy for you to review it. As soon as you will merge my gerrit patch, I will > contribute with other code mining. Ok.
> Ok that sounds logical. Team might have to check if they are installed as they are not part of SDK. No need to do that, the code mining for SCSM will host in Platformtext (like RevisonPainter and co). See bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=534600 > Yes, that will be better. Ok > Label only. Ok
> Yes, that will be better. @Sarika, could you start review my gerrit patch before I do that. I will try to updat egerrit patch ASAP. Thanks!
(In reply to Angelo ZERR from comment #25) > > Yes, that will be better. > > @Sarika, could you start review my gerrit patch before I do that. I will try > to updat egerrit patch ASAP. Thanks! ok.
Sorry, the target removal was unintentional. Sorry about it.
(In reply to Sarika Sinha from comment #21) > Some suggestions - > > 2. We can have a general enable/disable of this whole feature which can be > toggled by a checkbox in Preferences(like Enable Project Specific Settings). > This can also be provided as a toolbar button like we do for toggling > whitespace, breakoints etc. > Waiting for this.
> Waiting for this. Done, please review my last gerrit patch https://git.eclipse.org/r/#/c/126847/. Thanks!
(In reply to Angelo ZERR from comment #29) > > Waiting for this. > > Done, please review my last gerrit patch > https://git.eclipse.org/r/#/c/126847/. Thanks! Will look at it next week.
Created attachment 275950 [details] Different Line spacing
Created attachment 275951 [details] Font used
(In reply to Sarika Sinha from comment #31) > Created attachment 275950 [details] > Different Line spacing I agree that this doesn't look very good. The "N references" text looks to close from the line above and too far away from the line below (where it's actually attached). The spacing should probably be the other way round (more space with previous line, less with next one).
Some feedback - 1. From the attachment you can see that the line spacing is different for the code mining sentence. Code mining reference sentence looks closer to the field but it is actually related to the method main which is not clear visually. This is not acceptable. 2.There is no reference available for fields ? This is must. we need to have class, field and method references available for the first release. 3. The word General looks out of place as there is only only group, what are the other groups intended ? Then only we can justify the "General" group. 4. When we "Enable code mining" for a workspace - all the options will be selected or a selective list?
> 1. From the attachment you can see that the line spacing is different for the code mining sentence. Code mining reference sentence looks closer to the field but it is actually related to the method main which is not clear visually. This is not acceptable. It's a bug of code mining. > 2.There is no reference available for fields ? This is must. we need to have class, field and method references available for the first release. Having code mining for fields will add a lot information. I don't know if it's a good idea (TypeScript CodeLens for instances does like this). IMHO I think this big gerrit patch should provide only references/implementation for methods/constructor and another gerrit patch could provide references for fields. > 3. The word General looks out of place as there is only only group, what are the other groups intended ? Then only we can justify the "General" group. See JDT CodeMining preferences https://github.com/angelozerr/jdt-codemining/blob/master/images/JavaCodeMiningPreferences.png 4. When we "Enable code mining" for a workspace - all the options will be selected or a selective list? No, it exists one preference per code mining that user can configure it. By default I have disable all options, but we can discuss about that now we have an "Enable code mining".
Can you create a separate gerrit patch to support field code mining dependent on https://git.eclipse.org/r/#/c/126847/
> Can you create a separate gerrit patch to support field code mining dependent on https://git.eclipse.org/r/#/c/126847/ Yes sure! I will do that once this big gerrit patch will be merged. I would like to really having this gerrit patch too to add another code minings like method parameters, debug, etc
Mickael Istria should be able to help you to make the patch on https://git.eclipse.org/r/#/c/126847/ now itself, no need to wait for it to be merged. We need to test how does it perform.
Created attachment 275981 [details] Mnemonic example Mnemonics should be added for each entry in the preference page.
(In reply to Sarika Sinha from comment #36) > Can you create a separate gerrit patch to support field code mining > dependent on https://git.eclipse.org/r/#/c/126847/ We will need this patch latest by 1st week of November if we want this feature to be a part of M3.
> We will need this patch latest by 1st week of November if we want this feature to be a part of M3. Apart from this feature that I must implement, is there other thing you wish I update in this gerrit patch? If it's OK, is there any chance to merge it?
We will have a look this week and confirm if any thing else is required.
Thanks Sarika!
@Sarika I had a great contribution to JDT CodeMining (see https://github.com/angelozerr/jdt-codemining/issues/75#issuecomment-426679401) for references and I would kike to update my gerrit patch to add it. Before that I would like to know if you are OK. The new preferences give the capability to select where you wish to show references: * on classes * on methods After we could add too "on fields" like you wish to have. Are you OK, I update the gerrit patch with this 2 new preferences?
Created attachment 276430 [details] Code mining placement issue on Mac With the latest patch, we see a misplacement of line number on Mac OS as visible in the attachment.
Indeed @Lakshmi spoke about this problem in https://bugs.eclipse.org/bugs/show_bug.cgi?id=539618#c27, I'm working on this issue with 2 gerrit patches: * https://git.eclipse.org/r/#/c/130141/ (SWT StyledText which provides verticalIndent) * https://git.eclipse.org/r/#/c/130151/ which uses verticalIndent for codemining line header. Could you apply it and tell me if it works better? In Windows OS, it works well, but in Mac OS, it seems that there is some trouble (see discussion at https://git.eclipse.org/r/#/c/130141/)
(In reply to Angelo ZERR from comment #44) > @Sarika I had a great contribution to JDT CodeMining (see > https://github.com/angelozerr/jdt-codemining/issues/75#issuecomment- > 426679401) for references and I would kike to update my gerrit patch to add > it. Before that I would like to know if you are OK. > > The new preferences give the capability to select where you wish to show > references: > > * on classes > * on methods > > After we could add too "on fields" like you wish to have. > > Are you OK, I update the gerrit patch with this 2 new preferences? We can keep the * on classes * on methods changes in a separate patch but we want to add "fields" along with this change so please add "fields" now for 4.10 M3. Also I have posted a comment on gerrit, please check. Lakshmi has verified that line number issue is resolved with the suggested gerrit.
@Sarika please review my gerrit patch which: * provides fields minings * provides 3 new preferences for show references - On classes - On fields - On methods * don't show vertical bar. Please tell me if there are other thing you wish I do. > Lakshmi has verified that line number issue is resolved with the suggested gerrit. It seems that there is some trouble again https://bugs.eclipse.org/bugs/show_bug.cgi?id=539618, if I don't find the bug I will try to fix line number problem without verticalLine indent new API. But it's very hard for me, since I have not Mac OS.
CQ for this patch: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=18128
@Sarika, @Noopur, @Lakshmi please note the https://bugs.eclipse.org/bugs/show_bug.cgi?id=540950 was marged to master and should fix 2 problems when several codemining preferences changed: * problem with NPE : ---------------------------------------------------------- Caused by: org.eclipse.core.runtime.AssertionFailedException: null argument: at org.eclipse.core.runtime.Assert.isNotNull(Assert.java:88) at org.eclipse.core.runtime.Assert.isNotNull(Assert.java:76) at org.eclipse.jdt.internal.ui.javaeditor.EditorUtility.getEditorInputJavaElement(EditorUtility.java:450) at org.eclipse.jdt.internal.ui.javaeditor.codemining.JavaElementCodeMiningProvider.lambda$0(JavaElementCodeMiningProvider.java:78) at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1700) ... 4 more ---------------------------------------------------------- * even if there are several codeming installation, the process of compute codeminings and render is stopped when code mining provider are disposed. So it should not be a problem when there are several installation. Next step is to review https://git.eclipse.org/r/#/c/130151/ which improves a lot the render of code mining header annotation which uses the new SWT StyledText setLineVerticalIndent API https://git.eclipse.org/r/#/c/130141/. Please review it and I think the JDT codemining gerrit patch could be almost merged (I hope so!). I'm so motivated to continue to contribute with my other codeminings features for JDT.
Now codemining is based on new StyledText API verticalLineIndent. I hope this gerrit patch could be merged.
(In reply to Angelo ZERR from comment #51) > Now codemining is based on new StyledText API verticalLineIndent. I hope > this gerrit patch could be merged. @Angelo, https://git.eclipse.org/r/#/c/126847/ I see the last comment from Mickael, are you working on it ?
> @Angelo, https://git.eclipse.org/r/#/c/126847/ > I see the last comment from Mickael, are you working on it ? Oooops, no sorry @Sarika I had forgotten that. I will see it.
Gerrit change https://git.eclipse.org/r/126847 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=5adfd41ede0e57898fc2e990b69332e4a0b8eaef
Thanks Angelo and Mickael!! Please create new bugs for follow-up issues. @Angelo, Please keep an eye on the incoming bugs for this feature. We need N&N entry and F1 help documentation to be able to resolve this bug.
We need N&N by today as M3 will be released tomorrow. F1 help should come before RC1 release.
> We need N&N by today I will push my work ASAP, but there is some gerrit patches will change JDT codemining like "On classes" to for ... "Classes". N&N must be based on master or future gerrit patch?
(In reply to Angelo ZERR from comment #57) > > We need N&N by today > > I will push my work ASAP, but there is some gerrit patches will change JDT > codemining like "On classes" to for ... "Classes". N&N must be based on > master or future gerrit patch? Unfortunately, we cna not put a screenshot which is not part of M3 build now. Please ass the screenshot available with the M3 build. We will have to do a little re work to update the screenshot after the text change patch is delivered.
(In reply to Sarika Sinha from comment #58) Sorry for the typos - Unfortunately, we can not put a screenshot which is not part of M3 build. Please add the screenshot available with the M3 build.
(In reply to Sarika Sinha from comment #59) > (In reply to Sarika Sinha from comment #58) > > Sorry for the typos - > Unfortunately, we can not put a screenshot which is not part of M3 build. > Please add the screenshot available with the M3 build. If we are sure to put in the label changes then I'd make the screen shot with the new labels. That way we are sure to have the final N&N correct and save us some work.
New Gerrit change created: https://git.eclipse.org/r/132903
New Gerrit change created: https://git.eclipse.org/r/132904
Please review my gerrit patch https://git.eclipse.org/r/#/c/132904/ for N&N
Gerrit change https://git.eclipse.org/r/132904 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=73e6d337ae21ccb86bae733d59eb9e0e5e3ae7d3
(In reply to Eclipse Genie from comment #64) > Gerrit change https://git.eclipse.org/r/132904 was merged to [master]. > Commit: > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=73e6d337ae21ccb86bae733d59eb9e0e5e3ae7d3 Thanks for the N&N entry. I am closing this as bug 541407 has been opened for the F1 help documentation.
