Red Hat Bugzilla – Bug 431167
some imsettings code review
Last modified: 2008-06-24 23:02:10 EDT
I decided to look over some of the imsettings code. I haven't gone over all of it,
but here are some observations:
- no need to call g_object_notify inside a set_property implementation,
gobject does it for you. This also affects other set_property
implementations throughout the code.
- no need to protect g_free () with an if (foo != NULL), g_free is NULL-safe.
- _("No input methods is available on your system.") - the "is" is extraneous.
- leaks env_list
- argv = g_strsplit_set(cmd, " \t", -1) should probably use g_shell_parse_argv() ?
- gchar *s = g_strdup_printf("%d", pid);
write(fd, s, strlen(s)); leaks s
- there is at least one "goto end" between
p = g_path_get_dirname(f);
n = g_path_get_basename(f);
causing these strings to be leaked
- I think it would be cleaner if imsettings_request_new would take a reference
on the connection,
and imsettings_manager_real_start_im would unref the connection afterwards.
- There is at least one "goto end" between
oinfo = imsettings_info_new(conffile);
that will lead to leaking of oinfo
Thank you for reviewing.
The above things should be fixed in 0.99.1-1.fc9 except:
(In reply to comment #0)
> - no need to call g_object_notify inside a set_property implementation,
> gobject does it for you. This also affects other set_property
> implementations throughout the code.
missed some cleanup for that. already fixed in svn though. so should appears in
> - I think it would be cleaner if imsettings_request_new would take a reference
> on the connection,
> and imsettings_manager_real_start_im would unref the connection afterwards.
Actually the request is being sent through DBusGProxy and it increments the
reference counter to keep the connection alive. so I don't think unref there
would helps for something..
Instead I had unref outside IMSettingsRequest to free it.
Should be improved in 0.99.2.
Changing version to '9' as part of upcoming Fedora 9 GA.
More information and reason for this action is here: