Bug 203727 - Colorization of Build Console
Summary: Colorization of Build Console
Status: RESOLVED WONTFIX
Alias: None
Product: CDT
Classification: Tools
Component: cdt-build (show other bugs)
Version: 4.0   Edit
Hardware: All All
: P3 enhancement with 3 votes (vote)
Target Milestone: ---   Edit
Assignee: cdt-build-inbox@eclipse.org CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
: 141967 211639 286464 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-09-18 06:36 EDT by Nils Hagge CLA
Modified: 2013-10-14 16:22 EDT (History)
10 users (show)

See Also:


Attachments
Patch to add different colorization in the build console as implied by the preferences (8.20 KB, patch)
2007-09-18 06:36 EDT, Nils Hagge CLA
no flags Details | Diff
This patch enables colorization of the build console in CDT 4.0.2 (9.33 KB, patch)
2007-12-03 10:03 EST, Nils Hagge CLA
no flags Details | Diff
additional patch to fix error parser test (1.06 KB, patch)
2007-12-11 04:12 EST, Nils Hagge CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nils Hagge CLA 2007-09-18 06:36:20 EDT
Created attachment 78632 [details]
Patch to add different colorization in the build console as implied by the preferences

Under "Window | Preferences | C/C++ | Build Console" it is possible to specify different colors for output, information, and error texts. But only the "output color" shows any effect.

Examing the the implementation of the ErrorParserManager, I found out that these colors are not used. And more over that even the stdout and stderr streams from the externally called tools are merged before being sent to to the error parsers.

We find it helpful if the colorization of errors is different, as implied be the preference dialog.

I have fixed this by keeping error and output streams separate and passing it to the different streams provided by the build console. I also used the "info" stream for the messages generated at start of the build process.

I have attached a patch that introduces this functionality

Regards,
Nils Hagge
Comment 1 Doug Schaefer CLA 2007-09-18 07:52:35 EDT
Cool. Thanks for this. We can talk about it at the summit.

Would it also be possible to use the same mechanism to add hyperlinking to the console so that if you click on an error, it can open it up in the editor?
Comment 2 Nils Hagge CLA 2007-12-03 10:03:29 EST
Created attachment 84321 [details]
This patch enables colorization of the build console in CDT 4.0.2

I have updated my build console colorization patch from CDT 4.0.0 to CDT 4.0.2. Please take a look at it. Our in-house users do not want to miss the colored build console anymore.

It would be fine to have this patch merged into CDT.

(Hyperlinking is still an open issue I am working on.)
Comment 3 Heiko Gerdau CLA 2007-12-03 12:22:25 EST
> 
> It would be fine to have this patch merged into CDT.

Yes, please. 

> 
> (Hyperlinking is still an open issue I am working on.)
> 

Having that feature would be even greater. That would make me completely leave gnu emacs which I still use for resolving problems on bigger code changes.

Thanks for working on this.
Comment 4 Doug Schaefer CLA 2007-12-03 13:58:48 EST
Thanks, Nils. One thing I noticed is that you changed ErrorParserManager not to inherit OutputStream. I have no arguments with that.

However, it does cause a compile error in the GenericErrorParserTests. Could you provide an update patch that includes the necessary changes to the tests?
Comment 5 Nils Hagge CLA 2007-12-04 11:19:51 EST
Ok. I take a look at it.
Comment 6 Nils Hagge CLA 2007-12-11 04:12:49 EST
Created attachment 84940 [details]
additional patch to fix error parser test

I have added a patch that modifies the GenericParserTests such that it no longer calls "close()" on the ErrorParserManager. "close()" is called on the outputstream directly.
Comment 7 Jason Montojo CLA 2007-12-11 09:53:33 EST
*** Bug 211639 has been marked as a duplicate of this bug. ***
Comment 8 Andre Bossert CLA 2008-02-20 15:05:30 EST
I've used the patch from Nils with 4.0.0 and with 4.0.2 too. It works nice and i do not want to miss it anymore. Is it possible to apply it for 4.0.3?

It would be good to have background color in console like it described in 217189.
Comment 9 Doug Schaefer CLA 2008-02-20 15:35:01 EST
I'd prefer not to change the UI in a maintenance release like that. But we should probably look at it for 5.0.

As I mentioned, what I really want is hyperlinked build error messages so that we can click on errors and bring up the error marker. If the patch had that it would be applied pretty quickly I would think.
Comment 10 Nils Hagge CLA 2008-02-21 03:10:34 EST
I investigated the aspect of hyperlinking and already started to implement something. But it turned out to be much more difficult than I thought. I ran into threading problems when adding the hyperlink markers. As far as I understood the errorparsers are run from a thread that is not compatible with the mechanisms that process the hyperlinks.

On the other hand in the Eclipse framework there is already a concept called "LineTracker" that does exactly that. I already used line trackers successfully for other non-CDT based plugins. But the "BuildConsole" is somehow separate and cannot directly make use of them.

So, what I would suggest is to restructure the concept of the BuildConsole to migrate to some standard console. I guess this separation originates from the fact that CDT's build console has a much longer history that the colored console in eclipse itself.
Comment 11 Nils Hagge CLA 2008-02-21 03:21:26 EST
So my proposition is to apply my path as a first comprise. My colleagues also use it and do not want to miss it and never had problems with it. From my point of view the good point of the patch is that it does not change the structure of the way the error parsers are called and handled.

With the current implementation of the errorparsers I do not see a simple solution to integrate the hyperlinking (because of threading issues).

Thus, the current patch is very "economic", since it adds a remarkable benefit for acceptable modification and effort.

I am still willing to contribute a solution for the hyperlinking, but at work it cannot have a higher priority at the moment, since I do not get extra time for that.

Nils
Comment 12 Nils Hagge CLA 2008-02-21 03:34:26 EST
So my proposition is to apply my path as a first comprise. My colleagues also use it and do not want to miss it and never had problems with it. From my point of view the good point of the patch is that it does not change the structure of the way the error parsers are called and handled.

With the current implementation of the errorparsers I do not see a simple solution to integrate the hyperlinking (because of threading issues).

Thus, the current patch is very "economic", since it adds a remarkable benefit for acceptable modification and effort.

I am still willing to contribute a solution for the hyperlinking, but at work it cannot have a higher priority at the moment, since I do not get extra time for that.

Nils
Comment 13 Chris Recoskie CLA 2008-02-25 12:31:41 EST
I would caution against separating stdout and stderr without thoroughly testing.  See Bug 85196
Comment 14 Warren Paul CLA 2008-05-09 11:35:33 EDT
Is this still an issue?  It works fine in our product based on CDT 4.0.x, but we have our own builder.  We use the CDT build console though.  We pipe our own messages and stdout and stderr, and they all show up in the colors selected in the prefs.
Comment 15 Doug Schaefer CLA 2008-05-11 18:29:03 EDT
(In reply to comment #14)
> Is this still an issue?  It works fine in our product based on CDT 4.0.x, but
> we have our own builder.  We use the CDT build console though.  We pipe our own
> messages and stdout and stderr, and they all show up in the colors selected in
> the prefs.
> 

I've never seen the build console colorified. Are you sure you don't have any magic on top.

BTW, my real wish here is to add hyperlinks to compile errors to navigate to the line specified in the message.
Comment 16 Warren Paul CLA 2008-05-12 16:56:12 EDT
(In reply to comment #15)
> I've never seen the build console colorified. Are you sure you don't have any
> magic on top.
> 
> BTW, my real wish here is to add hyperlinks to compile errors to navigate to
> the line specified in the message.
> 

I looked at what we do versus what the make builder does.  We originally did something similar but changed it to get the colors working correctly for the various streams.  I created a quick patch for the make builder, but I'm not setup to test it.  If someone wants to try it out, here's a patch for HEAD:

### Eclipse Workspace Patch 1.0
#P org.eclipse.cdt.make.core
Index: src/org/eclipse/cdt/make/core/MakeBuilder.java
===================================================================
RCS file: /org.eclipse.cdt.make.core/src/org/eclipse/cdt/make/core/MakeBuilder.java,v
retrieving revision 1.1
diff -u -r1.1 MakeBuilder.java
--- src/org/eclipse/cdt/make/core/MakeBuilder.java	25 Mar 2008 18:25:33 -0000	1.1
+++ src/org/eclipse/cdt/make/core/MakeBuilder.java	12 May 2008 20:47:02 -0000
@@ -194,15 +194,17 @@
 					last = new Integer(100);
 				}
 				StreamMonitor streamMon = new StreamMonitor(new SubProgressMonitor(monitor, 100), cos, last.intValue());
-				ErrorParserManager epm = new ErrorParserManager(getProject(), workingDirectory, this, info.getErrorParsers());
-				epm.setOutputStream(streamMon);
-				OutputStream stdout = epm.getOutputStream();
-				OutputStream stderr = epm.getOutputStream();
+				ErrorParserManager stdoutStream = new ErrorParserManager(getProject(), workingDirectory, this, info.getErrorParsers());
+				stdoutStream.setOutputStream(streamMon);
+
+				ErrorParserManager stderrStream = new ErrorParserManager(getProject(), workingDirectory, this, info.getErrorParsers());
+				stderrStream.setOutputStream(console.getErrorStream());
+
 				// Sniff console output for scanner info
 				ConsoleOutputSniffer sniffer = ScannerInfoConsoleParserFactory.getMakeBuilderOutputSniffer(
-						stdout, stderr, getProject(), workingDirectory, null, this, null);
-				OutputStream consoleOut = (sniffer == null ? stdout : sniffer.getOutputStream());
-				OutputStream consoleErr = (sniffer == null ? stderr : sniffer.getErrorStream());
+						stdoutStream, stderrStream, getProject(), workingDirectory, null, this, null);
+				OutputStream consoleOut = (sniffer == null ? stdoutStream.getOutputStream() : sniffer.getOutputStream());
+				OutputStream consoleErr = (sniffer == null ? stderrStream.getOutputStream() : sniffer.getErrorStream());
 				Process p = launcher.execute(buildCommand, buildArguments, env, workingDirectory);
 				if (p != null) {
 					try {
@@ -241,18 +243,18 @@
 					buf = new StringBuffer(errorDesc);
 					buf.append(System.getProperty("line.separator", "\n")); //$NON-NLS-1$ //$NON-NLS-2$
 					buf.append("(").append(errMsg).append(")"); //$NON-NLS-1$ //$NON-NLS-2$
-					cos.write(buf.toString().getBytes());
-					cos.flush();
+					consoleErr.write(buf.toString().getBytes());
+					consoleErr.flush();
 				}
 
-				stdout.close();
-				stderr.close();
-
-				monitor.subTask(MakeMessages.getString("MakeBuilder.Creating_Markers")); //$NON-NLS-1$
 				consoleOut.close();
 				consoleErr.close();
-				epm.reportProblems();
-				cos.close();
+
+				monitor.subTask(MakeMessages.getString("MakeBuilder.Creating_Markers")); //$NON-NLS-1$
+				stdoutStream.close();
+				stderrStream.close();
+				stdoutStream.reportProblems();
+				stderrStream.reportProblems();
 			}
 		} catch (Exception e) {
 			CCorePlugin.log(e);


Of course similar changed would be needed for managed builder as well.

FYI, I looked at the hyperlinks in the console a couple of months ago.  I actually started doing it but it was more work than I anticipated and got pulled off on other things.  We polled our customers and they deemed it more valuable to try to improve error parsing and the problems view than adding more helpers to the console view itself.  We're currently evaluating improvements to the problems view.
Comment 17 Nils Hagge CLA 2008-05-13 02:59:12 EDT
Same is true for me. I already started to implement hyperlinking after filing in that patch that enables colorization in September 2007. As I stated that time the colorization works well and is very much appreciated by my colleagues. 

I reached a point where I thought that I also had solved the hyperlinking, but when testing I found out that I ran in serious threading issues that require much "more work" (as Paul also says). Nevertheless, I tried to dig any deeper and found out that in "main Eclipse" there is already a console available with all these features enabled. The problem is that the CDT build console is much "older" and follows a different programming model. Thus, it is not straight forward to migrate the CDT build console the inherit from the standard console. But it my point of view this is the only way to go for the future that is sustainable. For me it does not make sense to enrich the build console with these features anymore, because in that case we would support parallel development of two consoles.

Since my bosses wont fund this issue anyway, on my opinion I say that the colorization patch is the best compromise for an intermediate solution.

For the future I prefer a migration to the existing console in Eclipse.
Comment 18 Warren Paul CLA 2008-05-13 09:44:40 EDT
(In reply to comment #17)
> Same is true for me. I already started to implement hyperlinking after filing
> in that patch that enables colorization in September 2007. As I stated that
> time the colorization works well and is very much appreciated by my colleagues. 
> 
> I reached a point where I thought that I also had solved the hyperlinking, but
> when testing I found out that I ran in serious threading issues that require
> much "more work" (as Paul also says). Nevertheless, I tried to dig any deeper
> and found out that in "main Eclipse" there is already a console available with
> all these features enabled. The problem is that the CDT build console is much
> "older" and follows a different programming model. Thus, it is not straight
> forward to migrate the CDT build console the inherit from the standard console.
> But it my point of view this is the only way to go for the future that is
> sustainable. For me it does not make sense to enrich the build console with
> these features anymore, because in that case we would support parallel
> development of two consoles.
> 
> Since my bosses wont fund this issue anyway, on my opinion I say that the
> colorization patch is the best compromise for an intermediate solution.
> 
> For the future I prefer a migration to the existing console in Eclipse.
> 

I agree that the correct approach is to use TextConsole (and friends) which already supports hyperlinking.  This is what I started but soon realized it touched a lot more than I anticipated.  But it certainly can be done.  The other part of this of course is the logic to create the hyperlinks from the console output.  Maybe we can just create a new interface that error parsers can implement if they wish to provide hyperlinks in addition to problem markers.
Comment 19 Doug Schaefer CLA 2008-05-13 16:38:49 EDT
(In reply to comment #18)
> I agree that the correct approach is to use TextConsole (and friends) which
> already supports hyperlinking.  This is what I started but soon realized it
> touched a lot more than I anticipated.  But it certainly can be done.  The
> other part of this of course is the logic to create the hyperlinks from the
> console output.  Maybe we can just create a new interface that error parsers
> can implement if they wish to provide hyperlinks in addition to problem
> markers.
> 

That would be awesome. We do have the logic in the error parsers to do this. We just need a way to hook it up.

Is this something we can put on the table for CDT 5.1?
Comment 20 Nils Hagge CLA 2008-05-14 02:51:18 EDT
> I agree that the correct approach is to use TextConsole (and friends) which
> already supports hyperlinking.  This is what I started but soon realized it
> touched a lot more than I anticipated.  But it certainly can be done.  The
> other part of this of course is the logic to create the hyperlinks from the
> console output.  Maybe we can just create a new interface that error parsers
> can implement if they wish to provide hyperlinks in addition to problem
> markers.

I am also sure that using TextConsole is definitely possible and that that is the way to go for the future. I digged into that and made the same observations as Paul, i.e. it is possible but cannot be done in one day. I already used the TextConsole successfully for non-CDT based plugins.

> That would be awesome. We do have the logic in the error parsers to do this. 
> We just need a way to hook it up.
>
> Is this something we can put on the table for CDT 5.1?

I could spend some spare time on making a proposal, in case an API extension has a chance to be accepted by the CDT folks...



Comment 21 Warren Paul CLA 2008-05-14 17:30:17 EDT
I just stumbled across the bug in our own database about adding hyperlinks to the console.  Here was my last entry after investigating in the CDT 4.0.x branch:

OK, a little harder than I thought.  First of all the way CDT error parsing is
setup, we can't really tap into hyperlinking from existing error parsers.  The
error parsers and manager are all in cdt.core, while the build console and
friends are all in ui plugins.  So you can't go directly from error parser to
console.  For one thing, there's no API for such things, and adding an API
would add ui dependencies on core.  Hmm...  Why not have the API in core so the
console can just get the list of error parsers?  That was the hope, but the
error parser manager and the list of error parsers to use are constructed by
the individual builders (std, managed, carbide, etc).  I don't see a way to do
it.

Then I thought at least we could add some basic file/path/name/line regex's
from the console itself.  But converting the BuildConsole to a TextConsole
(with links) is much more involved than it first appeared.  Not only do we need
to change BuildConsole, but also its friends like BuildConsoleViewer,
BuildConsolePage, etc..
Comment 22 Andrew Gvozdev CLA 2009-08-12 21:08:24 EDT
*** Bug 286464 has been marked as a duplicate of this bug. ***
Comment 23 Andrew Gvozdev CLA 2010-03-14 22:54:01 EDT
*** Bug 141967 has been marked as a duplicate of this bug. ***
Comment 24 Andrew Gvozdev CLA 2010-03-14 22:57:58 EDT
Bug 295625 implemented error highlighting in Build Console. Not sure where this one is going if anywhere. It has a bunch of good ideas though.
Comment 25 Arnaud Lucien CLA 2010-04-06 05:59:44 EDT
(In reply to comment #24)
> Bug 295625 implemented error highlighting in Build Console. Not sure where this
> one is going if anywhere. It has a bunch of good ideas though.

Does this bug will be corrected in CDT 7.0 (I mean the "Colorization of Build Console") ?
Comment 26 Andrew Gvozdev CLA 2010-04-06 09:31:37 EDT
(In reply to comment #25)
> Does this bug will be corrected in CDT 7.0 (I mean the "Colorization of Build
> Console") ?
This bug is about letting CDT builders making use of error output stream - which is colored differently on the console. Right now the error stream is merged to stdout and so not colored differently. Bug 295625 makes use of different kind of colorization strategy parsing error markers.

So we are not going to add error stream colorization for CDT builders and I am marking this bug as WONTFIX. On the other hand, other builders are free to use it if they use build console independently.
Comment 27 CDT Genie CLA 2013-10-14 16:22:39 EDT
*** cdt git genie on behalf of Andrew Gvozdev ***

    bug 203727: colorization of MakeBuilder console

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=6b7f3b67f1088076a2cbfb9f577ba161dbeae25b