GNOME Bugzilla – Bug 752633
Several GtkPlacesSidebar methods need better documentation
Last modified: 2015-07-26 13:14:12 UTC
1. :populate-all property g_param_spec_boolean ("populate-all", P_("Show 'Other locations'"), P_("Whether the sidebar includes an item to show external locations"), Has been copied from :show-other-locations and not changed enough. 2. gtk_places_sidebar_set_show_connect_to_server() Marked deprecated in the .h file, but not in the .c file. 3. gtk_places_sidebar_set_show_enter_location() Marked deprecated in the .c file, but not in the .h file. Most of the documentation has been copied from gtk_places_sidebar_set_show_ connect_to_server() and not changed enough. 4. gtk_places_sidebar_set_drop_targets_visible() When visible is FALSE, the context argument is not used, and can be NULL. This is not documented. Isn't visible unnecessary? if (context) /* instead of if (visible) */ { /* ...... */ } else { /* ...... */ }
5. ::show-connect-to-server signal * Deprecated: 3.18: use #GtkPlacesSidebar::show-other-locations property to * connect to network servers. Is that correct? Should it be ::show-other-locations signal? Or possibly :show-other-locations property (with one colon)?
Reopening. Only partly fixed, I would say. Items 1, 2 and 5: Fixed 3. gtk_places_sidebar_set_show_enter_location() Not fixed 4. gtk_places_sidebar_set_drop_targets_visible() Not fixed. The description has been improved, but it's still not obvious whether 'context' can be NULL. I was wrong, when I assumed that 'visible' is unnecessary. 'context' can be NULL also when visible is TRUE. Wouldn't an (allow-none) annotation be appropriate?
(In reply to Kjell Ahlstedt from comment #2) > Reopening. Only partly fixed, I would say. > > Items 1, 2 and 5: Fixed > > 3. gtk_places_sidebar_set_show_enter_location() > Not fixed A patch would be appreciated > 4. gtk_places_sidebar_set_drop_targets_visible() > Not fixed. > The description has been improved, but it's still not obvious whether > 'context' > can be NULL. I was wrong, when I assumed that 'visible' is unnecessary. > 'context' can be NULL also when visible is TRUE. Wouldn't an (allow-none) > annotation be appropriate? Just because the code would not currently crash if you pass NULL doesn't mean that passing NULL should be a documented, supported use of the API.
CC-ing William Jon McCann, regarding item 3 (documentation of gtk_places_sidebar_set_show_enter_location(), which seems to be copied from gtk_places_sidebar_set_show_connect_to_server() and not much changed). He added the present documentation and may have an idea how it shall be changed. See bug 722211 comment 17, commit https://git.gnome.org/browse/gtk+/commit/?id=1e925a85ca2acb00f53ce18d72ab3abf003cecbb
Thanks. Much better. The only remaining question: Why is gtk_places_sidebar_set_show_enter_location() said to be deprecated in the .c file, but not in the .h file? My guess is that when Georges Basile Stavracas Neto added gtk_places_sidebar_ set_show_other_locations() and deprecated gtk_places_sidebar_set_show_ connect_to_server(), he by mistake added the deprecation text in the .c file to gtk_places_sidebar_set_show_enter_location() instead of to gtk_places_sidebar_set_show_connect_to_server() because the descriptions of those two functions were then almost identical. CC-ing him also. See bug 752034 comment 38, commit https://git.gnome.org/browse/gtk+/commit/?id=7db399d975b8f9626c21761dde5f2d5feeb6e305 I know I'm irritatingly pedantic. I promise not to add any more comments to this bug report, if this comment is ignored.