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 777609 - column-chooser: port from old-style type declarations
column-chooser: port from old-style type declarations
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: general
master
Other Linux
: Normal minor
: ---
Assigned To: Yash
Nautilus Maintainers
Depends on:
Blocks: 771777
 
 
Reported: 2017-01-22 14:27 UTC by Ernestas Kulik
Modified: 2017-02-15 09:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
column-chooser: Remove unnecessary macro-declaration and port to GDeclare type definiiton (23.12 KB, patch)
2017-02-01 13:50 UTC, Yash
needs-work Details | Review
column-chooser: Convert NautilusColumnChooser to final,Port to GDeclare type definition (20.82 KB, patch)
2017-02-01 16:48 UTC, Yash
needs-work Details | Review
column-chooser: Port to G_DECLARE* type declaration, Refactor NautilusColumnChooser (20.45 KB, patch)
2017-02-01 18:57 UTC, Yash
needs-work Details | Review
column-chooser: Port to G_DECLARE* type declaration (20.44 KB, patch)
2017-02-01 19:37 UTC, Yash
none Details | Review
column-chooser: Port to G_DECLARE* type declaration (20.62 KB, patch)
2017-02-01 20:00 UTC, Yash
committed Details | Review

Description Ernestas Kulik 2017-01-22 14:27:28 UTC
See bug 771777.
Comment 1 Yash 2017-02-01 13:50:50 UTC
Created attachment 344710 [details] [review]
column-chooser: Remove unnecessary macro-declaration and port to GDeclare type definiiton
Comment 2 Ernestas Kulik 2017-02-01 15:10:26 UTC
Review of attachment 344710 [details] [review]:

Thanks for working on this. :)

I grepped the sources and it appears that no other type derives from ColumnChooser, so it would make sense to make it final.

Your commit message also does not follow the format suggested here: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines
I won’t highlight the issues, because that usually means that the text in the link is never read. :p

::: src/nautilus-column-chooser.h
@@ +31,1 @@
+G_DECLARE_DERIVABLE_TYPE (NautilusColumnChooser, nautilus_column_chooser, NAUTILUS, COLUMN_CHOOSER, GtkBox);

It doesn’t look that this type is ever derived from, so try declaring it as final first.

@@ +40,3 @@
 GtkWidget *nautilus_column_chooser_new                 (NautilusFile *file);
 void       nautilus_column_chooser_set_settings    (NautilusColumnChooser   *chooser,
+						    char                   **visible_columns,

I can’t see what you’ve changed here, but leave it as it was.
Comment 3 Yash 2017-02-01 16:48:28 UTC
Created attachment 344723 [details] [review]
column-chooser: Convert NautilusColumnChooser to final,Port to GDeclare type definition

This patch also improves code readability and reduces the no. of macros being used.
Comment 4 Ernestas Kulik 2017-02-01 17:27:36 UTC
Review of attachment 344723 [details] [review]:

Almost there!

Your commit message is still not what we’re looking for. The subject line is way too long and the body is way too vague.
I implore you to read the tips section in the link I provided.

::: src/nautilus-column-chooser.c
@@ -106,3 @@
     oclass->constructed = nautilus_column_chooser_constructed;
 
-    signals[CHANGED] = g_signal_new

Why remove the signals?

::: src/nautilus-column-chooser.h
@@ +33,3 @@
 GtkWidget *nautilus_column_chooser_new                 (NautilusFile *file);
 void       nautilus_column_chooser_set_settings    (NautilusColumnChooser   *chooser,
+						    char                   **visible_columns,

Oh, trailing whitespace.

In any case, leave it be. If you want to perform style changes, please do so in a separate patch.
Comment 5 Yash 2017-02-01 18:57:28 UTC
Created attachment 344736 [details] [review]
column-chooser: Port to G_DECLARE* type declaration, Refactor NautilusColumnChooser

This patch reduces the no. of macros being used. It removes NautilusColumnChooserClass struct and moves the
details from NautilusColumnChooserDetails to _NautilusColumnChooser struct. It changes the signals[CHANGED]
and signals[USE_DEFAULT] class_offset parameter to 0 accordingly.
Comment 6 Ernestas Kulik 2017-02-01 19:11:31 UTC
Review of attachment 344736 [details] [review]:

Picking some more nits.

The subject line of the commit message should ideally fit in 50 characters.
Since it’s not that much of a refactoring, “Port to G_DECLARE* type declaration” should work well enough.

In the message body, refrain from retelling the diff. Try something like “Currently, the type declaration is done manually, which hurts readability blah blah. This patch does this and that, because reasons.”.

Focusing on the “why” is more important. Try using more general language when explaining the “what”.

::: src/nautilus-column-chooser.c
@@ +111,3 @@
                            G_TYPE_FROM_CLASS (chooser_class),
                            G_SIGNAL_RUN_LAST,
+                           0,NULL, NULL,

Add a space after the zero.

@@ +119,3 @@
                                G_TYPE_FROM_CLASS (chooser_class),
                                G_SIGNAL_RUN_LAST,
+                               0,NULL, NULL,

Here as well.
Comment 7 Yash 2017-02-01 19:37:20 UTC
Created attachment 344739 [details] [review]
column-chooser: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces the no. of macros being used and improves
readability. It removes NautilusColumnChooserClass struct and changes the NautilusColumnChooser's type to
final as there is no need for data-hiding here, and it is not being subclassed anywhere.
Comment 8 Ernestas Kulik 2017-02-01 19:50:16 UTC
Review of attachment 344739 [details] [review]:

Okay, this will be all from me, I promise.

::: src/nautilus-column-chooser.c
@@ +195,3 @@
     gboolean visible;
 
+

You added an extra newline here.

@@ +408,1 @@
                                                                       GTK_ICON_SIZE_SMALL_TOOLBAR);

Align the second argument.

@@ +416,1 @@
                                                                         GTK_ICON_SIZE_SMALL_TOOLBAR);

Align the second argument.
Comment 9 Yash 2017-02-01 20:00:01 UTC
Created attachment 344740 [details] [review]
column-chooser: Port to G_DECLARE* type declaration

Currently, the type declaration is done manually. This patch reduces the no. of macros being used and improves
readability. It removes NautilusColumnChooserClass struct and changes the NautilusColumnChooser's type to
final as there is no need for data-hiding here, and it is not being subclassed anywhere.
Comment 10 Ernestas Kulik 2017-02-01 20:05:22 UTC
Review of attachment 344740 [details] [review]:

LGTM now, thanks!
Comment 11 Ernestas Kulik 2017-02-15 09:15:46 UTC
Attachment 344740 [details] pushed as ca16997 - column-chooser: Port to G_DECLARE* type declaration