Created attachment 204778 [details] [review]
GtkWindow: new API to store state in GSettings

Created attachment 204779 [details] [review]
GtkWindow: new API to store state in GSettings

Forgot to actually cancel the source from the callback (which is needed
when it is directly invoked).

Attachment 204779 [details] pushed as 730765d - GtkWindow: new API to store state in GSettings

Review of attachment 204779 [details] [review]:

::: gtk/gtkwindow.c
@@ +4622,3 @@
+    }
+
+  g_source_remove (window->priv->state_save_id);

Why remove the source here when you are returning G_SOURCE_REMOVE 3 lines down anyway ?

@@ +5472,3 @@
+        g_source_remove (window->priv->state_save_id);
+
+      window->priv->state_save_id = g_timeout_add (1000, gtk_window_persistent_state_flush, window);

Could do g_timeout_add_seconds here, I guess

@@ +9814,3 @@
+ * ]|
+ *
+ * You may only call this function once per window.

Should perhaps also say something about choosing a different child_name for each window ? Although it could be entertaining to use the same name for all your toplevels...ah no, you only update from the settings initially, so it won't be nearly as entertaining.

@@ +9822,3 @@
+                                   GSettings   *settings,
+                                   const gchar *child_name)
+{

I'm not super-happy with the name. 'state' seems too generic, when we are really only dealing with some partial window geometry. 'setup' and 'persistent' are not my first choice either.

gtk_window_set_geometry_settings ?
gtk_window_set_saved_size ?

- Why do we only store this information? Why not for example the initial position of the window?
- Why is this window-specific? Shouldn't there be something more generic that works with all widgets? People often want to save the position of a GtkPaned ot the state of a GtkExpander. And of course the file chooser...
- How does this interact with non-resizable windows?

I feel that this is a very ad-hoc way of throwing state-keeping APIs into GTK widgets. I'd rather see people continuing to bind to regular properties, that seems to work fine. If it doesn't here, we should fix that roblem but not introduce random GSettings.

Note that GtkFileChooserDialog already uses gsettings to store window geometry. Worth checking out for comparison, if nothing else.

As for the idea of storing arbitrary toolkit state, look at bug 79285 for old discussion of the complexities. I too used to think it was a great idea when I was young.

With regard to the committed patch note that you also need to special case FULLSCREEN and to not save the width and height in that case, otherwise next time you get a huge windows.

This is the save state code in gedit btw, that worked fairly well for a long time http://git.gnome.org/browse/gedit/tree/gedit/gedit-window.c#n222


I am also not convinced by the timeout instead of simply saving when the window is closed: note that with apps with many windows (e.g. gedit) saving at close gets you the state of the last closed window (which is what you want), while with a timeout you get the state of a random window.

Relevant reading about state saving:

bug 79285

https://live.gnome.org/SessionManagement/SavingState



I've reverted the patch for now.

We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.

As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new