GNOME Bugzilla – Bug 764846
Code improvements in GtkApplication
Last modified: 2016-04-15 14:25:11 UTC
A patch set for GtkApplication, with small commits to avoid code duplication, add assertions and g_returns, improve the docs, and other code improvements to improve readability etc.
Created attachment 325655 [details] [review] app: avoid code duplication for setting accels The implementation of the deprecated functions is now based on the non-deprecated gtk_application_set_accels_for_action().
Created attachment 325656 [details] [review] app: add assertions for the | separator The | separator must always exist in the action_and_target values stored in the accels hash tables. Because before storing the key/value in the hash tables, action_and_target is normalized with gtk_print_action_and_target(). Assertions make the code clearer and safer.
Created attachment 325657 [details] [review] app: improve doc of gtk_application_set_accels_for_action() When reading the API for the first time I didn't know what was the "detailed" action name.
Created attachment 325658 [details] [review] app: improve doc of gtk_application_get_window_by_id()
Created attachment 325659 [details] [review] app: don't allow 0 ID for get_window_by_id() With a zero ID the function returned the most recently focused GtkWindow (not GtkApplicationWindow) associated with the GtkApplication. I think get_window_by_id() should have only one purpose instead of allowing some magic undocumented functionality.
Created attachment 325660 [details] [review] app: don't use deprecated function gtk_application_add_accelerator() is deprecated, but was still used inside IGNORE_DEPRECATIONS's.
Created attachment 325661 [details] [review] app: use g_set_object()
Created attachment 325662 [details] [review] app: add missing g_returns And have the g_returns in the same order as the function parameters.
Created attachment 325663 [details] [review] app: improve variable name to not confuse keys There are two hash tables: action_to_accels and accel_to_actions. So the key of one is a value of the other. Avoid the confusion with a better variable name.
Created attachment 325664 [details] [review] app: improve code of extract_accels_from_menu() sub_model is clearer than "m". And we don't use the key, so we can pass NULL instead.
Created attachment 325665 [details] [review] app: minor code improvements - use GDK_EVENT_PROPAGATE - pass better zero-values to gtk_init(), since the parameters are pointers.
It looks like git-bz didn't add the bug URL at the end of each commit… If a commit is accepted, I can add the URL and push the commit myself.
Review of attachment 325655 [details] [review]: doesn't look like much of a win to me, since they are already sharing accels_set_accels_for_action, but ok
Review of attachment 325655 [details] [review]: .
Review of attachment 325656 [details] [review]: I don't think this makes anything clearer and safer.
Review of attachment 325657 [details] [review]: ok
Review of attachment 325658 [details] [review]: sure
Review of attachment 325659 [details] [review]: I don't think that is true. Just looking at the code of the function, it can only return a GtkApplicationWindow or NULL.
Review of attachment 325660 [details] [review]: ok
Review of attachment 325661 [details] [review]: sure
Review of attachment 325662 [details] [review]: ::: gtk/gtkapplication.c @@ +1603,1 @@ application = gtk_window_get_application (window); This is not public api. We don't put these in internal functions @@ +1617,3 @@ + g_return_val_if_fail (GTK_IS_APPLICATION (application), FALSE); + g_return_val_if_fail (G_IS_ACTION_GROUP (action_group), FALSE); + Same here @@ +1630,3 @@ + g_return_if_fail (GTK_IS_WINDOW (window)); + g_return_if_fail (callback != NULL); + and here
Review of attachment 325663 [details] [review]: I don't think there is any confusion here.
Review of attachment 325664 [details] [review]: I don't think sub_model is much better, but the other changes in this patch are an improvement
Review of attachment 325665 [details] [review]: ok
Thanks for the quick review. (In reply to Matthias Clasen from comment #15) > Review of attachment 325656 [details] [review] [review]: > > I don't think this makes anything clearer and safer. Currently, the code is: > sep = strrchr (actions[i], '|'); > action_name = sep + 1; strrchr() returns NULL in the case '|' is not found. So if sep is NULL, there is a potential security hole since we try to access the string at NULL+1. If it is not a security hole, it can lead to a random crash which is more difficult to debug. By adding: > g_assert (sep != NULL); we acknowledge the fact that we have taken the NULL case into account (it's self-documented code), and we prevent a security hole or random crash.
(In reply to Matthias Clasen from comment #18) > Review of attachment 325659 [details] [review] [review]: > > I don't think that is true. Just looking at the code of the function, it can > only return a GtkApplicationWindow or NULL. Oops yeah, I was confused myself. Another reason to not allow a 0 ID. I'll fix the commit message.
Created attachment 325670 [details] [review] app: add missing g_returns And have the g_returns in the same order as the function parameters.
(In reply to Sébastien Wilmet from comment #26) > > I don't think that is true. Just looking at the code of the function, it can > > only return a GtkApplicationWindow or NULL. > > Oops yeah, I was confused myself. Another reason to not allow a 0 ID. I'll > fix the commit message. Actually no, returning NULL in case of a 0 ID is fine, sorry for the noise.
Review of attachment 325670 [details] [review]: sure
Comment on attachment 325656 [details] [review] app: add assertions for the | separator See my comment #25. I think it's better to add the asserts. gtk_print_action_and_target() always adds the '|' separator, and the code currently assumes that the '|' exists. Adding the asserts just confirm that, explicitly.
Review of attachment 325656 [details] [review]: I still disagree
Hum, and can I at least know _why_ you disagree? You know, in software development we can also give arguments.
Because I prefer not to litter my code with extraneous assertions.
In this case the assertions would make the code easier to understand. When I first read the code I thought that not taking the NULL case into account was an oversight. Checking the invariants with assertions is also a great way to make the code more robust. When we change an invariant, we must update correctly the code at every place where the code relied on the old invariant. If we forget to update the code at one place, the assertions will catch the error, and in some cases it will prevent security issues.
lets just agree to disagree, and not argue forever about this