CHE-15 - Java stacktrace support (From Platform to Che Workspace) #5396

vrubezhny · 2017-06-16T14:36:19Z

Adds the possibility to click on a stacktrace line in order to open
a specified class in editor.

Example 1:

Example 2:

Also fixes CHE-293 (#5001) - String Written to Dev-Machine stdout not Escaped Properly

Issue: https://issues.jboss.org/browse/CHE-15
Issue: https://issues.jboss.org/browse/CHE-293

see: #5001

Signed-off-by: Victor Rubezhny vrubezhny@redhat.com

What does this PR do?

Adds the possibility to click on a stacktrace line in order to open
a specified class in editor.
Also fixes CHE-293 (#5001) - String Written to Dev-Machine stdout not Escaped Properly

What issues does this PR fix or reference?

Issue: https://issues.jboss.org/browse/CHE-15
Issue: https://issues.jboss.org/browse/CHE-293
(Issue: #5001)

Changelog

The fix adds the possibility to click on a stacktrace line withing a console in order to open an according java class in editor and reveal it to the line according to the info that in provided in stacktrace line.

Release Notes

This version introduces a smart selection for the console outputs. When having exceptions or errors in your stacktrace, you can now directly jump into the file by clicking on it from the outputs. The editor will open with the file and point to the line corresponding to the one provided in the stacktrace. This allows to quickly navigate to the code which is raising an issue.

[Animated Gif]

The smart selection capability is available for Java and NodeJS stacktrace at this moment.

Docs PR

Will need to be cover by: https://github.com/eclipse/che-docs/issues/249

codenvy-ci · 2017-06-16T14:39:02Z

Can one of the admins verify this patch?

benoitf · 2017-06-16T14:42:18Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputConsole.java

@@ -39,23 +41,30 @@
    /** Follow output when printing text */
    private boolean followOutput = true;

+    private OutputCustomizer					customizer = null;


hello, is that the indent is expected ?

benoitf · 2017-06-16T14:42:33Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputConsole.java

        view.setDelegate(this);

        view.hideCommand();
        view.hidePreview();
        view.setReRunButtonVisible(false);
        view.setStopButtonVisible(false);
+


this line shouldn't be introduced

benoitf · 2017-06-16T14:42:40Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputConsole.java

    }
-
+    


this line(spaces) shouldn't be introduced

benoitf · 2017-06-16T14:43:08Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputConsole.java

@@ -89,7 +98,7 @@ public void printText(String text) {
    public void printText(String text, String color) {
        view.print(text, text.endsWith("\r"), color);

-        for (ActionDelegate actionDelegate : actionDelegates) {
+    	for (ActionDelegate actionDelegate : actionDelegates) {


indent is with spaces, not with tabs

you've to check in the whole file as I see mainly only tabs while spaces are expected.

benoitf · 2017-06-16T14:43:45Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputCustomizer.java

+	private static RegExp LINE_AT_EXCEPTION = compile("(\\s+at address:.+)");
+
+    private AppContext appContext;
+    private EditorAgent editorAgent;


the indent looks odd

benoitf · 2017-06-16T14:43:59Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputCustomizer.java

+import com.google.gwt.user.client.Timer;
+import com.google.inject.Inject;
+
+public class DefaultOutputCustomizer implements OutputCustomizer {


it is missing javadoc on the class. What it does, etc

benoitf · 2017-06-16T14:44:19Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputCustomizer.java

+public class DefaultOutputCustomizer implements OutputCustomizer {
+
+	private static RegExp LINE_AT = compile("(\\s+at .+)");
+	private static RegExp LINE_AT_EXCEPTION = compile("(\\s+at address:.+)");


it is missing the final keyword for the constants.

benoitf · 2017-06-16T14:44:39Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputCustomizer.java

+	 */
+	@Override
+	public String customize(String text) {
+    	String customText = text;


the formatting looks like it is not the expected one

benoitf · 2017-06-16T14:45:13Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputCustomizer.java

+    	String customText = text;
+
+    	MatchResult matcher = LINE_AT.exec(text);
+    	if(matcher != null) {


spaces around parenthesis

benoitf · 2017-06-16T14:46:08Z

ci-build

benoitf · 2017-06-16T14:47:16Z

as this issue is not a fix, it should include changelog and the release notes section.
@slemeur maybe documentation as well ?

vrubezhny · 2017-06-16T14:58:52Z

@benoitf I've added a couple of words to Change log. However, I don't know what is to be placed for Release Notes (especially once it not released yet)

benoitf · 2017-06-16T15:00:46Z

@vrubezhny Changelog should be a one-line entry, so maybe move your "changelog" text as "Release notes" text and it should be ok

codenvy-ci · 2017-06-16T15:01:31Z

Build # 2833 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2833/ to view the results.

benoitf · 2017-06-16T15:04:14Z

@vrubezhny also there was a compilation issue https://ci.codenvycorp.com/job/che-pullrequests-build/2833/

slemeur · 2017-06-16T15:48:07Z

Hi @vrubezhny !
This seems really cool.

@benoitf is right, this PR introduces a new feature so we need the release note section and the documentation prior to the merge. I can help on that.
We'll also need the QA team to review and validate the changes: @musienko-maxim.

Would you mind recording an animated gif to demo the new capability and attach it to the description of the PR?

vrubezhny · 2017-06-16T15:48:42Z

@benoitf The compilation is known: my two classes (intrf+impl) have "RedHat, Inc." in their headers instead of "Codenvy, S.A.". I don't feel like adding the whole che-core-ide-app as an exclusion to license-maven-plugin is a good approach. Is there a better solution? (Or I just have to use original CodeEnvy's header?)

vrubezhny · 2017-06-16T15:52:54Z

@slemeur I've copied text of ChangeLog to Release Notes. Please let me know if it's not enough.

And I'll try to record an animated gif... (not sure if I can do it on Fedora, though)

vrubezhny · 2017-06-16T17:17:18Z

@benoitf I believe I've fixed formatting and other issues you pointed to, as well as the 'missing headers' issue with build. Thanks for the review.

benoitf · 2017-06-16T19:16:53Z

ci-build

benoitf · 2017-06-16T19:18:30Z

@vrubezhny as there is the use of regexp and parsing, IMHO a unit test should be added to cover the analysis of the stack trace.
Else it will be hard to track if it's still working or not upon refactoring.

codenvy-ci · 2017-06-16T20:11:17Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2834/

Adds the possibility to click on a stacktrace line in order to open a specified class in editor. Also fixes CHE-293 (eclipse-che#5001) - String Written to Dev-Machine stdout not Escaped Properly Issue: https://issues.jboss.org/browse/CHE-15 Issue: https://issues.jboss.org/browse/CHE-293 see: eclipse-che#5001 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny · 2017-06-16T23:04:21Z

@slemeur I've added the animated gifs to the PR description.

@benoitf The commit is updated due to the issue in my code, so ci-build is required for the PR once again. Sorry about that.

benoitf · 2017-06-17T09:40:39Z

@vrubezhny Hello Victor. I'm not seing unit test as said yesterday. Also, could you add changes as adding a new commit (like using --fixup option) because, it's impossible to track what you changed between each review as we see only one commit even if you've done changes. Squashing/Rebasing can be done later when merging it. thanks.

vrubezhny · 2017-06-17T13:14:05Z

@benoitf Yep, I haven't made JUnit test yet - I'll work on it on Monday. Thanks for pointing me to '--fixup' option, I'll follow this policy.

slemeur · 2017-06-19T10:06:28Z

Hi @vrubezhny. Thanks for adding the animated gif, they are perfect. Looking at the code, I see that the regexp will properly cover java stacktraces. AFAIK it should also work for NodeJS but I'm bit worry that this might also create wrong interpretation for other type of stacktraces and bother the user.

So to start the discussion, should we:

check that the text found by the parser is actually a file before colorizing it and putting a link behind?
should we also handle urls?
allow the user to disable the smart selection in the console output
have the ability for the user to add regular expressions (in preferences for example)?
should we have the same behaviour in the terminal?

vrubezhny · 2017-06-20T14:36:21Z

Hi @vrubezhny. Thanks for adding the animated gif, they are perfect. Looking at the code, I see that the regexp will properly cover java stacktraces. AFAIK it should also work for NodeJS but I'm bit worry that this might also create wrong interpretation for other type of stacktraces and bother the user.

Hi @slemeur.
That's it. It's proposed to work with java-stacktraces printed out to a console, it's not intended to work with other kinds of stacktraces. With proposed OutputCustomizer interface we can develop different kinds of customizers. Also, we can add preferences to allow enabling/disabling to the customizers, or configure different kinds of consoles to use different customizers.

So to start the discussion, should we:

check that the text found by the parser is actually a file before colorizing it and putting a link behind?

We can't do that, first of all because of the performance reasons: it takes quite a lot of time (and I suppose a several asynchronous IO requests) to perform such a check. If we make it synchronously the console output would tend to freeze. Otherwise the text would not be printed in FIFO manner. As far as I know, desktop eclipse doesn't check for a file existence when a stacktrace is being printed out to a console, it performs searching when a user clicks on a colorized line.
The only thing I probably would verify (beside of matching the stacktrace line RegExp) is that file name really ends with ".java" to make sure that we're in "proper" stacktrace line.

should we also handle urls?

Not sure I understand right... Have an example of URLs appearance?

allow the user to disable the smart selection in the console output

Do you mean like "to disable" the console text customization (turning it to a link) and as such, not to provide a user with the possibility to search for a class by a stacktrace line? Not sure. I didn't see such a preference before.

have the ability for the user to add regular expressions (in preferences for example)?

If you're going to process the other kinds of stacktraces (like the javascript ones) then you should have the other kind of detection and post-processing (like parsing the line for packages/classes/file name/line number and/or adding, probably, javascript-to-java mappings to your search algorithm). This is not something that easy to realize as something that can be configured by preference and, imho, we should rather provide different customizers for users and let them a possibility to enable/disable them. Or hardly set up different customizers depending on the kind of console it should work on.

should we have the same behaviour in the terminal?

As far as I understand, my change configures the terminal to use this customizer. So, if a java stacktrace would appear in terminal it will be customized as well. If it's not required I can try to disable/remove this behaviour from terminal.

vrubezhny · 2017-06-23T20:37:59Z

@benoitf I'm going to add a commit with JUnit test to this PR. Should I use that --fixup option, or a separate commit would be OK?

benoitf · 2017-06-23T20:57:11Z

@vrubezhny it is up to you, in all cases it will add a new commit (fixup is generating a commit as well)
At the end of the review you can either automatically rebase if you used fixup or manually rebase
Or we squash commits when merging on github

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

benoitf · 2017-06-26T09:25:18Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputCustomizer.java

+                customText += text.substring(openBracket + "(".length(), closingBracket);
+                customText += "</a>";
+                customText += text.substring(closingBracket);
+                text = customText;


do you think that a String.format would be easier to read than concatenating tons of stuff ?

so bad idea (mine) as GWT is not supporting formatting 👎

String.format/MessageFormat.format/Formatter.format are not available for GWT application and it doesn't look like a GWT replacement exists.

benoitf · 2017-06-26T09:27:24Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/console/DefaultOutputCustomizer.java

+            public List<File> apply(Resource[] children) throws FunctionException {
+                final List<File> result = newArrayList();
+
+                for (Resource child : children) {


would it be simpler to read with streams / filter/ Collectors.toList() ?

benoitf · 2017-06-26T09:28:09Z

ide/che-core-ide-app/src/test/java/org/eclipse/che/ide/console/DefaultOutputCustomizerTest.java

+    public void testNonStackTraceLines() throws Exception {
+        testStackTraceLine(false, "[STDOUT] Listening for transport dt_socket at address: 4403", null);
+    }
+}


an empty blank line is required at the end of the file

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

…Workspace) Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny · 2017-06-26T13:48:30Z

@benoitf I've added fixups for:

turning org.eclipse.che.ide.console.DefaultOutputCustomizer.collectChildren(Container, Path) to use streams / filter/ Collectors.toList()
added new line to last line of DefaultOutputCustomizerTest
added new line before DefaultOutputCustomizer.collectChildren() method's closing bracket (somehow lost on formatting)

benoitf · 2017-06-26T13:51:49Z

@vparfonov ?

…lipse-che#5396) * CHE-15 - Java stacktrace support (From Platform to Che Workspace) Adds the possibility to click on a stacktrace line in order to open a specified class in editor. Also fixes CHE-293 (eclipse-che#5001) - String Written to Dev-Machine stdout not Escaped Properly Issue: https://issues.jboss.org/browse/CHE-15 Issue: https://issues.jboss.org/browse/CHE-293 see: eclipse-che#5001 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

sunix self-requested a review June 16, 2017 14:44

benoitf requested changes Jun 16, 2017

View reviewed changes

benoitf added the kind/enhancement A feature request - must adhere to the feature request template. label Jun 16, 2017

benoitf requested a review from slemeur June 16, 2017 14:47

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. team/ide labels Jun 16, 2017

vrubezhny force-pushed the che15 branch 2 times, most recently from 07f4354 to 7741e02 Compare June 16, 2017 17:10

vrubezhny force-pushed the che15 branch from 7741e02 to bbb4bd8 Compare June 16, 2017 20:59

fixup! CHE-15 - Java stacktrace support (From Platform to Che Workspace)

992de9e

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

benoitf reviewed Jun 26, 2017

View reviewed changes

fixup! CHE-15 - Java stacktrace support (From Platform to Che Workspace)

275fea9

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

benoitf requested a review from vparfonov June 26, 2017 13:35

benoitf assigned vrubezhny Jun 26, 2017

fixup! fixup! CHE-15 - Java stacktrace support (From Platform to Che …

ef31a58

…Workspace) Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

benoitf approved these changes Jun 26, 2017

View reviewed changes

fixup! fixup! CHE-15 - Java stacktrace support (From Platform to Che …

5726874

…Workspace) Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

slemeur approved these changes Jun 27, 2017

View reviewed changes

sunix approved these changes Jun 29, 2017

View reviewed changes

benoitf added the team/californication label Jun 29, 2017

vparfonov approved these changes Jun 30, 2017

View reviewed changes

benoitf merged commit 991b895 into eclipse-che:master Jun 30, 2017

benoitf added this to the 5.15.0 milestone Jun 30, 2017

vrubezhny mentioned this pull request Jul 1, 2017

String Written to Dev-Machine stdout not Escaped Properly #5001

Closed

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 2, 2017

vparfonov mentioned this pull request Mar 26, 2018

Navigation from Stack Trace from posted output to the files in opened workspace #9248

Closed

CHE-15 - Java stacktrace support (From Platform to Che Workspace) #5396

CHE-15 - Java stacktrace support (From Platform to Che Workspace) #5396

Conversation

vrubezhny commented Jun 16, 2017 • edited by slemeur Loading

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

codenvy-ci commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented Jun 16, 2017

benoitf commented Jun 16, 2017

vrubezhny commented Jun 16, 2017

benoitf commented Jun 16, 2017

codenvy-ci commented Jun 16, 2017

benoitf commented Jun 16, 2017

slemeur commented Jun 16, 2017

vrubezhny commented Jun 16, 2017 • edited Loading

vrubezhny commented Jun 16, 2017

vrubezhny commented Jun 16, 2017 • edited Loading

benoitf commented Jun 16, 2017

benoitf commented Jun 16, 2017

codenvy-ci commented Jun 16, 2017

vrubezhny commented Jun 16, 2017

benoitf commented Jun 17, 2017 • edited Loading

vrubezhny commented Jun 17, 2017

slemeur commented Jun 19, 2017

vrubezhny commented Jun 20, 2017 • edited Loading

vrubezhny commented Jun 23, 2017

benoitf commented Jun 23, 2017

Choose a reason for hiding this comment

benoitf Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrubezhny commented Jun 26, 2017

benoitf commented Jun 26, 2017

vrubezhny commented Jun 16, 2017 •

edited by slemeur

Loading

vrubezhny commented Jun 16, 2017 •

edited

Loading

vrubezhny commented Jun 16, 2017 •

edited

Loading

benoitf commented Jun 17, 2017 •

edited

Loading

vrubezhny commented Jun 20, 2017 •

edited

Loading

benoitf Jun 26, 2017 •

edited

Loading