Re: [PATCH v3 08/13] python/aqmp-tui: add syntax highlighting

From: John Snow <jsnow@redhat.com>
To: G S Niteesh Babu <niteesh.gs@gmail.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	Kashyap Chamarthy <kchamart@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Wainer Moschetta <wainersm@redhat.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Cleber Rosa <crosa@redhat.com>, Eric Blake <eblake@redhat.com>
Subject: Re: [PATCH v3 08/13] python/aqmp-tui: add syntax highlighting
Date: Mon, 16 Aug 2021 15:44:25 -0400	[thread overview]
Message-ID: <CAFn=p-ZR0BF_G05rn07tDiBWOQZNYhOb091-Dgs+JnXrG1OjFQ@mail.gmail.com> (raw)
In-Reply-To: <20210730201846.5147-9-niteesh.gs@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 6326 bytes --]

On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> Add syntax highlighting for the incoming and outgoing QMP messages.
> This is achieved using the pygments module which was added in a
> previous commit.
>
> The current implementation is a really simple one which doesn't
> allow for any configuration. In future this has to be improved
> to allow for easier theme config using an external config of
> some sort.
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
>  python/qemu/aqmp/aqmp_tui.py | 52 +++++++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
> index ab9ada793a..0d5ec62cb7 100644
> --- a/python/qemu/aqmp/aqmp_tui.py
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -19,6 +19,8 @@
>      Union,
>  )
>
> +from pygments import lexers
> +from pygments import token as Token
>  import urwid
>  import urwid_readline
>
> @@ -35,6 +37,22 @@
>  LOGGER = logging.getLogger()
>
>
> +palette = [
> +    (Token.Punctuation, '', '', '', 'h15,bold', 'g7'),
> +    (Token.Text, '', '', '', '', 'g7'),
> +    (Token.Name.Tag, '', '', '', 'bold,#f88', 'g7'),
> +    (Token.Literal.Number.Integer, '', '', '', '#fa0', 'g7'),
> +    (Token.Literal.String.Double, '', '', '', '#6f6', 'g7'),
> +    (Token.Keyword.Constant, '', '', '', '#6af', 'g7'),
> +    ('DEBUG', '', '', '', '#ddf', 'g7'),
> +    ('INFO', '', '', '', 'g100', 'g7'),
> +    ('WARNING', '', '', '', '#ff6', 'g7'),
> +    ('ERROR', '', '', '', '#a00', 'g7'),
> +    ('CRITICAL', '', '', '', '#a00', 'g7'),
> +    ('background', '', 'black', '', '', 'g7'),
> +]
> +
> +
>  def format_json(msg: str) -> str:
>      """
>      Formats given multiline JSON message into a single line message.
> @@ -57,17 +75,14 @@ def __init__(self, address: Union[str, Tuple[str,
> int]]) -> None:
>          self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
>          super().__init__()
>
> -    def add_to_history(self, msg: str) -> None:
> -        urwid.emit_signal(self, UPDATE_MSG, msg)
> +    def add_to_history(self, msg: str, level: Optional[str] = None) ->
> None:
> +        urwid.emit_signal(self, UPDATE_MSG, msg, level)
>
>      def _cb_outbound(self, msg: Message) -> Message:
>          # FIXME: I think the ideal way to omit these messages during
> in-TUI
> -        # logging will be to add a filter to the logger. We can use regex
> to
> -        # filter out messages starting with 'Request:' or 'Response:' but
> I
> -        # think a better approach will be encapsulate the message in an
> object
> -        # and filter based on the object. Encapsulation of the message
> will
> -        # also be necessary when we want different formatting of messages
> -        # inside TUI.
> +        # logging will be to add a filter to the logger. We can use
> +        # regex/startswith to filter out messages starting with
> 'Request:' or
> +        # 'Response:'. If possible please suggest other ideas.
>

We'll want to fix this up earlier in the series -- the finished version of
your series shouldn't have FIXME comments, and later patches in the series
shouldn't edit comments added earlier. We chatted on IRC about the right
strategy for filtering log messages, so I think V4 should have a strategy
in place to avoid this comment, right?

>          handler = LOGGER.handlers[0]
>          if not isinstance(handler, TUILogHandler):
>              LOGGER.debug('Request: %s', str(msg))
> @@ -156,6 +171,9 @@ def _get_formatted_address(self) -> str:
>              self._set_status('Server shutdown')
>
>      def run(self, debug: bool = False) -> None:
> +        screen = urwid.raw_display.Screen()
> +        screen.set_terminal_properties(256)
>

I'm not sure if this will work everywhere, but ... it's fine for now, I
assume.

> +
>          self.aloop = asyncio.get_event_loop()
>          self.aloop.set_debug(debug)
>
> @@ -167,6 +185,8 @@ def run(self, debug: bool = False) -> None:
>          event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>          main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
> 'background'),
>                                     unhandled_input=self.unhandled_input,
> +                                   screen=screen,
> +                                   palette=palette,
>                                     handle_mouse=True,
>                                     event_loop=event_loop)
>
> @@ -251,7 +271,8 @@ def __init__(self, master: App) -> None:
>          self.history = urwid.SimpleFocusListWalker([])
>          super().__init__(self.history)
>
> -    def add_to_history(self, history: str) -> None:
> +    def add_to_history(self,
> +                       history: Union[str, List[Tuple[str, str]]]) ->
> None:
>          self.history.append(urwid.Text(history))
>          if self.history:
>              self.history.set_focus(len(self.history) - 1)
> @@ -271,8 +292,15 @@ def __init__(self, master: App) -> None:
>          super().__init__(self.body)
>          urwid.connect_signal(self.master, UPDATE_MSG,
> self.cb_add_to_history)
>
> -    def cb_add_to_history(self, msg: str) -> None:
> -        self.history.add_to_history(msg)
> +    def cb_add_to_history(self, msg: str, level: Optional[str] = None) ->
> None:
> +        formatted = []
> +        if level:
> +            formatted.append((level, msg))
> +        else:
> +            lexer = lexers.JsonLexer()  # pylint: disable=no-member
> +            for token in lexer.get_tokens(msg):
> +                formatted.append(token)
> +        self.history.add_to_history(formatted)
>

This looks like it's conflating having a debugging level with receiving a
message that's already formatted.

i.e., the code here assumes that if it receives a level, it is ALSO already
formatted. That smells like trouble -- but I think there are some changes
to the logging and rendering you had planned for V4 anyway, I believe?

>
>
>  class Window(urwid.Frame):
> @@ -298,7 +326,7 @@ def emit(self, record: LogRecord) -> None:
>          level = record.levelname
>          msg = record.getMessage()
>          msg = f'[{level}]: {msg}'
> -        self.tui.add_to_history(msg)
> +        self.tui.add_to_history(msg, level)
>
>
>  def main() -> None:
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 8692 bytes --]