GNOME Bugzilla – Bug 572175
Remove deprecated GTK+ symbols
Last modified: 2010-04-14 14:35:23 UTC
http://live.gnome.org/GnomeGoals/RemoveDeprecatedSymbols/GTK%2B ./src/evolution-webcal-notify.c: toption = gtk_option_menu_new (); ./src/evolution-webcal-notify.c: gtk_option_menu_set_menu (GTK_OPTION_MENU (toption), menu); ./src/evolution-webcal-notify.c: gtk_option_menu_set_history (GTK_OPTION_MENU (toption),
Hmm. Can I trick Dobey into working on this?
Created attachment 144601 [details] [review] modified source file, deprecated symbols of gtk removed
Created attachment 144602 [details] I am sorry I just make a mistake. Upload again. I am sorry I just make a mistake. Upload again. It's my first contribution, I really hope that it would help.
Created attachment 144603 [details] [review] still incorrect. try again...
Hi, thanks for your patch! 1) Please use git commit and then "git format-patch HEAD^" to create patches. 2) Please keep the former indentation: -static void e_webcal_change_adjustment (GtkMenuShell * shell, - GtkWidget * spinner) { +static void e_webcal_change_adjustment (GtkComboBox * toption, + GtkWidget * spinner) { 3) Please keep the code style (opening the brakets in the same line) 4) Is your patch tested?
Created attachment 144888 [details] [review] patch to Remove-deprecated-GTK+-symbols 1), 2) and 3): OK, please see the attached patch. 4) Is your patch tested? ---- Yes, but I'm not sure it's fully tested or not. I just opened a webcal uri in console, setup the parameters, and checked that the subscription was successful.
Dobey: ping - can this get a review?
On a related note, also see a similar patch in Gedit today: http://git.gnome.org/browse/gedit/commit/?id=820ce8820a8db40eca5ca0b63f2d6e3a07416373
After applying the patch here I can finally compile evolution-webcal with -Werror-implicit-function-declaration -DGDK_DISABLE_DEPRECATED -DGTK_DISABLE_DEPRECATED -DG_DISABLE_DEPRECATED -DPANGO_DISABLE_DEPRECATED -DGDK_PIXBUF_DISABLE_DEPRECATED -DG_DISABLE_SINGLE_INCLUDES Haven't tried functionality though. (In reply to comment #6) > I just opened a webcal uri in console How exactly did you do that?
Review of attachment 144888 [details] [review]: Thanks for the patch, I inlined some comments below. ::: src/evolution-webcal-notify.c @@ +91,2 @@ } +static void e_webcal_change_adjustment (GtkComboBox * toption, Could you please rename `toption' to something else here and below, as it's not a GtkOptionMenu anymore? @@ +92,3 @@ + if (toption != NULL) { + GtkWidget * spinner) { +static void e_webcal_change_adjustment (GtkComboBox * toption, I believe this if() here is useless, as you just got a signal emitted on that very instance. @@ +463,3 @@ + gtk_combo_box_append_text (GTK_COMBO_BOX (toption), _("Minute(s)")); + gtk_combo_box_append_text (GTK_COMBO_BOX (toption), _("Hour(s)")); + gtk_combo_box_append_text (GTK_COMBO_BOX (toption), _("Day(s)")); Why did you change the strings here? If it's needed at all, it should belong in another, separate patch. @@ +471,3 @@ /* If the user changes the units of time, we want to update the spin + G_CALLBACK (e_webcal_change_adjustment), tspinb); + g_signal_connect (GTK_COMBO_BOX (toption), "changed", No need to cast to GTK_COMBO_BOX, g_signal_connect() takes a gpointer.
ShuiYu: Willing to come up with an updated patch according to Cosimo's last comment (or too demotivated after waiting for a review for months)?
Created attachment 158610 [details] [review] Remove deprecated GTK+ symbols Rework of the previous patch. Better now?
Review of attachment 158610 [details] [review]: Apart from this comment below, the patch looks fine now. ::: src/evolution-webcal-notify.c @@ +93,3 @@ + GtkWidget * spinner) { + switch (gtk_combo_box_get_active (combo)) { + case E_ICALSHARE_TIMEOUT_DAYS: No need to change the indentation of these 'case' statements here.
Cosimo, thanks for the review! ShuiYu & Roberto, thanks for the patches! I have committed this to git master: http://git.gnome.org/browse/evolution-webcal/commit/?id=f28289909e8af4405aa5cf63d6b12df8c4af2585
Comment on attachment 158610 [details] [review] Remove deprecated GTK+ symbols (committed with the indentation fix recommended by Cosimo)