[PATCH v2 0/6] python: AQMP-TUI Prototype

[PATCH v2 0/6] python: AQMP-TUI Prototype

[G S Niteesh Babu]

GitLab: https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/
Based-on: <20210701041313.1696009-1-jsnow@redhat.com>
     [PATCH 00/20] python: introduce Asynchronous QMP package

Updates in V2:
1) Moved loop related initialization to 'run' function in 'App' class
2) Added a module logger with support in TUI log messages.
3) Corrected usage of logging.info and logging.debug
4) Added an option in setup.cfg to silent pylint regarding duplicate-code
4) Modified the arguments list to the TUI

NOTE: I am not able to get the pipelines running after the v2 changes.
I was only able to test the changes locally using *make check*.

This patch series introduces AQMP-TUI prototype. This prototype has been
helpfull in letting us try out different ideas and giving some insights
into things that we had to take care of in the upcoming TUI. It was also
helpfull in finding out bugs in the AQMP library.

The intent for this patch series is to get comments on the architectural
design of the prototype. These comments will lay down the foundation for
the upcoming TUI.

G S Niteesh Babu (6):
  python: disable pylint errors for aqmp-tui
  python: Add dependencies for AQMP TUI
  python/aqmp-tui: Add AQMP TUI draft
  python: add optional pygments dependency
  python/aqmp-tui: add syntax highlighting
  python: add entry point for aqmp-tui

 python/Pipfile.lock          |  20 ++
 python/qemu/aqmp/aqmp_tui.py | 342 +++++++++++++++++++++++++++++++++++
 python/setup.cfg             |  36 +++-
 3 files changed, 397 insertions(+), 1 deletion(-)
 create mode 100644 python/qemu/aqmp/aqmp_tui.py

-- 
2.17.1

[PATCH v2 1/6] python: disable pylint errors for aqmp-tui

[G S Niteesh Babu]

Disable missing-docstring and fixme pylint warnings.
This is because since the AQMP is just a prototype
it is currently not documented properly and lot
of todo and fixme's are still in place.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/setup.cfg | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/python/setup.cfg b/python/setup.cfg
index bce8807702..1a552d672a 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -89,6 +89,8 @@ ignore_missing_imports = True
 # no Warning level messages displayed, use "--disable=all --enable=classes
 # --disable=W".
 disable=too-many-function-args,  # mypy handles this with less false positives.
+        missing-docstring, # FIXME
+        fixme, # FIXME
 
 [pylint.basic]
 # Good variable names which should always be accepted, separated by a comma.
-- 
2.17.1

[PATCH v2 2/6] python: Add dependencies for AQMP TUI

[G S Niteesh Babu]

Added dependencies for the upcoming AQMP TUI under the optional
'tui' group.

The same dependencies have also been added under the devel group
since no work around has been found for optional groups to imply
other optional groups.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/Pipfile.lock | 12 ++++++++++++
 python/setup.cfg    |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 8ab41a3f60..76cf1e4930 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -289,6 +289,18 @@
             "markers": "python_version < '3.8'",
             "version": "==3.10.0.0"
         },
+        "urwid": {
+            "hashes": [
+                "sha256:588bee9c1cb208d0906a9f73c613d2bd32c3ed3702012f51efe318a3f2127eae"
+            ],
+            "version": "==2.1.2"
+        },
+        "urwid-readline": {
+            "hashes": [
+                "sha256:018020cbc864bb5ed87be17dc26b069eae2755cb29f3a9c569aac3bded1efaf4"
+            ],
+            "version": "==0.13"
+        },
         "virtualenv": {
             "hashes": [
                 "sha256:14fdf849f80dbb29a4eb6caa9875d476ee2a5cf76a5f5415fa2f1606010ab467",
diff --git a/python/setup.cfg b/python/setup.cfg
index 1a552d672a..c62803bffc 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -44,11 +44,18 @@ devel =
     mypy >= 0.770
     pylint >= 2.8.0
     tox >= 3.18.0
+    urwid >= 2.1.2
+    urwid-readline >= 0.13
 
 # Provides qom-fuse functionality
 fuse =
     fusepy >= 2.0.4
 
+# AQMP TUI dependencies
+tui =
+    urwid >= 2.1.2
+    urwid-readline >= 0.13
+
 [options.entry_points]
 console_scripts =
     qom = qemu.qmp.qom:main
-- 
2.17.1

[PATCH v2 3/6] python/aqmp-tui: Add AQMP TUI draft

[G S Niteesh Babu]

Added a draft of AQMP TUI.

Implements the follwing basic features:
1) Command transmission/reception.
2) Shows events asynchronously.
3) Shows server status in the bottom status bar.

Also added necessary pylint, mypy configurations

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/qemu/aqmp/aqmp_tui.py | 332 +++++++++++++++++++++++++++++++++++
 python/setup.cfg             |  21 ++-
 2 files changed, 352 insertions(+), 1 deletion(-)
 create mode 100644 python/qemu/aqmp/aqmp_tui.py

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
new file mode 100644
index 0000000000..f853efc1f5
--- /dev/null
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -0,0 +1,332 @@
+# Copyright (c) 2021
+#
+# Authors:
+#  Niteesh Babu G S <niteesh.gs@gmail.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+import argparse
+import asyncio
+import logging
+from logging import Handler
+import signal
+
+import urwid
+import urwid_readline
+
+from .error import MultiException
+from .protocol import ConnectError
+from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
+from .util import create_task, pretty_traceback
+
+
+UPDATE_MSG = 'UPDATE_MSG'
+
+# Using root logger to enable all loggers under qemu and asyncio
+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'),
+    ('background', '', 'black', '', '', 'g7'),
+]
+
+
+class StatusBar(urwid.Text):
+    """
+    A simple Text widget that currently only shows connection status.
+    """
+    def __init__(self, text=''):
+        super().__init__(text, align='right')
+
+
+class Editor(urwid_readline.ReadlineEdit):
+    """
+    Support urwid_readline features along with
+    history support which lacks in urwid_readline
+    """
+    def __init__(self, master):
+        super().__init__(caption='> ', multiline=True)
+        self.master = master
+        self.history = []
+        self.last_index = -1
+        self.show_history = False
+
+    def keypress(self, size, key):
+        # TODO: Add some logic for down key and clean up logic if possible.
+        # Returning None means the key has been handled by this widget
+        # which otherwise is propogated to the parent widget to be
+        # handled
+        msg = self.get_edit_text()
+        if key == 'up' and not msg:
+            # Show the history when 'up arrow' is pressed with no input text.
+            # NOTE: The show_history logic is necessary because in 'multiline'
+            # mode (which we use) 'up arrow' is used to move between lines.
+            self.show_history = True
+            last_msg = self.history[self.last_index] if self.history else ''
+            self.set_edit_text(last_msg)
+            self.edit_pos = len(last_msg)
+            self.last_index += 1
+        elif key == 'up' and self.show_history:
+            if self.last_index < len(self.history):
+                self.set_edit_text(self.history[self.last_index])
+                self.edit_pos = len(self.history[self.last_index])
+                self.last_index += 1
+        elif key == 'meta enter':
+            # When using multiline, enter inserts a new line into the editor
+            # send the input to the server on alt + enter
+            self.master.cb_send_to_server(msg)
+            self.history.insert(0, msg)
+            self.set_edit_text('')
+            self.last_index = 0
+            self.show_history = False
+        else:
+            self.show_history = False
+            self.last_index = 0
+            return super().keypress(size, key)
+        return None
+
+
+class EditorWidget(urwid.Filler):
+    """
+    Wraps CustomEdit
+    """
+    def __init__(self, master):
+        super().__init__(Editor(master), valign='top')
+
+
+class HistoryBox(urwid.ListBox):
+    """
+    Shows all the QMP message transmitted/received
+    """
+    def __init__(self, master):
+        self.master = master
+        self.history = urwid.SimpleFocusListWalker([])
+        super().__init__(self.history)
+
+    def add_to_history(self, history):
+        self.history.append(urwid.Text(history))
+        if self.history:
+            self.history.set_focus(len(self.history) - 1)
+
+
+class HistoryWindow(urwid.Frame):
+    """
+    Composes the HistoryBox and EditorWidget
+    """
+    def __init__(self, master):
+        self.master = master
+        self.editor = EditorWidget(master)
+        self.editor_widget = urwid.LineBox(self.editor)
+        self.history = HistoryBox(master)
+        self.body = urwid.Pile([('weight', 80, self.history),
+                                ('weight', 20, self.editor_widget)])
+        super().__init__(self.body)
+        urwid.connect_signal(self.master, UPDATE_MSG, self.cb_add_to_history)
+
+    def cb_add_to_history(self, msg, level=None):
+        formatted = []
+        if level:
+            msg = f'[{level}]: {msg}'
+            formatted.append(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)
+
+
+class Window(urwid.Frame):
+    """
+    This is going to be the main window that is going to compose other
+    windows. In this stage it is unnecesssary but will be necessary in
+    future when we will have multiple windows and want to the switch between
+    them and display overlays
+    """
+    def __init__(self, master):
+        self.master = master
+        footer = StatusBar()
+        body = HistoryWindow(master)
+        super().__init__(body, footer=footer)
+
+
+class App(QMP):
+    def __init__(self, address):
+        urwid.register_signal(type(self), UPDATE_MSG)
+        self.window = Window(self)
+        self.address = address
+        self.aloop = None
+        self.loop = None
+        super().__init__()
+
+    def add_to_history(self, msg, level=None):
+        urwid.emit_signal(self, UPDATE_MSG, msg, level)
+
+    def _cb_outbound(self, msg):
+        LOGGER.debug('Request: %s', str(msg))
+        self.add_to_history('<-- ' + str(msg))
+        return msg
+
+    def _cb_inbound(self, msg):
+        LOGGER.debug('Response: %s', str(msg))
+        self.add_to_history('--> ' + str(msg))
+        return msg
+
+    async def wait_for_events(self):
+        async for event in self.events:
+            self.handle_event(event)
+
+    async def _send_to_server(self, msg):
+        # TODO: Handle more validation errors (eg: ValueError)
+        try:
+            await self._raw(bytes(msg, 'utf-8'))
+        except ExecuteError:
+            LOGGER.info('Error response from server for msg: %s', msg)
+        except ExecInterruptedError:
+            LOGGER.info('Error server disconnected before reply')
+            # FIXME: Handle this better
+            # Show the disconnected message in the history window
+            urwid.emit_signal(self, UPDATE_MSG,
+                              '{"error": "Server disconnected before reply"}')
+            self.window.footer.set_text("Server disconnected")
+        except Exception as err:
+            LOGGER.error('Exception from _send_to_server: %s', str(err))
+            raise err
+
+    def cb_send_to_server(self, msg):
+        create_task(self._send_to_server(msg))
+
+    def unhandled_input(self, key):
+        if key == 'esc':
+            self.kill_app()
+
+    def kill_app(self):
+        # TODO: Work on the disconnect logic
+        create_task(self._kill_app())
+
+    async def _kill_app(self):
+        # It is ok to call disconnect even in disconnect state
+        try:
+            await self.disconnect()
+            LOGGER.debug('Disconnect finished. Exiting app')
+        except MultiException as err:
+            LOGGER.info('Multiple exception on disconnect: %s', str(err))
+            # Let the app crash after providing a proper stack trace
+            raise err
+        raise urwid.ExitMainLoop()
+
+    def handle_event(self, event):
+        # FIXME: Consider all states present in qapi/run-state.json
+        if event['event'] == 'SHUTDOWN':
+            self.window.footer.set_text('Server shutdown')
+
+    async def connect_server(self):
+        try:
+            await self.connect(self.address)
+            self.window.footer.set_text("Connected to {:s}".format(
+                f"{self.address[0]}:{self.address[1]}"
+                if isinstance(self.address, tuple)
+                else self.address
+            ))
+        except ConnectError as err:
+            LOGGER.debug('Cannot connect to server %s', str(err))
+            self.window.footer.set_text('Server shutdown')
+
+    def run(self, debug=False):
+        self.screen.set_terminal_properties(256)
+
+        self.aloop = asyncio.get_event_loop()
+        self.aloop.set_debug(debug)
+
+        # Gracefully handle SIGTERM and SIGINT signals
+        cancel_signals = [signal.SIGTERM, signal.SIGINT]
+        for sig in cancel_signals:
+            self.aloop.add_signal_handler(sig, self.kill_app)
+
+        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
+        self.loop = urwid.MainLoop(self.window,
+                                   unhandled_input=self.unhandled_input,
+                                   handle_mouse=True,
+                                   event_loop=event_loop)
+
+        create_task(self.wait_for_events(), self.aloop)
+        create_task(self.connect_server(), self.aloop)
+        try:
+            self.loop.run()
+        except Exception as err:
+            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
+            raise err
+
+
+class TUILogHandler(Handler):
+    def __init__(self, tui):
+        super().__init__()
+        self.tui = tui
+
+    def emit(self, record):
+        level = record.levelname
+        msg = record.getMessage()
+        self.tui.add_to_history(msg, level)
+
+
+def parse_address(address):
+    """
+    This snippet was taken from qemu.qmp.__init__.
+    pylint complaints about duplicate code so it has been
+    temprorily disabled. This should be fixed once qmp is
+    replaced by aqmp in the future.
+    """
+    components = address.split(':')
+    if len(components) == 2:
+        try:
+            port = int(components[1])
+        except ValueError:
+            raise ValueError(f'Bad Port value in {address}') from None
+        return (components[0], port)
+    return address
+
+
+def main():
+    parser = argparse.ArgumentParser(description='AQMP TUI')
+    parser.add_argument('qmp_server', help='Address of the QMP server'
+                        '< UNIX socket path | TCP addr:port >')
+    parser.add_argument('--log-file', help='The Log file name')
+    parser.add_argument('--log-level', help='Log level <debug|info|error>',
+                        default='debug')
+    parser.add_argument('--debug', action='store_true',
+                        help='Enable debug mode for asyncio loop'
+                        'Generates lot of output, makes TUI unusable when'
+                        'logs are logged in the TUI itself.'
+                        'Use only when logging to a file')
+    args = parser.parse_args()
+
+    try:
+        address = parse_address(args.qmp_server)
+    except ValueError as err:
+        parser.error(err)
+
+    app = App(address)
+
+    if args.log_file:
+        LOGGER.addHandler(logging.FileHandler(args.log_file))
+    else:
+        LOGGER.addHandler(TUILogHandler(app))
+
+    log_levels = {'debug': logging.DEBUG,
+                  'info': logging.INFO,
+                  'error': logging.ERROR}
+
+    if args.log_level not in log_levels:
+        parser.error('Invalid log level')
+    LOGGER.setLevel(log_levels[args.log_level])
+
+    app.run(args.debug)
+
+
+if __name__ == '__main__':
+    main()  # type: ignore
diff --git a/python/setup.cfg b/python/setup.cfg
index c62803bffc..7e5aae66c7 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -81,8 +81,22 @@ namespace_packages = True
 # fusepy has no type stubs:
 allow_subclassing_any = True
 
+[mypy-qemu.aqmp.aqmp_tui]
+disallow_untyped_defs = False
+disallow_incomplete_defs = False
+check_untyped_defs = False
+# urwid and urwid_readline have no type stubs:
+allow_subclassing_any = True
+
+# The following missing import directives are because these libraries do not
+# provide type stubs. Allow them on an as-needed basis for mypy.
 [mypy-fuse]
-# fusepy has no type stubs:
+ignore_missing_imports = True
+
+[mypy-urwid]
+ignore_missing_imports = True
+
+[mypy-urwid_readline]
 ignore_missing_imports = True
 
 [pylint.messages control]
@@ -97,6 +111,11 @@ ignore_missing_imports = True
 # --disable=W".
 disable=too-many-function-args,  # mypy handles this with less false positives.
         missing-docstring, # FIXME
+        # pylint complaints about duplicate code in aqmp_tui. The snippet is
+        # taken from qemu/qmp and there is no way to turn this check of in
+        # module itself. So this has been added here and has to be removed
+        # in the future when qmp is removed.
+        duplicate-code, # FIXME
         fixme, # FIXME
 
 [pylint.basic]
-- 
2.17.1

Re: [PATCH v2 3/6] python/aqmp-tui: Add AQMP TUI draft

[John Snow]

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

On Tue, Jul 13, 2021 at 6:07 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> Added a draft of AQMP TUI.
>
> Implements the follwing basic features:
> 1) Command transmission/reception.
> 2) Shows events asynchronously.
> 3) Shows server status in the bottom status bar.
>
> Also added necessary pylint, mypy configurations
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
>  python/qemu/aqmp/aqmp_tui.py | 332 +++++++++++++++++++++++++++++++++++
>  python/setup.cfg             |  21 ++-
>  2 files changed, 352 insertions(+), 1 deletion(-)
>  create mode 100644 python/qemu/aqmp/aqmp_tui.py
>
> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
> new file mode 100644
> index 0000000000..f853efc1f5
> --- /dev/null
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -0,0 +1,332 @@
> +# Copyright (c) 2021
> +#
> +# Authors:
> +#  Niteesh Babu G S <niteesh.gs@gmail.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +import argparse
> +import asyncio
> +import logging
> +from logging import Handler
> +import signal
> +
> +import urwid
> +import urwid_readline
> +
> +from .error import MultiException
> +from .protocol import ConnectError
> +from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
> +from .util import create_task, pretty_traceback
> +
> +
> +UPDATE_MSG = 'UPDATE_MSG'
> +
> +# Using root logger to enable all loggers under qemu and asyncio
> +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'),
> +    ('background', '', 'black', '', '', 'g7'),
> +]
> +
>

It looks like this bled forward, this part belongs in the next patch. Can
you fix this and re-send?

jsnow@scv ~/s/q/python (review)> make check-dev
ACTIVATE .dev-venv
make[1]: Entering directory '/home/jsnow/src/qemu/python'
JOB ID     : f766a463cfc6bd3f0d6286e0653752bb8bc5ea6f
JOB LOG    :
/home/jsnow/avocado/job-results/job-2021-07-20T13.55-f766a46/job.log
 (1/4) tests/flake8.sh: FAIL: Exited with status: '1' (0.36 s)
 (2/4) tests/isort.sh: PASS (0.11 s)
 (3/4) tests/mypy.sh: FAIL: Exited with status: '1' (0.36 s)
 (4/4) tests/pylint.sh: FAIL: Exited with status: '2' (6.62 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 3 | SKIP 0 | WARN 0 | INTERRUPT 0 |
CANCEL 0
JOB TIME   : 7.80 s
Log file "stdout" content for test "1-tests/flake8.sh" (FAIL):
qemu/aqmp/aqmp_tui.py:30:6: F821 undefined name 'Token'
qemu/aqmp/aqmp_tui.py:31:6: F821 undefined name 'Token'
qemu/aqmp/aqmp_tui.py:32:6: F821 undefined name 'Token'
qemu/aqmp/aqmp_tui.py:33:6: F821 undefined name 'Token'
qemu/aqmp/aqmp_tui.py:34:6: F821 undefined name 'Token'
qemu/aqmp/aqmp_tui.py:35:6: F821 undefined name 'Token'
qemu/aqmp/aqmp_tui.py:138:21: F821 undefined name 'lexers'

While you're at it, you might as well rebase on top of AQMP v2.

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

Re: [PATCH v2 3/6] python/aqmp-tui: Add AQMP TUI draft

[Niteesh G. S.]

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

On Tue, Jul 20, 2021 at 11:27 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On Tue, Jul 13, 2021 at 6:07 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> Added a draft of AQMP TUI.
>>
>> Implements the follwing basic features:
>> 1) Command transmission/reception.
>> 2) Shows events asynchronously.
>> 3) Shows server status in the bottom status bar.
>>
>> Also added necessary pylint, mypy configurations
>>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> ---
>>  python/qemu/aqmp/aqmp_tui.py | 332 +++++++++++++++++++++++++++++++++++
>>  python/setup.cfg             |  21 ++-
>>  2 files changed, 352 insertions(+), 1 deletion(-)
>>  create mode 100644 python/qemu/aqmp/aqmp_tui.py
>>
>> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
>> new file mode 100644
>> index 0000000000..f853efc1f5
>> --- /dev/null
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -0,0 +1,332 @@
>> +# Copyright (c) 2021
>> +#
>> +# Authors:
>> +#  Niteesh Babu G S <niteesh.gs@gmail.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later.  See the COPYING file in the top-level directory.
>> +
>> +import argparse
>> +import asyncio
>> +import logging
>> +from logging import Handler
>> +import signal
>> +
>> +import urwid
>> +import urwid_readline
>> +
>> +from .error import MultiException
>> +from .protocol import ConnectError
>> +from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
>> +from .util import create_task, pretty_traceback
>> +
>> +
>> +UPDATE_MSG = 'UPDATE_MSG'
>> +
>> +# Using root logger to enable all loggers under qemu and asyncio
>> +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'),
>> +    ('background', '', 'black', '', '', 'g7'),
>> +]
>> +
>>
>
> It looks like this bled forward, this part belongs in the next patch. Can
> you fix this and re-send?
>
> jsnow@scv ~/s/q/python (review)> make check-dev
> ACTIVATE .dev-venv
> make[1]: Entering directory '/home/jsnow/src/qemu/python'
> JOB ID     : f766a463cfc6bd3f0d6286e0653752bb8bc5ea6f
> JOB LOG    :
> /home/jsnow/avocado/job-results/job-2021-07-20T13.55-f766a46/job.log
>  (1/4) tests/flake8.sh: FAIL: Exited with status: '1' (0.36 s)
>  (2/4) tests/isort.sh: PASS (0.11 s)
>  (3/4) tests/mypy.sh: FAIL: Exited with status: '1' (0.36 s)
>  (4/4) tests/pylint.sh: FAIL: Exited with status: '2' (6.62 s)
> RESULTS    : PASS 1 | ERROR 0 | FAIL 3 | SKIP 0 | WARN 0 | INTERRUPT 0 |
> CANCEL 0
> JOB TIME   : 7.80 s
> Log file "stdout" content for test "1-tests/flake8.sh" (FAIL):
> qemu/aqmp/aqmp_tui.py:30:6: F821 undefined name 'Token'
> qemu/aqmp/aqmp_tui.py:31:6: F821 undefined name 'Token'
> qemu/aqmp/aqmp_tui.py:32:6: F821 undefined name 'Token'
> qemu/aqmp/aqmp_tui.py:33:6: F821 undefined name 'Token'
> qemu/aqmp/aqmp_tui.py:34:6: F821 undefined name 'Token'
> qemu/aqmp/aqmp_tui.py:35:6: F821 undefined name 'Token'
> qemu/aqmp/aqmp_tui.py:138:21: F821 undefined name 'lexers'
>
> While you're at it, you might as well rebase on top of AQMP v2.
>
Ah sorry, messed up while rebasing. I'll send a v3 fixing this(and other
upcoming comments)
and will also rebase on top of AQMP v2.

>
>

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

Re: [PATCH v2 3/6] python/aqmp-tui: Add AQMP TUI draft

[John Snow]

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

On Tue, Jul 13, 2021 at 6:07 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> Added a draft of AQMP TUI.
>
> Implements the follwing basic features:
> 1) Command transmission/reception.
> 2) Shows events asynchronously.
> 3) Shows server status in the bottom status bar.
>
> Also added necessary pylint, mypy configurations
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
>  python/qemu/aqmp/aqmp_tui.py | 332 +++++++++++++++++++++++++++++++++++
>  python/setup.cfg             |  21 ++-
>  2 files changed, 352 insertions(+), 1 deletion(-)
>  create mode 100644 python/qemu/aqmp/aqmp_tui.py
>
> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
> new file mode 100644
> index 0000000000..f853efc1f5
> --- /dev/null
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -0,0 +1,332 @@
> +# Copyright (c) 2021
> +#
> +# Authors:
> +#  Niteesh Babu G S <niteesh.gs@gmail.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +import argparse
> +import asyncio
> +import logging
> +from logging import Handler
> +import signal
> +
> +import urwid
> +import urwid_readline
> +
> +from .error import MultiException
> +from .protocol import ConnectError
> +from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
> +from .util import create_task, pretty_traceback
> +
> +
> +UPDATE_MSG = 'UPDATE_MSG'
> +
> +# Using root logger to enable all loggers under qemu and asyncio
> +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'),
> +    ('background', '', 'black', '', '', 'g7'),
> +]
> +
> +
> +class StatusBar(urwid.Text):
> +    """
> +    A simple Text widget that currently only shows connection status.
> +    """
> +    def __init__(self, text=''):
> +        super().__init__(text, align='right')
> +
> +
> +class Editor(urwid_readline.ReadlineEdit):
> +    """
> +    Support urwid_readline features along with
> +    history support which lacks in urwid_readline
> +    """
> +    def __init__(self, master):
> +        super().__init__(caption='> ', multiline=True)
> +        self.master = master
> +        self.history = []
> +        self.last_index = -1
> +        self.show_history = False
> +
> +    def keypress(self, size, key):
> +        # TODO: Add some logic for down key and clean up logic if
> possible.
> +        # Returning None means the key has been handled by this widget
> +        # which otherwise is propogated to the parent widget to be
> +        # handled
> +        msg = self.get_edit_text()
> +        if key == 'up' and not msg:
> +            # Show the history when 'up arrow' is pressed with no input
> text.
> +            # NOTE: The show_history logic is necessary because in
> 'multiline'
> +            # mode (which we use) 'up arrow' is used to move between
> lines.
> +            self.show_history = True
> +            last_msg = self.history[self.last_index] if self.history else
> ''
> +            self.set_edit_text(last_msg)
> +            self.edit_pos = len(last_msg)
> +            self.last_index += 1
> +        elif key == 'up' and self.show_history:
> +            if self.last_index < len(self.history):
> +                self.set_edit_text(self.history[self.last_index])
> +                self.edit_pos = len(self.history[self.last_index])
> +                self.last_index += 1
> +        elif key == 'meta enter':
> +            # When using multiline, enter inserts a new line into the
> editor
> +            # send the input to the server on alt + enter
> +            self.master.cb_send_to_server(msg)
> +            self.history.insert(0, msg)
> +            self.set_edit_text('')
> +            self.last_index = 0
> +            self.show_history = False
> +        else:
> +            self.show_history = False
> +            self.last_index = 0
> +            return super().keypress(size, key)
> +        return None
> +
> +
> +class EditorWidget(urwid.Filler):
> +    """
> +    Wraps CustomEdit
> +    """
> +    def __init__(self, master):
> +        super().__init__(Editor(master), valign='top')
> +
> +
> +class HistoryBox(urwid.ListBox):
> +    """
> +    Shows all the QMP message transmitted/received
> +    """
> +    def __init__(self, master):
> +        self.master = master
> +        self.history = urwid.SimpleFocusListWalker([])
> +        super().__init__(self.history)
> +
> +    def add_to_history(self, history):
> +        self.history.append(urwid.Text(history))
> +        if self.history:
> +            self.history.set_focus(len(self.history) - 1)
> +
> +
> +class HistoryWindow(urwid.Frame):
> +    """
> +    Composes the HistoryBox and EditorWidget
> +    """
> +    def __init__(self, master):
> +        self.master = master
> +        self.editor = EditorWidget(master)
> +        self.editor_widget = urwid.LineBox(self.editor)
>

It's a little confusing that "editor" is of type EditorWidget but
"editor_widget" is urwid.LineBox.


> +        self.history = HistoryBox(master)
> +        self.body = urwid.Pile([('weight', 80, self.history),
> +                                ('weight', 20, self.editor_widget)])
> +        super().__init__(self.body)
> +        urwid.connect_signal(self.master, UPDATE_MSG,
> self.cb_add_to_history)
> +
> +    def cb_add_to_history(self, msg, level=None):
> +        formatted = []
> +        if level:
> +            msg = f'[{level}]: {msg}'
> +            formatted.append(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)
> +
> +
> +class Window(urwid.Frame):
> +    """
> +    This is going to be the main window that is going to compose other
> +    windows. In this stage it is unnecesssary but will be necessary in
> +    future when we will have multiple windows and want to the switch
> between
> +    them and display overlays
> +    """
> +    def __init__(self, master):
> +        self.master = master
> +        footer = StatusBar()
> +        body = HistoryWindow(master)
> +        super().__init__(body, footer=footer)
> +
> +
> +class App(QMP):
> +    def __init__(self, address):
> +        urwid.register_signal(type(self), UPDATE_MSG)
> +        self.window = Window(self)
> +        self.address = address
> +        self.aloop = None
> +        self.loop = None
> +        super().__init__()
> +
> +    def add_to_history(self, msg, level=None):
> +        urwid.emit_signal(self, UPDATE_MSG, msg, level)
> +
> +    def _cb_outbound(self, msg):
> +        LOGGER.debug('Request: %s', str(msg))
> +        self.add_to_history('<-- ' + str(msg))
> +        return msg
> +
> +    def _cb_inbound(self, msg):
> +        LOGGER.debug('Response: %s', str(msg))
> +        self.add_to_history('--> ' + str(msg))
> +        return msg
> +
>

[DEBUG]: Response seems to duplicate the "--> {...}" incoming messages.

The debug stuff is nice to have because it gets saved to the logfile, but
is there some way to omit it from the history view, even when --debug is
on? I think we simply don't need to see the responses twice. What do you
think?


> +    async def wait_for_events(self):
> +        async for event in self.events:
> +            self.handle_event(event)
> +
> +    async def _send_to_server(self, msg):
> +        # TODO: Handle more validation errors (eg: ValueError)
> +        try:
> +            await self._raw(bytes(msg, 'utf-8'))
> +        except ExecuteError:
> +            LOGGER.info('Error response from server for msg: %s', msg)
> +        except ExecInterruptedError:
> +            LOGGER.info('Error server disconnected before reply')
> +            # FIXME: Handle this better
> +            # Show the disconnected message in the history window
> +            urwid.emit_signal(self, UPDATE_MSG,
> +                              '{"error": "Server disconnected before
> reply"}')
> +            self.window.footer.set_text("Server disconnected")
> +        except Exception as err:
> +            LOGGER.error('Exception from _send_to_server: %s', str(err))
> +            raise err
>

Type non-JSON or a JSON value that isn't an object will crash the whole
application.

You need to look out for these:

  File "/home/jsnow/src/qemu/python/qemu/aqmp/qmp_protocol.py", line 479,
in _raw
    msg = Message(msg)
  File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 71, in
__init__
    self._obj = self._deserialize(self._data)
  File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 158, in
_deserialize
    raise DeserializationError(emsg, data) from err
qemu.aqmp.message.DeserializationError: Failed to deserialize QMP message.
  raw bytes were: b'q\n\n'

  File "/home/jsnow/src/qemu/python/qemu/aqmp/qmp_protocol.py", line 479,
in _raw
    msg = Message(msg)
  File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 71, in
__init__
    self._obj = self._deserialize(self._data)
  File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 160, in
_deserialize
    raise UnexpectedTypeError(
qemu.aqmp.message.UnexpectedTypeError: QMP message is not a JSON object.
  json value was: []

There's also ValueError and TypeError, but I think the way you've written
the shell here that there's not much of a chance to actually see these --
they show up when serializing a python object, but you do bytes(msg) which
means we use the *deserialization* interface to validate user input, so you
might not actually see the other errors here ...

Still, you theoretically could if somehow
serialize(deserialize(bytes(msg)))) raised those errors. I don't expect
that they would, but you may as well treat all of these errors the same:
the input by the user is garbage and cannot be used. No need to exit or
crash.


> +
> +    def cb_send_to_server(self, msg):
> +        create_task(self._send_to_server(msg))
> +
> +    def unhandled_input(self, key):
> +        if key == 'esc':
> +            self.kill_app()
> +
> +    def kill_app(self):
> +        # TODO: Work on the disconnect logic
> +        create_task(self._kill_app())
> +
> +    async def _kill_app(self):
> +        # It is ok to call disconnect even in disconnect state
> +        try:
> +            await self.disconnect()
> +            LOGGER.debug('Disconnect finished. Exiting app')
> +        except MultiException as err:
> +            LOGGER.info('Multiple exception on disconnect: %s', str(err))
> +            # Let the app crash after providing a proper stack trace
> +            raise err
> +        raise urwid.ExitMainLoop()
>

I got rid of MultiException in v2, thankfully.... !

If the server goes away, aqmp-shell shows the disconnect debug messages
well enough, but then when hitting 'esc' afterwards, we get an Exception
printed out to the terminal. Ideally, aqmp-shell should call disconnect()
as soon as it notices a problem and not only when we call _kill_app.


> +
> +    def handle_event(self, event):
> +        # FIXME: Consider all states present in qapi/run-state.json
> +        if event['event'] == 'SHUTDOWN':
> +            self.window.footer.set_text('Server shutdown')
> +
> +    async def connect_server(self):
> +        try:
> +            await self.connect(self.address)
> +            self.window.footer.set_text("Connected to {:s}".format(
> +                f"{self.address[0]}:{self.address[1]}"
> +                if isinstance(self.address, tuple)
> +                else self.address
> +            ))
> +        except ConnectError as err:
> +            LOGGER.debug('Cannot connect to server %s', str(err))
> +            self.window.footer.set_text('Server shutdown')
> +
> +    def run(self, debug=False):
> +        self.screen.set_terminal_properties(256)
> +
> +        self.aloop = asyncio.get_event_loop()
> +        self.aloop.set_debug(debug)
> +
> +        # Gracefully handle SIGTERM and SIGINT signals
> +        cancel_signals = [signal.SIGTERM, signal.SIGINT]
> +        for sig in cancel_signals:
> +            self.aloop.add_signal_handler(sig, self.kill_app)
> +
> +        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
> +        self.loop = urwid.MainLoop(self.window,
> +                                   unhandled_input=self.unhandled_input,
> +                                   handle_mouse=True,
> +                                   event_loop=event_loop)
> +
> +        create_task(self.wait_for_events(), self.aloop)
> +        create_task(self.connect_server(), self.aloop)
> +        try:
> +            self.loop.run()
> +        except Exception as err:
> +            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
> +            raise err
> +
> +
> +class TUILogHandler(Handler):
> +    def __init__(self, tui):
> +        super().__init__()
> +        self.tui = tui
> +
> +    def emit(self, record):
> +        level = record.levelname
> +        msg = record.getMessage()
> +        self.tui.add_to_history(msg, level)
> +
> +
> +def parse_address(address):
> +    """
> +    This snippet was taken from qemu.qmp.__init__.
> +    pylint complaints about duplicate code so it has been
> +    temprorily disabled. This should be fixed once qmp is
> +    replaced by aqmp in the future.
> +    """
> +    components = address.split(':')
> +    if len(components) == 2:
> +        try:
> +            port = int(components[1])
> +        except ValueError:
> +            raise ValueError(f'Bad Port value in {address}') from None
> +        return (components[0], port)
> +    return address
> +
>

You can just import the old QMP package and use the old function directly.
That will save you the trouble of needing to silence the duplicate code
checker too.


> +
> +def main():
> +    parser = argparse.ArgumentParser(description='AQMP TUI')
> +    parser.add_argument('qmp_server', help='Address of the QMP server'
> +                        '< UNIX socket path | TCP addr:port >')
> +    parser.add_argument('--log-file', help='The Log file name')
> +    parser.add_argument('--log-level', help='Log level
> <debug|info|error>',
> +                        default='debug')
> +    parser.add_argument('--debug', action='store_true',
> +                        help='Enable debug mode for asyncio loop'
> +                        'Generates lot of output, makes TUI unusable when'
> +                        'logs are logged in the TUI itself.'
> +                        'Use only when logging to a file')
> +    args = parser.parse_args()
> +
> +    try:
> +        address = parse_address(args.qmp_server)
> +    except ValueError as err:
> +        parser.error(err)
> +
> +    app = App(address)
> +
>

Initializing the app can go down below the logging initialization stuff,
because the init method engages the logging module to set up the loggers,
but we want to initialize the logging paradigm ourselves before anything
touches it.


> +    if args.log_file:
> +        LOGGER.addHandler(logging.FileHandler(args.log_file))
> +    else:
> +        LOGGER.addHandler(TUILogHandler(app))
> +
> +    log_levels = {'debug': logging.DEBUG,
> +                  'info': logging.INFO,
> +                  'error': logging.ERROR}
>

There are more log levels than just 'debug', 'info' and 'error' ... There's
probably a way to avoid having to re-write the mapping yourself. Something
in the logging module can help here.


> +
> +    if args.log_level not in log_levels:
> +        parser.error('Invalid log level')
> +    LOGGER.setLevel(log_levels[args.log_level])
> +
>

You can initialize the app here instead.


> +    app.run(args.debug)
> +
>
>
 I didn't pass "--debug", but I still got DEBUG messages in the history
panel.

What I'd like to see is only WARNING messages and above in the application
unless I pass --debug, and then I want to see additional messages.


Looking good otherwise, I think it's shaping up. Thanks!

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

Re: [PATCH v2 3/6] python/aqmp-tui: Add AQMP TUI draft

[Niteesh G. S.]

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

On Wed, Jul 21, 2021 at 12:34 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Tue, Jul 13, 2021 at 6:07 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> Added a draft of AQMP TUI.
>>
>> Implements the follwing basic features:
>> 1) Command transmission/reception.
>> 2) Shows events asynchronously.
>> 3) Shows server status in the bottom status bar.
>>
>> Also added necessary pylint, mypy configurations
>>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> ---
>>  python/qemu/aqmp/aqmp_tui.py | 332 +++++++++++++++++++++++++++++++++++
>>  python/setup.cfg             |  21 ++-
>>  2 files changed, 352 insertions(+), 1 deletion(-)
>>  create mode 100644 python/qemu/aqmp/aqmp_tui.py
>>
>> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
>> new file mode 100644
>> index 0000000000..f853efc1f5
>> --- /dev/null
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -0,0 +1,332 @@
>> +# Copyright (c) 2021
>> +#
>> +# Authors:
>> +#  Niteesh Babu G S <niteesh.gs@gmail.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later.  See the COPYING file in the top-level directory.
>> +
>> +import argparse
>> +import asyncio
>> +import logging
>> +from logging import Handler
>> +import signal
>> +
>> +import urwid
>> +import urwid_readline
>> +
>> +from .error import MultiException
>> +from .protocol import ConnectError
>> +from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
>> +from .util import create_task, pretty_traceback
>> +
>> +
>> +UPDATE_MSG = 'UPDATE_MSG'
>> +
>> +# Using root logger to enable all loggers under qemu and asyncio
>> +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'),
>> +    ('background', '', 'black', '', '', 'g7'),
>> +]
>> +
>> +
>> +class StatusBar(urwid.Text):
>> +    """
>> +    A simple Text widget that currently only shows connection status.
>> +    """
>> +    def __init__(self, text=''):
>> +        super().__init__(text, align='right')
>> +
>> +
>> +class Editor(urwid_readline.ReadlineEdit):
>> +    """
>> +    Support urwid_readline features along with
>> +    history support which lacks in urwid_readline
>> +    """
>> +    def __init__(self, master):
>> +        super().__init__(caption='> ', multiline=True)
>> +        self.master = master
>> +        self.history = []
>> +        self.last_index = -1
>> +        self.show_history = False
>> +
>> +    def keypress(self, size, key):
>> +        # TODO: Add some logic for down key and clean up logic if
>> possible.
>> +        # Returning None means the key has been handled by this widget
>> +        # which otherwise is propogated to the parent widget to be
>> +        # handled
>> +        msg = self.get_edit_text()
>> +        if key == 'up' and not msg:
>> +            # Show the history when 'up arrow' is pressed with no input
>> text.
>> +            # NOTE: The show_history logic is necessary because in
>> 'multiline'
>> +            # mode (which we use) 'up arrow' is used to move between
>> lines.
>> +            self.show_history = True
>> +            last_msg = self.history[self.last_index] if self.history
>> else ''
>> +            self.set_edit_text(last_msg)
>> +            self.edit_pos = len(last_msg)
>> +            self.last_index += 1
>> +        elif key == 'up' and self.show_history:
>> +            if self.last_index < len(self.history):
>> +                self.set_edit_text(self.history[self.last_index])
>> +                self.edit_pos = len(self.history[self.last_index])
>> +                self.last_index += 1
>> +        elif key == 'meta enter':
>> +            # When using multiline, enter inserts a new line into the
>> editor
>> +            # send the input to the server on alt + enter
>> +            self.master.cb_send_to_server(msg)
>> +            self.history.insert(0, msg)
>> +            self.set_edit_text('')
>> +            self.last_index = 0
>> +            self.show_history = False
>> +        else:
>> +            self.show_history = False
>> +            self.last_index = 0
>> +            return super().keypress(size, key)
>> +        return None
>> +
>> +
>> +class EditorWidget(urwid.Filler):
>> +    """
>> +    Wraps CustomEdit
>> +    """
>> +    def __init__(self, master):
>> +        super().__init__(Editor(master), valign='top')
>> +
>> +
>> +class HistoryBox(urwid.ListBox):
>> +    """
>> +    Shows all the QMP message transmitted/received
>> +    """
>> +    def __init__(self, master):
>> +        self.master = master
>> +        self.history = urwid.SimpleFocusListWalker([])
>> +        super().__init__(self.history)
>> +
>> +    def add_to_history(self, history):
>> +        self.history.append(urwid.Text(history))
>> +        if self.history:
>> +            self.history.set_focus(len(self.history) - 1)
>> +
>> +
>> +class HistoryWindow(urwid.Frame):
>> +    """
>> +    Composes the HistoryBox and EditorWidget
>> +    """
>> +    def __init__(self, master):
>> +        self.master = master
>> +        self.editor = EditorWidget(master)
>> +        self.editor_widget = urwid.LineBox(self.editor)
>>
>
> It's a little confusing that "editor" is of type EditorWidget but
> "editor_widget" is urwid.LineBox.
>
Fixed.

>
>
>> +        self.history = HistoryBox(master)
>> +        self.body = urwid.Pile([('weight', 80, self.history),
>> +                                ('weight', 20, self.editor_widget)])
>> +        super().__init__(self.body)
>> +        urwid.connect_signal(self.master, UPDATE_MSG,
>> self.cb_add_to_history)
>> +
>> +    def cb_add_to_history(self, msg, level=None):
>> +        formatted = []
>> +        if level:
>> +            msg = f'[{level}]: {msg}'
>> +            formatted.append(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)
>> +
>> +
>> +class Window(urwid.Frame):
>> +    """
>> +    This is going to be the main window that is going to compose other
>> +    windows. In this stage it is unnecesssary but will be necessary in
>> +    future when we will have multiple windows and want to the switch
>> between
>> +    them and display overlays
>> +    """
>> +    def __init__(self, master):
>> +        self.master = master
>> +        footer = StatusBar()
>> +        body = HistoryWindow(master)
>> +        super().__init__(body, footer=footer)
>> +
>> +
>> +class App(QMP):
>> +    def __init__(self, address):
>> +        urwid.register_signal(type(self), UPDATE_MSG)
>> +        self.window = Window(self)
>> +        self.address = address
>> +        self.aloop = None
>> +        self.loop = None
>> +        super().__init__()
>> +
>> +    def add_to_history(self, msg, level=None):
>> +        urwid.emit_signal(self, UPDATE_MSG, msg, level)
>> +
>> +    def _cb_outbound(self, msg):
>> +        LOGGER.debug('Request: %s', str(msg))
>> +        self.add_to_history('<-- ' + str(msg))
>> +        return msg
>> +
>> +    def _cb_inbound(self, msg):
>> +        LOGGER.debug('Response: %s', str(msg))
>> +        self.add_to_history('--> ' + str(msg))
>> +        return msg
>> +
>>
>
> [DEBUG]: Response seems to duplicate the "--> {...}" incoming messages.
>
The debug stuff is nice to have because it gets saved to the logfile, but
> is there some way to omit it from the history view, even when --debug is
> on? I think we simply don't need to see the responses twice. What do you
> think?
>
Ahh, I totally missed this. I didn't really try out the TUILogHandler much.
I just made sure I was
able to log all levels of messages properly.
Yup, I too feel we should omit the inbound/outbound debug messages
during logging inside TUI.

+    async def wait_for_events(self):
>> +        async for event in self.events:
>> +            self.handle_event(event)
>> +
>> +    async def _send_to_server(self, msg):
>> +        # TODO: Handle more validation errors (eg: ValueError)
>> +        try:
>> +            await self._raw(bytes(msg, 'utf-8'))
>> +        except ExecuteError:
>> +            LOGGER.info('Error response from server for msg: %s', msg)
>> +        except ExecInterruptedError:
>> +            LOGGER.info('Error server disconnected before reply')
>> +            # FIXME: Handle this better
>> +            # Show the disconnected message in the history window
>> +            urwid.emit_signal(self, UPDATE_MSG,
>> +                              '{"error": "Server disconnected before
>> reply"}')
>> +            self.window.footer.set_text("Server disconnected")
>> +        except Exception as err:
>> +            LOGGER.error('Exception from _send_to_server: %s', str(err))
>> +            raise err
>>
>
> Type non-JSON or a JSON value that isn't an object will crash the whole
> application.
>
> You need to look out for these:
>
>   File "/home/jsnow/src/qemu/python/qemu/aqmp/qmp_protocol.py", line 479,
> in _raw
>     msg = Message(msg)
>   File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 71, in
> __init__
>     self._obj = self._deserialize(self._data)
>   File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 158, in
> _deserialize
>     raise DeserializationError(emsg, data) from err
> qemu.aqmp.message.DeserializationError: Failed to deserialize QMP message.
>   raw bytes were: b'q\n\n'
>
>   File "/home/jsnow/src/qemu/python/qemu/aqmp/qmp_protocol.py", line 479,
> in _raw
>     msg = Message(msg)
>   File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 71, in
> __init__
>     self._obj = self._deserialize(self._data)
>   File "/home/jsnow/src/qemu/python/qemu/aqmp/message.py", line 160, in
> _deserialize
>     raise UnexpectedTypeError(
> qemu.aqmp.message.UnexpectedTypeError: QMP message is not a JSON object.
>   json value was: []
>
> There's also ValueError and TypeError, but I think the way you've written
> the shell here that there's not much of a chance to actually see these --
> they show up when serializing a python object, but you do bytes(msg) which
> means we use the *deserialization* interface to validate user input, so you
> might not actually see the other errors here ...
>
> Still, you theoretically could if somehow
> serialize(deserialize(bytes(msg)))) raised those errors. I don't expect
> that they would, but you may as well treat all of these errors the same:
> the input by the user is garbage and cannot be used. No need to exit or
> crash.
>
Will fix.

>
>
>> +
>> +    def cb_send_to_server(self, msg):
>> +        create_task(self._send_to_server(msg))
>> +
>> +    def unhandled_input(self, key):
>> +        if key == 'esc':
>> +            self.kill_app()
>> +
>> +    def kill_app(self):
>> +        # TODO: Work on the disconnect logic
>> +        create_task(self._kill_app())
>> +
>> +    async def _kill_app(self):
>> +        # It is ok to call disconnect even in disconnect state
>> +        try:
>> +            await self.disconnect()
>> +            LOGGER.debug('Disconnect finished. Exiting app')
>> +        except MultiException as err:
>> +            LOGGER.info('Multiple exception on disconnect: %s', str(err))
>> +            # Let the app crash after providing a proper stack trace
>> +            raise err
>> +        raise urwid.ExitMainLoop()
>>
>
> I got rid of MultiException in v2, thankfully.... !
>
Nice :).

>
> If the server goes away, aqmp-shell shows the disconnect debug messages
> well enough, but then when hitting 'esc' afterwards, we get an Exception
> printed out to the terminal. Ideally, aqmp-shell should call disconnect()
> as soon as it notices a problem and not only when we call _kill_app.
>
Will refactor.

> +
>> +    def handle_event(self, event):
>> +        # FIXME: Consider all states present in qapi/run-state.json
>> +        if event['event'] == 'SHUTDOWN':
>> +            self.window.footer.set_text('Server shutdown')
>> +
>> +    async def connect_server(self):
>> +        try:
>> +            await self.connect(self.address)
>> +            self.window.footer.set_text("Connected to {:s}".format(
>> +                f"{self.address[0]}:{self.address[1]}"
>> +                if isinstance(self.address, tuple)
>> +                else self.address
>> +            ))
>> +        except ConnectError as err:
>> +            LOGGER.debug('Cannot connect to server %s', str(err))
>> +            self.window.footer.set_text('Server shutdown')
>> +
>> +    def run(self, debug=False):
>> +        self.screen.set_terminal_properties(256)
>> +
>> +        self.aloop = asyncio.get_event_loop()
>> +        self.aloop.set_debug(debug)
>> +
>> +        # Gracefully handle SIGTERM and SIGINT signals
>> +        cancel_signals = [signal.SIGTERM, signal.SIGINT]
>> +        for sig in cancel_signals:
>> +            self.aloop.add_signal_handler(sig, self.kill_app)
>> +
>> +        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>> +        self.loop = urwid.MainLoop(self.window,
>> +                                   unhandled_input=self.unhandled_input,
>> +                                   handle_mouse=True,
>> +                                   event_loop=event_loop)
>> +
>> +        create_task(self.wait_for_events(), self.aloop)
>> +        create_task(self.connect_server(), self.aloop)
>> +        try:
>> +            self.loop.run()
>> +        except Exception as err:
>> +            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
>> +            raise err
>> +
>> +
>> +class TUILogHandler(Handler):
>> +    def __init__(self, tui):
>> +        super().__init__()
>> +        self.tui = tui
>> +
>> +    def emit(self, record):
>> +        level = record.levelname
>> +        msg = record.getMessage()
>> +        self.tui.add_to_history(msg, level)
>> +
>> +
>> +def parse_address(address):
>> +    """
>> +    This snippet was taken from qemu.qmp.__init__.
>> +    pylint complaints about duplicate code so it has been
>> +    temprorily disabled. This should be fixed once qmp is
>> +    replaced by aqmp in the future.
>> +    """
>> +    components = address.split(':')
>> +    if len(components) == 2:
>> +        try:
>> +            port = int(components[1])
>> +        except ValueError:
>> +            raise ValueError(f'Bad Port value in {address}') from None
>> +        return (components[0], port)
>> +    return address
>> +
>>
>
> You can just import the old QMP package and use the old function directly.
> That will save you the trouble of needing to silence the duplicate code
> checker too.
>
OK. Will change it.

> +
>> +def main():
>> +    parser = argparse.ArgumentParser(description='AQMP TUI')
>> +    parser.add_argument('qmp_server', help='Address of the QMP server'
>> +                        '< UNIX socket path | TCP addr:port >')
>> +    parser.add_argument('--log-file', help='The Log file name')
>> +    parser.add_argument('--log-level', help='Log level
>> <debug|info|error>',
>> +                        default='debug')
>> +    parser.add_argument('--debug', action='store_true',
>> +                        help='Enable debug mode for asyncio loop'
>> +                        'Generates lot of output, makes TUI unusable
>> when'
>> +                        'logs are logged in the TUI itself.'
>> +                        'Use only when logging to a file')
>> +    args = parser.parse_args()
>> +
>> +    try:
>> +        address = parse_address(args.qmp_server)
>> +    except ValueError as err:
>> +        parser.error(err)
>> +
>> +    app = App(address)
>> +
>>
>
> Initializing the app can go down below the logging initialization stuff,
> because the init method engages the logging module to set up the loggers,
> but we want to initialize the logging paradigm ourselves before anything
> touches it.
>
I can't move it below the initialization of loggers because TUILogHandler
requires a reference to the App class.
It was the simplest way I could think of to get the log messages inside the
TUI. Any ideas on how can I refactor it?

+    if args.log_file:
>> +        LOGGER.addHandler(logging.FileHandler(args.log_file))
>> +    else:
>> +        LOGGER.addHandler(TUILogHandler(app))
>> +
>> +    log_levels = {'debug': logging.DEBUG,
>> +                  'info': logging.INFO,
>> +                  'error': logging.ERROR}
>>
>
> There are more log levels than just 'debug', 'info' and 'error' ...
> There's probably a way to avoid having to re-write the mapping yourself.
> Something in the logging module can help here.
>
Yes, found a way.
https://github.com/python/cpython/blob/c8e35abfe304eb052a5220974006072c37d4b06a/Lib/logging/__init__.py#L119

>
>
>> +
>> +    if args.log_level not in log_levels:
>> +        parser.error('Invalid log level')
>> +    LOGGER.setLevel(log_levels[args.log_level])
>> +
>>
>
> You can initialize the app here instead.
>
>
>> +    app.run(args.debug)
>> +
>>
>>
>  I didn't pass "--debug", but I still got DEBUG messages in the history
> panel.
>


> What I'd like to see is only WARNING messages and above in the application
> unless I pass --debug, and then I want to see additional messages.
>

The --debug option is to enable the event loop logging and not the TUI
logging.
You can change the TUI logging levels using the --log-level option.
Maybe I'll rename the --debug option to --asyncio-debug and set the default
--log-level to WARNING.


> Looking good otherwise, I think it's shaping up. Thanks!
>

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

[PATCH v2 4/6] python: add optional pygments dependency

[G S Niteesh Babu]

Added pygments as optional dependency for AQMP TUI.
This is required for the upcoming syntax highlighting feature
in AQMP TUI.
The dependency has also been added in the devel optional group.

Added mypy 'ignore_missing_imports' for pygments since it does
not have any type stubs.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/Pipfile.lock | 8 ++++++++
 python/setup.cfg    | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 76cf1e4930..2c6d779348 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -200,6 +200,14 @@
             ],
             "version": "==2.0.0"
         },
+        "pygments": {
+            "hashes": [
+                "sha256:a18f47b506a429f6f4b9df81bb02beab9ca21d0a5fee38ed15aef65f0545519f",
+                "sha256:d66e804411278594d764fc69ec36ec13d9ae9147193a1740cd34d272ca383b8e"
+            ],
+            "markers": "python_version >= '3.5'",
+            "version": "==2.9.0"
+        },
         "pylint": {
             "hashes": [
                 "sha256:082a6d461b54f90eea49ca90fff4ee8b6e45e8029e5dbd72f6107ef84f3779c0",
diff --git a/python/setup.cfg b/python/setup.cfg
index 7e5aae66c7..63f5156c03 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -46,6 +46,7 @@ devel =
     tox >= 3.18.0
     urwid >= 2.1.2
     urwid-readline >= 0.13
+    Pygments >= 2.9.0
 
 # Provides qom-fuse functionality
 fuse =
@@ -55,6 +56,7 @@ fuse =
 tui =
     urwid >= 2.1.2
     urwid-readline >= 0.13
+    Pygments >= 2.9.0
 
 [options.entry_points]
 console_scripts =
@@ -99,6 +101,9 @@ ignore_missing_imports = True
 [mypy-urwid_readline]
 ignore_missing_imports = True
 
+[mypy-pygments]
+ignore_missing_imports = True
+
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
 # can either give multiple identifiers separated by comma (,) or put this
-- 
2.17.1

[PATCH v2 5/6] python/aqmp-tui: add syntax highlighting

[G S Niteesh Babu]

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index f853efc1f5..9ee91f0e99 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -12,6 +12,8 @@
 from logging import Handler
 import signal
 
+from pygments import lexers
+from pygments import token as Token
 import urwid
 import urwid_readline
 
@@ -33,6 +35,11 @@
     (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'),
 ]
 
@@ -133,7 +140,7 @@ def cb_add_to_history(self, msg, level=None):
         formatted = []
         if level:
             msg = f'[{level}]: {msg}'
-            formatted.append(msg)
+            formatted.append((level, msg))
         else:
             lexer = lexers.JsonLexer()  # pylint: disable=no-member
             for token in lexer.get_tokens(msg):
@@ -162,6 +169,7 @@ def __init__(self, address):
         self.address = address
         self.aloop = None
         self.loop = None
+        self.screen = urwid.raw_display.Screen()
         super().__init__()
 
     def add_to_history(self, msg, level=None):
@@ -249,8 +257,10 @@ def run(self, debug=False):
             self.aloop.add_signal_handler(sig, self.kill_app)
 
         event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
-        self.loop = urwid.MainLoop(self.window,
+        self.loop = urwid.MainLoop(urwid.AttrMap(self.window, 'background'),
                                    unhandled_input=self.unhandled_input,
+                                   screen=self.screen,
+                                   palette=palette,
                                    handle_mouse=True,
                                    event_loop=event_loop)
 
-- 
2.17.1

[PATCH v2 6/6] python: add entry point for aqmp-tui

[G S Niteesh Babu]

Add an entry point for aqmp-tui. This will allow it to be run from
the command line using "aqmp-tui localhost:1234"
More options available in the TUI can be found using "aqmp-tui -h"

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/setup.cfg | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/setup.cfg b/python/setup.cfg
index 63f5156c03..082bb6d68b 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -68,6 +68,7 @@ console_scripts =
     qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
     qemu-ga-client = qemu.qmp.qemu_ga_client:main
     qmp-shell = qemu.qmp.qmp_shell:main
+    aqmp-tui = qemu.aqmp.aqmp_tui:main
 
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
-- 
2.17.1

Re: [PATCH v2 0/6] python: AQMP-TUI Prototype

[Niteesh G. S.]

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

Hello all,

UPDATE:  The pipelines have run and all tests passed #336491916
Usually, the pipelines start running as soon as I push my code. But this
time they took longer to start and there was no sign of starting. This is my
first experience with pipelines so I assumed I messed up something from
my side.

Thanks,
Niteesh.

On Wed, Jul 14, 2021 at 3:37 AM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> GitLab:
> https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/
> Based-on
> <https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/Based-on>:
> <20210701041313.1696009-1-jsnow@redhat.com>
>      [PATCH 00/20] python: introduce Asynchronous QMP package
>
> Updates in V2:
> 1) Moved loop related initialization to 'run' function in 'App' class
> 2) Added a module logger with support in TUI log messages.
> 3) Corrected usage of logging.info and logging.debug
> 4) Added an option in setup.cfg to silent pylint regarding duplicate-code
> 4) Modified the arguments list to the TUI
>
> NOTE: I am not able to get the pipelines running after the v2 changes.
> I was only able to test the changes locally using *make check*.
>
> This patch series introduces AQMP-TUI prototype. This prototype has been
> helpfull in letting us try out different ideas and giving some insights
> into things that we had to take care of in the upcoming TUI. It was also
> helpfull in finding out bugs in the AQMP library.
>
> The intent for this patch series is to get comments on the architectural
> design of the prototype. These comments will lay down the foundation for
> the upcoming TUI.
>
> G S Niteesh Babu (6):
>   python: disable pylint errors for aqmp-tui
>   python: Add dependencies for AQMP TUI
>   python/aqmp-tui: Add AQMP TUI draft
>   python: add optional pygments dependency
>   python/aqmp-tui: add syntax highlighting
>   python: add entry point for aqmp-tui
>
>  python/Pipfile.lock          |  20 ++
>  python/qemu/aqmp/aqmp_tui.py | 342 +++++++++++++++++++++++++++++++++++
>  python/setup.cfg             |  36 +++-
>  3 files changed, 397 insertions(+), 1 deletion(-)
>  create mode 100644 python/qemu/aqmp/aqmp_tui.py
>
> --
> 2.17.1
>
>

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

Re: [PATCH v2 0/6] python: AQMP-TUI Prototype

[John Snow]

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

On Wed, Jul 14, 2021 at 3:07 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

> Hello all,
>
> UPDATE:  The pipelines have run and all tests passed #336491916
> Usually, the pipelines start running as soon as I push my code. But this
> time they took longer to start and there was no sign of starting. This is
> my
> first experience with pipelines so I assumed I messed up something from
> my side.
>
> Thanks,
> Niteesh.
>
> On Wed, Jul 14, 2021 at 3:37 AM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> GitLab:
>> https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/
>> Based-on
>> <https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/Based-on>:
>> <20210701041313.1696009-1-jsnow@redhat.com>
>>      [PATCH 00/20] python: introduce Asynchronous QMP package
>>
>> Updates in V2:
>> 1) Moved loop related initialization to 'run' function in 'App' class
>> 2) Added a module logger with support in TUI log messages.
>> 3) Corrected usage of logging.info and logging.debug
>> 4) Added an option in setup.cfg to silent pylint regarding duplicate-code
>> 4) Modified the arguments list to the TUI
>>
>> NOTE: I am not able to get the pipelines running after the v2 changes.
>> I was only able to test the changes locally using *make check*.
>>
>>
Why not?


> This patch series introduces AQMP-TUI prototype. This prototype has been
>> helpfull in letting us try out different ideas and giving some insights
>> into things that we had to take care of in the upcoming TUI. It was also
>> helpfull in finding out bugs in the AQMP library.
>>
>> The intent for this patch series is to get comments on the architectural
>> design of the prototype. These comments will lay down the foundation for
>> the upcoming TUI.
>>
>> G S Niteesh Babu (6):
>>   python: disable pylint errors for aqmp-tui
>>   python: Add dependencies for AQMP TUI
>>   python/aqmp-tui: Add AQMP TUI draft
>>   python: add optional pygments dependency
>>   python/aqmp-tui: add syntax highlighting
>>   python: add entry point for aqmp-tui
>>
>>  python/Pipfile.lock          |  20 ++
>>  python/qemu/aqmp/aqmp_tui.py | 342 +++++++++++++++++++++++++++++++++++
>>  python/setup.cfg             |  36 +++-
>>  3 files changed, 397 insertions(+), 1 deletion(-)
>>  create mode 100644 python/qemu/aqmp/aqmp_tui.py
>>
>>
Thanks Niteesh, a few general comments that don't relate directly to the
code:

1. It would be nice to be able to highlight/copy-paste things out of the
history window, I seemingly can't right now.

2. It would be nice if the mouse scroll wheel worked on the history panel.

3. A greeting message like the old qmp-shell might be nice to see. It would
be good if it explained how to quit the program (esc, ctrl^c) and send
messages (alt+enter).

4. Some control hints or reminder text in the footer might be nice, for how
to quit, send a message, etc.

For the next revision, I may ask you to start looking into making sure that
mypy and pylint pass without exemptions. Do the best you can, and get as
far as you are able. You can leave the warnings disabled for V3, but I'd
like you to start taking a look now so that you know where the trouble
spots are.

Thanks!
--js

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

Re: [PATCH v2 0/6] python: AQMP-TUI Prototype

[Niteesh G. S.]

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

On Wed, Jul 21, 2021 at 12:39 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Wed, Jul 14, 2021 at 3:07 PM Niteesh G. S. <niteesh.gs@gmail.com>
> wrote:
>
>> Hello all,
>>
>> UPDATE:  The pipelines have run and all tests passed #336491916
>> Usually, the pipelines start running as soon as I push my code. But this
>> time they took longer to start and there was no sign of starting. This is
>> my
>> first experience with pipelines so I assumed I messed up something from
>> my side.
>>
>> Thanks,
>> Niteesh.
>>
>> On Wed, Jul 14, 2021 at 3:37 AM G S Niteesh Babu <niteesh.gs@gmail.com>
>> wrote:
>>
>>> GitLab:
>>> https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/
>>> Based-on
>>> <https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/Based-on>:
>>> <20210701041313.1696009-1-jsnow@redhat.com>
>>>      [PATCH 00/20] python: introduce Asynchronous QMP package
>>>
>>> Updates in V2:
>>> 1) Moved loop related initialization to 'run' function in 'App' class
>>> 2) Added a module logger with support in TUI log messages.
>>> 3) Corrected usage of logging.info and logging.debug
>>> 4) Added an option in setup.cfg to silent pylint regarding duplicate-code
>>> 4) Modified the arguments list to the TUI
>>>
>>> NOTE: I am not able to get the pipelines running after the v2 changes.
>>> I was only able to test the changes locally using *make check*.
>>>
>>>
> Why not?
>
I have already updated the status of this
https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg04059.html
All pipelines have run and all passed.

>
>
>> This patch series introduces AQMP-TUI prototype. This prototype has been
>>> helpfull in letting us try out different ideas and giving some insights
>>> into things that we had to take care of in the upcoming TUI. It was also
>>> helpfull in finding out bugs in the AQMP library.
>>>
>>> The intent for this patch series is to get comments on the architectural
>>> design of the prototype. These comments will lay down the foundation for
>>> the upcoming TUI.
>>>
>>> G S Niteesh Babu (6):
>>>   python: disable pylint errors for aqmp-tui
>>>   python: Add dependencies for AQMP TUI
>>>   python/aqmp-tui: Add AQMP TUI draft
>>>   python: add optional pygments dependency
>>>   python/aqmp-tui: add syntax highlighting
>>>   python: add entry point for aqmp-tui
>>>
>>>  python/Pipfile.lock          |  20 ++
>>>  python/qemu/aqmp/aqmp_tui.py | 342 +++++++++++++++++++++++++++++++++++
>>>  python/setup.cfg             |  36 +++-
>>>  3 files changed, 397 insertions(+), 1 deletion(-)
>>>  create mode 100644 python/qemu/aqmp/aqmp_tui.py
>>>
>>>
> Thanks Niteesh, a few general comments that don't relate directly to the
> code:
>
> 1. It would be nice to be able to highlight/copy-paste things out of the
> history window, I seemingly can't right now.
>
> 2. It would be nice if the mouse scroll wheel worked on the history panel.
>
> 3. A greeting message like the old qmp-shell might be nice to see. It
> would be good if it explained how to quit the program (esc, ctrl^c) and
> send messages (alt+enter).
>
> 4. Some control hints or reminder text in the footer might be nice, for
> how to quit, send a message, etc.
>
I'll update the status here as I start working on them one by one.

For the next revision, I may ask you to start looking into making sure that
> mypy and pylint pass without exemptions. Do the best you can, and get as
> far as you are able. You can leave the warnings disabled for V3, but I'd
> like you to start taking a look now so that you know where the trouble
> spots are.
>
Sure.



> Thanks!
> --js
>

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

Re: [PATCH v2 0/6] python: AQMP-TUI Prototype

[John Snow]

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

On Wed, Jul 21, 2021 at 2:09 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

> On Wed, Jul 21, 2021 at 12:39 AM John Snow <jsnow@redhat.com> wrote:
>
>> On Wed, Jul 14, 2021 at 3:07 PM Niteesh G. S. <niteesh.gs@gmail.com>
>> wrote:
>>
>> Why not?
>>
> I have already updated the status of this
> https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg04059.html
>

Sorry, I missed this.

Thanks Niteesh, a few general comments that don't relate directly to the
>> code:
>>
>> 1. It would be nice to be able to highlight/copy-paste things out of the
>> history window, I seemingly can't right now.
>>
>> 2. It would be nice if the mouse scroll wheel worked on the history panel.
>>
>> 3. A greeting message like the old qmp-shell might be nice to see. It
>> would be good if it explained how to quit the program (esc, ctrl^c) and
>> send messages (alt+enter).
>>
>> 4. Some control hints or reminder text in the footer might be nice, for
>> how to quit, send a message, etc.
>>
>

> I'll update the status here as I start working on them one by one.
>
>
OK - They don't need to go into this series, these are just some
observations. All of these items seem like good candidates for standalone
follow-up patches to happen in another series that follows this one.


> For the next revision, I may ask you to start looking into making sure
>> that mypy and pylint pass without exemptions. Do the best you can, and get
>> as far as you are able. You can leave the warnings disabled for V3, but I'd
>> like you to start taking a look now so that you know where the trouble
>> spots are.
>>
>

> Sure.
>
>
I'll be on PTO for the next three business days, returning 2021-07-27 -- If
you get blocked on other tasks, try adding mypy type hints using this
downtime.

Thanks again,
--js

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