On Mon, Aug 16, 2021 at 5:20 PM Niteesh G. S. <niteesh...@gmail.com> wrote:
>
>
> On Tue, Aug 17, 2021 at 1:14 AM John Snow <js...@redhat.com> wrote:
>
>>
>>
>> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh...@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...@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!
