Am 05.11.2015 um 10:15 schrieb Laércio de Sousa:
> This patch brings up a new XCB backend client, translated from original
> xlibclient.c, and based on latest Xephyr code. This XCB backend
> brings some improvements already present in Xephyr, like
> fullscreen and output support.
>
> This patch will also change configure.ac to make the new XCB
> backend the default one for building the driver. For switching back
> to Xlib backend, pass configure option --with-backend=xlib.
>
> Signed-off-by: Laércio de Sousa <laercioso...@sme-mogidascruzes.sp.gov.br>
> ---
[...]
> + else if (reply->major_version < major || reply->minor_version < minor)
> + {
> + xf86DrvMsg(scrnIndex,
> + X_ERROR,
> + "Host X server doesn't support RandR %d.%d, needed for
> Option \"Output\" usage.\n",
> + major, minor);
Why is 4.0 not better than 1.1? Shouldn't this check be replaced with the
following?
if (reply->major_version < major ||
(reply->major_version == major && reply->minor_version < minor))
> + free(reply);
> + return FALSE;
> + }
> +
> + free(reply);
> + return TRUE;
> +}
[...]
> +static void
> +_NestedClientSetWMClass(NestedClientPrivatePtr pPriv,
> + const char* wm_class)
> +{
> + size_t class_len = strlen(wm_class) + 1;
> + char *class_hint = malloc(class_len);
> +
> + if (class_hint)
> + {
> + strcpy(class_hint, wm_class);
> + xcb_change_property(pPriv->conn,
> + XCB_PROP_MODE_REPLACE,
> + pPriv->window,
> + XCB_ATOM_WM_CLASS,
> + XCB_ATOM_STRING,
> + 8,
> + class_len,
> + class_hint);
> + free(class_hint);
> + }
Why is this strcpy needed?
> +}
> +
[...]
> +static void
> +_NestedClientCreateWindow(NestedClientPrivatePtr pPriv)
> +{
> + xcb_size_hints_t sizeHints;
> +
> + sizeHints.flags = XCB_ICCCM_SIZE_HINT_P_POSITION
> + | XCB_ICCCM_SIZE_HINT_P_SIZE
> + | XCB_ICCCM_SIZE_HINT_P_MIN_SIZE
> + | XCB_ICCCM_SIZE_HINT_P_MAX_SIZE;
> + sizeHints.min_width = pPriv->width;
> + sizeHints.max_width = pPriv->width;
> + sizeHints.min_height = pPriv->height;
> + sizeHints.max_height = pPriv->height;
> +
> + pPriv->window = xcb_generate_id(pPriv->conn);
> + pPriv->img = NULL;
> +
> + xcb_create_window(pPriv->conn,
> + XCB_COPY_FROM_PARENT,
> + pPriv->window,
> + pPriv->rootWindow,
> + 0, 0, 100, 100, /* will resize */
Why not creating it at the correct size and position and skip at least the first
of the following two xcb_configure_window()? (Although I don't understand the
second one either)
> + 0,
> + XCB_WINDOW_CLASS_COPY_FROM_PARENT,
> + pPriv->visual->visual_id,
> + pPriv->attr_mask,
> + pPriv->attrs);
> +
> + xcb_icccm_set_wm_normal_hints(pPriv->conn,
> + pPriv->window,
> + &sizeHints);
> +
> + if (pPriv->usingFullscreen)
> + _NestedClientSetFullscreenHint(pPriv);
> +
> + _NestedClientSetDeleteWindowHint(pPriv);
> + _NestedClientSetWindowTitle(pPriv, "");
> + _NestedClientSetWMClass(pPriv, "Xorg");
> +
> + {
> + uint32_t mask = XCB_CONFIG_WINDOW_WIDTH | XCB_CONFIG_WINDOW_HEIGHT;
> + uint32_t values[2] = { pPriv->width, pPriv->height };
> + xcb_configure_window(pPriv->conn, pPriv->window, mask, values);
> + }
> +
> + xcb_map_window(pPriv->conn, pPriv->window);
> +
> + {
> + uint32_t mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
> + uint32_t values[2] = { pPriv->x, pPriv->y };
> + xcb_configure_window(pPriv->conn, pPriv->window, mask, values);
> + }
> +}
> +
[...]
> +void
> +NestedClientUpdateScreen(NestedClientPrivatePtr pPriv,
> + int16_t x1, int16_t y1,
> + int16_t x2, int16_t y2)
> +{
> + if (pPriv->usingShm)
> + xcb_image_shm_put(pPriv->conn, pPriv->window,
> + pPriv->gc, pPriv->img,
> + pPriv->shminfo,
> + x1, y1, x1, y1, x2 - x1, y2 - y1, FALSE);
> + else
> + xcb_image_put(pPriv->conn, pPriv->window, pPriv->gc,
> + pPriv->img, x1, y1, 0);
> +
> + xcb_aux_sync(pPriv->conn);
> +}
Does this really need a sync? Wouldn't a flush be enough? Or is the sync only
needed in the usingShm case so that the data isn't modified too early?
> +static inline void
> +_NestedClientProcessExpose(NestedClientPrivatePtr pPriv,
> + xcb_generic_event_t *ev)
> +{
> + xcb_expose_event_t *xev = (xcb_expose_event_t *)ev;
> + NestedClientUpdateScreen(pPriv,
> + xev->x,
> + xev->y,
> + xev->x + xev->width,
> + xev->y + xev->height);
> +}
> +
> +static inline void
> +_NestedClientProcessClientMessage(NestedClientPrivatePtr pPriv,
> + xcb_generic_event_t *ev)
> +{
> + xcb_client_message_event_t *cmev = (xcb_client_message_event_t *)ev;
> +
> + if (cmev->data.data32[0] == atom_WM_DELETE_WINDOW)
> + {
> + /* XXX: Is there a better way to do this? */
> + xf86DrvMsg(pPriv->scrnIndex,
> + X_INFO,
> + "Nested client window closed.\n");
> + free(ev);
> + exit(0);
You can as well just let the WM kill you if that's all that you do. :-)
(No, I'm not totally serious)
> + }
> +}
> +
[...]
> +void
> +NestedClientCheckEvents(NestedClientPrivatePtr pPriv)
> +{
> + xcb_generic_event_t *ev;
> +
> + while (TRUE)
> + {
> + ev = xcb_poll_for_event(pPriv->conn);
In the name of micro-optimization I will mention xcb_poll_for_queued_event()
once and then shut up again (the idea being to poll only in the first iteration
and afterwards using poll_for_queued_event, but I don't think that this really
makes much of a difference.
> + if (!ev)
> + {
> + if (_NestedClientConnectionHasError(pPriv->scrnIndex,
> + pPriv->conn))
> + {
> + /* XXX: Is there a better way to do this? */
> + xf86DrvMsg(pPriv->scrnIndex,
> + X_ERROR,
> + "Connection with host X server lost.\n");
> + free(ev);
> + NestedClientCloseScreen(pPriv);
> + exit(1);
> + }
> +
> + break;
> + }
> +
> + switch (ev->response_type & ~0x80)
> + {
> + case XCB_EXPOSE:
> + _NestedClientProcessExpose(pPriv, ev);
> + break;
> + case XCB_CLIENT_MESSAGE:
> + _NestedClientProcessClientMessage(pPriv, ev);
> + break;
> + case XCB_MOTION_NOTIFY:
> + _NestedClientProcessMotionNotify(pPriv, ev);
> + break;
> + case XCB_KEY_PRESS:
> + _NestedClientProcessKeyPress(pPriv, ev);
> + break;
> + case XCB_KEY_RELEASE:
> + _NestedClientProcessKeyRelease(pPriv, ev);
> + break;
> + case XCB_BUTTON_PRESS:
> + _NestedClientProcessButtonPress(pPriv, ev);
> + break;
> + case XCB_BUTTON_RELEASE:
> + _NestedClientProcessButtonRelease(pPriv, ev);
> + break;
> + }
> +
> + free(ev);
> + xcb_flush(pPriv->conn);
Why is this flushing inside of the event loop? Wouldn't a flush after the loop
be enough?
> + }
> +}
[...]
Besides the above (minor) points, this look good to me, but I don't really have
much of a clue.
Cheers,
Uli
--
"Because I'm in pain," he says. "That's the only way I get your attention."
He picks up the box. "Don't worry, Katniss. It'll pass."
He leaves before I can answer.
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
