On 18/01/17 11:06, Christophe Fergeau wrote:
> On Wed, Jan 18, 2017 at 10:56:59AM -0200, Eduardo Lima (Etrunko) wrote:
>> On 18/01/17 10:53, Christophe Fergeau wrote:
>>> On Wed, Jan 18, 2017 at 10:08:42AM -0200, Eduardo Lima (Etrunko) wrote:
>>>> On 18/01/17 09:53, Eduardo Lima (Etrunko) wrote:
>>>>> On 17/01/17 14:00, Christophe Fergeau wrote:
>>>>>> On Fri, Jan 13, 2017 at 07:11:07PM -0200, Eduardo Lima (Etrunko) wrote:
>>>>>>> The OvirtForeignMenu pointer is needed by the new ISO list dialog, and
>>>>>>> we make it acessible via property to avoid interdependency between
>>>>>>> objects.
>>>>>>>
>>>>>>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> src/remote-viewer-iso-list-dialog.c | 31 +++++++++++++++++++++++--------
>>>>>>> src/remote-viewer-iso-list-dialog.h | 2 +-
>>>>>>> src/remote-viewer.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 61 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-viewer-iso-list-dialog.c
>>>>>>> index 0fbea28..858719c 100644
>>>>>>> --- a/src/remote-viewer-iso-list-dialog.c
>>>>>>> +++ b/src/remote-viewer-iso-list-dialog.c
>>>>>>> @@ -24,6 +24,7 @@
>>>>>>>
>>>>>>> #include "remote-viewer-iso-list-dialog.h"
>>>>>>> #include "virt-viewer-util.h"
>>>>>>> +#include "ovirt-foreign-menu.h"
>>>>>>>
>>>>>>> G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, GTK_TYPE_DIALOG)
>>>>>>>
>>>>>>> @@ -33,11 +34,16 @@ G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, GTK_TYPE
>>>>>>> struct _RemoteViewerISOListDialogPrivate
>>>>>>> {
>>>>>>> GtkWidget *stack;
>>>>>>> + OvirtForeignMenu *foreign_menu;
>>>>>>> };
>>>>>>>
>>>>>>> static void
>>>>>>> remote_viewer_iso_list_dialog_dispose(GObject *object)
>>>>>>> {
>>>>>>> + RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object);
>>>>>>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>>>>>>> +
>>>>>>> + g_clear_object(&priv->foreign_menu);
>>>>>>> G_OBJECT_CLASS(remote_viewer_iso_list_dialog_parent_class)->dispose(object);
>>>>>>> }
>>>>>>>
>>>>>>> @@ -94,13 +100,22 @@ remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
>>>>>>> }
>>>>>>>
>>>>>>> GtkWidget *
>>>>>>> -remote_viewer_iso_list_dialog_new(GtkWindow *parent)
>>>>>>> +remote_viewer_iso_list_dialog_new(GtkWindow *parent, GObject *foreign_menu)
>>>>>>> {
>>>>>>> - return g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
>>>>>>> - "title", _("Change CD"),
>>>>>>> - "transient-for", parent,
>>>>>>> - "border-width", 18,
>>>>>>> - "default-width", 400,
>>>>>>> - "default-height", 300,
>>>>>>> - NULL);
>>>>>>> + GtkWidget *dialog;
>>>>>>> + RemoteViewerISOListDialog *self;
>>>>>>> +
>>>>>>> + g_return_val_if_fail(foreign_menu != NULL, NULL);
>>>>>>> +
>>>>>>> + dialog = g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
>>>>>>> + "title", _("Change CD"),
>>>>>>> + "transient-for", parent,
>>>>>>> + "border-width", 18,
>>>>>>> + "default-width", 400,
>>>>>>> + "default-height", 300,
>>>>>>> + NULL);
>>>>>>> +
>>>>>>> + self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
>>>>>>> + self->priv->foreign_menu = OVIRT_FOREIGN_MENU(g_object_ref(foreign_menu));
>>>>>>
>>>>>> Fwiw, a construct-only GObject property would be more expected (but you
>>>>>> can keep it this way i f you think it's better).
>>>>>
>>>>>
>>>>> No objections, I will change it.
>>>>
>>>>
>>>> Thinking better about this, it is not possible to make it a construct
>>>> only, because the ovirt object is created later on the process, when
>>>> remote-viewer has already started. Also, there is the possibility of the
>>>> pointer being set or not, depending on how the application was invoked.
>>>
>>> I don't understand what you mean here. In the part of the code quoted
>>> above, a RemoteViewerISOListDialog instance is created, and right after
>>> that, we set self->priv->foreign_menu on that instance, so in this case,
>>> a CONSTRUCT_ONLY property would be fine.
>>
>> The ovirt-foreign-menu property is installed as a member of RemoteViewer
>> object, not RemoteViewerISODialog.
>
> Oh, what I was saying is that
>
> + dialog = g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
> + "title", _("Change CD"),
> + "transient-for", parent,
> + "border-width", 18,
> + "default-width", 400,
> + "default-height", 300,
> + NULL);
> +
> + self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
> + self->priv->foreign_menu = OVIRT_FOREIGN_MENU(g_object_ref(foreign_menu));
>
> should probably be
>
> + dialog = g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
> + "title", _("Change CD"),
> + "transient-for", parent,
> + "border-width", 18,
> + "default-width", 400,
> + "default-height", 300,
> + "ovirt-foreign-menu", foreign_menu,
> + NULL);
>
> But as I was saying, it's not that important here :)
Oh, now I see, indeed I think it does not make much difference here,
only more code to deal with the property.
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko@xxxxxxxxxx
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
