[virt-tools-list] [PATCH virt-viewer v2] Update usage of GObject private structures

[Eduardo Lima (Etrunko)]

On 2/11/19 3:31 PM, Daniel P. Berrangé wrote:
> On Mon, Feb 11, 2019 at 06:24:37PM +0100, Christophe Fergeau wrote:
>> Hey,
>>
>> Looks good to me, 2 comments below:
>>
>> On Thu, Feb 07, 2019 at 10:54:58AM -0200, Eduardo Lima (Etrunko) wrote:
>>
>>> diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-viewer-iso-list-dialog.c
>>> index 3505211..8e25b72 100644
>>> --- a/src/remote-viewer-iso-list-dialog.c
>>> +++ b/src/remote-viewer-iso-list-dialog.c
>>> @@ -114,7 +109,7 @@ remote_viewer_iso_list_dialog_class_init(RemoteViewerISOListDialogClass *klass)
>>>  static void
>>>  remote_viewer_iso_list_dialog_show_files(RemoteViewerISOListDialog *self)
>>>  {
>>> -    self->priv = DIALOG_PRIVATE(self);
>>> +    self->priv = remote_viewer_iso_list_dialog_get_instance_private(self);
>>
>> Is this really needed here when it's already done in
>> remote_viewer_iso_list_dialog_init? (such a change would belong in a
>> separate commit though)
>>
>>>      gtk_stack_set_visible_child_full(GTK_STACK(self->priv->stack), "iso-list",
>>>                                       GTK_STACK_TRANSITION_TYPE_NONE);
>>>      gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, TRUE);
>>> @@ -262,7 +257,8 @@ static void
>>>  remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
>>>  {
>>>      GtkWidget *content = gtk_dialog_get_content_area(GTK_DIALOG(self));
>>> -    RemoteViewerISOListDialogPrivate *priv = self->priv = DIALOG_PRIVATE(self);
>>> +    RemoteViewerISOListDialogPrivate *priv = self->priv =
>>> +        remote_viewer_iso_list_dialog_get_instance_private(self);
>>
>> Hiding the initialization of 'self->priv' in the middle of local
>> variable declaration/initialization is not the most readable thing ever,
>> this could be split too ;)
> 
> Unless the class is is intended to be derivable, best practice for glib
> is to not in fact use the FooPrivate struct paradigm. Instead the
> RemoteViewerISOListDialog struct would be defined in the .c file
> and just contain the private fields directly.

Yes, it makes sense. I changed the dialog code so that the struct
definitions are now in the source file instead of in the header.

> 
> This is what the G_DECLARE_FINAL_TYPE() macro from glib 2.44 does.
> 
> Only the G_DECLARE_DERIVABLE_TYPE() macro uses a "Private" struct.
> 
> We currently depend on glib 2.40 as min version, but if we don't
> want to bump to 2.444, we could easily backport this new macro
> and use it in our code. It eliminates lots of tedious boilerplate
> code in the headers.
> 
>> Apart from this, looks good to me,
>>
>> Reviewed-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Regards,
> Daniel
> 


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com