>
> On Mon, 2018-07-09 at 05:21 -0400, Frediano Ziglio wrote:
> > >
> > > Pass the pointer to X cursor struct directly instead of using a lambda
> > > to fill in the pixels.
> > >
> > > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > > ---
> > > src/cursor-updater.cpp | 28 ++++++++++++++--------------
> > > 1 file changed, 14 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/cursor-updater.cpp b/src/cursor-updater.cpp
> > > index 8f65e83..5901172 100644
> > > --- a/src/cursor-updater.cpp
> > > +++ b/src/cursor-updater.cpp
> > > @@ -23,16 +23,17 @@ namespace streaming_agent {
> > >
> > > namespace {
> > >
> > > -void send_cursor(StreamPort &stream_port, unsigned width, unsigned
> > > height,
> > > int hotspot_x, int hotspot_y,
> > > - std::function<void(uint32_t *)> fill_cursor)
> > > +void send_cursor(StreamPort &stream_port, XFixesCursorImage *cursor)
> > > {
> > > - if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH || height >=
> > > STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> > > + if (cursor->width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > > + cursor->height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> > > + {
> >
> > style
>
> Ok, so you left me guessing what the issue is, I suppose you want me to
> do this?
>
> if (cursor->width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> cursor->height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> return;
> }
>
> Sure... but I'll not spare you the rant :)
>
Sorry, was not meant to be a rant, just though you just missed it.
> This is much worse on the eyes and makes little sense to me, especially
> along with the brace rules on functions and classes.
>
> (I'm done :))
>
Yes, the combination of our rules in this case does not make clear when
the if condition ends. Maybe parenthesis can help in this case, like
if ((cursor->width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH) ||
(cursor->height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)) {
return;
}
> > > return;
> > > }
> > >
> > > size_t cursor_size =
> > > sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > > - width * height * sizeof(uint32_t);
> > > + cursor->width * cursor->height * sizeof(uint32_t);
> > > std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> > >
> > > StreamDevHeader
> > > &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> > > @@ -45,13 +46,15 @@ void send_cursor(StreamPort &stream_port, unsigned
> > > width,
> > > unsigned height, int h
> > > memset(&cursor_msg, 0, sizeof(cursor_msg));
> > >
> > > cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > > - cursor_msg.width = width;
> > > - cursor_msg.height = height;
> > > - cursor_msg.hot_spot_x = hotspot_x;
> > > - cursor_msg.hot_spot_y = hotspot_y;
> > > + cursor_msg.width = cursor->width;
> > > + cursor_msg.height = cursor->height;
> > > + cursor_msg.hot_spot_x = cursor->xhot;
> > > + cursor_msg.hot_spot_y = cursor->yhot;
> > >
> > > uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
> > > - fill_cursor(pixels);
> > > + for (unsigned i = 0; i < cursor->width * cursor->height; ++i) {
> > > + pixels[i] = cursor->pixels[i];
> > > + }
> > >
> > > std::lock_guard<std::mutex> guard(stream_port.mutex);
> > > stream_port.write(msg.get(), cursor_size);
> > > @@ -95,11 +98,8 @@ CursorUpdater::CursorUpdater(StreamPort *stream_port)
> > > :
> > > stream_port(stream_port)
> > > }
> > >
> > > last_serial = cursor->cursor_serial;
> > > - auto fill_cursor = [cursor](uint32_t *pixels) {
> > > - for (unsigned i = 0; i < cursor->width * cursor->height;
> > > ++i)
> > > - pixels[i] = cursor->pixels[i];
> > > - };
> > > - send_cursor(*stream_port, cursor->width, cursor->height,
> > > cursor->xhot, cursor->yhot, fill_cursor);
> > > +
> > > + send_cursor(*stream_port, cursor);
> > > }
> > > }
> > >
> >
> > Why bounding send_cursor to a specific (X11) implementation?
>
> We have no other implementation and the function is local, so it can be
> made generic when and if needed, no real reason to make the code more
> complex now.
>
> If there was an elegant way to make the send_cursor method independent,
> I'd use it, but I don't see one. I don't like the lambda much and I
> remember c3d had an issue with it too, though I forgot what it was
> exactly. This is a preparation patch for encapsulating the sending of
> the messages which is mostly based on his work.
>
> > Would not be better to add another overloaded send_cursor instead?
>
> Not sure what you mean by overloaded send_cursor?
>
Another send_cursor function that accepts a XFixesCursorImage and
call the other send_cursor.
> > I don't thing cursor can be nullptr, in this case I would use a reference
> > instead, better constant, I don't see why we should change it.
>
> Sure, I just used a pointer because that's what I had, and yeah, it
> should be const.
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel