[virt-tools-list] [virt-viewer v2 2/6] ovirt: Add OvirtForeignMenu class

[Christophe Fergeau]

Hey,

On Fri, Jun 27, 2014 at 04:42:35PM -0500, Jonathon Jongsma wrote:
> Something seems wrong about storing GtkMenuItem objects here.
> OvirtForeignMenu is owned by the application, so there's a single
> OvirtForeignMenu object for the entire application. However, each
> display window has its own GtkMenu representing this foreign menu. So
> 'current_iso_menuitem' is a pointer to a single menu item from a single
> window. The intent of storing these two values seems to be so that we
> can keep the check-mark on the old menu item until we get notified that
> ovirt has changed the CD.  However, if the 'current_iso_menuitem' refers
> to a menu item in a different window than new menuitem you just clicked,
> it won't work as expected.
> 
> In addition, it appears that we re-create and replace these GtkMenus any
> time the foreign menu 'file' or 'files' properties change. At first I
> was concerned that this would lead to us holding weak references to a
> destroyed GtkMenuItem, but luckily we avoid this issue because: 
>  - The act of creating the GtkMenu also updates the
> 'current_iso_menuitem' variable. So a stale 'current_iso_menuitem'
> automatically gets overwritten with a valid one when we create the new
> GtkMenu.
> - The next_iso_menuitem variable does not get updated by creating a new
> GtkMenu, so it could theoretically become invalid. But when
> updated_cdrom_cb() is called, we copy this value to current_iso_menuitem
> just before notifying the 'file' property. This notification causes us
> to re-create the GtkMenu, which overwrites this (invalid)
> current_iso_menuitem value that we just wrote.
> 
> (note that as it is currently implemented, current_iso_menuitem will
> almost always hold the menuitem from the last window, which may not even
> be displayed).
> 
> The rest of the patch looks OK.
> 

Thanks for the detailed analysis/explanation, this was indeed quite
weird! And these menu item members are actually not very useful, it's
fairly easy to remove them, and this makes the code simpler... Thanks
for pointing this out :)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140807/6edb359e/attachment.sig>