GNOME Bugzilla – Bug 758227
Crop palette improvements
Last modified: 2016-02-17 13:18:16 UTC
The mockups for the crop palette have been updated. See: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/photos/wires-editing.png
Created attachment 315924 [details] [review] Initial Crop palette improvements
Review of attachment 315924 [details] [review]: Thanks for the patch, Umang. I quickly tried it and it seems to work, but I'll take a closer look later. Here are a few comments for the time being: ::: src/photos-tool-crop.c @@ +65,3 @@ GtkWidget *combo_box; GtkWidget *grid; + GtkWidget *lock_aspect_ratio; lock_check_button or lock_button will be a better name, because right now it sounds like a gboolean. @@ +309,3 @@ gint active; + active = gtk_combo_box_get_active (GTK_COMBO_BOX (self->combo_box)) + 1; /* +1 Index because "FREE" is excluded from CONSTRAINTS[]*/ I think it will be better to put this comment at the top near the CONSTRAINTS array (explaining why FREE needs to be the first element) instead of sprinkling it all over the code. @@ +800,3 @@ + + lock_aspect_ratio_active = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(self->lock_aspect_ratio)); + if (lock_aspect_ratio_active) For gbooleans, we put the function call directly inside the if (...). @@ +1121,3 @@ gtk_orientable_set_orientation (GTK_ORIENTABLE (self->grid), GTK_ORIENTATION_VERTICAL); gtk_grid_set_row_spacing (GTK_GRID (self->grid), 12); + Please remove the trailing whitespace. They make the patch confusing by introducing useless hard to read changes. @@ +1122,3 @@ gtk_grid_set_row_spacing (GTK_GRID (self->grid), 12); + + self->lock_aspect_ratio = gtk_check_button_new_with_label ("Lock aspect ratio"); "Lock aspect ratio" should be enclosed in a _() so that it gets marked for translation. @@ +1123,3 @@ + + self->lock_aspect_ratio = gtk_check_button_new_with_label ("Lock aspect ratio"); + gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(self->lock_aspect_ratio),TRUE); Please add spaces before the opening parentheses. @@ +1133,1 @@ Trailing whitespaces. @@ +1141,3 @@ gtk_container_add (GTK_CONTAINER (self->grid), self->reset_button); g_signal_connect_swapped (self->reset_button, "clicked", G_CALLBACK (photos_tool_crop_reset_clicked), self); + Ditto.
Created attachment 316194 [details] [review] Initial Crop palette improvements
Created attachment 316195 [details] [review] Initial Crop palette improvements
Review of attachment 316195 [details] [review]: Thanks for the new patch, Umang. A few comments: ::: src/photos-tool-crop.c @@ +316,1 @@ g_return_if_fail (active >= 0); I would feel more confident if we used the same logic that we use below to calculate 'active'. With the current logic, this seems correct, but a bit too fragile. @@ +784,3 @@ + { + active = gtk_combo_box_get_active (GTK_COMBO_BOX (self->combo_box)) + 1; + g_return_if_fail (active >= 0); Shouldn't this assertion now be "active >= 1"? We want to assert that something is selected in the combo box, which means gtk_combo_box_get_active cannot return -1. Since we add 1 to it, active has to be greater than or equal to 1. @@ +803,3 @@ + gtk_revealer_set_reveal_child (GTK_REVEALER (self->revealer), TRUE); + else + gtk_revealer_set_reveal_child (GTK_REVEALER (self->revealer), FALSE); This can be simplified by using this when creating the widgets: g_object_bind_property (self->lock_button, "active", self->revealer, "reveal-child", G_BINDING_SYNC_CREATE); @@ +1124,3 @@ + self->lock_button = gtk_check_button_new_with_label (_("Lock aspect ratio")); + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (self->lock_button), TRUE); + gtk_container_add (GTK_CONTAINER (self->grid), GTK_WIDGET (self->lock_button)); self->lock_button is already a GtkWidget *, so this cast is not needed. @@ +1136,1 @@ gtk_container_add (GTK_CONTAINER (self->grid), self->combo_box); Shouldn't the combo box be added to the revealer?
Created attachment 316405 [details] [review] tool-crop: Use a GtkCheckButton to represent "free" I fixed up the patch and pushed it.
Created attachment 316406 [details] [review] tool-crop: Reset should revert to the initial state
The next step would be to replace the GtkComboBox with a GtkListBox. Use 'object-select-symbolic' for the tick mark.
Accordion effect Video casts with two different implementations: 1) gtk_revealer_set_transition_duration (step_timer_for_each_row) - https://youtu.be/hQ8z0-kVtOQ 2)gtk_revealer_set_transition_duration (same_timer_for_all_rows) -https://youtu.be/T-yacoHGnrc 3) To understand better the effect #2 - https://youtu.be/Lir3oiacUxo
(In reply to Umang Jain from comment #9) > Accordion effect Video casts with two different implementations: Wrong bug. The accordion effect is bug 758554
Allan, how about adding a "Facebook cover" constraint for cropping: https://www.facebook.com/help/266520536764594/
Created attachment 318199 [details] [review] ComboBox to ListBox Conversion Demo Video after implementation of ListBox: https://youtu.be/rCvGGpMouY8
Review of attachment 318199 [details] [review]: Thanks for your effort, Umang. Looking quite nice. One quick comment: ::: src/photos-tool-crop.c @@ +1160,3 @@ self->model = gtk_list_store_new (4, G_TYPE_INT, G_TYPE_STRING, G_TYPE_UINT, G_TYPE_UINT); + self->list_box = gtk_list_box_new(); + gtk_list_box_set_selection_mode (GTK_LIST_BOX(self->list_box), GTK_SELECTION_SINGLE); The selection mode should be NONE to match the mockups. We should also set a header function. You'll find some examples in the rest of the code.
Review of attachment 318199 [details] [review]: I see a lot of WARNINGS and CRITICALs. Those need to be fixed. ::: src/photos-tool-crop.c @@ +1169,3 @@ + GtkWidget *row_label; + + overlay = gtk_overlay_new (); We shouldn't be using a GtkOverlay here. That is what is causing your tick marks to get overlaid. Instead, pack a HORIZONTAL GtkGrid inside the GtkListBoxRow, and pack the label and image into it. Look at the gnome-initial-setup code for examples, if you are confused.
Created attachment 318484 [details] [review] ComboBox to ListBox Conversion
Created attachment 318486 [details] [review] ComboBox to ListBox Conversion
Review of attachment 318486 [details] [review]: I am still getting these CRITICALs: (gnome-photos:8692): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-photos:8692): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-photos:8692): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-photos:8692): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-photos:8692): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-photos:8692): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed I have pointed out one suspect below. ::: src/photos-tool-crop.c @@ +64,2 @@ GeglRectangle bbox_source; GtkListStore *model; The GtkListStore is unused. It should be removed. @@ +780,1 @@ +/*Set Tick icon visible to selected row*/ This comment doesn't add any value. It is obvious from reading the code. @@ +801,3 @@ + + icon = g_object_get_data (G_OBJECT(activated), "icon"); + gtk_widget_set_visible (icon, TRUE); Move this into the for loop and use: gtk_widget_set_visible (icon, other_row == activated); @@ +849,3 @@ + g_signal_handlers_block_by_func (self->lock_button, photos_tool_crop_lock_toggled, self); + + if (active == -1) /*Reset*/ Spurious change. @@ +860,1 @@ } Umm... I don't understand why this logic has to change so much. @@ +1170,3 @@ self->model = gtk_list_store_new (4, G_TYPE_INT, G_TYPE_STRING, G_TYPE_UINT, G_TYPE_UINT); + self->list_box = gtk_list_box_new(); + gtk_list_box_set_selection_mode (GTK_LIST_BOX(self->list_box), GTK_SELECTION_NONE); The list box is still lacking a header. You will see examples in the rest of the code. @@ +1184,3 @@ + + row_label = gtk_label_new (CONSTRAINTS[i].name); + gtk_widget_set_halign (row_label, GTK_ALIGN_END); Why END? If anything, it should be START because we want the label to stick to the start/left edge. In this case, since we are not setting hexpand to TRUE, I don't think we need to set any halign at all. @@ +1188,3 @@ + icon = gtk_image_new_from_icon_name (PHOTOS_ICON_OBJECT_SELECT_SYMBOLIC, GTK_ICON_SIZE_INVALID); + gtk_widget_set_no_show_all (icon, TRUE); + gtk_widget_set_halign (icon, GTK_ALIGN_START); See above. Is the halign really needed? @@ +1189,3 @@ + gtk_widget_set_no_show_all (icon, TRUE); + gtk_widget_set_halign (icon, GTK_ALIGN_START); + gtk_widget_set_valign (icon, GTK_ALIGN_CENTER); What about this? @@ +1193,3 @@ + + gtk_grid_attach (GTK_GRID (grid), icon, 1, 0, 1, 1); + gtk_grid_attach (GTK_GRID (grid), row_label, 0, 0, 1, 1); Use gtk_orientable_set_orientation to make it a HORIZONTAL grid, and just use gtk_container_add. Add each widget immediately after creating it and setting the properties for consistency. @@ +1195,3 @@ + gtk_grid_attach (GTK_GRID (grid), row_label, 0, 0, 1, 1); + + g_object_set_data_full (G_OBJECT (row),"icon", icon, g_object_unref); We shouldn't use a GDestroyNotify because the row's g_object_data_full doesn't add a reference to icon. I think, this is what leads to those CRITICALs. If you want to use a GDestroyNotify, then you should add a reference by using g_object_ref. In this case, since the row is icon's parent, I don't think we need to add a reference. The row already has one. Just use g_object_set_data without a GDestroyNotify. @@ +1203,3 @@ * palette. */ + Rogue newline. @@ +1210,3 @@ + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (self->lock_button), TRUE); + g_signal_connect_swapped (self->lock_button, "toggled", G_CALLBACK (photos_tool_crop_lock_toggled), self); + Trailing whitespace. ::: src/photos-tool-crop.h @@ +43,3 @@ +void photos_tool_crop_set_active (PhotosToolCrop *self,gint active); +void photos_tool_crop_set_row_tick (PhotosToolCrop *self); +void photos_tool_crop_lock_toggled (PhotosToolCrop *self); What are these for? These are not used elsewhere, so shouldn't be in the header.
Review of attachment 318486 [details] [review]: ::: src/photos-tool-crop.c @@ +1170,3 @@ self->model = gtk_list_store_new (4, G_TYPE_INT, G_TYPE_STRING, G_TYPE_UINT, G_TYPE_UINT); + self->list_box = gtk_list_box_new(); + gtk_list_box_set_selection_mode (GTK_LIST_BOX(self->list_box), GTK_SELECTION_NONE); Taking a second look at the design, headers aren't needed. Sorry for the noise.
Created attachment 319174 [details] [review] Restrict the direct use of gtk_combo_box_get_active This patch restricts the direct use gtk_combo_box_get_active. Instead of directly using it, the value of combo box active item is stored in an instance variable and then used everywhere else. This will allow to go ahead with ListBox implementation easily and in a conceptual manner.
Created attachment 319498 [details] [review] Migration to GtkListBox from ComboBox
Created attachment 320684 [details] [review] tool-crop, Aspect Ratio: Renaming "Golden Cut" to "Golden Ratio"
Review of attachment 319174 [details] [review]: Thanks for the patch, Umang. This isn't exactly what I had in mind: ::: src/photos-tool-crop.c @@ +1177,3 @@ self->combo_box = gtk_combo_box_new_with_model (GTK_TREE_MODEL (self->model)); gtk_container_add (GTK_CONTAINER (self->revealer), self->combo_box); + g_signal_connect_swapped (self->combo_box, "changed", G_CALLBACK (photos_tool_crop_get_active), self); This changes the semantics too much. We should create a separate handler for GtkComboBox::changed where we would get the active index and then call photos_tool_crop_active_changed.
Review of attachment 319498 [details] [review]: ::: src/photos-tool-crop.c @@ +2,3 @@ * Photos - access, organize and share your photos on GNOME * Copyright © 2015 Red Hat, Inc. + * Copyright © 2015,2016 Umang Jain Missing whitespace. @@ +1153,3 @@ self->crop = g_action_map_lookup_action (G_ACTION_MAP (app), "crop-current"); + self->list_box = gtk_list_box_new(); Ditto.
Created attachment 320726 [details] [review] tool-crop: Restrict use of gtk_combo_box_get_active I rewrote the patch to not change the semantics of the larger code.
Created attachment 320727 [details] [review] tool-crop: Replace the GtkComboBox with a GtkListBox Rebased your GtkListBox patch on top of the previous one. I took the liberty to add some margin around the row, reduced the size of the tick image a bit, and added some column spacing to make it look a bit more like the mockups.
Review of attachment 320684 [details] [review]: ::: src/photos-tool-crop.c @@ +133,3 @@ { PHOTOS_TOOL_CROP_ASPECT_RATIO_ORIGINAL, N_("Original Size"), 0, 0 }, { PHOTOS_TOOL_CROP_ASPECT_RATIO_SCREEN, N_("Screen"), 0, 0 }, + { PHOTOS_TOOL_CROP_ASPECT_RATIO_BASIS, N_("Golden Ratio"), 100000, 161803}, "Golden ratio" as per the design. We should fix "Original Size" too.
Created attachment 320862 [details] [review] tool-crop: Rename "Golden Cut" to "Golden ratio" Fixed and pushed.
Here are the icons: https://github.com/gnome-design-team/gnome-mockups/tree/master/photos/assets
Created attachment 320936 [details] [review] tool-crop: Fix GtkListBox active Fall out from 30a27404a4cd421f905ed139e2a5571e186d6503
Review of attachment 320936 [details] [review]: The change looks good, but the commit message needs work. This is actually a fall out from commit a9bc17bff12a51489a034c9f2faa8da5ab031af9 not commit 30a27404a4cd421f905ed139e2a5571e186d6503 Also, I would use something a little more descriptive than "Fix GtkListBox active".
Created attachment 320971 [details] [review] tool-crop: Fix off-by-one error for images with a constrained crop Fixed and pushed.
Created attachment 321435 [details] [review] tool-crop: Add aspect ratio string to the corresponding constraints
Review of attachment 321435 [details] [review]: ::: src/photos-tool-crop.c @@ +985,3 @@ + context = gtk_widget_get_style_context (ratio_label); + gtk_style_context_add_class (context, "dim-label"); + gtk_container_add (GTK_CONTAINER (grid), ratio_label); This will create multiple GtkLabel widgets if we re-activate the tool. Instead we should create and pack the label in photos_tool_crop_init where we populate the list box. We can then fill in the ratio later. Possibly using g_object_get_data.
Created attachment 321460 [details] [review] tool-crop: Add aspect ratio strings to the corresponding constraints
Review of attachment 321460 [details] [review]: Nice patch. Looks mostly good. There are some minor issues and we are missing the bit about not dimming the active ratio. ::: src/photos-tool-crop.c @@ +957,2 @@ gboolean got_bbox_source; + gdouble constraint_aspect_ratio; This can go inside the loop too. @@ +974,3 @@ + + constraint_aspect_ratio = photos_tool_crop_calculate_aspect_ratio (self, i + 1); + label_text = g_strdup_printf ("%.3f", constraint_aspect_ratio); It should be "%.2f" to match the mockups. Secondly, we should enclose the string within _() because it needs to be marked for translation, and add a comment explaining what this about to translators. @@ +1249,3 @@ + gtk_container_add (GTK_CONTAINER (grid), ratio_label); + gtk_widget_set_hexpand (ratio_label, TRUE); + gtk_widget_set_hexpand (grid, FALSE); Better to put this where we are creating the grid.
Created attachment 321483 [details] [review] tool-crop: Add aspect ratio strings to the corresponding constraints Fixed and pushed to master.
Let's close this bug now. The crop palette is much more like the mockups than we started. We are still missing the icons and rotating functionality. Those will require more work and we can address them in separate bugs. Thanks for all the patches and hard work Umang!