GNOME Bugzilla – Bug 771777
port to G_DECLARE*_TYPE
Last modified: 2017-11-05 21:17:14 UTC
porting nautilus code to G_DECLARE*_TYPE shall reduce a a huge number of boilerplate code. That would make nautilus code better and easier for newcomers and others alike.
Yes, however this is a implementation detail, and it's too abstract. Not sure it worth to have a bug report. Only case I can think of is to assign newcomers tasks, but we already discussed this in the channel and we decided this task was too tedious for newcomers. So I'm closing as not a bug.
(In reply to Carlos Soriano from comment #1) > Only case I can think of is to assign newcomers tasks, but we already > discussed this in the channel and we decided this task was too tedious for > newcomers. We did? I only remember you agreeing with me that NautilusFilesView was maybe too much for a newcomer (the newcomer I had in mind was one, who has little coding experience). Otherwise, I think this is great. > Yes, however this is a implementation detail, and it's too abstract. Not > sure it worth to have a bug report. This could be a metabug for tracking (i.e. playing with dependencies, for easier discovery and whatnot).
Two against one, you win :) reopening and marking for newcomers.
Created attachment 358077 [details] [review] list-view: ported declaration to G_DECLARE* This patch ports declaration of NautilusListView to the G_DECLARE* format.
Created attachment 358078 [details] [review] icon-info: ported declaration to G_DECLARE* This patch ports declaration of NautilusListView to the G_DECLARE* format.
Created attachment 358079 [details] [review] icon-info: ported declaration to G_DECLARE* This patch ports declaration of NautilusIconInfo to the G_DECLARE* format.
Review of attachment 358077 [details] [review]: Thanks! ::: src/nautilus-list-view.h @@ +33,3 @@ typedef struct NautilusListViewDetails NautilusListViewDetails; +struct _NautilusListView{ Move the struct to the source file. No reason to expose it. @@ +40,1 @@ GType nautilus_list_view_get_type (void); Unnecessary, the G_DECLARE_FINAL_TYPE macro declares this for us.
Review of attachment 358078 [details] [review]: Looks good, thanks. ::: src/nautilus-icon-info.h @@ +54,1 @@ GType nautilus_icon_info_get_type (void) G_GNUC_CONST; Not needed.
(In reply to Ernestas Kulik from comment #7) > Review of attachment 358077 [details] [review] [review]: > > Thanks! > > ::: src/nautilus-list-view.h > @@ +33,3 @@ > typedef struct NautilusListViewDetails NautilusListViewDetails; > > +struct _NautilusListView{ > > Move the struct to the source file. No reason to expose it. Scratch this part, let me dig a little deeper.
Created attachment 358080 [details] [review] search-engine-tracker: ported declaration to G_DECLARE* This patch ports declaration of NautilusSearchEngineTracker to the G_DECLARE* format.
Created attachment 358081 [details] [review] search-engine-model: ported declaration to G_DECLARE* This patch ports declaration of NautilusSearchEngineModel to the G_DECLARE* format.
Review of attachment 358077 [details] [review]: ::: src/nautilus-list-view.h @@ +33,3 @@ typedef struct NautilusListViewDetails NautilusListViewDetails; +struct _NautilusListView{ My bad, there is reason to expose it. We’ll have to live with it for now. Do move the brace to a new line, though.
Review of attachment 358080 [details] [review]: Huzzah! ::: src/nautilus-search-engine-tracker.h @@ +27,3 @@ #define NAUTILUS_TYPE_SEARCH_ENGINE_TRACKER (nautilus_search_engine_tracker_get_type ()) +G_DECLARE_FINAL_TYPE (NautilusSearchEngineTracker, nautilus_search_engine_tracker, NAUTILUS, SEARCH_ENGINE_TRACKER, GObject) +/* Forgot your comments there. :p @@ +36,3 @@ typedef struct NautilusSearchEngineTrackerDetails NautilusSearchEngineTrackerDetails; +struct _NautilusSearchEngineTracker { I take it someone is poking the internal state of this one, too? If so, move the brace to a new line.
Created attachment 358082 [details] [review] special-location-bar: ported declaration to G_DECLARE* This patch ports declaration of NautilusSpecialLocationBar to the G_DECLARE* format.
(In reply to Vyas Giridhar from comment #14) > Created attachment 358082 [details] [review] [review] > special-location-bar: ported declaration to G_DECLARE* > > This patch ports declaration of NautilusSpecialLocationBar to > the G_DECLARE* format. Could you create a new bug for each patch? Thanks.
(In reply to Ernestas Kulik from comment #15) > (In reply to Vyas Giridhar from comment #14) > > Created attachment 358082 [details] [review] [review] [review] > > special-location-bar: ported declaration to G_DECLARE* > > > > This patch ports declaration of NautilusSpecialLocationBar to > > the G_DECLARE* format. > > Could you create a new bug for each patch? Thanks. For any new one, I can deal with the current ones being here.
Review of attachment 358082 [details] [review]: Thanks! We can take it one step further, though. :) ::: src/nautilus-special-location-bar.h @@ +30,3 @@ typedef struct NautilusSpecialLocationBarPrivate NautilusSpecialLocationBarPrivate; +struct _NautilusSpecialLocationBar This one we can definitely move in to the source file. As a bonus, get rid of the NautilusSpecialLocationBarPrivate struct by merging it with this one. (That also means removing things like g_type_class_add_private()). @@ +44,1 @@ GType nautilus_special_location_bar_get_type (void) G_GNUC_CONST; Unneeded.
Review of attachment 358081 [details] [review]: This is more less the same as special-location-bar, in that we can clean it up further. ::: src/nautilus-search-engine-model.h @@ +30,3 @@ typedef struct NautilusSearchEngineModelDetails NautilusSearchEngineModelDetails; +struct _NautilusSearchEngineModel { As with special-location-bar, move it to the source file, merge the details struct with this one and get ride of any mention of private data, etc. @@ +37,1 @@ GType nautilus_search_engine_model_get_type (void); This is unneeded.
Created attachment 358153 [details] [review] list-view: ported declaration to G_DECLARE* This patch ports declaration of NautilusListView to the G_DECLARE* format.
(In reply to Ernestas Kulik from comment #7) > @@ +40,1 @@ > GType nautilus_list_view_get_type (void); > > Unnecessary, the G_DECLARE_FINAL_TYPE macro declares this for us. It doesn't. https://github.com/GNOME/glib/blob/master/gobject/gtype.h#L1395
(In reply to Vyas Giridhar from comment #20) > (In reply to Ernestas Kulik from comment #7) > > > @@ +40,1 @@ > > GType nautilus_list_view_get_type (void); > > > > Unnecessary, the G_DECLARE_FINAL_TYPE macro declares this for us. > > It doesn't. > https://github.com/GNOME/glib/blob/master/gobject/gtype.h#L1395 The line after the one you linked to is exactly that.
I will re upload the patches with the changes
Created attachment 358155 [details] [review] list-view: ported declaration to G_DECLARE* This patch ports declaration of NautilusListView to the G_DECLARE* format.
Created attachment 358156 [details] [review] icon-info: ported declaration to G_DECLARE* This patch ports declaration of NautilusIconInfo to the G_DECLARE* format.
Review of attachment 358155 [details] [review]: Looks fine, thanks.
Review of attachment 358156 [details] [review]: Yup.
Attachment 358155 [details] pushed as aec6026 - list-view: ported declaration to G_DECLARE* Attachment 358156 [details] pushed as 2660473 - icon-info: ported declaration to G_DECLARE*
Created attachment 358159 [details] [review] search-engine-model: ported declaration to G_DECLARE* This patch ports declaration of NautilusSearchEngineModel to the G_DECLARE* format.
Review of attachment 358159 [details] [review]: Looks good.
Created attachment 358168 [details] [review] search-engine-tracker: ported declaration to G_DECLARE* This patch ports declaration of NautilusSearchEngineTracker to the G_DECLARE* format.
Created attachment 358169 [details] [review] special-location-bar: ported declaration to G_DECLARE* This patch ports declaration of NautilusSpecialLocationBar to the G_DECLARE* format.
Review of attachment 358168 [details] [review]: Sure. I’ll fix up the indentation on my end, no need to waste time. ::: src/nautilus-search-engine-tracker.c @@ +36,2 @@ { + GObject parent; We indent using spaces (4 of them).
Review of attachment 358169 [details] [review]: Thanks! ::: src/nautilus-special-location-bar.c @@ +29,2 @@ { + GtkInfoBar parent; Another tab used here.
Attachment 358159 [details] pushed as 5882308 - search-engine-model: ported declaration to G_DECLARE* Attachment 358168 [details] pushed as ddeca26 - search-engine-tracker: ported declaration to G_DECLARE* Attachment 358169 [details] pushed as 5898ce2 - special-location-bar: ported declaration to G_DECLARE*
All done, we can go home. Thanks Ernestas for keeping with this so much!