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

Work with moving sequences #168

Merged
merged 11 commits into from
Feb 20, 2020

Conversation

tszmytka
Copy link
Collaborator

Currently the plugin incorrectly handles control sequences like:

  • CUU Cursor Up
  • CUD Cursor Down
  • CUF Cursor Forward
  • CUB Cursor Back
  • EL Erase in Line

Using many popular tools in a job, like docker, produces output similar to:

+ docker-compose down
Stopping container_1               ... 
Stopping container_2               ... 
Stopping container_3               ... 
�[4A�[2K
Stopping container_1               done
�[4B�[2A�[2K

This PR contains a fix - the mentioned sequences will not produce any output - as well as a test case against regression.
Merging this may very well fix #151 as the actual problem looks very similar.

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, especially for adding tests! The change seems ok to me, this is pretty much what I proposed at the end of #151 (comment).

I added one minor comment, and I think we might as well override all methods on AnsiOutputStream that are not already overridden by AnsiHtmlOutputStream at the same time to do the same thing. What do you think?

@tszmytka
Copy link
Collaborator Author

I've provided additional adjustments so closing the original conversation.

  • All AnsiOutputStream methods are overridden

  • Made them all call AnsiAttributeElement.Emitter#emitInvisibleSequence previously emitRedundantReset

  • Restructured EmitterImpl to not rely on any special values (null or otherwise) and equipped the method param with @Nonnull. Now there is no way of anyone providing "", null or anything else and triggering the special behavior.
    I really don't like that guerilla-style EmitterImpl class defined out of nowhere in the middle of a method but changing that would require a broader refactoring.

  • Added tests

@dwnusbaum what do you think?

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tszmytka The changes look great to me, thanks for adding tests for everything!

*/
default void emitRedundantReset() { }
default void emitInvisibleSequence() { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This breaks binary compatibility. We could avoid it by keeping emitRedundantReset and having it call emitInvisibleSequence internally, but it doesn't look like emitRedundantReset was ever used by any other plugins, so I think the change is fine as-is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This breaks binary compatibility.

Are there other plugins extending ansicolor-plugin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatebot plugin implements AnsiAttributeElement.Emitter (the usage looks weird, it probably shouldn't be extending ansicolor), but the plugin looks obsolete and has less than 50 installs, so I think it can be ignored.

@tszmytka
Copy link
Collaborator Author

Do I need to do anything else in order to merge this?

@dwnusbaum
Copy link
Member

@tszmytka I just merged #169, can you resolve the conflict? Once we have a passing CI build, I am happy to merge and release in the next few days (CC @dblock as a heads up).

Could you also update CHANGELOG.md in this PR with your changes?

@tszmytka
Copy link
Collaborator Author

@tszmytka I just merged #169, can you resolve the conflict? Once we have a passing CI build, I am happy to merge and release in the next few days (CC @dblock as a heads up).

Could you also update CHANGELOG.md in this PR with your changes?

@dwnusbaum All done. Thanks for the help.

@dwnusbaum dwnusbaum merged commit 9aed68a into jenkinsci:master Feb 20, 2020
@dwnusbaum
Copy link
Member

@tszmytka Thanks for fixing the bug!

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

Successfully merging this pull request may close these issues.

Erase Line (EL) control sequence not handled correctly
2 participants