GNOME Bugzilla – Bug 757765
[PATCHes] Remove the horrific state-saver system.
Last modified: 2017-01-23 05:59:05 UTC
Created attachment 315070 [details] [review] Remove ephy_initial_state_add_expander function. Yay, there is a pseudo-GSettings integrated in the code. \o/ Let’s remove it. Here are three patches for the easier part of the job, more to come. They are built on top of the patches 5 and 6 of bug 757669.
Created attachment 315071 [details] [review] Cleaning as EphyBookmarkProperties is not resizable.
Created attachment 315072 [details] [review] Remove ephy_initial_state_add_paned function.
Created attachment 315285 [details] [review] Save EphyBookmarksEditor state the usual way. A fourth patch.
Created attachment 315334 [details] [review] Save EphyBookmarksEditor state the usual way. The bookmarks’ editor shouldn’t be fullscreenable.
Created attachment 315335 [details] [review] Save EphyBookmarksEditor state the usual way. Oops.
Should probably add a minimum (‘via’ “range”) to “bookmarks-editor-paned”, and same for the height and width of EphyBookmarksEditor.
Created attachment 315360 [details] [review] Save EphyWindow state the usual way. The hard part.
Created attachment 315361 [details] [review] Remove ephy-initial-state helpers. The cleanings.
Created attachment 315362 [details] [review] Remove unused functions. More or less unrelated cleanings.
Review of attachment 315070 [details] [review]: ::: src/bookmarks/ephy-bookmark-properties.c @@ +209,3 @@ } + settings = g_settings_new (EPHY_PREFS_WIDGETS_SCHEMA); This needs fixed; you forgot to g_object_unref() it. @@ +394,3 @@ gtk_widget_set_sensitive (GTK_WIDGET (properties->topics_scrolled_window), !lockdown); + settings = g_settings_new (EPHY_PREFS_WIDGETS_SCHEMA); Ditto
Review of attachment 315071 [details] [review]: OK
Review of attachment 315072 [details] [review]: ::: data/org.gnome.epiphany.gschema.xml @@ +263,3 @@ <summary>State of the expander in the bookmarks' properties dialog.</summary> </key> + <key type="i" name="bookmarks-editor-paned"> Probably it would be better for the name to include "width"? @@ +265,3 @@ + <key type="i" name="bookmarks-editor-paned"> + <default>200</default> + <summary>Width of the sidebar of the bookmarks' editor dialog.</summary> No apostrophe: the bookmarks are not possessing the editor. ::: src/bookmarks/ephy-bookmarks-editor.c @@ +1425,3 @@ + GSettings *settings; + + settings = g_settings_new (EPHY_PREFS_WIDGETS_SCHEMA); Leaked @@ +1717,3 @@ EPHY_INITIAL_STATE_WINDOW_SAVE_SIZE | EPHY_INITIAL_STATE_WINDOW_SAVE_POSITION); + + settings = g_settings_new (EPHY_PREFS_WIDGETS_SCHEMA); Leaked
Review of attachment 315335 [details] [review]: FYI, when I use accepted-commit-now with comments, I mean fix the comments then commit. When I use reviewed, like here, it means I want to review it again before you commit. ::: src/bookmarks/ephy-bookmarks-editor.c @@ +1506,3 @@ + EphyBookmarksEditor *editor = EPHY_BOOKMARKS_EDITOR (widget); + + GTK_WIDGET_CLASS (ephy_bookmarks_editor_parent_class)->size_allocate (widget, allocation); Technically this works, but it's very strange to "chain up" to the parent implementation of the function from a signal handler. Anyway, you'll want to leave this code unchanged: see below for why. @@ +1534,3 @@ +on_window_destroy (GtkWidget *widget) +{ + GSettings *settings = g_settings_new (EPHY_PREFS_WIDGETS_SCHEMA); Leaked @@ +1537,3 @@ + EphyBookmarksEditor *editor = EPHY_BOOKMARKS_EDITOR (widget); + + g_settings_set_int (settings, Please don't leave these extra spaces. @@ +1782,3 @@ + G_CALLBACK (on_window_state_event), editor); + g_signal_connect (editor, "destroy", + G_CALLBACK (on_window_destroy), editor); Hehe, I'm only slightly surprised this worked, but it is quite wrong. :D Don't connect with g_signal_connect; rather, override the virtual signals in class_init. For example, look at how example_app_activate() and example_app_startup() override gtk_application_activate() and gtk_application_startup() in <https://developer.gnome.org/gtk3/stable/ch01s04.html#id-1.2.3.12.5>. Also, if you were going to keep these calls to connect(), you should have passed NULL rather than editor for the last parameter, the user data parameter, since you didn't need to use the user data.
Review of attachment 315360 [details] [review]: You might also look into fixing the default window size of incognito mode windows. It's way too small currently. Ideally, it would be possible to save window state for incognito mode, instead of deleting it all each time, but that would require a bit more work I guess, since EPHY_STATES_INI_FILE will be saved in the profile directory that gets deleted. ::: src/ephy-window.c @@ +3353,3 @@ +static GSettings * +get_gsettings (void) Since you allocate a new GSettings object, I would name this with "create" or "new." @@ +3363,3 @@ + GSettingsBackend *backend; + backend = g_keyfile_settings_backend_new (g_build_filename (ephy_dot_dir (), + EPHY_STATES_INI_FILE, Good idea, this turned out much nicer than tracking states manually. @@ +3403,3 @@ + +static void +on_window_destroy (GtkWidget *widget) Any reason you have to use destroy() for this? Can't you do this in finalize() instead? @@ +3405,3 @@ +on_window_destroy (GtkWidget *widget) +{ + GSettings *settings = get_gsettings (); Leaked @@ +3607,3 @@ + { + GtkWindow *window = GTK_WINDOW (widget); + GSettings *gsettings = get_gsettings (); Leaked. @@ +3615,3 @@ + + gtk_window_set_default_size (window, + priv->current_width, The whitespace here is not quite right. Use tabs as far as you can go, then use spaces only for the remainder of the space. It's probably time to convert this file to the new 4-space coding style, but we'll get to that later. @@ +3628,3 @@ + g_signal_connect (window, "window-state-event", + G_CALLBACK (on_window_state_event), window); + g_signal_connect (window, "destroy", Again, don't do this; rather, override these signals in class_init. (And same comment about the whitespace.)
Review of attachment 315361 [details] [review]: Great, but you'll need to reupload this again in order to keep the patches ordered properly in Bugzilla; otherwise it gets too confusing to track which order they apply in.
Review of attachment 315362 [details] [review]: Hard to believe I'm rejecting a patch that removes unused functions, but I think we should keep these until we're able to delete the entire EphyNode class. That's a goal, but it requires a lot of rewriting in the bookmarks backend.
By the way, normally when me make incompatible changes, we write a profile migrator to migrate settings from the old format to the new format. But you only changed how we handle window size and expanders, and I think it's fine that these will get reset once on upgrade, so I don't expect you to write a migrator, especially since that would require not deleting all the code you did this to delete.
Comment on attachment 315070 [details] [review] Remove ephy_initial_state_add_expander function. Any plans to get back to this?
(In reply to Arnaud B. from comment #5) > Created attachment 315335 [details] [review] [review] > Save EphyBookmarksEditor state the usual way. > > Oops. OK, I removed EphyInitialState a while back, so almost all these patches are obsolete now... I'm going to resurrect this one, though, to solve bug #777055.