GNOME Bugzilla – Bug 789696
Modernize code
Last modified: 2018-05-04 13:10:09 UTC
The codebase of bijiben is pretty old. Let it be simplified by 1. Use G_DECLARE_*TYPE 2. Use private members only for Derivable classes 3. Don't initialize class instances with 0/NULL manually, as they are default. 4. Use GtkBuilder templates for UI 5. ... The wip work is in the branch https://git.gnome.org/browse/bijiben/log/?h=wip/sadiq/modernize Thanks
OK, here are a few comments so far: 1. https://git.gnome.org/browse/bijiben/commit/?h=wip/sadiq/modernize&id=b9d29ae75f1597b81061b76f780d1112df57cd33 1a. instead of "bjb-bijiben:..." in the commit title, please use "application:..."; 1b. there are some files (bjb-controller.c, bjb-empty-results-box.c and bjb-import-dialog.c) that you just removed the #include "bjb-bijiben.h" header file, so I think you should keep that in another commit, to not mix it up with those that actually started to use the new bjb-application.h file; 2. https://git.gnome.org/browse/bijiben/commit/?h=wip/sadiq/modernize&id=486e40bf70eab58ee0e4d1ef6467c4ade53660d1 2a. I'd like to see a separated section for /org/gnome/bijiben/gtk, so we could keep only menus.ui there (bjb.gresource.xml); 2b. any reason for not using compressed="true" and preprocess="xml-stripblanks" properties? 3. https://git.gnome.org/browse/bijiben/commit/?h=wip/sadiq/modernize&id=66c1a8f95241b159600c24d029de5cd5d2e04cdd OK 4. https://git.gnome.org/browse/bijiben/commit/?h=wip/sadiq/modernize&id=f7f2a8541de4f1d3706f0673634de3f2046fd0e9 OK 5. https://git.gnome.org/browse/bijiben/commit/?h=wip/sadiq/modernize&id=754314c91f73b370b7e8e91e7130eb5de8ecdc60 OK 6. https://git.gnome.org/browse/bijiben/commit/?h=wip/sadiq/modernize&id=9b03957dcbb3c0256af2658b3a9b9ab52cb9b92b 6a. please explain why we should avoid gd_tagged_entry in the commit message; Also, it would be nice if the commit message were more verbose and follow the standard lengths (https://wiki.gnome.org/Newcomers/SubmitPatch). Please add a few lines explaining this is an initiative to clean up the code and keeping it maintainable. As you can see, these are just small changes I would like to see, mainly for documentation purposes. Once that is done I can push it to master (BTW, there were new translations, so you need to update your branch again :( )
(In reply to Isaque Galdino from comment #1) > OK, here are a few comments so far: > 2b. any reason for not using compressed="true" and > preprocess="xml-stripblanks" properties? > No, I shall do that in some following commit. > Once that is done I can push it to master (BTW, there were new translations, > so you need to update your branch again :( ) There is a conflict with master. The last unicode change commit[0] might have to be reverted. [0] https://git.gnome.org/browse/bijiben/commit/?id=bf3c4c3bb382f23e6263fb57d0d7350894cf9901
(In reply to Mohammed Sadiq from comment #2) > There is a conflict with master. The last unicode change commit[0] might > have to be reverted. > Any reason why the branch can’t be rebased and the conflict resolved?
(In reply to Piotr Drąg from comment #3) > (In reply to Mohammed Sadiq from comment #2) > > There is a conflict with master. The last unicode change commit[0] might > > have to be reverted. > > > > Any reason why the branch can’t be rebased and the conflict resolved? Not much of an issue. Since the commit was smaller, I hoped reverting that would be an easier solution. Anyway, the conflict has been resolved.
Created attachment 362793 [details] [review] color-button: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference * Also, to be consistant with gnome coding standards, use 'self' as identifier name when referencing self. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 362842 [details] [review] load-more-button: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 362843 [details] [review] load-more-button: Don't override dispose handler Nothing was done in the overridden dispose handler, So just avoid it. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363011 [details] [review] settings: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363012 [details] [review] controller: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. * Don't intialize members explicitly with NULL. Because all members are initialized with NULL/0 by default. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363013 [details] [review] editor-toolbar: Remove unused variables Many widget ids from templates are set, but never used. Let it be removed. We can re-introduce them in case we use them some day This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363037 [details] [review] import-dialog: Avoid setting application manually There is no need of passing and storing the default application details. They can be abtained with g_application_get_default(). This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363039 [details] [review] main-toolbar: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. * Don't intialize members explicitly with NULL/0. Because all members are initialized with NULL/0 by default. This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 362793 [details] [review]: It looks good.
Review of attachment 362842 [details] [review]: Good.
Review of attachment 362843 [details] [review]: Good.
Review of attachment 363011 [details] [review]: In other patches you changed whatever object name as self in class methods, so I was expecting you to do the same here. For the patch itself it's good, but it would be nice if you have done that change as well. Or you can create a new one with those changes, whatever suites you better.
Review of attachment 363012 [details] [review]: Good.
Review of attachment 363037 [details] [review]: Nice one, very good, thanks!
Review of attachment 363039 [details] [review]: Good.
Review of attachment 363013 [details] [review]: Good.
Comment on attachment 363011 [details] [review] settings: Port to G_DECLARE_FINAL_TYPE Committed with suggested changes
All patches commmitted. Thanks for the review.
Created attachment 363111 [details] [review] import-dialog: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363112 [details] [review] main-view: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. * Don't intialize members explicitly with NULL/0. Because all members are initialized with NULL/0 by default. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363113 [details] [review] note-view: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363114 [details] [review] organize-dialog: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. * Don't intialize members explicitly with NULL/0. Because all members are initialized with NULL/0 by default. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363115 [details] [review] selection-toolbar: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363118 [details] [review] window-base: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. * Don't intialize members explicitly with NULL/0. Because all members are initialized with NULL/0 by default. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363127 [details] [review] biji-item: Port to G_DECLARE_DERIVABLE_TYPE This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363130 [details] [review] notebook: port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. * Don't intialize members explicitly with NULL/0. Because all members are initialized with NULL/0 by default. * Adapt main-view and controller accordingly This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 363111 [details] [review]: Good.
Review of attachment 363112 [details] [review]: Good.
Review of attachment 363113 [details] [review]: Good.
Review of attachment 363114 [details] [review]: Good.
Review of attachment 363115 [details] [review]: Good.
Review of attachment 363118 [details] [review]: Good.
Review of attachment 363127 [details] [review]: Thanks for your patch. Would you please explain a little bit more what does your patch did? Thanks.
Review of attachment 363130 [details] [review]: Good.
Created attachment 363188 [details] [review] biji-item: Port to G_DECLARE_DERIVABLE_TYPE * Use G_DECLARE_DERIVABLE_TYPE to avoid boilerplate code * Adapt private member identifier names to suite with G_DECLARE_DERIVABLE_TYPE signature * Adapt bjb-controller to avoid warnings with non-const variables This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 363188 [details] [review]: Thanks for your patch. It fixed compilation in my box, so it's a winner. Please push it ASAP.
Comment on attachment 363188 [details] [review] biji-item: Port to G_DECLARE_DERIVABLE_TYPE Thanks for the review. Pushed to master
Created attachment 363556 [details] [review] import-dialog: Use symbolic icons Using symbolic icons will improve the UI, and help us remove some old code that was looking for the application icons manually. Let us not care whether those application is actually installed, but only check if their data directory is present. Which means that even the application (Gnote/Tomboy) got uninstalled, their notes can be imported in Bijiben. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363585 [details] [review] info-set: Don't manually reset struct members g_slice_new0() sets allocated memory to 0. It is thus redundant to set them again to 0. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363653 [details] [review] tomboy-reader: port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 363875 [details] [review] note-id: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. * Don't intialize members explicitly with NULL/0. Because all members are initialized with NULL/0 by default. This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 363556 [details] [review]: Hi Sadiq, thanks for your patch. Although I understand this is related to our effort to modernize code, I rather would related it to a new bug fix, just for import-dialog, because this looks like more of a feature change than a code clean up. I'll approve it anyway, but for other stuff that sound like changing behaviour or feature, please open an specific bug, so we can track it a little better.
Review of attachment 363585 [details] [review]: Good.
Review of attachment 363653 [details] [review]: Good.
Review of attachment 363875 [details] [review]: Good.
Comment on attachment 363556 [details] [review] import-dialog: Use symbolic icons Thanks for the review. I shall file new bugs for such changes next time onwards.
Created attachment 364003 [details] [review] manager: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 364004 [details] [review] lazy-deserializer: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 364005 [details] [review] lazy-serializer: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 364006 [details] [review] note-obj: Port to G_DECLARE_DERIVABLE_TYPE * Use G_DECLARE_DERIVABLE_TYPE to avoid boilerplate code * Adapt private member identifier names to suite with G_DECLARE_DERIVABLE_TYPE signature This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 364003 [details] [review]: In this patch you only commented out BIJI_MANAGER_PRIVATE define instead of removing it. Please take a look: @@ -77,7 +79,7 @@ static GParamSpec *properties[BIJI_MANAGER_PROPERTIES] = { NULL, }; static void biji_manager_initable_iface_init (GInitableIface *iface); static void biji_manager_async_initable_iface_init (GAsyncInitableIface *iface); -#define BIJI_MANAGER_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), BIJI_TYPE_MANAGER, BijiManagerPrivate)) +/* #define BIJI_MANAGER_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), BIJI_TYPE_MANAGER, BijiManagerPrivate)) */
Review of attachment 364004 [details] [review]: Good.
Review of attachment 364005 [details] [review]: Good.
Review of attachment 364006 [details] [review]: Good.
Comment on attachment 364003 [details] [review] manager: Port to G_DECLARE_FINAL_TYPE Thanks for the review. Pushed with changes.
Created attachment 364157 [details] [review] controller: Fixed missing cast In commit 63f883710b91fbe8db3fd51fd48370f1e06cb2fe, it was added a regression. Build was failing due to a missing cast in most_recent_item_first method of BjbController object. This patch adds the missing cast, so the code can be build again.
Created attachment 364158 [details] [review] controller: Fixed missing cast In commit 63f883710b91fbe8db3fd51fd48370f1e06cb2fe, it was added a regression. Build was failing due to a missing cast in most_recent_item_first method of BjbController object. This patch adds the missing cast, so the code can be build again.
Review of attachment 364157 [details] [review]: This commit was duplicated here!
Review of attachment 364158 [details] [review]: Pushed to master.
Created attachment 364310 [details] [review] settings-dialog: Don't override getter and setter The overridden function didn't do anything useful. So better not override. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 364318 [details] [review] organize-dialog: Remove dead code
Created attachment 364375 [details] [review] search-toolbar: Remove unused headers
Created attachment 364410 [details] [review] provider: Port to G_DECLARE_DERIVABLE_TYPE * Use G_DECLARE_DERIVABLE_TYPE to avoid boilerplate code * Adapt private member identifier names to suite with G_DECLARE_DERIVABLE_TYPE signature This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 364411 [details] [review] local-provider: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 364412 [details] [review] memo-provider: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 364310 [details] [review]: Good.
Review of attachment 364318 [details] [review]: Good.
Review of attachment 364375 [details] [review]: Good.
Review of attachment 364410 [details] [review]: Good.
Review of attachment 364411 [details] [review]: Good.
Review of attachment 364412 [details] [review]: Good.
Thanks for the review, pushed to master. Attachment 364310 [details] pushed as 0e3c6c9 - settings-dialog: Don't override getter and setter Attachment 364318 [details] pushed as 6b79c04 - organize-dialog: Remove dead code Attachment 364375 [details] pushed as e5ecec7 - search-toolbar: Remove unused headers Attachment 364410 [details] pushed as 7aa5f24 - provider: Port to G_DECLARE_DERIVABLE_TYPE Attachment 364411 [details] pushed as 7254ce2 - local-provider: Port to G_DECLARE_FINAL_TYPE Attachment 364412 [details] pushed as dd325ce - memo-provider: Port to G_DECLARE_FINAL_TYPE
Created attachment 365347 [details] [review] window-base: Simplify saving/loading window states Follow https://wiki.gnome.org/HowDoI/SaveWindowState * Use tuples in gsetting schema to save size and position. * Save changes to variable instead of saving to gsettings after every timemout. * Save the changes back to gsettings when the window is destroyed.
Created attachment 365470 [details] [review] main-toolbar: Don't handle window drag Dragging window by clicking GtkHeaderBar is done by default. Don't redo it.
Created attachment 365672 [details] [review] editor-toolbar: Remove private struct * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 365347 [details] [review]: Good.
Review of attachment 365470 [details] [review]: Good, thx.
Review of attachment 365672 [details] [review]: Good.
Created attachment 366796 [details] [review] own-cloud-provider: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 366797 [details] [review] own-cloud-note: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 366796 [details] [review]: lgtm. Push to master after the suggested change ::: src/libbiji/provider/biji-own-cloud-provider.c @@ +836,3 @@ + self->queue = g_queue_new (); + self->path = NULL; + self->folder = NULL; Instance struct members are 0/NULL initialized by default. So the two lines can be removed.
Review of attachment 366797 [details] [review]: lgtm. Push to master after the suggested changes ::: src/libbiji/provider/biji-own-cloud-note.c @@ -321,3 +320,3 @@ - self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, BIJI_TYPE_OWN_CLOUD_NOTE, BijiOwnCloudNotePrivate); - self->priv->cancellable = g_cancellable_new (); - self->priv->id = NULL; + self->cancellable = g_cancellable_new (); + self->id = NULL; + self->needs_save = FALSE; Same as above (remove id and needs_save line)
Created attachment 366858 [details] [review] own-cloud-provider: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Created attachment 366860 [details] [review] own-cloud-note: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
Review of attachment 366858 [details] [review]: lgtm. Please push to master. Thanks
Review of attachment 366860 [details] [review]: Please update the patch and push to master. Thanks ::: src/libbiji/provider/biji-own-cloud-note.c @@ -321,3 +320,3 @@ - self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, BIJI_TYPE_OWN_CLOUD_NOTE, BijiOwnCloudNotePrivate); - self->priv->cancellable = g_cancellable_new (); - self->priv->id = NULL; + self->cancellable = g_cancellable_new (); + self->id = NULL; + self->needs_save = FALSE; Did you forget to make the change?
Created attachment 366882 [details] [review] own-cloud-note: Port to G_DECLARE_FINAL_TYPE * Use G_DECLARE_FINAL_TYPE to avoid boilerplate code * Don't use private members. This class isn't derivable. So there isn't any difference. This is a part of effort to clean up codebase and make the code more maintainable.
(In reply to Mohammed Sadiq from comment #98) > Review of attachment 366860 [details] [review] [review]: > > Please update the patch and push to master. Thanks > > ::: src/libbiji/provider/biji-own-cloud-note.c > @@ -321,3 +320,3 @@ > - self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, BIJI_TYPE_OWN_CLOUD_NOTE, > BijiOwnCloudNotePrivate); > - self->priv->cancellable = g_cancellable_new (); > - self->priv->id = NULL; > + self->cancellable = g_cancellable_new (); > + self->id = NULL; > + self->needs_save = FALSE; > > Did you forget to make the change? My mistake. Fixed.
Review of attachment 366882 [details] [review]: lgtm. push to master
Review of attachment 366858 [details] [review]: Thx.
Review of attachment 366882 [details] [review]: Thx.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-notes/issues/82.