GNOME Bugzilla – Bug 782204
nautilus-window:port from old-style type declarations
Last modified: 2017-05-06 11:51:58 UTC
Port to G_DECLARE_*_TYPE Style Decls
Created attachment 351161 [details] [review] nautilus-window: port to G_DECLARE_DERIVABLE_TYPE declaration nautilus-window: port to G_DECLARE_DERIVABLE_TYPE declaration
Review of attachment 351161 [details] [review]: It seems that you attached a raw diff. Follow https://wiki.gnome.org/Newcomers/SubmitPatch for a guide on how to submit patches. The code looks fine overall, but it’ll be easier to review the actual changes after you’ve separated unrelated ones. ::: src/nautilus-window.c @@ +71,2 @@ /* Forward and back buttons on the mouse */ + Whitespace. @@ +141,3 @@ guint sidebar_width_handler_id; guint bookmarks_id; +}NautilusWindowPrivate; Leave a space after the closing brace. @@ +2338,3 @@ window = NAUTILUS_WINDOW (object); + NautilusWindowPrivate *priv = nautilus_window_get_instance_private(window); Leave a space before the opening parenthesis. @@ +2596,3 @@ { g_assert (NAUTILUS_IS_WINDOW (window)); + NautilusWindowPrivate *priv = nautilus_window_get_instance_private(window); Variable declarations go before code. ::: src/nautilus-window.h @@ +63,3 @@ +}; + + Don’t move things around for no reason, please. @@ +110,3 @@ guint distance, NautilusWindowOpenFlags flags); + Left some whitespace here. @@ +113,2 @@ void nautilus_window_hide_view_menu (NautilusWindow *window); +void nautilus_window_reset_menus (NautilusWindow *window); It’s nice that you do this, but do style changes separately.
Created attachment 351169 [details] [review] nautilus-window: port to G_DECLARE_*_TYPE decl This patch ports the manual definition of macros to G_Declare type declaration, which makes the code cleaner and easier to read and understand.
Review of attachment 351169 [details] [review]: Thanks for working on this. :) We’ll work on the style now (and other nits to be picked). ::: src/nautilus-window.c @@ +158,3 @@ static GParamSpec *properties[NUM_PROPERTIES] = { NULL, }; +G_DEFINE_TYPE_WITH_PRIVATE (NautilusWindow, nautilus_window, GTK_TYPE_APPLICATION_WINDOW) Unrelated change. @@ +363,3 @@ gpointer user_data) { + NautilusWindow *window = user_data; Why remove the cast? @@ +442,3 @@ on_location_changed (NautilusWindow *window) { + NautilusWindowPrivate *priv = nautilus_window_get_instance_private (window); Leave some vertical space between declarations and code. Keep the style consistent. @@ +552,3 @@ NautilusWindowOpenFlags flags) { + NautilusWindowPrivate *priv = nautilus_window_get_instance_private (window); The assignment would probably make more sense after the assertions. We tend to follow the C89 convention of separating declarations, assignments and code, so you might want to do the same in the rest of the places. @@ +558,2 @@ connect_slot (window, slot); + g_signal_handlers_block_by_func (priv->notebook, Try not to remove vertical spaces. ::: src/nautilus-window.h @@ +49,3 @@ #include "nautilus-window-slot.h" +struct _NautilusWindowClass { Opening brace goes to a new line. @@ +66,3 @@ + GFile *location, + GError *error, + gpointer user_data); Why move this? @@ +92,3 @@ NautilusWindowSlot *target_slot); +void nautilus_window_new_tab (NautilusWindow *window); Unrelated change. @@ +108,3 @@ guint distance, NautilusWindowOpenFlags flags); + Left some trailing whitespace here. @@ +111,2 @@ void nautilus_window_hide_view_menu (NautilusWindow *window); +void nautilus_window_reset_menus (NautilusWindow *window); Unrelated change. @@ +125,3 @@ + NautilusWindowSlot *slot); +void nautilus_window_sync_title (NautilusWindow *window, + NautilusWindowSlot *slot); Also unrelated. @@ +136,3 @@ + GdkDragContext *context); +void nautilus_window_search (NautilusWindow *window, + const gchar *text); Unrelated as well.
Created attachment 351205 [details] [review] nautilus-window: port to G_DECLARE_*_TYPE decl This patch ports the manual definition of macros to G_Declare type declaration, which makes the code cleaner and easier to read and understand.
Review of attachment 351205 [details] [review]: Thanks!
Attachment 351205 [details] pushed as ff7e8f7 - window: port to G_DECLARE_*_TYPE decl