[Spice-devel] [PATCH 14/22] Create classes encapsulating the X11 display cursor capture

[Lukáš Hrázký]

On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> There are two classes:
> - The X11CursorUpdater class sends the messages. It can be used when
>   no thread is required.
> - The X11CursorThread spawns an X11CursorUpdater in a new thread.
> 
> These classes will be moved to a separate file in a later patch, but
> are kept in the same file at this stage to make it easier to track
> changes and bisect.
> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 118 +++++++++++++++++++++++++++---------------
>  1 file changed, 76 insertions(+), 42 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 8bbd457..d1996fd 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -206,6 +206,80 @@ private:
>      }
>  };
>  
> +class X11CursorUpdater
> +{
> +public:
> +    X11CursorUpdater(Stream &stream);
> +    ~X11CursorUpdater();
> +    void send_cursor_changes();
> +
> +private:
> +    Stream &stream;
> +    Display *display;
> +};
> +
> +class X11CursorThread
> +{
> +public:
> +    X11CursorThread(Stream &stream);
> +    static void record_cursor_changes(X11CursorThread *self) { self->updater.send_cursor_changes(); }
> +
> +private:
> +    X11CursorUpdater updater;
> +    std::thread thread;
> +};

Ok, I was wondering what you had in mind when you said you'll probably
make two classes. What is the X11CursorThread useful for, though? It is
basically a glue, which isn't necessary, if you turn X11CursorUpdater
into a functor? (as per my original "AgentRunner" patch, let me know if
I need to clarify here)

The only other practical thing I see it doing is wrapping these two
lines into a single call:

std::thread cursor_th(X11CursorUpdater(...));
cursor_th.detach();

I think these two calls would be fine in main() below and we could
leave out the glue class...

(I also have my doubts about the detach, perhaps there should be a
proper join, haven't looked into it yet. Might be causing the Ctrl-C
not working.)

> +
> +X11CursorUpdater::X11CursorUpdater(Stream &stream)
> +    : stream(stream),
> +      display(XOpenDisplay(NULL))
> +{
> +    if (display == NULL) {
> +        throw Error("failed to open display").syslog();
> +    }
> +}
> +
> +X11CursorUpdater::~X11CursorUpdater()
> +{
> +    XCloseDisplay(display);
> +}
> +
> +void X11CursorUpdater::send_cursor_changes()
> +{
> +    unsigned long last_serial = 0;
> +
> +    int event_base, error_base;
> +    if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> +        syslog(LOG_WARNING, "XFixesQueryExtension failed, not sending cursor changes\n");
> +        return; // also terminates the X11CursorThread if that's how we were launched
> +    }
> +    Window rootwindow = DefaultRootWindow(display);
> +    XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
> +
> +    while (true) {
> +        XEvent event;
> +        XNextEvent(display, &event);
> +        if (event.type != event_base + 1) {
> +            continue;
> +        }
> +
> +        XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> +        if (!cursor || cursor->cursor_serial == last_serial) {
> +            continue;
> +        }
> +
> +        last_serial = cursor->cursor_serial;
> +        stream.send<X11CursorMessage>(cursor);
> +    }
> +}
> +
> +X11CursorThread::X11CursorThread(Stream &stream)
> +    : updater(stream),
> +      thread(record_cursor_changes, this)
> +{
> +    thread.detach();
> +}
> +
> +
>  }} // namespace spice::streaming_agent
>  
>  static bool quit_requested = false;
> @@ -377,31 +451,6 @@ static void usage(const char *progname)
>      exit(1);
>  }
>  
> -static void cursor_changes(Stream *stream, Display *display, int event_base)
> -{
> -    unsigned long last_serial = 0;
> -
> -    while (1) {
> -        XEvent event;
> -        XNextEvent(display, &event);
> -        if (event.type != event_base + 1) {
> -            continue;
> -        }
> -
> -        XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> -        if (!cursor) {
> -            continue;
> -        }
> -
> -        if (cursor->cursor_serial == last_serial) {
> -            continue;
> -        }
> -
> -        last_serial = cursor->cursor_serial;
> -        stream->send<X11CursorMessage>(cursor);
> -    }
> -}
> -
>  static void
>  do_capture(Stream &stream, const char *streamport, FILE *f_log)
>  {
> @@ -554,25 +603,10 @@ int main(int argc, char* argv[])
>          }
>      }
>  
> -    Display *display = XOpenDisplay(NULL);
> -    if (display == NULL) {
> -        syslog(LOG_ERR, "failed to open display\n");
> -        return EXIT_FAILURE;
> -    }
> -    int event_base, error_base;
> -    if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> -        syslog(LOG_ERR, "XFixesQueryExtension failed\n");
> -        return EXIT_FAILURE;
> -    }
> -    Window rootwindow = DefaultRootWindow(display);
> -    XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
> -
> -    Stream stream(streamport);
> -    std::thread cursor_th(cursor_changes, &stream, display, event_base);
> -    cursor_th.detach();
> -
>      int ret = EXIT_SUCCESS;
>      try {
> +        Stream stream(streamport);
> +        X11CursorThread cursor_thread(stream);
>          do_capture(stream, streamport, f_log);
>      }
>      catch (Error &err) {