Turn on line wrapping in usage and status messages, adds ANSI color to doctor and analysis messages. by gspencergoog · Pull Request #22656 · flutter/flutter

gspencergoog · 2018-10-04T06:28:46Z

This turns on text wrapping for usage messages and status messages. When on a terminal, wraps to the width of the terminal. When writing to a non-terminal, wrap lines at a default column width (currently defined to be 100 chars). If --no-wrap is specified, then no wrapping occurs. If --wrap-column is specified, wraps to that column (if --wrap is on).

Adds ANSI color to the doctor and analysis output on terminals. This is in this PR with the wrapping, since wrapping needs to know how to count visible characters in the presence of ANSI sequences. (This is just one more step towards re-implementing all of Curses for Flutter. :-)) Will not print ANSI sequences when sent to a non-terminal, or of --no-color is specified.

Fixes ANSI color and bold sequences so that they can be combined (bold, colored text), and a small bug in indentation calculation for wrapping.

Since wrapping is now turned on, also removed many redundant '\n's in the code.

Some examples of how things look now (in a narrow terminal):

flutter analyze:

flutter doctor -v:

gspencergoog · 2018-10-05T02:28:27Z

One thing to think about for the review: Would it be better to just do no wrapping at all if not outputting to a terminal? Right now it wraps at 100 chars in that case.

xster

LGTM

xster · 2018-10-05T01:29:40Z

I think we can add a test for this one too.

xster · 2018-10-05T01:30:07Z

Document the params in the dartdoc

xster · 2018-10-05T01:30:22Z

xster · 2018-10-05T19:43:01Z

dartdocs for these. I couldn't figure out what they are from name.

xster · 2018-10-05T19:45:50Z

No, I want to remove them all, since the next line adds a reset that would cancel them all, or be redundant anyhow.

I meant swapping *m to +m. I assume you want 1 or more rather than 0 or more.

Oh, I see. Yeah, definitely, although it's probably moot, since ^[[m isn't a valid ANSI escape (or commonly typed sequence!). Done.

xster · 2018-10-05T19:46:14Z

Maybe describe a bit what issues this solves? Since there are some risks associated with the regex

xster · 2018-10-05T19:47:41Z

Why is the above public and this is not? Seems like they should be known to the user to the same extent.

Yeah, I guess so. It kind of seemed like an implementation detail, but I suppose you might want to know it to take it into account somewhere. Done.

xster · 2018-10-05T19:49:08Z

Just make it clear about whether it's in addition or instead of indent.

Good point. Done.

xster · 2018-10-05T19:55:46Z

Why not the same with hangingIndent?

Because I forgot to add it when I added hangingIndent. Fixed.

xster · 2018-10-05T19:58:58Z

some indent issues :D

xster · 2018-10-06T00:07:37Z

Still LGTM

… color to doctor and analysis messages. (flutter#22656)" This reverts commit e438632 because it breaks 160 benchmarks, and several devicelab tests, due to changing the format of the output.

… color to doctor and analysis messages. (#22656)" (#22759) This reverts commit e438632 because it breaks 160 benchmarks, and several devicelab tests, due to changing the format of the output.

…color to doctor and analysis messages. (flutter#22656) This turns on text wrapping for usage messages and status messages. When on a terminal, wraps to the width of the terminal. When writing to a non-terminal, wrap lines at a default column width (currently defined to be 100 chars). If --no-wrap is specified, then no wrapping occurs. If --wrap-column is specified, wraps to that column (if --wrap is on). Adds ANSI color to the doctor and analysis output on terminals. This is in this PR with the wrapping, since wrapping needs to know how to count visible characters in the presence of ANSI sequences. (This is just one more step towards re-implementing all of Curses for Flutter. :-)) Will not print ANSI sequences when sent to a non-terminal, or of --no-color is specified. Fixes ANSI color and bold sequences so that they can be combined (bold, colored text), and a small bug in indentation calculation for wrapping. Since wrapping is now turned on, also removed many redundant '\n's in the code.

This attempts to re-land #22656. There are two changes from the original: I turned off wrapping completely when not sending output to a terminal. Previously I had defaulted to wrapping at and arbitrary 100 chars in that case, just to keep long messages from being too long, but that turns out the be a bad idea because there are tests that are relying on the specific form of the output. It's also pretty arbitrary, and mostly people sending output to a non-terminal will want unwrapped text. I found a better way to terminate ANSI color/bold sequences, so that they can be embedded within each other without needed quite as complex a dance with removing redundant sequences. As part of these changes, I removed the Logger.supportsColor setter so that the one source of truth for color support is in AnsiTerminal.supportsColor. * Turn on line wrapping again in usage and status messages, adds ANSI color to doctor and analysis messages. (#22656) This turns on text wrapping for usage messages and status messages. When on a terminal, wraps to the width of the terminal. When writing to a non-terminal, wrap lines at a default column width (currently defined to be 100 chars). If --no-wrap is specified, then no wrapping occurs. If --wrap-column is specified, wraps to that column (if --wrap is on). Adds ANSI color to the doctor and analysis output on terminals. This is in this PR with the wrapping, since wrapping needs to know how to count visible characters in the presence of ANSI sequences. (This is just one more step towards re-implementing all of Curses for Flutter. :-)) Will not print ANSI sequences when sent to a non-terminal, or of --no-color is specified. Fixes ANSI color and bold sequences so that they can be combined (bold, colored text), and a small bug in indentation calculation for wrapping. Since wrapping is now turned on, also removed many redundant '\n's in the code.

rmacnak-google · 2018-11-01T20:37:12Z

Can we disable this when printing command lines in verbose mode? The line breaks mean I can no longer just copy and paste the command, and the commands I'm interested in running manually are usually very long and tedious to fix up.

gspencergoog · 2018-11-01T20:40:01Z

I'm not sure which "command lines" you're referring to, but you can always add --no-wrap to turn it off, or pipe it to cat.

rmacnak-google · 2018-11-01T20:46:52Z

I mean the "executing: long/path/gen_snapshot --many --options" etc. --no-wrap works for me, thanks!

googlebot added the cla: yes label Oct 4, 2018

gspencergoog changed the title ~~Turn on line wrapping in usage and status messages, adds ANSI color to doctor messages.~~ Turn on line wrapping in usage and status messages, adds ANSI color to doctor and analysis messages. Oct 4, 2018

gspencergoog added the work in progress; do not review label Oct 4, 2018

gspencergoog force-pushed the wrap_usage branch 7 times, most recently from bc80da0 to 0136b41 Compare October 5, 2018 00:54

gspencergoog requested a review from xster October 5, 2018 00:56

gspencergoog removed the work in progress; do not review label Oct 5, 2018

gspencergoog force-pushed the wrap_usage branch from 0136b41 to 6f09f0e Compare October 5, 2018 02:26

xster reviewed Oct 5, 2018

View reviewed changes

gspencergoog added 6 commits October 5, 2018 16:44

Turn on wrapping for help output of flutter command.

04c741a

Tests broken, but with progress

beff31e

Rebase on master, clean up more wrapping

7838919

Don't count ANSI sequences as characters when wrapping.

c201030

Add outputPreferences, colorize analysis output

d5982f5

Fix wrapText interface.

cda4c21

gspencergoog force-pushed the wrap_usage branch 3 times, most recently from bbb8b57 to 5cf50c3 Compare October 5, 2018 23:55

Review Changes

37944e2

gspencergoog force-pushed the wrap_usage branch from 5cf50c3 to 37944e2 Compare October 6, 2018 00:12

gspencergoog merged commit e438632 into flutter:master Oct 6, 2018

gspencergoog mentioned this pull request Oct 8, 2018

Re-land text wrapping/color PR #22831

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Aug 9, 2021

Conversation

gspencergoog commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Oct 5, 2018

Uh oh!

xster 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

xster commented Oct 6, 2018

Uh oh!

rmacnak-google commented Nov 1, 2018

Uh oh!

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

Uh oh!

Uh oh!

rmacnak-google commented Nov 1, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gspencergoog commented Oct 4, 2018 •

edited

Loading

gspencergoog commented Nov 1, 2018 •

edited

Loading