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

Another issue with timestamper #275

Open
hcldan opened this issue Jan 9, 2024 · 3 comments
Open

Another issue with timestamper #275

hcldan opened this issue Jan 9, 2024 · 3 comments
Labels

Comments

@hcldan
Copy link

hcldan commented Jan 9, 2024

Jenkins and plugins versions report

Environment
Paste the output here

What Operating System are you using (both controller, and any agents involved in the problem)?

Ubuntu

Reproduction steps

  1. Timestamper enabled globally for all pipeline jobs
  2. AnsiColor 1.0.3
  3. Run a build and get really weird looking output.

Expected Results

Expect the console color blocks to have an uninterrupted display for each line.

Actual Results

image

image

The div is rendering as block forcing a new line after the timestamp span.

Anything else?

No response

Are you interested in contributing a fix?

No response

@hcldan hcldan added the bug label Jan 9, 2024
@chill389cc
Copy link

chill389cc commented May 29, 2024

I'd like to bump this. It occurs on the vga and gnome-terminal modes because those modes set defaultFg and defaultBg to non-null values, which creates a wrapping <div> tag around the line. Since the timestamp is a <span> which gets added before ansiColor's div, the div gets bumped to a new line because <span>'s are naturally inline but <div>'s are not. Here is the line of code causing this:

if (defaultFg != null || defaultBg != null) {
openTag(new AnsiAttributeElement(AnsiAttrType.DEFAULT, "div", "style=\"" +
(defaultBg != null ? "background-color: " + colorMap.getNormal(defaultBg) + ";" : "") +
(defaultFg != null ? "color: " + colorMap.getNormal(defaultFg) + ";" : "") + "\""));
}

We could just change the <div> to a <span> there but it doesn't look quite as clean with the background colors, there are gaps between each line that have the overall pages background color so its not as pretty as it looks with the <div>'s. However, I'd still prefer a solution that didn't use <div>'s so that these themes didn't conflict with other plugins.

You can get the same appearance by adding display: inline-block; to the style tag of the outer spans (what are currently divs). However, I'm seeing one issue where the closing </span> tag gets put on the next line of text which prevents the next line of text from appearing with a carriage return. Instead, it appears immediately after the preceeding line. I'm not explaining that very well, but I'll attach screenshots of what I mean.

This is what I get if I use <span>'s and add display: inline-block:
image

This is what it should look like:
image

This is the code I've edited to get to what the first screenshot looks like:

if (defaultFg != null || defaultBg != null) {
-    openTag(new AnsiAttributeElement(AnsiAttrType.DEFAULT, "div", "style=\"" +
+    openTag(new AnsiAttributeElement(AnsiAttrType.DEFAULT, "span", "style=\"" +
+        "display: inline-block;" +
        (defaultBg != null ? "background-color: " + colorMap.getNormal(defaultBg) + ";" : "") +
        (defaultFg != null ? "color: " + colorMap.getNormal(defaultFg) + ";" : "") + "\""));
}

@dblock
Copy link
Member

dblock commented May 30, 2024

@chill389cc Want to try writing some tests and PRing your change?

@chill389cc
Copy link

@dblock I understand that creating a failing unit test outlining the problem I identified in my previous post would be the most helpful way to fix the problem. Unfortunately I don't have as much time to look at this as I'd like. I've created a branch and I've pushed up my WIP code that I showed above. I've tried to make a unit test for this problem but I haven't pushed anything. I think the problem appears in the annotating of multiple consecutive lines, so it isn't as simple as creating a test like assertThat(annotate(<something multi-line>, AnsiColorMap.VGA), is(<not what it should>). If I get a chance in the future I'll poke around more but that's what I've got for now. Thanks for your work on this plugin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants