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

[John Snow <jsnow@redhat.com>]

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

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!

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