Skip to content

Use Rich for Printing and Robust Progress/Wait Bars#740

Merged
freakboy3742 merged 8 commits into
beeware:mainfrom
rmartin16:print-in-bar
Jun 3, 2022
Merged

Use Rich for Printing and Robust Progress/Wait Bars#740
freakboy3742 merged 8 commits into
beeware:mainfrom
rmartin16:print-in-bar

Conversation

@rmartin16

@rmartin16 rmartin16 commented May 12, 2022

Copy link
Copy Markdown
Member

These changes leverage Rich for all terminal output.

  • Since Rich is very colorful by default, nearly all effects to the text are disabled by default.
    • URLs are still highlighted by default.
  • Rich markup can be enabled for specific output with markup=True; this is true for logging as well as input prompting.
  • By default, all debug and deep debug output is now less bright than other logging.
  • The implemented Rich progress bar is largely modeled after the pip download bar.
  • The wait bar is simple but succinctly informs users of ongoing tasks and that they completed.

Note: While a progress or wait bar is ongoing, additional output can only be printed via Rich. This means, for instance, a wait bar cannot be used to wrap a subprocess that may output the terminal itself; otherwise, that process' output will awkwardly be combined with the Rich bar.


Original Post Content

A quick and dirty POC for much more robust progress and status bars. Although, a bit of bait and switch since this simply
implements Textualize/rich. As I was assessing what it would take to enhance the existing bars to allow for interspersed printing, it really started feeling like the wrong approach. And something that implemented more of a curses-like approach would be much more fitting. Although, not using an established library would definitely be a reinvention of the wheel.

@freakboy3742, if you're amenable to using rich, I can formalize this PR; please let me know your thoughts.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742

freakboy3742 commented May 13, 2022

Copy link
Copy Markdown
Member

@rmartin16 I've been meaning to take a closer look at rich, and this gave me as good a reason as any :-)

I'm OK with adding rich as a dependency. It's becoming a relatively well-accepted part of the Python ecosystem, so it's not especially controversial to add it; and the API looks relatively straightforward. However, I have some reservations about the specifics - IMHO, some of the default style choices provided by rich are have arguable UX benefits.

The progress bar is OK; I'd rather if it was trimmed rather than "full width of terminal" (similar to what pip does). This is a fundamental issue of typography and layout that publications like Bringhurst's "Elements of Typographic Style" have encouraged for a long time. Long lines aren't visually scannable; 40-75 character lines are encouraged, (with 66 generally being considered ideal).

The "wait bar" is another matter. I've historically had pretty good eyesight, but I could only barely see the "throbbing" of the icon on the waitbar. I don't know if that's the choice of icon, or the BRIGHT GREEN color that it was using, but I could hardly see that something was happening.

That leads to the other major concern I have about introducing Rich - I'm not wild about rich's Colors Everywhere approach. Colors can be very useful. When used well, they can draw attention to important details, or highlight points of concern. I'd be 100% on board with introducing colours for warnings to make them stand out, or using gray/faded colours for some of the app-level annotations (ie. the [myapp] prefix on lines) to make them not stand out. However, "lets make everything a technicolor rainbow because we can" is an easy trap to fall into.

On top of that, I'm acutely aware of the impact color choices have on people with color blindness (my son is color blind) - or even on people who have white vs black console backgrounds.

That said - I'm not opposed to adding colors or other rich-derived UI elements. With some stylistic tweaks, I'm on board. I haven't done enough investigation to see what customisation options exist; and be prepared for me to sweat the minor details as we go down this path.

@rmartin16

rmartin16 commented May 17, 2022

Copy link
Copy Markdown
Member Author

The Colors Everywhere is real here....they are definitely not being subtle :) It appears, though, to largely be possible to tell Rich to use the "default" color scheme of the terminal (albeit with some exceptions I've found).

From reviewing how Rich is structured, here is what I'm thinking; the code is still POC quality...want to agree on design first.

Leveraging Rich Markup

  • Rich provides a straightforward markup language to control presentation in the terminal.
  • To use this markup, printing to the screen must be delegated to Rich via rich.console.Console.print() (or via other abstractions).
  • Since printing has already been consolidated to console.py, a single rich.console.Console will be instantiated (as recommended by Rich) for both briefcase.Log and briefcase.Console.
  • In this way, any markup used throughout briefcase will be respected.
  • This does come with the caveat that Rich will rather aggressively try to make things prettier; for instance, any hyperlinks are colored and underlined. I set highlight=False for the rich.console.Console to avoid this.
  • Additionally, since Rich uses [tag]content[/tag] to drive all of its markup, any time you want to print a left bracket [, it needs to be escaped with a backslash....rich.markup.escape is provided to abstract this. Of note, emoji=False for Console would only be necessary to avoid :rocket: becoming 🚀.
  • I added an example of using Rich's markup to "dim" the [myapp] prefixes for the briefcase create command.

Wait/Status Bar

  • You're absolutely right; that bright green status bar I added was pretty obnoxious...I had lifted it directly from Rich's examples.
  • That said, Rich comes with a built-in demo of all "spinner" options: run python -m rich.spinner to check them out.
  • I picked what I thought might be a decent conservative option....please override with what you think best fits.
  • The spinner_style argument controls the color of the spinner; the "message" appears to use the terminal's default coloring always.
  • Code snippet to demonstrate this wait bar:
from time import sleep
from rich.console import Console as RichConsole
rich_console = RichConsole(highlight=False)
wait_bar = rich_console.status("Waiting to boot up...", spinner="bouncingBar", spinner_style="default")
with wait_bar:
    for idx in range(10):
        print(f">>> status update {idx}")
        sleep(1)

Progress Bar

  • So, the progress bar options are practically limitless....I expect personal preference to have strong influence here.
  • Rich provides an enumeration for its progress bar components.
  • Given pip was mentioned, I dug deep in to their code to model an implementation after theirs....as such:
    • Two spaces to offset from the line above it (this assumes the line above is always left aligned...pip tracks the current level of indentation to avoid this assumption)
    • Line spinner using terminal's default color to indicate "something" is happening before progress actually starts
    • Bar column with width=50 that uses Rich's default bar column color scheme....their choices do seem to have enough contrast to work on most terminals.
    • Percentage of progress using the terminal's default color scheme.
    • Finally, time remaining (which becomes time elapsed when done) using Rich's default colors for this object (the colors are hard coded for this object for some reason...).
      • I think the time remaining column is debatable since this is a generic progress bar where progression may not be relatively constant over time. Showing elapsed time would probably suit most generic cases better.
      • Of course, that also opens the door to creating multiple progress bars including a specific download progress bar that could even display megabytes per second.
  • As made somewhat evident in briefcase.commands.base with the task creation, Rich allows a single progress bar to effectively track multiple ongoing tasks. In this way, multiple bars can actually be shown when only using a single actual progress bar.
  • Code snippet below to demonstrate this bar:
from time import sleep
from rich.console import Console as RichConsole
from rich.progress import BarColumn, Progress, SpinnerColumn, TextColumn, TimeRemainingColumn
rich_console = RichConsole(highlight=False)

def main():
	progress_bar = Progress(
		"  ",
		SpinnerColumn("line", speed=1.5, style="default"),
		BarColumn(bar_width=50),
		TextColumn("{task.percentage:>3.1f}%", style="default"),
		"•",
		TimeRemainingColumn(compact=True, elapsed_when_finished=True),
		console=rich_console,
	)
	print("Downloading package...")
	total = 10
	task_id = progress_bar.add_task("task desc", total=total, start=True)
	with progress_bar:
		for idx in range(total):
			print(f">>> status update {idx}")
			progress_bar.update(task_id, advance=1)
			sleep(1)

main()

@freakboy3742

Copy link
Copy Markdown
Member

Leveraging Rich Markup

👍 This all makes sense.

  • This does come with the caveat that Rich will rather aggressively try to make things prettier; for instance, any hyperlinks are colored and underlined. I set highlight=False for the rich.console.Console to avoid this.

Ironically, the URL highlighting is the one thing I could probably live with, as it matches native HTML stylesheets, and the blue they've chosen isn't as eye-searing as some of the other colour choices. Would a custom theme that allows us to selectively turns on some of the highlighting choices be an option?

  • Additionally, since Rich uses [tag]content[/tag] to drive all of its markup, any time you want to print a left bracket [, it needs to be escaped with a backslash....rich.markup.escape is provided to abstract this. Of note, emoji=False for Console would only be necessary to avoid :rocket: becoming 🚀.
  • I added an example of using Rich's markup to "dim" the [myapp] prefixes for the briefcase create command.

Hrm. This seems... potentially explosive. Imagine if a user had an app named [foo], or used [foo] in a path - the output is going to be munged in creative ways. It sounds like we're going to need to treat any user-provided content the same as you would on a website to avoid injection attacks - a console wrapper API that separates the format string from the content, so that all content can be auto-escaped. Otherwise, we're going to spend our time playing whack-a-mole working out when the escaping process is eating content.

It also makes me wonder if we might want to abstract the [myapp] piece into a context-manager like - something that is attached to the console so that all messages issued in a "per app" context get a clean, marked up prefix.

Wait/Status Bar

  • I picked what I thought might be a decent conservative option....please override with what you think best fits.

The option you've picked looks reasonable enough.

My ideal outcome would be something that matches the graphical style of the progress bar - but that doesn't appear to exist out of the box based on that built-in demo. The rich.progress demo does have a "thinking" example... but it's also an inaccessible multicoloured yawn. Are the colours on that customisable at all?

Progress Bar

  • So, the progress bar options are practically limitless....I expect personal preference to have strong influence here.

😄

* I think the time remaining column is debatable since this is a generic progress bar where progression may not be relatively constant over time. Showing elapsed time would probably suit most generic cases better.

🤷 Sure - time remaining will be 100% A Lie (tm), but it's better than a poke in the eye with a sharp stick, and at least gives an indication of how long you have to wait.

* Of course, that also opens the door to creating multiple progress bars including a specific download progress bar that could even display megabytes per second.

MB/s is a fun metric, but not an especially actionable one. If it turns out your download is slow, it's not like you could point it at a different download source. And if the download is slow, that's going to be obvious from the "time remaining: 4 hours" indicator - MB/s won't tell you anything new.

  • As made somewhat evident in briefcase.commands.base with the task creation, Rich allows a single progress bar to effectively track multiple ongoing tasks. In this way, multiple bars can actually be shown when only using a single actual progress bar.

Again - a cute idea, but if we're in a situation where we need multiple progress bars, I think we need to revisit the UX.

  • Code snippet below to demonstrate this bar:

That looks fine. If nothing else, it's at least consistent with pip, so it won't be surprising to anyone.

@rmartin16

rmartin16 commented May 18, 2022

Copy link
Copy Markdown
Member Author

Selective default formatting:

Would a custom theme that allows us to selectively turns on some of the highlighting choices be an option?

A custom highlighter can be specified for rich.console.Console using regex's; and we can cherry pick the default regex's we want. Although, like you say, simply customizing the default sytling of the default theme is probably a better approach....bit of an allowlist versus blocklist argument in a way... (note to self: python -m rich.default_styles will be useful here)

Printing user content:

It also makes me wonder if we might want to abstract the [myapp] piece into a context-manager like - something that is attached to the console so that all messages issued in a "per app" context get a clean, marked up prefix.

Agreed; something will definitely need to accommodate this....I struggled to come up with anything quickly but I'll experiment some more.

Wait/status bar graphic:

My ideal outcome would be something that matches the graphical style of the progress bar - but that doesn't appear to exist out of the box based on that built-in demo. The rich.progress demo does have a "thinking" example... but it's also an inaccessible multicoloured yawn. Are the colours on that customisable at all?

I mocked up this example below that uses rich.progress.Progress to implement a wait bar. The color scheme is customizable with my example using black and white at 6 characters wide.

from time import sleep
from rich.console import Console as RichConsole
from rich.progress import BarColumn, Progress
rich_console = RichConsole(highlight=False)
wait_bar = Progress(
	BarColumn(bar_width=6, style="black", pulse_style="white"),
	"Waiting to boot up...",
	transient=True,
	console=rich_console,
)
wait_bar.add_task("task desc", start=False)
with wait_bar:
	for idx in range(10):
		print(f">>> status update {idx}")
		sleep(1)

@rmartin16

rmartin16 commented May 18, 2022

Copy link
Copy Markdown
Member Author

Selective default formatting Part 2:

After further considering how to limit Rich's default coloring, I think it makes more sense to create a custom highlighter class for Rich's Console. While changing the Rich's default theme settings (which are numerous) achieves the same outcome, the strategy has a much broader effect and we're really just trying to limit how our instantiated console prints things.

I reviewed Rich's default highlighting conditions and summarized them below:

  • Tags (e.g. [tag]content[/tag])
  • Attributes (e.g. <attrib var=attrib_value>content</attrib>)
  • Braces (i.e. []{}())
  • IP Addresses
  • Hardware Addresses
  • UUIDs
  • Function calls
  • Boolean values
  • Ellipsis
  • Numbers
  • Paths/Filenames
  • Strings
  • URLs

Demonstration:

from rich.console import Console
from rich.highlighter import RegexHighlighter

text="\n".join(["\[openTag] \[/closeTag]", "<attrib attrib_var=attrib_val> </attrib>", "1.1.1.1", "20e6:1994::0000", "0cf0d5e0-dd83-41b6-b51b-359a9be54ac2", "klass.method()", "True False", "sigh...", "123 1.23 1+4j", "//europa/music/awesome.mp4", "https://www.google.com/asdf", "'string' \"string\""])

class RichConsoleHighlighter(RegexHighlighter):
    base_style = "repr."
    highlights = [r"(?P<url>(file|https|http|ws|wss)://[-0-9a-zA-Z$_+!`(),.?/;:&=%#]*)"]

Console().print(text)
Console(highlighter=RichConsoleHighlighter()).print(text)

Also, by default, Rich implements soft wrapping to avoid breaking words between lines in the terminal....fyi.

@rmartin16

Copy link
Copy Markdown
Member Author

Printing [myapp] Prefixes

I, initially, played around with some helper functions to handle the formatting for [myapp].

For instance:

def rich_dim(text):
    return rich_fmt(text, "dim")


def rich_fmt(text, tag=None):
    text = escape(text)
    if tag is not None:
        text = f"[{tag}]{text}[/{tag}]"
    return text

Then you could just call rich_dim() on some text to do all this formatting. Although, in practice, this becomes a bit of a mess:

self.logger.info(f"{rich_dim(f'[{app.app_name}]')} Removing old application bundle...")

I then considered your idea about a context manager tied back to console. I added a context manager called prefix to Log that can add a message prefix to all info logging calls. Although, this ends up permeating down the stack to all logging calls.

There is also the problem of everywhere else that app.app_name (or other user content for that matter) is printed. A quick solution is to add app_name_safe to AppConfig but I'm not too keen on how that muddles the data and its presentation.

I'll get back to it tomorrow. Any additional thoughts in the mean time appreciated :)

@rmartin16 rmartin16 force-pushed the print-in-bar branch 2 times, most recently from 6cb249f to 071d7d9 Compare May 19, 2022 19:00
@rmartin16

Copy link
Copy Markdown
Member Author

OK, I think I've landed on an approach I like. My comments on "selective default formatting" above stand (i.e. use a custom highlighter for the Rich Console).

Printing [myapp] Prefixes Part 2

The crux of this approach to managing [myapp] prefixes (as well as printing user content in general) is to print everything with Rich markup turned off by default. As I reviewed the codebase, it became apparent what a major headache it would be to ensure Rich markup was handled appropriately everywhere....especially given how sparingly markup would be useful; so, I turned it off by default. (Note: disabling markup does not disable highlighting.)

To use markup, calls to Log or Console simply need to include markup=True as a kwarg. Additionally, the caller is responsible for ensuring the message text is properly escaped before making the call.

As for [myapp] prefixes....I implemented the idea of a general prefix to Log that would be "dimmer" than the message...as well as wrapped in brackets. Now, given there's already the concept of a preface, this may be considered confusing. However, I separate the two ideas like this: the preface is intended to represent the type of message while the prefix represents the context of the message. In this strategy, to prepend a [myapp] prefix, the caller adds prefix=app.app_name to the Log call.

Progress Bar

Rich's API design for a "progress display" requires users to create a progress bar and add at least one "task" to generate an on-screen progress bar. This isn't especially onerous but it does expose Rich's progress bar implementation anywhere we use a progress bar. If we wanted to avoid this, I could implement Console.progress_bar similar to how tqdm works in that it would be a wrapper on the object that's already being iterated. This is also how pip uses Rich's progress bar. Of note, though, this would completely change how the progress bar works since it would need to assume how to increment the bar; in the "file download" case, such an approach would need to assume the "length" of what's coming out of the iterator is what's driving the progress bar. Just putting this out there if you aren't too keen on the "task stuff" in base.py.

Wait Bar

On top of the changes to the wait bar I detailed previously, I also added a couple other features.

  • Remove pulsing bar when done
    • Now, the pulsing bar proceeding the message will be removed when the wait bar is exited. This is much more aesthetically pleasing to me.
    • Furthermore, the transient argument is now available to have the wait bar disappear altogether on exit.
  • Appending a done message
    • A message will be appended to the wait bar message when the wait bar has exited successfully. (Success meaning no exception was raised.)
    • This done message defaults to the word "done" currently but can be overridden for each created wait bar.

With all this said, I think that's all the scope of this PR. (For instance, I'll create another PR to implement this new wait bar for opening the iOS simulator). Once we can hash the details for these high level changes, I'll update the rest of the codebase to use it as well as fix all the tests this is gonna break.

@rmartin16 rmartin16 force-pushed the print-in-bar branch 2 times, most recently from 80989f5 to 7d4946a Compare May 19, 2022 21:13
@freakboy3742

Copy link
Copy Markdown
Member

Apologies for the delayed response; I've been knee deep in Android build system code, and wanted to give your comments the consideration they deserve.

tl;dr - I think this is converging on a really good place. It's getting lots of useful aspects of rich without going full technicolor rainbow, and the implementation isn't especially onerous.

Selective default formatting Part 2:

After further considering how to limit Rich's default coloring, I think it makes more sense to create a custom highlighter class for Rich's Console. While changing the Rich's default theme settings (which are numerous) achieves the same outcome, the strategy has a much broader effect and we're really just trying to limit how our instantiated console prints things.

I reviewed Rich's default highlighting conditions and summarized them below:

+1 to these settings. It would be worth adding a docstring in the code to summarise what we're doing (and why) so future explorers know why it's been done.

Printing [myapp] Prefixes Part 2

The crux of this approach to managing [myapp] prefixes (as well as printing user content in general) is to print everything with Rich markup turned off by default. As I reviewed the codebase, it became apparent what a major headache it would be to ensure Rich markup was handled appropriately everywhere....especially given how sparingly markup would be useful; so, I turned it off by default. (Note: disabling markup does not disable highlighting.)

To use markup, calls to Log or Console simply need to include markup=True as a kwarg. Additionally, the caller is responsible for ensuring the message text is properly escaped before making the call.

+1. This is a nice compromise. It exposes the functionality in case it might be useful, but it won't be "explosive by default".

As for [myapp] prefixes....I implemented the idea of a general prefix to Log that would be "dimmer" than the message...as well as wrapped in brackets. Now, given there's already the concept of a preface, this may be considered confusing. However, I separate the two ideas like this: the preface is intended to represent the type of message while the prefix represents the context of the message. In this strategy, to prepend a [myapp] prefix, the caller adds prefix=app.app_name to the Log call.

Makes sense. In practice, I don't think we're likely to use both a prefix and a preface, but it's good to know we've got a clear story.

Sweating the little styling details - I wonder if it would make more sense to make the default rendering color for DEBUG/DEEP_DEBUG output light gray as well. When there's a lot of debug output, it still needs to be legible, but it's less important that it's immediately legible; and it's beneficial to be able to pick out the "real" output from the debug output.

Progress Bar

Rich's API design for a "progress display" requires users to create a progress bar and add at least one "task" to generate an on-screen progress bar. This isn't especially onerous but it does expose Rich's progress bar implementation anywhere we use a progress bar. If we wanted to avoid this, I could implement Console.progress_bar similar to how tqdm works in that it would be a wrapper on the object that's already being iterated. This is also how pip uses Rich's progress bar. Of note, though, this would completely change how the progress bar works since it would need to assume how to increment the bar; in the "file download" case, such an approach would need to assume the "length" of what's coming out of the iterator is what's driving the progress bar. Just putting this out there if you aren't too keen on the "task stuff" in base.py.

I think I can live with the API exposure. It's not too onerous, and the API that is being exposed makes sense in the circumstances.

Wait Bar

These changes all look great. I quite like this new black and white wait bar; my only suggestion would be to make it a little wider (say, 40 chars, with a 5 character spacer on the left) so it's thematically consistent with the way we render progress bars like the download bar.

With all this said, I think that's all the scope of this PR. (For instance, I'll create another PR to implement this new wait bar for opening the iOS simulator). Once we can hash the details for these high level changes, I'll update the rest of the codebase to use it as well as fix all the tests this is gonna break.

Sounds like a plan. I'm wary of splitting this into too many PRs - but I'm also appreciative that monster PRs take too long to get through the whole process. At the very most, I'd want to see 1 PR for the core functionality, plus one per platform; but if we can get it in less PRs than that (1 for the core, plus 1 to roll it out everywhere), then I'd be OK with that too.

@freakboy3742

Copy link
Copy Markdown
Member

@rmartin16 FYI - I've just merged #744, which applies black to the entire codebase, so there are some merge issues to contend with. You may find git merge main --strategy-option ours helpful as an approach to perform the merge, but accept your own code if there's a discrepancy. If you need any help, yell out - I'm happy to assist.

@rmartin16 rmartin16 force-pushed the print-in-bar branch 4 times, most recently from 05bf8a5 to e7c3286 Compare May 31, 2022 22:32
@rmartin16

Copy link
Copy Markdown
Member Author

@freakboy3742,

I wonder if it would make more sense to make the default rendering color for DEBUG/DEEP_DEBUG output light gray as well

I think this is a great idea; it's perfectly legible to me while allowing me to keep my attention on info and above logging.

I quite like this new black and white wait bar; my only suggestion would be to make it a little wider (say, 40 chars, with a 5 character spacer on the left) so it's thematically consistent with the way we render progress bars like the download bar.

I'm not sure I entirely follow this; can you please describe the full layout of the wait bar you're imagining?

I think my primary question might stem from what to do with the message associated with the wait bar. If the wait bar is 40 characters wide, then putting the message to the right of the bar looks pretty awkward....but it also feels a little weird on the left side. I think you might mean for the message to be on the line above the wait bar....if so, I mocked up what that would take....its a little more sophisticated use of Rich but not too bad after enough playing around with it. And so it's clear, the use of rich.live.Live derives from a desire to keep the message actually with the pulsing wait bar in the terminal in the event of interleaved printing. Be default, RIch's progress bar doesn't support stacking things....other than multiple progress bars that is.

Additionally, I updated all uses of [app.app_name] to use the new prefix markup....and I'm experimenting with wait bars for the iOS simulator.

@rmartin16 rmartin16 force-pushed the print-in-bar branch 3 times, most recently from 9d60b03 to 6c50861 Compare May 31, 2022 22:54
@freakboy3742

Copy link
Copy Markdown
Member

I quite like this new black and white wait bar; my only suggestion would be to make it a little wider (say, 40 chars, with a 5 character spacer on the left) so it's thematically consistent with the way we render progress bars like the download bar.

I'm not sure I entirely follow this; can you please describe the full layout of the wait bar you're imagining?

It's a little hard to render in Github Markdown, but I'm thinking something like:

Starting emulator beePhone...
     #####-----#####-----#####-----#####----- Waiting for emulator to start...

That would then be comparable with the layout of progress bars:

Collecting toga-android>=0.3.0.dev18
  Downloading toga_android-0.3.0.dev34-py3-none-any.whl (41 kB)
     ######################################## 41.4/41.4 kB 752.4 kB/s eta 0:00:00

Additionally, I updated all uses of [app.app_name] to use the new prefix markup....and I'm experimenting with wait bars for the iOS simulator.
👍

@rmartin16 rmartin16 force-pushed the print-in-bar branch 6 times, most recently from 4a8f83b to 69323f7 Compare June 2, 2022 23:41
@rmartin16 rmartin16 changed the title Robust Progress and Status Bars Use Rich for Printing and Robust Progress/Wait Bars Jun 3, 2022
 - Use Rich to print all output to terminal for Rich markup support
 - Use Rich progress display to implement robust Progress and Wait bars
@rmartin16

Copy link
Copy Markdown
Member Author

This is everything for us to review in this PR. A few notes.

I updated the Wait Bar to match your layout; interested in any thoughts on the other functionality I implemented for the Wait Bar. I did discover an issue with a use-case I've been targeting for the Wait Bar, i.e. wrapping calls to subprocess. I found that if spawned process prints to the terminal, it really doesn't play nice with the Wait Bar since Rich can't ensure everything ends up on the screen properly. So, while I have asked Rich if there's some magic available here...I suspect we will need to go back to the idea of a subprocess.run_in_wait_bar method....that'll be separate, though.

Example of spawned process printing in wait bar
import subprocess
import sys
import time
from briefcase.console import Console
cmd = 'from time import sleep\n'\
'print("line 1")\n'\
'sleep(1)\n'\
'print("line 2")\n'\
'print("line 3")\n'\
'sleep(1)\n'\
'print("last line")\n'

def main():
    with Console().wait_bar("waiting..."):
        r=subprocess.run([sys.executable, "-c", cmd], check=True)
        time.sleep(3)

main()

I also hit that race condition in the tests for subprocess where stop_func is never evaluated because is_alive() returns False immediately. I re-implemented the sleep call....but differently this time to actually do something. I also have stop_func return False initially because CodeCov also complained at one point that the sleep call in stream_output wasn't tested....

@rmartin16 rmartin16 marked this pull request as ready for review June 3, 2022 00:58
@codecov

codecov Bot commented Jun 3, 2022

Copy link
Copy Markdown

Codecov Report

Merging #740 (6626a5e) into main (670c423) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
src/briefcase/commands/build.py 100.00% <ø> (ø)
src/briefcase/integrations/docker.py 92.50% <ø> (ø)
src/briefcase/commands/base.py 98.21% <100.00%> (+<0.01%) ⬆️
src/briefcase/commands/create.py 99.63% <100.00%> (ø)
src/briefcase/commands/dev.py 95.16% <100.00%> (ø)
src/briefcase/commands/package.py 85.36% <100.00%> (ø)
src/briefcase/commands/update.py 100.00% <100.00%> (ø)
src/briefcase/commands/upgrade.py 95.16% <100.00%> (ø)
src/briefcase/console.py 100.00% <100.00%> (+0.79%) ⬆️
src/briefcase/integrations/android_sdk.py 98.98% <100.00%> (-0.01%) ⬇️
... and 6 more

@freakboy3742 freakboy3742 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is awesome!

I've made a couple of small tweaks during review:

  • I've reduced the final size of the wait bar. After actually living with it, the 40 char wide bar was visually consistent with progress bars, but a little garish. Cutting it down to 20 means there's only one "pulse", but it's still in the same general style as the progress bars,
  • I've made the default style for warnings yellow, and the default style for errors red. They're sufficiently rare that you don't normally see them; but when you do, they need to stand out. I think that's a good match for the "gray on debug" style.
  • I've added iOS waitbar support. This was partially because I didn't want iOS to be left behind, partially to save you the work (since you've done so much on this already!), and partially so that I could make sure I understood how all the new stuff works. It was... disturbingly simple to implement - which is a testament to the great API you (and, I guess, Will McGuigan) have put together here.

Lastly, I've promoted this to a full feature in the release note. There's no way we're going to add this much polish and slide it under the radar as a "miscellaneous improvement" :-)

I'm really happy with where this has ended up. The style improvements are subtle but significant improvements, without being overbearing. Thanks for doing all the heavy lifting - it's much appreciated!

@freakboy3742 freakboy3742 merged commit 1e7d5b6 into beeware:main Jun 3, 2022
@rmartin16 rmartin16 deleted the print-in-bar branch June 3, 2022 15:39
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.

2 participants