GNOME Bugzilla – Bug 613062
GNOME Goal: Use accessor functions instead direct access
Last modified: 2010-06-20 09:21:13 UTC
Created attachment 156287 [details] [review] First set of patches to achieve GnomeGoals/UseGseal More Information: http://live.gnome.org/GnomeGoals/UseGseal I'm working on a bunch of patches targeting git-master for libgdu-gtk. The first set is attached. They are fairly trivial, but must be done. Hints and help are appreciated Sincerely Salomon Sickert
Created attachment 156301 [details] [review] Second set of patches to achieve GnomeGoals/UseGseal
Created attachment 156413 [details] [review] This set of patches corrects some errors found in the previous. The last remaining function to clean up is "static void gdu_volume_grid_realize (GtkWidget *widget)" in gdu-volume-grid.c
Review of attachment 156413 [details] [review]: Thank you for the patch. I'm not the maintainer, but I'd like to note some things: - Your patch has unnecessary type checks - Try to store variables instead calling the functions several times Some examples below: ::: src/gdu-gtk/gdu-drive-benchmark-dialog.c @@ +558,3 @@ if (!dialog->priv->deleted) { + gtk_widget_get_allocation (GTK_WIDGET (dialog->priv->drawing_area), &allocation); + GtkAllocation allocation; This GTK_WIDGET cast is not needed @@ +967,3 @@ size = gdu_device_get_size (gdu_dialog_get_device (GDU_DIALOG (dialog))); + gtk_widget_get_allocation (GTK_WIDGET (widget), &allocation); + GtkAllocation allocation; This GTK_WIDGET cast is not needed ::: src/gdu-gtk/gdu-gtk.c @@ +357,3 @@ gtk_container_set_border_width (GTK_CONTAINER (hbox), 5); hbox = gtk_hbox_new (FALSE, 12); + gtk_box_pack_start (GTK_BOX (gtk_dialog_get_content_area (GTK_DIALOG (dialog))), hbox, TRUE, TRUE, 0); I think is better: GtkWidget *content_area; content_area = gtk_dialog_get_content_area(); ans then use the "content_area variable". The same for "action_area"
Created attachment 156446 [details] [review] Corrected patch Thanks for the review and correction. I corrected my patch and rebased it against master. This patch should be ready for merge.
Created attachment 156447 [details] [review] Second corrected patch, seperated out for better review With this patch "make CFLAGS+="-DGSEAL_ENABLE"" builds without errors. The function "gdu_volume_grid_realize (GtkWidget *widget)" should get some attention.
Review of attachment 156447 [details] [review]: Hello! Again thank you for the patch. I'm not the maintainer, but I'd like to post some comments: ::: src/gdu-gtk/gdu-volume-grid.c @@ +415,1 @@ + GtkAllocation allocation; Try to maintain the variable declarations in the beginning of the function @@ +442,3 @@ + gdk_window_set_user_data (gtk_widget_get_window(widget), grid); + gdk_window_set_user_data (gtk_widget_get_window (widget), grid); + attributes_mask)); This line is duplicated @@ +446,1 @@ + gtk_widget_set_style (widget, gtk_style_attach (gtk_widget_get_style (widget), gtk_widget_get_window(widget))); You should use gtk_widget_style_attach () here @@ +1495,2 @@ gboolean need_animation_timeout; + GtkAllocation allocation; Try to maintain the variable declarations in the beginning of the function
Created attachment 158399 [details] [review] Corrected patch
Thanks for the patience and the review. I corrected the patch and rebased it against master.
I think the patch is missing a proper bump of GTK2_REQUIRED=2.17.2 in configure.ac
Salomon: Now that you've made all those efforts, would you just fix the issue highlighted in comment 9 so that developers can commit your patch? Thanks!
Created attachment 163527 [details] [review] Corrected of the usegseal.patch Sorry, I missed the comment 9. But here is the corrected patch, also rebased against the recent development version. But it's great, if it gets accepted. Sincerely Salomon
Comment on attachment 163527 [details] [review] Corrected of the usegseal.patch committed with some fixes: - Indentation - Use gtk_widget_style_attach() commit c5fa9573899fb53f8165d3e778377b0a00c0f098
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Thanks Salomon!
Thanks everybody! http://git.gnome.org/browse/gnome-disk-utility/commit/?id=c5fa9573899fb53f8165d3e778377b0a00c0f098