Fix #91: Stack trace detection not working in colored console

[mihnita]

What it does

Removes the ANSI escape sequences before detecting links.

How to test

See eclipse-platform/eclipse.platform#535

The easiest way to reproduce was to create a new project and println the stack trace.

Attached a small file reproducing the problem.
I've tested with ANSI Support enabled and disabled, and I've checked that the links work and jump at the proper line.

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

Main.java.zip

[akurtakov]

@jjohnstn would you please review/test/etc.? This sounds important enough to even justify RC2.

[laeubi]

What I'm wondering is if TextConsole should be extended in a way to get the "plaintext", this can then return just the normal text as a default implementation but the ansi console might return the unescaped text.

[mihnita]

What I'm wondering is if TextConsole should be extended in a way to get the "plaintext",
this can then return just the normal text as a default implementation but the ansi console
might return the unescaped text.

Yes, I think that would be a good solution.

In a comment to the open issue (eclipse-platform/eclipse.platform#535) I also said "And probably even better would be to implement the idea in issue eclipse-platform/eclipse.platform#539"

But that would require a bigger refactoring, not enough time for it right now.

Mihai

[jjohnstn]

@jjohnstn would you please review/test/etc.? This sounds important enough to even justify RC2.

Ok.

[laeubi]

@mihnita to be honest I think it then would be better to delay this to the next release... This currently is only an issue with m2e (and even there we not yet can reproduce it with default maven builds) and one can switch of colors there if required. Adding explicit ANSI escape removal to this code parts do not seem very useful and probably makes the code much more complex than it helps, e.g. this will only work for these explicit cases but not for probably others...

The ANSI support is great, but adding ANSI workarounds everywhere seems not to scale well...

[iloveeclipse]

This sounds important enough to even justify RC2.

I vote against inclusion into RC1 or RC2.

• ANSI support is rarely used, it is a "nice to have"
• stack trace detection in general is very important feature
• code changes non trivial regex patterns
• no regression tests added
• too risky / not important for RC

[iloveeclipse]

@mihnita : I believe we had tests for stack trace detection, at least you should see it from git history.
Would be good if you can add new test that verify that ANSI is really and properly filtered out :-)

[SarikaSinha]

+1 for moving to next release.
Regex changes are not easy to test and verify at this later stage.

[jjohnstn]

Agreed, will wait for next release.

[mihnita]

Ack. Makes sense.
I wasn't sure if it was important enough for a temporary / quick patch or not.

[laeubi]

@mihnita but you can already prepare PR for your "bigger refactoring" part so we can merge this early after branches are open...

[mihnita]

Updated, tested everything, and it is ready to merge.

Notes:

• I see (and I'm also annoyed :-) by the "Merge branch 'eclipse-jdt:master' into master" commits
  That's my bad, I started working in master. And then GitHub did that when I clicked "Sync fork"
  I am fine to drop this CL, remove and recreate my fork, reapply the fix in a branch, test, commit, to have a clean PR.
  But I didn't want to throw away context, and the work of people who already looked at this.
  Let me know if you prefer to merge this "as is", or want me to go "the clean way"

• Back to this comment:

  What I'm wondering is if TextConsole should be extended in a way to get the "plaintext",
  this can then return just the normal text as a default implementation but the ansi console
  might return the unescaped text.

  Yes, I think that would be a good solution. So I've spent some time on it over the holydays.
  But is it not a simple refactoring, so I don't have yet a working implementation.
  I think the way to go would be updating the the ConsoleDocument, not TextConsole?
  And the "raw content" of the document would be plain text, no escapes, but with Positions.
  Do you think that it sounds like a bad direction to go?
  It is also weird that there are 2 ConsoleDocument classes (org.eclipse.m2e.core.ui.internal.console.ConsoleDocument in m2e-core and org.eclipse.ui.internal.console.ConsoleDocument in eclipse.platform.debug)
  So making something to share between the two might be tricky :-(

---

TLDR:

What I propose is:

• land this PR for a quick fix as the next item might take a while
  It would be "as is" or cleanup (new fork, no merge messages), just let me know.
• create a refactoring / cleanup issue for a way to "hide" the escapes from APIs, either in TextConsole or ConsoleDocument

[laeubi]

I think we should not apply a quick-fix here as it unnecessary complicates things. The stacktraces are just one example and I think the ANSI console should best "hide" the ANSI nature that is when I copy/paste things, copy without escapes should be default and for API it should return a plain text and all else (HTML, RTF, with escapes) should be special cases one needs to explcitily trigger.

[iloveeclipse]

I see (and I'm also annoyed :-) by the "Merge branch 'eclipse-jdt:master' into master" commits
That's my bad, I started working in master. And then GitHub did that when I clicked "Sync fork"
I am fine to drop this CL, remove and recreate my fork, reapply the fix in a branch, test, commit, to have a clean PR.

You don't need to recreate your fork.
All what you need is (sounds complicated, but it isn't if you do this from Eclipse History view)

1. create a dedicated "temp" branch locally at your current master head
2. checkout master branch
3. hard reset master to the last "origin" master commit
4. pull from origin to get latest state on your local master
5. on head of master create new "right" branch
6. cherry-pick the commit from the "temp" branch
7. push and create new PR.

[laeubi]

Even better is to use feature-branches instead of working on the master:

1. rightclick on the repository new branch, name it "temp"
2. switch to master
3. choose hard reset as described by @iloveeclipse
4. now again create a new branch, e.g. issue_<github issue number>
5. then choose merge, select your temp branch but choose the option "squash changes"
6. Now push the branch and change the PR to use your new source branch (or create a new PR)

[mihnita]

Created PR #174 using a feature branch.

Thank you both for the detailed instructions.
My hope was to somehow "salvage" this PR, with all the discussion thread. :-)
Well, it is what it is.