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.
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@xxxxxxxxxx>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
