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 764846 - Code improvements in GtkApplication
Code improvements in GtkApplication
Status: RESOLVED WONTFIX
Product: gtk+
Classification: Platform
Component: Class: GtkApplication
3.20.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2016-04-10 13:07 UTC by Sébastien Wilmet
Modified: 2016-04-15 14:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
app: avoid code duplication for setting accels (2.52 KB, patch)
2016-04-10 13:08 UTC, Sébastien Wilmet
committed Details | Review
app: add assertions for the | separator (1.71 KB, patch)
2016-04-10 13:08 UTC, Sébastien Wilmet
rejected Details | Review
app: improve doc of gtk_application_set_accels_for_action() (883 bytes, patch)
2016-04-10 13:08 UTC, Sébastien Wilmet
committed Details | Review
app: improve doc of gtk_application_get_window_by_id() (827 bytes, patch)
2016-04-10 13:08 UTC, Sébastien Wilmet
committed Details | Review
app: don't allow 0 ID for get_window_by_id() (1.34 KB, patch)
2016-04-10 13:09 UTC, Sébastien Wilmet
rejected Details | Review
app: don't use deprecated function (1.15 KB, patch)
2016-04-10 13:09 UTC, Sébastien Wilmet
committed Details | Review
app: use g_set_object() (1.82 KB, patch)
2016-04-10 13:09 UTC, Sébastien Wilmet
committed Details | Review
app: add missing g_returns (5.37 KB, patch)
2016-04-10 13:09 UTC, Sébastien Wilmet
none Details | Review
app: improve variable name to not confuse keys (1.27 KB, patch)
2016-04-10 13:09 UTC, Sébastien Wilmet
rejected Details | Review
app: improve code of extract_accels_from_menu() (1.32 KB, patch)
2016-04-10 13:10 UTC, Sébastien Wilmet
committed Details | Review
app: minor code improvements (1.19 KB, patch)
2016-04-10 13:10 UTC, Sébastien Wilmet
committed Details | Review
app: add missing g_returns (4.27 KB, patch)
2016-04-10 15:02 UTC, Sébastien Wilmet
committed Details | Review

Description Sébastien Wilmet 2016-04-10 13:07:47 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.
Comment 1 Sébastien Wilmet 2016-04-10 13:08:35 UTC
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().
Comment 2 Sébastien Wilmet 2016-04-10 13:08:42 UTC
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.
Comment 3 Sébastien Wilmet 2016-04-10 13:08:49 UTC
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.
Comment 4 Sébastien Wilmet 2016-04-10 13:08:58 UTC
Created attachment 325658 [details] [review]
app: improve doc of gtk_application_get_window_by_id()
Comment 5 Sébastien Wilmet 2016-04-10 13:09:17 UTC
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.
Comment 6 Sébastien Wilmet 2016-04-10 13:09:25 UTC
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.
Comment 7 Sébastien Wilmet 2016-04-10 13:09:36 UTC
Created attachment 325661 [details] [review]
app: use g_set_object()
Comment 8 Sébastien Wilmet 2016-04-10 13:09:45 UTC
Created attachment 325662 [details] [review]
app: add missing g_returns

And have the g_returns in the same order as the function parameters.
Comment 9 Sébastien Wilmet 2016-04-10 13:09:53 UTC
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.
Comment 10 Sébastien Wilmet 2016-04-10 13:10:00 UTC
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.
Comment 11 Sébastien Wilmet 2016-04-10 13:10:19 UTC
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.
Comment 12 Sébastien Wilmet 2016-04-10 13:14:00 UTC
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.
Comment 13 Matthias Clasen 2016-04-10 13:23:03 UTC
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
Comment 14 Matthias Clasen 2016-04-10 13:24:29 UTC
Review of attachment 325655 [details] [review]:

.
Comment 15 Matthias Clasen 2016-04-10 13:26:17 UTC
Review of attachment 325656 [details] [review]:

I don't think this makes anything clearer and safer.
Comment 16 Matthias Clasen 2016-04-10 13:26:58 UTC
Review of attachment 325657 [details] [review]:

ok
Comment 17 Matthias Clasen 2016-04-10 13:29:14 UTC
Review of attachment 325658 [details] [review]:

sure
Comment 18 Matthias Clasen 2016-04-10 13:32:33 UTC
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.
Comment 19 Matthias Clasen 2016-04-10 13:35:03 UTC
Review of attachment 325660 [details] [review]:

ok
Comment 20 Matthias Clasen 2016-04-10 13:37:26 UTC
Review of attachment 325661 [details] [review]:

sure
Comment 21 Matthias Clasen 2016-04-10 13:41:13 UTC
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
Comment 22 Matthias Clasen 2016-04-10 13:42:53 UTC
Review of attachment 325663 [details] [review]:

I don't think there is any confusion here.
Comment 23 Matthias Clasen 2016-04-10 13:45:10 UTC
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
Comment 24 Matthias Clasen 2016-04-10 13:45:46 UTC
Review of attachment 325665 [details] [review]:

ok
Comment 25 Sébastien Wilmet 2016-04-10 14:46:14 UTC
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.
Comment 26 Sébastien Wilmet 2016-04-10 14:48:59 UTC
(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.
Comment 27 Sébastien Wilmet 2016-04-10 15:02:41 UTC
Created attachment 325670 [details] [review]
app: add missing g_returns

And have the g_returns in the same order as the function parameters.
Comment 28 Sébastien Wilmet 2016-04-10 15:08:45 UTC
(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.
Comment 29 Matthias Clasen 2016-04-12 02:05:28 UTC
Review of attachment 325670 [details] [review]:

sure
Comment 30 Sébastien Wilmet 2016-04-12 18:46:51 UTC
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.
Comment 31 Matthias Clasen 2016-04-12 19:32:50 UTC
Review of attachment 325656 [details] [review]:

I still disagree
Comment 32 Sébastien Wilmet 2016-04-14 08:36:32 UTC
Hum, and can I at least know _why_ you disagree? You know, in software development we can also give arguments.
Comment 33 Matthias Clasen 2016-04-14 14:08:16 UTC
Because I prefer not to litter my code with extraneous assertions.
Comment 34 Sébastien Wilmet 2016-04-15 13:57:22 UTC
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.
Comment 35 Matthias Clasen 2016-04-15 14:25:11 UTC
lets just agree to disagree, and not argue forever about this