GNOME Bugzilla – Bug 742359
Improve the code
Last modified: 2015-01-05 10:51:07 UTC
Use some of the latest stuff to improve the code
Created attachment 293756 [details] [review] Do not include INSTALL generated file
Created attachment 293757 [details] [review] Do not use deprecated margin properties
Created attachment 293758 [details] [review] pairing-dialog use G_DEFINE_WITH_PRIVATE
Created attachment 293759 [details] [review] pairing-dialog: use "use-header-bar" property
Created attachment 293760 [details] [review] pairing-dialog: no need to use g_clear_pointer on finalize
Created attachment 293761 [details] [review] pairing-dialog: no need to handle the action area anymore
Created attachment 293762 [details] [review] chooser-button: no need to keep a static parent_class var
Created attachment 293763 [details] [review] chooser-button: use g_clear_object
Created attachment 293765 [details] [review] chooser: use G_DEFINE_WITH_PRIVATE
Created attachment 293766 [details] [review] settings-row: use G_DEFINE_TYPE_WITH_PRIVATE
Created attachment 293767 [details] [review] settings-widget: use G_DEFINE_TYPE_WITH_PRIVATE
Created attachment 293768 [details] [review] settings: use icon-name intead of stock
Created attachment 293770 [details] [review] pairing-dialog: use a template to create the UI
Created attachment 293771 [details] [review] settings-row: port to use a template
Created attachment 293772 [details] [review] Bump glib requirement to 2.38 This is needed because of G_DEFINE_TYPE_WITH_PRIVATE
Created attachment 293773 [details] [review] Bump gtk version to 3.12 This is needed because of gtk_dialog_get_header_bar
Review of attachment 293756 [details] [review]: Sure.
Review of attachment 293757 [details] [review]: Looks fine.
Review of attachment 293756 [details] [review]: Need to prefix the commit subject with "build: "
Review of attachment 293757 [details] [review]: Need to prefix the commit subject with "settings: "
Review of attachment 293758 [details] [review]: Prefix commit subject with "settings: " I don't really see the use for that change though... Note that gnome-bluetooth isn't branched for stable yet.
Review of attachment 293759 [details] [review]: That looks fine, was it tested?
Review of attachment 293760 [details] [review]: Sure.
Review of attachment 293761 [details] [review]: Yep.
Attachment 293756 [details] pushed as 344457f - Do not include INSTALL generated file Attachment 293757 [details] pushed as 10fa6cf - Do not use deprecated margin properties
Comment on attachment 293760 [details] [review] pairing-dialog: no need to use g_clear_pointer on finalize Attachment 293760 [details] pushed as 3bcebc7 - pairing-dialog: no need to use g_clear_pointer on finalize
Review of attachment 293762 [details] [review]: Sure.
Review of attachment 293763 [details] [review]: Yep.
Review of attachment 293765 [details] [review]: Sure.
Review of attachment 293766 [details] [review]: Sure.
Attachment 293762 [details] pushed as cc1f3be - chooser-button: no need to keep a static parent_class var Attachment 293763 [details] pushed as 34a1712 - chooser-button: use g_clear_object
Review of attachment 293767 [details] [review]: Sure.
Review of attachment 293768 [details] [review]: Yep.
Review of attachment 293770 [details] [review]: Sure. ::: lib/bluetooth-pairing-dialog.c @@ -260,3 @@ - "/org/gnome/bluetooth/settings.ui", - &error); - if (error != NULL) { Use the retval from gtk_builder_add_from_resource instead of checking the error, and assert if it fails. No point in the settings panel failing in weird ways. @@ +325,3 @@ + /* Bind class to template */ + gtk_widget_class_set_template_from_resource (widget_class, + "/org/gnome/bluetooth/bluetooth-pairing-dialog.ui"); On the same line is fine, we have wide screens.
Review of attachment 293771 [details] [review]: Looks good otherwise. ::: lib/bluetooth-settings-row.c @@ +98,2 @@ /* Spinner */ + /* FIXME: why do we need this? */ It's not used, probably existed before we started setting priv->spinner.
Review of attachment 293772 [details] [review]: Yes.
Review of attachment 293772 [details] [review]: And don't forget to prefix the subject with "build: "
(In reply to comment #34) > Review of attachment 293770 [details] [review]: > > Sure. > > ::: lib/bluetooth-pairing-dialog.c > @@ -260,3 @@ > - "/org/gnome/bluetooth/settings.ui", > - &error); > - if (error != NULL) { > > Use the retval from gtk_builder_add_from_resource instead of checking the > error, and assert if it fails. No point in the settings panel failing in weird > ways. Missread the patch? This is removed > > @@ +325,3 @@ > + /* Bind class to template */ > + gtk_widget_class_set_template_from_resource (widget_class, > + > "/org/gnome/bluetooth/bluetooth-pairing-dialog.ui"); > > On the same line is fine, we have wide screens. OK
Review of attachment 293773 [details] [review]: Ditto about the subject prefix ("build: ")
(In reply to comment #38) > (In reply to comment #34) > > Review of attachment 293770 [details] [review] [details]: > > > > Sure. > > > > ::: lib/bluetooth-pairing-dialog.c > > @@ -260,3 @@ > > - "/org/gnome/bluetooth/settings.ui", > > - &error); > > - if (error != NULL) { > > > > Use the retval from gtk_builder_add_from_resource instead of checking the > > error, and assert if it fails. No point in the settings panel failing in weird > > ways. > > Missread the patch? This is removed Misread indeed.
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.