Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to define a default background color as part of a color map #24

Closed
sschuberth opened this issue Feb 21, 2014 · 38 comments
Closed
Labels
feature request ✨ New feature to be added wontfix

Comments

@sschuberth
Copy link
Member

It would be nice if one could define a (named) default background color as part of a color map, e.g. "Black". In fact, one would probably need to be able to distinguish between the "dark" and "bright" flavors of a color. So as a first step, the according rows in the color map should be labeled "dark" / "bright" (or "low" / "high" or something), and then the background color could be chosen from a combo-box that contains "Black (dark)", "Black (bright)", "Red (dark)", "Red (bright)" and so on.

@dblock
Copy link
Member

dblock commented May 24, 2014

What would that look like? I mean Jenkins displays text whichever way it likes, it looks like newer versions are all on a black background. Then any colored ANSI sequence is translated directly but this plugin. How would setting a background color affect that?

@sschuberth
Copy link
Member Author

In the vanilla Jenkins 1.564 I'm running I see no black background for the console. I only saw it on Jenkins instances that have Doony installed (in fact, this is where my idea for the black console background came from).

For people who do not like to install Doony for whatever reason (for example, because it does too much of customization) it would be great if this plugin could just "clear" the console to a specified color before the output starts. That is, it should change the default background color if none is specified as an ANSI sequence, so that regular ASCII console output would look like this screenshot (maybe except that "anonymous" would not be red).

Thinking about it, this would also require to be able to change the default foreground color. Otherwise, if you choose to clear the console background to black, you would get black text on black background :-) So maybe a less generic but simpler option would be an "invert default colors" options, which makes the default text white on black instead of black on white?

@ejelly
Copy link
Collaborator

ejelly commented May 26, 2014

I recently made a change to support the ANSI escape codes which return to the "default" fg/bg color, this change is not in a release yet. Right now it just closes color change tags opened by the plugin, returning to whatever the colors were before. If that is a desired feature, then I see no problem in adding a feature which opens color change tags to custom features before any logging output, making "set default fg/bg" return to that.

What do you think, @dblock ?

@dblock
Copy link
Member

dblock commented May 26, 2014

The more the features the better, right? I think setting the default fg/bg color would be a great addition. We can go color-crazy, check out this project's issues :)

@ejelly
Copy link
Collaborator

ejelly commented May 26, 2014

I agree :) I'll look into it!

@ejelly
Copy link
Collaborator

ejelly commented Jan 25, 2015

Finally got to it. #41 contains a solution. It's per job as opposed to per colormap, but is there any convincing reason to have it per colormap?

@sschuberth
Copy link
Member Author

IIRC my motivation to request this to be per colormap was consistency. It seemed to make more sense to me to define all colors in the same place, and in a way that applies to all jobs that use a specific colormap. For example, I for sure would like all of my jobs to use black as the default background color and white as the default foregound color. It would be tedious if I would need to go through all job configs for this instead of just changing the colormap that of all of the jobs use.

@sschuberth
Copy link
Member Author

Also, I just read about issue #39. I realize that the issue is not about adding another color preset, but if it was, and if default bg and fg color were defined as part of the preset, one could fake the visual appearance of a Powershell by setting the default bg color to #012456 and the default fg color to #EEEDF0 as part of the Powershell preset.

@ejelly
Copy link
Collaborator

ejelly commented Jan 25, 2015

@dblock, do you agree? I can easily change it to be a part of the colormap instead.

@dblock
Copy link
Member

dblock commented Jan 25, 2015

I think I do.

@ejelly
Copy link
Collaborator

ejelly commented Jan 25, 2015

In the end I think you're right, it does make more sense as part of the colormap.

@ejelly
Copy link
Collaborator

ejelly commented Jan 26, 2015

@sschuberth, #41 was just merged into master. Can you try it out?

@sschuberth
Copy link
Member Author

I've compiled the plugin and installed it into a public Jenkins instance. Unfortunately, it does not seem to fully work, see https://dscho.cloudapp.net/job/sdk-create-packages/67/console. I've used the xterm colormap and changed the default bg / fg colors to black / white. This is what I see:

  • The black bg color does only seem to apply to a few lines in the beginning.
  • Black is not actually black, but dark gray.

EDIT: Interestingly, after refreshing the console page in the browser, all of the bg is actually black (expect the very first line containing the first timestamp). I was watching the page as output was written to the console, and when doing so, the bg was still white at first.

@ejelly
Copy link
Collaborator

ejelly commented Jan 26, 2015

Not reproducible in my setup, I see the black (or rather dark gray) background throughout the whole console output, and it also seems to be fully enclosed by the div. Can you tell us what browser you are using and attach a screenshot?
As for the color: that's actually what the xterm color map defines as black background color. You could change the black to a darker color or select another colormap.
Note that issue #16 is still open, meaning that same colors are not supported (yet, I plan to implement the full xterm-256color map), but that should not really matter for the default background color in this case, it will rather be a nuisance with high intensity foreground colors that the output tries to switch to.

@sschuberth
Copy link
Member Author

I checked with current FF and Chrome versions. Maybe it's an issue in conjunction with the Timestamper plugin? If I look at the source of the page linked in my previous comment I see <span class="timestamp"><b>10:10:06</b> </span><div style="background-color: #4C4C4C;color: #E5E5E5;">, so the first timestamp indeed comes before setting the bg color:

screenshot-dscho cloudapp net 2015-01-26 22-54-33

And maybe that plugin is also responsible for the other effect I'm seeing with the real-time updates of the console.

@sschuberth
Copy link
Member Author

Here's how it looks like for me when looking at the console while the build is still running:

zwischenablage-1

Same thing with the timestamper disabled for this job:

zwischenablage-2

@sschuberth
Copy link
Member Author

As for the color: that's actually what the xterm color map defines as black background color.

Hmm, the xterm colormap defines black as #000000 / #4C4C4C for the normal / bright intensities, respectively. If I choose the bg color to be black, it should take the normal intensity of whatever I have defined as black, not the light intensity as it currently does.

EDIT: Maybe the confusion is because of the variable naming in the source code, see https://github.com/dblock/jenkins-ansicolor-plugin/pull/41/files#diff-4a291b2a40133436f6148d6091f93966R11. I believe the XTermFg and XTermBg variables need to be swapped: The normal intensity colors should be XTermBg, and the bright ones XTermFg. Same for all other colormaps. Or am I mistaken?

@ejelly
Copy link
Collaborator

ejelly commented Jan 26, 2015

Very strange with the few colored lines. I've only tested with Safari so far, where it appears fine, visually and sourcecode-wise. Will try with Chrome and/or Firefox.

As for the different color values: fg is "foreground" and bg is "background", nothing strictly to do with intensities. I do not know where the default color maps have been pulled from, @dblock will know, as this is actually his projects and I'm just a contributor.

Regarding bright intensity: Where various "real" terminals, like VGA adapters, use bright intensity, we turn on a bold typeface instead (I think the ANSI attribute is even actually called "bold"). So unless I missed something else, it behaves correctly, but allowing the common terminal behavior of allowing a bright intensity color map instead of a bold typeface is something we have thought about. #16 is sort of related to that, which is why I mentioned it.

Thanks for testing this, by the way! Hugely appreciated. Since the patch influences the document structure and also changes some long-standing defaults (vga and gnome-terminal have default background colors now), I want it to get some exposure before it is released and pulled in with updates.

@sschuberth
Copy link
Member Author

fg is "foreground" and bg is "background", nothing strictly to do with intensities.

Correct. This is one of the issues I've addressed as part of PR #42, see https://github.com/sschuberth/jenkins-ansicolor-plugin/commit/2ec4bf9b4c690e3b4a472ad9e3d5528b9ac607b4.

Where various "real" terminals, like VGA adapters, use bright intensity, we turn on a bold typeface instead

Yeah, but my point is that "real" VGA adapters cannot use bright colors as backgrounds, as only 3 bits are reserved for the background color index. See https://github.com/sschuberth/jenkins-ansicolor-plugin/commit/8b04bcbd9fa15df15f82c6b7d51e4f7b3f424bc3 and the link in its commit message.

With my changes, the console output now looks like at https://dscho.cloudapp.net/job/sdk-create-packages/72/console (same settings as before).

@ejelly
Copy link
Collaborator

ejelly commented Jan 27, 2015

You are right, using the bright color map for background colors was a bug and #42 addresses it. Please also see the comment I left there.

EDIT: Interestingly, after refreshing the console page in the browser, all of the bg is actually black (expect the very first line containing the first timestamp). I was watching the page as output was written to the console, and when doing so, the bg was still white at first.

It seems that browsers don't really like the dangling, open div during output, which is only closed after the output is finished. I will see what other options there are (unfortunately we are very limited in what jenkins plugins can do there).

@sschuberth
Copy link
Member Author

It seems that browsers don't really like the dangling, open div during output

Hmm, how about not wrapping the console output in a div for which the bg color is set, but setting the bg color of the whole page using some css that the plugin inserts to the page?

@ejelly
Copy link
Collaborator

ejelly commented Jan 27, 2015

Hmm, how about not wrapping the console output in a div for which the bg color is set, but setting the bg color of the whole paging using some css that the plugin inserts to the page?

Good idea, no idea how/if that's possible through the Jenkins plugin API though. I'll see what I can find.

@ejelly
Copy link
Collaborator

ejelly commented Feb 2, 2015

Turns out that for some reason, during running output Jenkins wraps every chunk of output in individual <pre> elements, additionally to a <pre> that surrounds the whole output. I haven't understood yet why it does that, and it's bound to break our formatting in general (it just became obvious with this change) while the job is still running. A stopped job doesn't exhibit that problem.

One hacky solution would be to close all elements after each write (or each eol) in the LineTransformationOutputStream and reopen the elements at the start of the next call. I am not particularly happy with that and am browsing the plugin API for other ideas, but once again that API seems very limited with respect to console output.

Actually, maybe it's more sensible to get Jenkins to just stop double-wrapping the running output in <pre> elements.

@ejelly
Copy link
Collaborator

ejelly commented Feb 2, 2015

Actually, maybe it's more sensible to get Jenkins to just stop double-wrapping the running output in <pre> elements.

This seems to be done by Jenkins to work around an old IE bug. The fun never ends. I'll shop around for a workaround that's less hostile to what our plugin does, and am also harvesting the faint hope that the affected IE version might be old that Jenkins is not supporting it anymore (but I really would not count on that).

@sschuberth
Copy link
Member Author

I'm not sure how the Jenkins API works, but wouldn't it be possible to watch the output for the opening outer <pre class="console-output"> tag, and replace it with something like <pre class="console-output" style="background:#000000;color:#AAAAAA">?

@ejelly
Copy link
Collaborator

ejelly commented Feb 2, 2015

Unfortunately, I don't think that's possible. But see for yourself if you like, BuildWrapper is a good starting point for looking at the API. Honestly, sometimes I feel like I'm missing something tremendous in the API.

But while what you suggest would be neat for setting the default color only once, it wouldn't solve the general problem: As soon as the lifetime of some set attribute crosses one of those nested pre block boundaries, the HTML becomes essentially broken. I think it was like that forever, it was just hard to notice without obvious default colors.

@ejelly
Copy link
Collaborator

ejelly commented Feb 3, 2015

I made a new pull request, #48, which should fix that problem.

But attention, @dblock, do not make a new release at this time! While developing the workaround, I noticed that because #42 renames some configuration fields, the required fields are now missing and cause NullPointerExceptions for existing configurations.

I want to keep @sschuberth's renamed field names because they make sense, but we absolutely must put some code in place that migrates old configurations, otherwise updating users will have a broken Jenkins configurations!

@sschuberth
Copy link
Member Author

because #42 renames some configuration fields, the required fields are now missing and cause NullPointerExceptions for existing configurations.

Sorry for that. While I did expect that renaming fields would require some users to adjust their config, I did not think it would cause any NPEs.

@dblock
Copy link
Member

dblock commented Feb 3, 2015

Now that you know that #42 caused an issue, do open a bug.

@ejelly
Copy link
Collaborator

ejelly commented Feb 3, 2015

Sorry for that. While I did expect that renaming fields would require some users to adjust their config, I did not think it would cause any NPEs.

Didn't think of it either. Just goes to show us that we need to test changes on volunteer before pushing them out to everyone.

Do we have some beta testers for pre-releases? I've since become an OS developer and neither use Jenkins nor am directly working around people who use it, otherwise I would persuade them to be our guinea pigs.

@dblock
Copy link
Member

dblock commented Feb 3, 2015

I am happy to beta test in our production jenkins, will need instructions on how (you should write a doc :)).

@ejelly
Copy link
Collaborator

ejelly commented Feb 11, 2015

Currently swamped at work, will take care of all once I have a free moment and energy again. Possibly next weekend or later.

@sschuberth
Copy link
Member Author

Now that you know that #42 caused an issue, do open a bug.

FYI, so far I've been unable to reproduce the NPE.

@ejelly
Copy link
Collaborator

ejelly commented Mar 5, 2015

This really needs some attention, but my available time has gotten worse, not better. Let's check in 2 weeks.

@ejelly
Copy link
Collaborator

ejelly commented Apr 28, 2015

Two weeks have come and passed, and I still haven't found the time, but I'm not giving up yet 8) This should only need minimal changes now and would be a valuable addition, after all. I have a configuration at home that exhibits the NPE, useful for testing.

@kenichi-shibata
Copy link

any update on this one?

@dblock
Copy link
Member

dblock commented Jul 14, 2016

@fr-kshibata please feel free to contribute

@stale
Copy link

stale bot commented May 28, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 28, 2021
@stale stale bot closed this as completed Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request ✨ New feature to be added wontfix
Projects
None yet
Development

No branches or pull requests

4 participants