GNOME Bugzilla – Bug 757669
[PATCHes] EphyBookmarkProperties: Use template.
Last modified: 2015-11-21 00:33:51 UTC
Created attachment 314961 [details] [review] EphyBookmarkProperties: Make final class. Here are a few patch to move the UI stuff of the EphyBookmarkProperties’ class in an UI file.
Created attachment 314962 [details] [review] EphyBookmarkProperties: Use template.
Created attachment 314964 [details] [review] EphyTopicsEntry: Make final class.
Created attachment 314965 [details] [review] EphyTopicsEntry: Make final class. Forgot a cleanup.
Created attachment 314966 [details] [review] EphyTopicsPalette: Make final class.
Review of attachment 314962 [details] [review]: Impressive cleanup ::: src/Makefile.am @@ +105,3 @@ RESOURCE_FILES = \ resources/about.css \ + resources/bookmark-properties.ui \ Use one fewer tab character ::: src/bookmarks/ephy-bookmark-properties.c @@ +307,1 @@ object = G_OBJECT_CLASS (ephy_bookmark_properties_parent_class)->constructor (type, We should get rid of constructor overrides wherever possible, but init is too soon to do this sort of work; it's kind of obvious, so you should remove this comment. I bet you can override constructed instead. It would be educational to try this in a follow-up patch, if you want. Object construction is a bit of a complicated mess, but as a general rule, whenever you're tempted to override constructor, you can probably override constructed instead. I've never needed constructor in new code. Details: https://developer.gnome.org/gobject/unstable/chapter-gobject.html#gobject-instantiation @@ +432,1 @@ + object_class->constructor = ephy_bookmark_properties_constructor; Don't line them up. (Have I really not noticed so far that you've been doing this? :)
Created attachment 315029 [details] [review] EphyTopicsPalette: Move UI stuff in EphyBookmarkProperties.
Created attachment 315030 [details] [review] EphyTopicsPalette: Inherit from GtkListStore, not from GtkTreeView.
Created attachment 315031 [details] [review] EphyTopicsPalette: Move UI stuff in EphyBookmarkProperties. Oops, forgot to clean the UI file.
(In reply to Arnaud B. from comment #7) > Created attachment 315030 [details] [review] [review] > EphyTopicsPalette: Inherit from GtkListStore, not from GtkTreeView. Dunno why, but the patch doesn't apply anymore, even though there were definitely no changes to these files... try rebasing I guess.
315030 is the logical end of 315029/315031, try applying the latter first.
Created attachment 315057 [details] [review] EphyTopicsPalette: Inherit from GtkListStore, not from GtkTreeView. Just for reordering.
It's too hard to evaluate the changes in these last patches, since the dialog is quite broken currently by GTK+ theme changes, both with and without your patches. I can't see any of the checkboxes in the bookmarks properties dialog, only the checkmarks themselves (with and without the patches).
Created attachment 315068 [details] [review] EphyTopicsPalette: Move UI stuff in EphyBookmarkProperties. Rebuilt against master.
Created attachment 315069 [details] [review] EphyTopicsPalette: Inherit from GtkListStore, not from GtkTreeView. Rebuilt against master.
Created attachment 315433 [details] [review] ephy-topics-entry.h: Remove unused function declaration.
Review of attachment 315068 [details] [review]: ::: src/bookmarks/ephy-bookmark-properties.c @@ +288,3 @@ + + /* Protect against toggling separators. */ + if (topic == NULL) return; Put the return on the next line, please. ::: src/bookmarks/ephy-topics-palette.h @@ +39,3 @@ + COLUMN_SELECTED, + COLUMNS +}; This shouldn't be in the header file. We don't want it visible to other files.
Review of attachment 315069 [details] [review]: OK Keep in mind Allan's design calls for removing the bookmarks dialog and properties dialog, so this is probably not a good area to focus further time.
Review of attachment 315433 [details] [review]: OK
Comment on attachment 315068 [details] [review] EphyTopicsPalette: Move UI stuff in EphyBookmarkProperties. As the enum *is* shared between two files, I’ve renamed its members before commiting.