-
Notifications
You must be signed in to change notification settings - Fork 29.7k
refactor the flutter run detailed help message #14651
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
Conversation
|
Looking over that help text, I may update the above to remove the leading |
| printStatus("- 't': dump the rendering tree of the app (debugDumpRenderTree)"); | ||
| if (isRunningDebug) | ||
| printStatus("- 'L': dump the layer tree (debugDumpLayerTree)"); | ||
| printStatus("- 'S': dump accessibility in geometric order; 'U' for inverse hit test order (debugDumpSemantics)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably put these on two lines at this point. That would let you be less terse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| printStatus("- 'L': dump the layer tree (debugDumpLayerTree)"); | ||
| printStatus("- 'S': dump accessibility in geometric order; 'U' for inverse hit test order (debugDumpSemantics)"); | ||
| printStatus(''); | ||
| printStatus('Flutter toggles and other:'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that title is going to look ugly in release builds, where there's only one command here. :-)
| if (isRunningDebug) | ||
| printStatus("- 'p': toggle the display of construction lines (debugPaintSizeEnabled)"); | ||
| if (isRunningDebug) | ||
| printStatus("- 'o': simulate different operating systems, (defaultTargetPlatform)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraneous comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
|
Can you add tests that verify the exact output for release, profile, and debug? Or at least dump the output here so we can look at it. |
|
Updated based on feedback, and reviewing the --debug, --profile, and --release help text: --debug: --profile: --release: |
|
What about instead of ? |
|
"On device UI" feels weird, maybe "Inspectors"? |
|
Rather than "dump accessibility" maybe "dump accessibility tree" for consistency with the others. |
|
If the only thing we show in release builds is the line saying how to get the help, then maybe replace that with just "Development tools are not built into release builds, so no commands are available. For hot reload, inspectors, and other development tools, consider using a debug build." |
|
I'd probably merge "Flutter framework toggles" into the "On device UI" section. You know, in general maybe the section headers aren't worth it. Maybe omit them entirely? |
|
The 'd' command would be interesting to show too I think |
|
We should definitely show everything. What is 'd'? :-) |
|
I think it detaches without killing the client process |
|
Updated!
And:
I'd like to keep them - I think the help text here could use some structure, at least in --debug mode, given how many there are. The --debug mode has the most options, and is the mode users will be running in the most, so optimizing the UI for that mode makes sense. There are other changes we could do here, but I'd like to same additional work for future PRs. |
| printStatus('Framework toggles and utilities:'); | ||
| if (isRunningDebug) { | ||
| printStatus( | ||
| "- 'p': toggle the display of construction lines (debugPaintSizeEnabled)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's avoid wrapping here, to keep the code consistent (makes the reader less likely to wonder why some are different than others)
| printStatus( | ||
| "- 'o': simulate different operating systems (defaultTargetPlatform)"); | ||
| } | ||
| if (flutterDevices.any((FlutterDevice d) => d.device.supportsScreenshot)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we not need this check?
| printStatus("- 'w': dump the widget hierarchy of the app (debugDumpApp)"); | ||
| printStatus("- 't': dump the rendering tree of the app (debugDumpRenderTree)"); | ||
| if (isRunningDebug) | ||
| printStatus("- 'L': dump the layer tree (debugDumpLayerTree)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as i can tell, this works fine in non-debug mode too
| if (details) | ||
| printStatus('Reloading:'); | ||
| printStatus( | ||
| "$fire To hot reload your app on the fly, press 'r'; to restart the app entirely, press 'R'.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want this message even if details is false. We found in UX studies that it was critical.
| if (details) { | ||
| printHelpDetails(); | ||
| printStatus('To repeat this help message, press "h". To quit, press "q".'); | ||
| printStatus("To repeat this help message, press 'h'; to quit, press 'q'."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use the same format for 'h', 'q', and 'd' as we do for everything else, in details mode.
|
Can you add some tests that check what the output is in each of these cases? Such tests will make it much easier for future reviewers to review changes to this code.
|
Sounds good; I don't have the cycles to address right now. I'll close this PR in the interest of clearing up the review queue, and file an issues to track improvements to the help message. |
The 'h' help message from flutter run has gotten pretty long. I'm looking to add one more item to it, so want to take the opportunity to re-structure the output a bit before hand.
current short-form (unchanged):
previous long-form help:
updated long-form help: