[Console] Support content detection for ANSI-escaped console text #535
Conversation
|
I don't think it's a good idea to start adding such features in m2e. ANSI is a standard so those shoudl be directly added into Eclipse Console, as you could see in https://bugs.eclipse.org/bugs/show_bug.cgi?id=112948 . Adding it in standard Eclipse will benefit to a much wider world and attract more contributors who need this independently of Maven, and would also remove this duty from m2e, that can then keep focusing on Maven integration. |
HannesWell
force-pushed
the
coloredConsole
branch
from
329106d
to
50fd705
Compare
I actually agree with you and would really appreciate if this would be part of standard Eclipse. It would then probably require a separate way to get the text without escape characters and to place a link using the unescpaed text's indices. But I think this should be feasible like I just did it here. Maybe just in more general form. The reason I wanted to add this workaround to m2e is the time I expect support for ANSI escaped text will take until it will be available in standard Eclipse. I fear it will take some more time until the feature requested by the mentioned bug is available since it is dormant for three years now. I just tried to revive it yesterday and at least until now there was no reaction, but this of course still can change. At least the Maintainer of the ANSI-Console plug-ins seem to be active on GitHub so there might be a chance we can benefit from his knowledge. I would also be fine if this becomes an intermediate "workaround", that is removed when there is support in the standard Eclipse console. You probably prefer to wait completely until this becomes available in Standard Eclipse, don't you? |
|
For the cucumber-eclipse project I use the ANSI-Console it works without any issues and having colors is quite cool: https://github.com/mihnita/ansi-econsole and the author seems to want contribute the code ... |
That's up to you ;) Platform is open for contributions, and fast at reviewing/merging if everything is internal.
Those workarounds are harmful for the wider community:
The rule for best community OSS is to treat projects you depend on just like your projects and contribute to it as much as necessary for the success of your project. For this particular change, it would be nice to extract this ANSI support as a specific IConsoleLineTracker first, and then to just put this IConsoleLineTracker into org.eclipse.debug.ui. |
Its not always easy especially as eclipse-platform is still not on github. Anyways if there is a need for it I could try to prepare a contribution.... |
I know, its just that at the moment there are other things I would like to do first and it would take me some time to get familiar with the field of ANSI escaped text and the styling of Console print-out. Therefore I thought this relatively small change, to just not get harmed by colored print-outs, would be acceptable although I knew it was not ideal.
I fully agree with all of your statements. Just as I said above. OK, then lets abandon this change and hope (or better work) for a standard Eclipse console that supports colors.
You mean that there is something like an
It is indeed very cool. I really like to use the ANSI-Console plug-in in combination with the m2e-chromatic plug-in which was just created because the Standard Eclipse Console does not support colors natively. See Bug 544307. Yes Mihai Nita expressed his interest in contributing his Plugin to Eclipse in Bug 112948 about three years ago but then nothing much happened. However I suggest to create and issue at https://github.com/mihnita/ansi-econsole to ask him if he wants to start/continue his work towards a contribution and to offer my (and Christoph if you want to participate also your) assistance for the 'Eclipse procedures'.
I would first try to convince Mihai Nita to work on this since he seems to have a lot of experience in the field and I'm not eager to work on this or do you really want to do it @laeubi? |
m2e could simply include the ANSI console right now together with m2e-chromatic as it is apache licenced it would only require a CQ.
There are many bugs in eclipse that even have longer no reaction from eclipse committers so well its just a matter of of priorities maybe.
At laest @mihnita has had fixed two bug upon my request it seems fair to at least offering some help if desired. Its not a million LOC project anyways :-) |
|
I opened mihnita/ansi-econsole#70 |
As a last resort we could think about that but I would prefer to try a transfer first, since it also allows better/deeper integration and the ANSI escaping can be considered in other changes and is not broken every second release.
It wouldn't surprise me if priorities are the case, I think everybody experienced it too and only sporadic reactions don't push priorities IMHO. Therefore my previous suggestion to express our interest first and see if he is interested again. :)
That's right :)
Thanks for creating the issue! |
|
Hi all, First, I must apologize for being unresponsive. The last few months have been "a bit busy", and in fact I think that will still be the case until end of August or so. I've answered to issue mihnita/ansi-econsole#70 But TLDR: I will check it if is possible to dual license Eclipse & Apache, Mihai |
|
Now that we have built in support for Console colors in Eclipse-Debug (eclipse-platform/eclipse.platform.debug#50) and support of that in M2E (#873) I think we can and even should consider this again. Otherwise the link-creation and debugger attachment don't work with the colored console. |
HannesWell
force-pushed
the
coloredConsole
branch
2 times, most recently
from
3ac3aba
to
f99728c
Compare
| @@ -212,14 +222,27 @@ private String getText(IRegion lineRegion) throws BadLocationException { | |||
| return line0; | |||
| } | |||
|
|
|||
| private String getLineText(IRegion lineRegion) throws BadLocationException { | |||
| return console.getDocument().get(lineRegion.getOffset(), lineRegion.getLength()).strip(); | |||
| private String getLineText(IRegion region, List<int[]> removedLocations) throws BadLocationException { | |||
There was a problem hiding this comment.
@mihnita does the console provide a better (built in) way to obtain the unescaped text? Or should we add that to the console, maybe in a more efficient way?
There was a problem hiding this comment.
@mihnita does the console provide a better (built in) way to obtain the unescaped text?
Or should we add that to the console, maybe in a more efficient way?
Nothing public.
The only place where that is needed is to put text in clipboard without codes.
And that is a simple regexp replace in AnsiConsoleUtils
AnsiConsoleUtils.ESCAPE_SEQUENCE_REGEX_TXT.matcher(plainText).replaceAll("");
There was a problem hiding this comment.
BTW, I know why the external maven uses colors on the level and the embedded one does not.
org.eclipse.m2e.maven.runtime requires org.eclipse.m2e.maven.runtime.slf4j.simple, which contains (in jars the simple slf4j logger (slf4j-simple-*.jar)
That logger does not output colors.
The external maven has a maven-slf4j-provider-*.jar in the libs folder that implements what's needed to behave like a slf4j provider.
And that provider produces color output for the levels.
What we see colored right now with the embedded maven are messages logged.
Because if I do logger.warning("Hello \033[91m RED \033[m world!"); the message itself has colors.
Some options:
- leave it as is
- remove
slf4j-simple, takemaven-slf4j-provider, and we will behave 100% like the external maven - replace
slf4j-simplewith logback, which allows for colors and a lot of customization. - replace
slf4j-simplewith our own slf4j-provider, where we have full control and we know that nobody will change it behind our back
I played with option 3, and worked without problems.
And I don't expect trouble with option 2 either.
Option 4 would be more work.
There was a problem hiding this comment.
I've also checked option 2, using maven-slf4j-provider
Works, and the output looks like the one from external maven.
There was a problem hiding this comment.
@mihnita does the console provide a better (built in) way to obtain the unescaped text?
Or should we add that to the console, maybe in a more efficient way?Nothing public. The only place where that is needed is to put text in clipboard without codes. And that is a simple regexp replace in
AnsiConsoleUtilsAnsiConsoleUtils.ESCAPE_SEQUENCE_REGEX_TXT.matcher(plainText).replaceAll("");
Thanks for that hint. Unfortunately that class is not accessible because the package is not exported.
In general and as it has already been said by Mickael earlier in this PR, unescaping of the console document text should not be done in M2E because others are probably also need this capability (getting the unescaped text, adding a link based on the unescaped offsets and lengths, etc.). Therefore I think it is better handled in the o.e.ui.console or o.e.debug.ui.
I'm thinking about having something like un-escaped views of an IRegion that provides the text without escape characters. Maybe it is even better to add an getUnescapedDocument() to the IConsole.
Or simply just build the entire document model by default on the unescaped text. So that if one really wants the raw text, it has to be obtained via getRawDocument() or so.
But that is something we should discuss in a dedicated issue in eclipse.platform.debug.
I also noticed that if the output is colored links are not displayed. But that is also an issue for platform.debug.
For now to support colored print-out in M2E I think this is the way we have to go until platform provides a more general one.
There was a problem hiding this comment.
BTW, I know why the external maven uses colors on the level and the embedded one does not.
What you describes makes totally sense to me, although I have not yet fully tested it but you are right with the jars in the external maven installation.
I'm in favor of option 2. The logging is a bit difficult here, because we want to have the standard logging when we run a Maven-build launched from the IDE, but at the same time want to use the logback implementation when maven runs in the IDE and that only be achieved the way it is now (at least nobody had a better idea how to make that generally better).
But option 2 looks like a very good way to go that is simple to achieve. Do you want to provide a PR for that?
You just have to replace the slf4j-simple by org.apache.maven:maven-slf4j-provider in the org.eclipse.m2e.maven.runtime.slf4j.simple and adjust the version to match the embedded maven version. For that you probably want to move the maven-core.version property up to the m2e-maven-runtime/pom.xml.
Actually we should then remove the simple suffix from org.eclipse.m2e.maven.runtime.slf4j.simple and set it's version also to the maven version. In the require-bundle in the org.eclipse.m2e.maven.runtime/pom.xml has to be adjusted to the names as well.
Option 3 and 4 seem to be a bit overkill respectively overprotective. We already rely on what Maven is giving us and therefore we can rely on the logger as well.
There was a problem hiding this comment.
Looks like the renaming is a bit more work. It is probably simpler to split this, if you want you can just replace the logger impl and I take care of all other adjustments in the maven-components. Alternatively I can do all at once and mention you as coauthor of the commit.
There was a problem hiding this comment.
What do you think about adding org.apache.maven:maven-slf4j-provider directly to org.eclipse.m2e.maven.runtime and removing org.eclipse.m2e.maven.runtime.slf4j.simple completely?
It really is part of maven, covered by the same license, same version, etc.
M.
There was a problem hiding this comment.
What do you think about adding
org.apache.maven:maven-slf4j-providerdirectly toorg.eclipse.m2e.maven.runtimeand removingorg.eclipse.m2e.maven.runtime.slf4j.simplecompletely?
I already thought about that for a while. But until know there where technical difficulties that make it harder to merge the sfl4j-simple jar into m2e.maven.runtime than keeping it separate.
But I had a new idea that should make it simple. I'm currently trying it out and will let you know if that has worked.
If it works as I intend it, then it should be a simple change to use maven-slf4j-provider.
There was a problem hiding this comment.
It worked, see #875
HannesWell
force-pushed
the
coloredConsole
branch
3 times, most recently
from
6331f7e
to
bfff9a6
Compare
|
Just created eclipse-platform/eclipse.platform#539 to handle the issue of making ConsoleLineTrackers work with ANSI-escape sequences in general. |
HannesWell
force-pushed
the
coloredConsole
branch
from
bfff9a6
to
f0774b2
Compare
|
Until eclipse-platform/eclipse.platform#539 is resolved we should have this change to keep the console-line tracking working with colored print-out. |
With this PR the content detection of the
ConsoleLineTracker, namely the detection of Running tests and the detection of forked VMs waiting for a debugger connection, also work for colored (i.e. ANSI escaped) print-outs.Colored print-out is activated using the command line arguments
-Dstyle.color=alwaysor if one has them2e-chromaticandANSI-Consoleplug-ins installed from the marketplace.I know that in general we should not support external plug-ins specifically. But the mentioned arguments is built into Maven and does not require any of the mentioned marketplace plug-ins (although the print out then becomes hardly readable). Therefore proper support can be tested independently and it requires not too much code. Therefore I think it's acceptable.