GNOME Bugzilla – Bug 780424
Port widgets to templates
Last modified: 2018-01-23 10:12:05 UTC
We should start to use more *.ui files to declare the GUI.
Created attachment 348539 [details] [review] embed: Port to template
Created attachment 348540 [details] [review] main-window: Port to template
Created attachment 348541 [details] [review] main-toolbar: Rename variable
Created attachment 348542 [details] [review] main-toolbar: Remove unecessary function call
Created attachment 348543 [details] [review] main-toolbar: Port to template
*** Bug 776513 has been marked as a duplicate of this bug. ***
Review of attachment 348539 [details] [review]: Thanks for working on this Alessandro! Somehow it doesn't apply on top of master or gnome-3-24. Does it need a rebase? Some quick comments: ::: src/photos-embed.ui @@ +1,1 @@ +<?xml version="1.0" encoding="UTF-8"?> Could you please add a copyright/license notice here? src/photos-share-dialog.ui has one. @@ +4,3 @@ + <template class="PhotosEmbed" parent="GtkBox"> + <property name="visible">1</property> + <property name="orientation">GTK_ORIENTATION_VERTICAL</property> Does this work? Shouldn't it be 'vertical' instead? @@ +12,3 @@ + <property name="visible">1</property> + <property name="homogeneous">1</property> + <property name="transition-type">GTK_STACK_TRANSITION_TYPE_CROSSFADE</property> Does this work? Shouldn't it be 'crossfade' instead?
Review of attachment 348540 [details] [review]: ::: src/photos-main-window.c @@ -458,3 @@ - widget_class->delete_event = photos_main_window_delete_event; - widget_class->key_press_event = photos_main_window_key_press_event; - widget_class->window_state_event = photos_main_window_window_state_event; I slightly prefer to set them through the virtual function pointers, when it is a direct parent class, and use the XML only as a replacement for g_signal_connect*. The vfuncs are generally faster than signals. While that doesn't matter in this case, it makes a difference for things that can be called from a fast path. eg., "draw". ::: src/photos-main-window.ui @@ +5,3 @@ + <property name="resizable">1</property> + <property name="show-menubar">0</property> + <property name="window-position">GTK_WIN_POS_CENTER</property> Shouldn't it be 'center'?
Review of attachment 348541 [details] [review]: ::: src/photos-main-toolbar.c @@ +55,3 @@ GtkWidget *selection_button; GtkWidget *selection_menu; + GtkWidget *header_bar; Nitpick: would be nice to also retain the order.
Created attachment 348551 [details] [review] main-toolbar: Rename a variable
(In reply to Debarshi Ray from comment #7) > Review of attachment 348539 [details] [review] [review]: > @@ +12,3 @@ > + <property name="visible">1</property> > + <property name="homogeneous">1</property> > + <property > name="transition-type">GTK_STACK_TRANSITION_TYPE_CROSSFADE</property> > > Does this work? Shouldn't it be 'crossfade' instead? It seems to work anyway. From https://developer.gnome.org/gtk3/stable/GtkBuilder.html GtkBuilder can parse textual representations for the most common property types: [...] enumerations (can be specified by their name, nick or integer value) Furthermore, gtk-builder-tool (https://developer.gnome.org/gtk3/stable/gtk-builder-tool.html) doesn't complain about it.
Created attachment 348621 [details] [review] dropdown: Port to template
Created attachment 348622 [details] [review] embed: Port to template
Created attachment 348623 [details] [review] main-window: Port to template
Created attachment 348624 [details] [review] main-toolbar: Remove unecessary function call
Created attachment 348625 [details] [review] main-toolbar: Port to template
(In reply to Alessandro Bono from comment #11) > (In reply to Debarshi Ray from comment #7) > > Review of attachment 348539 [details] [review] [review] [review]: > > @@ +12,3 @@ > > + <property name="visible">1</property> > > + <property name="homogeneous">1</property> > > + <property > > name="transition-type">GTK_STACK_TRANSITION_TYPE_CROSSFADE</property> > > > > Does this work? Shouldn't it be 'crossfade' instead? > > It seems to work anyway. > > From https://developer.gnome.org/gtk3/stable/GtkBuilder.html > GtkBuilder can parse textual representations for the most common property > types: [...] enumerations (can be specified by their name, nick or integer > value) > > Furthermore, gtk-builder-tool > (https://developer.gnome.org/gtk3/stable/gtk-builder-tool.html) doesn't > complain about it. Yes, you're right. I think I have always seen enums being referred to by their integer value or nickname. So I never though the C name would work. Since it works, I agree that it is a better choice in a C program. It's more consistent with C code which helps when grepping.
Review of attachment 348622 [details] [review]: Thanks for the new patches, Alessandro! ::: src/photos-embed.ui @@ +20,3 @@ +--> +<interface> + <requires lib="gtk+" version="3.20"/> One minor thing. Does this XML really need gtk+ >= 3.20? I don't see anything that's so new. It seems to me that the version is often just mindlessly put in the XML. We should either maintain / use the right version, or just remove it. Actually ensuring that we have the right version across each and every XML file seems tedious. Instead we could just do a run-time check at startup - like we do for GEGL.
Review of attachment 348621 [details] [review]: ::: src/photos-dropdown.c @@ -121,3 @@ - context = gtk_widget_get_style_context (GTK_WIDGET (self)); - gtk_style_context_add_class (context, "photos-dropdown"); - gtk_widget_hide (GTK_WIDGET(self)); We should have already removed the "hide" and the custom style class during the GtkPopover port. Sorry for overlooking it. @@ -124,1 @@ gtk_widget_show_all (GTK_WIDGET (self->grid)); This can be moved to the XML as visible=1. That's also what gnome-documents does. @@ +127,3 @@ object_class->dispose = photos_dropdown_dispose; + + widget_class = GTK_WIDGET_CLASS (class); Nitpick: should be in the definition line.
Created attachment 348694 [details] [review] dropdown: Remove redundant function call I took the liberty to split out the Popover specific bits into separate commits, fixed the above nits and pushed to master.
Created attachment 348695 [details] [review] dropdown, theme: Remove custom photos-dropdown style class
Created attachment 348696 [details] [review] dropdown: Port to template
Review of attachment 348622 [details] [review]: ::: src/photos-embed.ui @@ +28,3 @@ + <property name="visible">1</property> + <child type="overlay"> + <object class="GtkStack" id="stack"> The GtkStack is not an overlaid child. Only the SpinnerBox is. @@ +30,3 @@ + <object class="GtkStack" id="stack"> + <property name="visible">1</property> + <property name="transition-type">GTK_STACK_TRANSITION_TYPE_CROSSFADE</property> I don't know if it makes a difference, but strictly speaking, we should set the "expand" child property. @@ +34,3 @@ + </child> + <child type="overlay"> + <object class="PhotosSpinnerBox" id="spinner_box"/> We need to fix the construction of SpinnerBox, so that photos_spinner_new is the same as 'g_object_new (PHOTOS_TYPE_SPINNER_BOX, NULL)'. Right now it isn't.
Created attachment 348921 [details] [review] spinner-box: Don't set any extra properties in g_object_new
Created attachment 348922 [details] [review] embed: Port to template
Created attachment 348923 [details] [review] embed: Port to template I had misplaced the "expand".
Review of attachment 348623 [details] [review]: Looks good to me. ::: src/photos-main-window.ui @@ +20,3 @@ +--> +<interface> + <requires lib="gtk+" version="3.20"/> Nitpick: let's drop the gtk+ version. They tend to get copy-pasted around and often don't reflect reality.
Created attachment 351532 [details] [review] main-window: Port to template Pushed after dropping the 'requires'.
Review of attachment 348624 [details] [review]: Sure, why not. (We are not using the virtual functions that gnome-documents uses to let different MainToolbar implementations create their own search bars, and since commit ad0bab7c194f21b09b4d711a0aa2d9e3a68997b2 this function is just a one liner.)
Review of attachment 348625 [details] [review]: Thanks Alessandro. Looks good apart from some teeny-weeny nitpicks. ::: src/photos-main-toolbar.c @@ +811,3 @@ object_class->set_property = photos_main_toolbar_set_property; + widget_class = GTK_WIDGET_CLASS (class); Nitpick: should go with the definition. ::: src/photos-main-toolbar.ui @@ +20,3 @@ +--> +<interface> + <requires lib="gtk+" version="3.20"/> Nitpick: let's drop the gtk+ version. They tend to get copy-pasted around and often don't reflect reality. @@ +25,3 @@ + <property name="orientation">GTK_ORIENTATION_VERTICAL</property> + <child> + <object class="PhotosHeaderBar" id="header_bar"/> Nitpick: should have 'visible=1' to match the C code. Doesn't seem to make a difference in practice. ::: src/photos.gresource.xml @@ +28,3 @@ <file alias="export-dialog.ui" preprocess="xml-stripblanks" compressed="true">photos-export-dialog.ui</file> <file alias="main-window.ui" preprocess="xml-stripblanks" compressed="true">photos-main-window.ui</file> + <file alias="main-toolbar.ui" preprocess="xml-stripblanks" compressed="true">photos-main-toolbar.ui</file> Nitpick: alphabetical order.
Created attachment 351541 [details] [review] main-toolbar: Port to template
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-photos/issues/62.