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

Fix Dict limit printing of small values ending with color #45521

Merged
merged 10 commits into from
Aug 30, 2022

Conversation

Liozou
Copy link
Member

@Liozou Liozou commented May 30, 2022

The recent #37568 added handling of colors in Dict printing with :limit => true in the IOContext. There is a lingering bug when the last character of the printed value is colored, which results in an additional character being printed and color-bleeding, for example in the following picture:
bugsmall

This is due to IgnoreAnsiIterator skipping the last characters of the value (since they are ANSI end delimiters), then seeing that the entire line has not been printed, and thus wrongly assuming that the limit size has been reached.

With this PR:
fixsmall

@Liozou Liozou added the display and printing Aesthetics and correctness of printed representations of objects. label May 30, 2022
@Liozou
Copy link
Member Author

Liozou commented May 30, 2022

This PR does not handle the remaining color-bleeding problem if the value printed does actually reach the limit size
bugbig

@Liozou Liozou marked this pull request as draft May 31, 2022 08:30
@KristofferC
Copy link
Member

Did you mark this as draft because you intend to also fix the issue noted in the last comment?

@Liozou
Copy link
Member Author

Liozou commented Jun 1, 2022

Yes and I think the Windows CI error might be legit because I don't see it on other builds. I'll try to look at it this weekend.

Regarding the issue in the last comment, I can think of a fix consisting in recording the encountered ANSI sequences directly in the IgnoreAnsiIterator and printing the corresponding end delimiters if they were not printed before and we hit the limit size. I need to think on how to do that efficiently, but it seems doable.

@Liozou
Copy link
Member Author

Liozou commented Jun 6, 2022

The diff ends up a bit larger than I thought but to summarize:

  • the text_colors const is moved from util.jl to show.jl to make it available earlier, but the definition is not changed.
  • all cases of color bleeding in Dict limit printing should now be handled. Specifically:
    • if the input string contains the adequate ANSI end delimiters at the position of truncation, those are used.
    • otherwise, the appropriate end delimiters are added before the truncating characters.
  • performance-wise, there should be no difference on strings without ANSI delimiters, and the additional overhead is a Dict lookup on each ANSI delimiters so it should be minimal.

Examples:
smallbig
bigsmall

@Liozou Liozou marked this pull request as ready for review June 6, 2022 09:56
@KristofferC
Copy link
Member

What's the reason this became so complicated? It seems that naively you just want to write the text but only count characters, when the total number of characters you want is reached, you turn all attributes off and print the three dots, similar to the first implementation that had some off by one or something.

@Liozou
Copy link
Member Author

Liozou commented Jun 6, 2022

You're right, it was probably too complicated to maintain. My goal was to modify the string as little as possible, hence only adding a "\033[0m" ANSI sequence when strictly necessary, which meant not adding it when all ANSI start delimiters had their corresponding end delimiters... But let's just simplify it all. In the new commit 596ab8a I just append "\033[0m" if we encountered an ANSI delimiter in the string and the last encountered delimiter was not "\033[0m" itself.

As an aside, the previous implementation did not handle characters with width larger than 1 correctly in some corner cases (d10 in the tests would print a 16-long line where the limit is 15), so that is fixed as well. And this implementation avoids matching the regex against the entire string, which could be wasteful if it is very long.

@KristofferC
Copy link
Member

This doesn't seem 100% correct still?

gnome-shell-screenshot-583m03

The alignment on the second row is off?

@Liozou Liozou force-pushed the dictsmallcolorvalues branch from 596ab8a to 485723e Compare August 25, 2022 10:06
@Liozou Liozou force-pushed the dictsmallcolorvalues branch from 5f9f311 to 1ad167f Compare August 26, 2022 23:40
@Liozou
Copy link
Member Author

Liozou commented Aug 27, 2022

Thanks for noticing! There was a rpad call down the file which messed up the alignment since it does not take ANSI delimiters into account. I integrated the rpad logic to _truncate_at_width_or_chars to fix that (and incidentally tweaked the optional arguments arguments of that function).
I reworked the PR around the new ANSIIterator which iterates over the input string but groups the sequence of ANSI delimiter characters into a new ANSIDelimiter type. That way, it's easy to handle differently a regular Char from an ANSIDelimiter and I think everything is clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants