[Spice-devel] [PATCH spice-gtk 1/2] channel-display: Make monitors array contain monitors in id order

[Marc-André Lureau]

On Wed, Jan 16, 2013 at 3:35 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> Please, please stop being so damn stubborn, and listen to what other
> people have to say once in a while.

You are not reading my arguments and the code. Please take time to
read and understand too.

> "
> spice-widget.c: update_monitor_area()
>
>     c = &g_array_index(monitors, SpiceDisplayMonitorConfig, d->monitor_id);
>
>
> virt-viewer-session-spice.c: virt_viewer_session_spice_display_monitors():
>
>         display = g_ptr_array_index(displays, monitor->id);
> "

That's exactly what I explained in the previous mail.



>>> that the monitors array can now contain 0x0 sized (iow disabled)
>>> monitors,
>>> remote-viewer already checks for this.
>>
>> Yes, because it builds its own array.
>
> Not true, it gets the monitors property directly from the display channel:
>
> virt-viewer-session-spice.c: virt_viewer_session_spice_display_monitors():
>
>     g_object_get(channel,
>                  "monitors", &monitors,
>                  "monitors-max", &monitors_max,
>                  NULL);
>
> And then it creates max_monitors VirtViewerDisplaySpice
> objects, and then walks the monitors array enabling
> any VirtViewerDisplaySpice where width != 0 && height != 0:
>
>     for (i = 0; i < monitors->len; i++) {
>         SpiceDisplayMonitorConfig *monitor = &g_array_index(monitors,
> SpiceDisp
>
>         display = g_ptr_array_index(displays, monitor->id);
>         g_return_if_fail(display != NULL);
>
>         if (monitor->width == 0 || monitor->width == 0)
>             continue;
>
>         virt_viewer_display_set_enabled(VIRT_VIEWER_DISPLAY(display), TRUE);
>         virt_viewer_display_set_desktop_size(VIRT_VIEWER_DISPLAY(display),
>                                              monitor->width,
> monitor->height);
>     }
>
>
> See how it is addressing the monitors property of the display-channel
> by id !! let me single out the line where this happens for you (again):
>
>
>         display = g_ptr_array_index(displays, monitor->id);

yes, because the array is "sparse" this time:
g_ptr_array_set_size(displays, monitors_max);

>
>>> Thus this patch:
>>> 1) Makes the monitors[i]->id == i assumption be true
>>
>> No need for changing that.
>
> /me bangs head on table.
>
> There is a need to change that because the monitors property of the display
> channel is a 1 on 1 copy of the monitors message on the wire, and for that
> message monitors[i]->id == i is simply *not* true, yet it is assumed in
> 2 places in the code, as I've pointed out *twice* now!

sorry, your argument is false


--
Marc-André Lureau