otk/ui: introduce some sort of ui

[supakeen]

We've spoken about being nice to users before; @schuellerf is working on adding a lot of annotated data to our data structures, I've spent a morning on writing a tiny bit of UI.

The idea is:

• output goes to stdout
• ui and logging goes to stderr
• when stderr is not a tty (e.g. redirected away) we don't show a ui
• the ui is everything that isn't logging, logging is still there and will intersperse with the ui

Based on #277.

[supakeen]

Might need better locking and signal handling around the thread but I didn't encounter any issues so far. We can always move all output into a single object and have it lock.

[mvo5]

Some quick high-level thoughts:

• would be nice to unify the log/ui concepts, otherwise I see a lot of "ui.print(some-info")";log.info("some-info") in the code (also verbose probably means the same for ui/logs)
• not print if it's not a tty is unusal IME, most tools seem to not do spinners or color but not printing anything is rare (I think?)
• tests :)

[supakeen]

Some quick high-level thoughts:

• would be nice to unify the log/ui concepts, otherwise I see a lot of "ui.print(some-info")";log.info("some-info") in the code (also verbose probably means the same for ui/logs)

Yes, however; see the answer for the second point.

• not print if it's not a tty is unusal IME, most tools seem to not do spinners or color but not printing anything is rare (I think?)

Yes! So in my case the UI is meant to be not-important, only for human eyes. Normal log messages are always shown (interspersed with the UI) and any redirection turns off everything related to the UI. How do you feel?

• tests :)

Of course! (UI's always fun to test).

[schuellerf]

A real "spinner" would be nice :-D
but that's probably an overkill

diff --git a/src/otk/ui.py b/src/otk/ui.py
index 3fd3496..410438a 100644
--- a/src/otk/ui.py
+++ b/src/otk/ui.py
@@ -1,3 +1,4 @@
+import os
 import sys
 import time
 import threading
@@ -13,20 +14,36 @@ class _Spinner:
     active: bool
     thread: threading.Thread
 
+    spin_chars = '⠋⠙⠹⠸⢰⣰⣠⣄⣆⡆⠇⠏'
+
     def __init__(self, prompt: str) -> None:
         self.prompt = prompt
 
+    @classmethod
+    def is_ansi_terminal(cls):
+        term = os.getenv('TERM', '')
+        return term.startswith('xterm') or 'ansi' in term or 'vt100' in term
+
     def _loop(self):
-        sys.stderr.write(self.prompt)
+        sys.stderr.write(self.prompt + " ")
         sys.stderr.flush()
 
         # No spinner business if we aren't printing to tty's.
         if sys.stderr.isatty():
+            i = 0
+            ansi = _Spinner.is_ansi_terminal()
             while self.active:
-                sys.stderr.write(".")
+                if ansi:
+                    i = (i + 1) % len(_Spinner.spin_chars)
+                    sys.stderr.write(_Spinner.spin_chars[i] + '\x1b[D')
+                else:
+                    sys.stderr.write(".")
                 sys.stderr.flush()
                 time.sleep(0.1)
+            if ansi:
+                sys.stderr.write("\x1b[D. ")
+                sys.stderr.flush()
+
         # Only write a newline if the prompt was actually there
         if self.prompt:
             sys.stderr.write("\n")

[supakeen]

There are a lot of ways to make the UI prettier, I think the core issue for this right now is my decision to have a UI that is separate from log output and which gets disabled on any redirection. If we decide that that's an OK thing to do then I'll add some tests, we can merge, and we can iterate.

[schuellerf]

I'm fine with anything a little more verbose "for humans"

[schuellerf]

although some tests are missing, right? :-)

[mvo5]

Some quick high-level thoughts:

• would be nice to unify the log/ui concepts, otherwise I see a lot of "ui.print(some-info")";log.info("some-info") in the code (also verbose probably means the same for ui/logs)

Yes, however; see the answer for the second point.

• not print if it's not a tty is unusal IME, most tools seem to not do spinners or color but not printing anything is rare (I think?)

Yes! So in my case the UI is meant to be not-important, only for human eyes. Normal log messages are always shown (interspersed with the UI) and any redirection turns off everything related to the UI. How do you feel?
[..]

I think it is preferable to have a single way to do output, I'm not sure there is a thing as "not important output", it depends on context, when doing a bugreport usually all "log" level output is important but when running in "normal" mode it is not. So having a single way to output seems useful, otherwise we would do things like "ui.print("doing something"); logger.info("doing something") or we would have "ui.print("doing something") and no logger but then when asking for a debug log via "otk compile foo.yaml > output.log" we would miss "doing something" in the output which may (or may not) be an important clue).

Also I think building blocks that are orthogonal are important, anytime something is kinda-similar to a different concept we already have I feel uneasy. With this class we have now logging that does output and a new ui that does output. And depending on context (isatty()) some is turned on or off. And depending on verbosity level in logging some is turned on and some is turned off.

In many ways this is very similar to logging, we want to control "debug", "warn" etc and a way to increase or decrease verbosity.
So my suggestion would be to have a UI/Output/Logger class that implements some abstract methods like "Output.Print()", "Output.{Start,Stop}Spinner()" (or WithSpinner() as context manager) etc. Then on non-tty system the spinner would just do nothing. We could set verbosity level with that, we could have a multiplexer to have syslog output and tty output etc.

[edit: sorry, parts where maybe a bit terse/badly phrased, I rewrote the section about "not important output"]

[mvo5]

It looks like there are some commits in this PR that are not related to the title of the PR, is that intentional?

[supakeen]

It looks like there are some commits in this PR that are not related to the title of the PR, is that intentional?

Yes, it needs rebasing again as it was based on top of #277 which just got merged.

[schuellerf]

As an intermediate solution to the valuable input of @mvo5 - should we info/debug-log the text here, to avoid duplication and have those interesting messages also in the logs?
( could as well be the "else" of isatty()? )

[mvo5]

I was asked to re-review this but afaict nothing since my comments in #272 (comment) has changed so they still apply.

[supakeen]

Indeed nothing changed. I'll be taking this PR in the direction you mentioned in the comment.

[supakeen]

I've adjusted this PR to have a ui.Terminal that wraps a logging.Logger instance. It a spinner and motd methods right now. Spinner does nothing if not isatty().

This allows us to take control of the delegated things, expand functionality, and act as a filter and/or formatter (colors, etc) for output based on the settings of ui.Terminal or isatty(). The global logging output is still configured by the entrypoint, this might/could be moved into the ui at some point.