Allow escape sequences to span multiple lines and support default fg/bg colors #137

dwnusbaum · 2018-11-16T15:54:56Z

Fixes #135.

This PR currently adds tests and fixes for 2 issues:

If an escape sequence is not closed at the end of a line, its tag cannot be closed and its style modifications will carry over to all subsequent lines.
- This PR currently closes all tags that are open at the end of a line and then reopens them on the next line so that ANSI sequences in the line can close those tags correctly. I am not sure whether this is the desired behavior, or if we should instead close any tags that are open at the end of a line and leave them closed for good.
When using a color map with a default foreground or background, the default foreground and background are not used.
- This PR handles default foreground and background settings. I am not sure how often default foreground/background colors are used, so maybe we don't care about this, but it was easy enough to add support for it.

dblock · 2018-11-16T17:38:10Z

This test seems to have passed ...

dwnusbaum · 2018-11-16T17:39:06Z

src/test/java/hudson/plugins/ansicolor/AnsiColorBuildWrapperTest.java

    }

+    @Test
+    @Ignore


@dblock The test is ignored, so it never even ran.

dwnusbaum · 2018-11-19T17:02:37Z

I am working on a fix for this which may help with JENKINS-34019 because the fix here will close open tags at the end of each line and reopen them on the next line, which seems to be the suggested approach to fix that ticket.

…ing them

src/main/java/hudson/plugins/ansicolor/AnsiAttributeElement.java

src/main/java/hudson/plugins/ansicolor/AnsiHtmlOutputStream.java

dwnusbaum · 2018-11-19T17:36:16Z

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

-        return this;
+        return openTags == nextOpenTags
+                ? this
+                : new ColorConsoleAnnotator(colorMapName, charset, nextOpenTags);


Based on my reading of the ConsoleAnnotator Javadocs, if we want to track state from line-to-line then we need to handle it by returning a new instance like this. Maybe wrong to return this instead of creating a new state when the open tags match.

I thought the suggested fix for JENKINS-34019 was just to close all outstanding tags at the end of a line, as if this were a user error, not to try to make them apply across lines.

When a style continues to the next line, the ConsoleAnnotatorFactory can insert a closing tag at the end of the line and an opening tag at the beginning of the next line. This prevents the timestamps from being affected.

Full comment. What you propose would certainly be simpler, but I'm not sure what the expected behavior is. What do you think should be the behavior for the test case I added?

I feel like it does not matter much, since it is unlikely people would have multiline formatting intentionally, so it is OK to keep the implementation simpler.

#145 shows a problem with output of terraform at the command line, and based on a screenshot of the text it looks like it might be using multiline formatting, but I'm not sure. I'll leave this as-is for now until I have time to investigate in more depth.

Confirmed via

docker run --rm hashicorp/terraform plan 2>&1 | od -a

(Though it also looks like the Terraform output is buggy—ANSI sequences are not nestable, and it is emitting several esc [ 0 m sequences in a row.)

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

…r cleanup

dwnusbaum · 2018-11-21T18:00:25Z

src/test/java/hudson/plugins/ansicolor/AnsiColorBuildWrapperTest.java

+            System.out.print(html);
+            assertThat(html.replaceAll("<span style=\"display: none\">.+?</span>", ""),
+                allOf(
+                    containsString("<b><span style=\"color: #1E90FF;\">This text should be bold and blue\n</span></b>"),


The literal \n characters in these tests would probably cause them to fail on Windows.

dwnusbaum · 2018-11-21T18:01:12Z

src/test/java/hudson/plugins/ansicolor/AnsiColorBuildWrapperTest.java

        });
    }

+    @Test


I should create a JIRA issue for these and add @Issue.

dwnusbaum · 2018-12-04T16:48:13Z

Needs to be merged with master after the change to use comments instead of display: none in #142.

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

dblock · 2018-12-16T22:46:38Z

@jglick Can I leave this to you to review/merge/comment?

jglick · 2019-01-03T22:51:37Z

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

+            try (AnsiHtmlOutputStream ansiOs = new AnsiHtmlOutputStream(outgoing, colorMap, emitter, openTags);
+                    CountingOutputStream incoming = new CountingOutputStream(ansiOs)) {
+                emitter.incoming = incoming;
+                // We need to write one UTF-16 code unit at a time so that byte offsets match Java character offsets when inserting HTML.


Needs to be resolved in favor of #143, right?

jglick

Not sure the extra complexity is worth it for a relatively uncommon use case, but maybe users would consider lack of multiline support a regression from the old implementation.

Anyway, it has test coverage, so (other than the logging buglet) go for it.

jglick · 2019-01-04T19:00:57Z

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

        this.colorMapName = colorMapName;
-        LOGGER.fine("creating annotator with colorMapName=" + colorMapName);
+        this.openTags = openTags;
+        LOGGER.log(Level.FINE, "creating annotator with colorMapName={0} openTags={2}", new Object[] { colorMapName, openTags });


{1} not {2}

jglick · 2019-01-04T19:03:06Z

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

-            AnsiColorMap colorMap = Jenkins.get().getDescriptorByType(AnsiColorBuildWrapper.DescriptorImpl.class).getColorMap(colorMapName);
+        List<AnsiAttributeElement> nextOpenTags = openTags;
+        // TODO: As a performance improvement, we could create a branch where `s.indexOf('\u001B') == -1` but other
+        // conditions are true that surrounds the text in the appropriate tags without going through AnsiHtmlOutputStream.


Unnecessary—this case is likely quite rare.

jglick · 2019-01-04T19:05:29Z

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

+                    int outCount = outgoing.getCount() + adjustment;
+                    // All ANSI escapes sequences contain at least 2 bytes on modern platforms, so any HTML emitted
+                    // directly after the first character is received is due to the initialization process of the stream and
+                    // belongs at position 0 (i.e. default background/foreground colors).


I think I need more caffeine to understand this. :-)

Yeah this code was and still is quite confusing to me. Thankfully, the if statement is covered by the newly added tests, and if it is removed the tests fail, so we should be able to avoid future regressions.

jglick · 2019-01-04T19:07:38Z

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java

+                if (colorMap.getDefaultBackground() != null || colorMap.getDefaultForeground() != null) {
+                    // The default color scheme will be opened automatically at the beginning of the stream on the next
+                    // line, so we don't want to duplicate it.
+                    nextOpenTags.remove(0);


.subList(1) might be less scary to readers, though I see that the getter does make a fresh copy.

Good point. I tried switching to sublist, but the implementation is not serializable so it would require another copy, which seems excessive, so I added a comment to help clarify for future readers but left the original code.

Reproduce jenkinsci#135 in a test

e6b8ff5

dwnusbaum requested a review from jglick November 16, 2018 15:54

dwnusbaum mentioned this pull request Nov 16, 2018

sgr0 not supported anymore #135

Closed

dwnusbaum commented Nov 16, 2018

View reviewed changes

dwnusbaum changed the title ~~Demonstrate problem with multiline escape sequences~~ Demonstrate problem with escape sequences spanning multiple lines Nov 16, 2018

Fix jenkinsci#137 by closing open tags at the end of lines and reopen…

cfa7144

…ing them

dwnusbaum commented Nov 19, 2018

View reviewed changes

dwnusbaum added 2 commits November 19, 2018 15:07

Close whole AnsiHtmlOutputStream instead of inidividual tags and mino…

a95ea81

…r cleanup

Support color maps that set default foreground/background colors

2c19b61

dwnusbaum commented Nov 21, 2018

View reviewed changes

Track character offsets correctly for multi-byte characters

a2a74eb

dwnusbaum commented Dec 4, 2018

View reviewed changes

src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java Outdated Show resolved Hide resolved

dwnusbaum mentioned this pull request Dec 4, 2018

java.lang.IndexOutOfBoundsException with v0.6.0 #139

Closed

dwnusbaum added 4 commits December 4, 2018 11:58

Merge branch 'master' into multiline-sequence-bug

7d8b58d

Handle multi-byte characters correctly instead of truncating them

c1d7aab

Add Japanese text to non-ascii test case

50893ab

Remove unused code

ce424f3

dwnusbaum mentioned this pull request Dec 4, 2018

Fix #139: Handle non-ASCII characters correctly when annotating text #143

Merged

dblock assigned jglick Dec 16, 2018

jglick reviewed Jan 3, 2019

View reviewed changes

dwnusbaum mentioned this pull request Jan 4, 2019

Control characters appearing in job output in 0.6.0 #145

Closed

dwnusbaum added 2 commits January 4, 2019 12:06

Merge branch 'master' into multiline-sequence-bug

be7df32

Remove unnecessary implementation of readResolve

31bb937

dwnusbaum changed the title ~~Demonstrate problem with escape sequences spanning multiple lines~~ Allow escape sequences to span multiple lines and support default fg/bg colors Jan 4, 2019

jglick approved these changes Jan 4, 2019

View reviewed changes

Address review feedback and add changelog entry

200bf2b

dwnusbaum merged commit 2db09bc into jenkinsci:master Jan 4, 2019

dwnusbaum deleted the multiline-sequence-bug branch January 4, 2019 21:44

dwnusbaum mentioned this pull request Feb 7, 2019

Newline produces wild results #150

Closed

basil mentioned this pull request Jun 23, 2019

[JENKINS-58102] GlobalAnnotator does not annotate lines that are part of a multi-line color sequence jenkinsci/timestamper-plugin#28

Merged

tszmytka mentioned this pull request Jun 5, 2020

Leaky formatting #190

Merged

                   }
+                  @Test
+                  @Ignore

                       });
                   }
+                  @Test

Uh oh!

Allow escape sequences to span multiple lines and support default fg/bg colors #137

Allow escape sequences to span multiple lines and support default fg/bg colors #137

Uh oh!

Conversation

dwnusbaum commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblock commented Nov 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum commented Nov 19, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum commented Dec 4, 2018

Uh oh!

Uh oh!

dblock commented Dec 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dwnusbaum commented Nov 16, 2018 •

edited

Loading

dwnusbaum Nov 19, 2018 •

edited

Loading

jglick Jan 3, 2019 •

edited

Loading