[PATCH xf86-video-nested 09/10] Introduce a new XCB client backend, and make it the default one.
Laércio de Sousa
laerciosousa at sme-mogidascruzes.sp.gov.br
Fri Nov 6 05:10:15 PST 2015
> [...]
> > + 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))
>
You're absolutely right! Fixed here. Thank you!
> > +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?
>
I've copied over this piece of code from Xephyr. In that context,
class_hint stores a concatenation bewteen two strings, so that strcpy is
needed, but here the WM_CLASS string is much more simple, so that strcpy is
not needed. I'll remove it. Thanks!
> > +}
> > +
> [...]
> > +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)
>
TBH, for the window size case, I don't know either. It's just the way it's
done in Xephyr, whose code I've copied here. I'll drop the first
xcb_configure_window() and wait for any complaints. Than you!
For the position case, however, there's a reason: setting it after
xcb_map_window() call is the only way I've found to prevent modern WMs from
"ignoring" my explicit positioning and putting nested Xorg window where it
wants. I'll comment that code for clarity.
> +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?
>
I'll replace it with xcb_flush() and see if anyone complains. I've also put
a xcb_flush() right after window creation, just in case. Thank you!
>
> > +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)
>
I want to terminate nested Xorg properly when I click on its close window
button. Without this exit(0) call, the window will be destroyed, but X
server will keep running. My complain about this exit(0) is that such an
"abrupt" termination may lead to memory leaks due to some allocated
pointers not being properly free'd. Does it proceed?
>
> > + }
> > +}
> > +
> [...]
> > +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.
>
OK, it's quite easy to implement, so I'm changing the code here. Thank you
for the suggestion!
>
> > + 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?
>
Well... Comparing it with other XCB snippets I've found in the web, it
seems this xcb_flush() call is not needed at all. Calling it right atfer
window creation or redrawing (XCB_EXPOSE event only) should be enough,
right?
>
> > + }
> > +}
> [...]
>
> Besides the above (minor) points, this look good to me, but I don't really
> have
> much of a clue.
>
Once again, thank you very much for the appointments!
--
*Laércio de Sousa*
*Orientador de Informática*
*Escola Municipal "Professor Eulálio Gruppi"*
*Rua Ismael da Silva Mello, 559, Mogi Moderno*
*Mogi das Cruzes - SPCEP 08717-390*
Telefone: (11) 4726-8313
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20151106/43430c7f/attachment.html>
More information about the xorg-devel
mailing list