After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 613062 - GNOME Goal: Use accessor functions instead direct access
GNOME Goal: Use accessor functions instead direct access
Status: RESOLVED FIXED
Product: gnome-disk-utility
Classification: Core
Component: libgdu-gtk
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-disk-utility-maint
Depends on:
Blocks:
 
 
Reported: 2010-03-16 17:13 UTC by Salomon Sickert
Modified: 2010-06-20 09:21 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
First set of patches to achieve GnomeGoals/UseGseal (7.00 KB, patch)
2010-03-16 17:13 UTC, Salomon Sickert
none Details | Review
Second set of patches to achieve GnomeGoals/UseGseal (5.80 KB, patch)
2010-03-16 20:09 UTC, Salomon Sickert
none Details | Review
This set of patches corrects some errors found in the previous. (17.07 KB, patch)
2010-03-18 00:03 UTC, Salomon Sickert
needs-work Details | Review
Corrected patch (19.99 KB, patch)
2010-03-18 12:36 UTC, Salomon Sickert
none Details | Review
Second corrected patch, seperated out for better review (3.92 KB, patch)
2010-03-18 12:40 UTC, Salomon Sickert
needs-work Details | Review
Corrected patch (24.49 KB, patch)
2010-04-10 23:52 UTC, Salomon Sickert
none Details | Review
Corrected of the usegseal.patch (24.76 KB, patch)
2010-06-13 18:52 UTC, Salomon Sickert
committed Details | Review

Description Salomon Sickert 2010-03-16 17:13:43 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
Comment 1 Salomon Sickert 2010-03-16 20:09:58 UTC
Created attachment 156301 [details] [review]
Second set of patches to achieve GnomeGoals/UseGseal
Comment 2 Salomon Sickert 2010-03-18 00:03:44 UTC
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
Comment 3 Javier Jardón (IRC: jjardon) 2010-03-18 00:47:49 UTC
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"
Comment 4 Salomon Sickert 2010-03-18 12:36:43 UTC
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.
Comment 5 Salomon Sickert 2010-03-18 12:40:33 UTC
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.
Comment 6 Javier Jardón (IRC: jjardon) 2010-03-29 15:50:52 UTC
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
Comment 7 Salomon Sickert 2010-04-10 23:52:53 UTC
Created attachment 158399 [details] [review]
Corrected patch
Comment 8 Salomon Sickert 2010-04-10 23:57:00 UTC
Thanks for the patience and the review.

I corrected the patch and rebased it against master.
Comment 9 André Klapper 2010-05-03 11:23:00 UTC
I think the patch is missing a proper bump of GTK2_REQUIRED=2.17.2 in configure.ac
Comment 10 Milan Bouchet-Valat 2010-06-13 13:42:17 UTC
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!
Comment 11 Salomon Sickert 2010-06-13 18:52:42 UTC
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 12 Javier Jardón (IRC: jjardon) 2010-06-20 03:35:50 UTC
Comment on attachment 163527 [details] [review]
Corrected of the usegseal.patch

committed with some fixes:
- Indentation
- Use gtk_widget_style_attach()

commit c5fa9573899fb53f8165d3e778377b0a00c0f098
Comment 13 Javier Jardón (IRC: jjardon) 2010-06-20 03:36:03 UTC
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.
Comment 14 Javier Jardón (IRC: jjardon) 2010-06-20 03:36:35 UTC
Thanks Salomon!