qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [PATCH v3 08/13] python/aqmp-tui: add syntax highlighting
Date: Wed, 18 Aug 2021 14:55:13 -0400



On Mon, Aug 16, 2021 at 5:20 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:


On Tue, Aug 17, 2021 at 1:14 AM John Snow <jsnow@redhat.com> wrote:


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?
Yes, this has been addressed in the v4.
We avoid logging messages when we log the messages to the TUI screen. This is done by checking if TUILogHandler is installed on the logger or not.
 
         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.
I'll add a note here. 
+
         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?
Why do you think it is trouble?
When a level is provided, it is most probably going to be text messages or QMP messages with text. In either case, there is no formatting to be done. In the second case, we don't have a JSON formattor that can format
a JSON message along with text without breaking. So I thought it is better to leave the formatting to the user and just use the level to provide syntax highlighting.


Because it's an indirect type inference -- you are *indirectly* measuring whether or not a message has been formatted yet. Using the presence or absence of 'level' is an indirect way of proving the condition; are there not more direct ways?

Talking on IRC, I see that you have plans for an abstracted Item view that might make this a null point, though -- so I'll drop this line of interrogation for now.

Thanks!


reply via email to

[Prev in Thread] Current Thread [Next in Thread]