GNOME Bugzilla – Bug 575544
gnome-session-properties needs button to save session
Last modified: 2014-03-05 11:39:09 UTC
Please describe the problem: With the fix to http://bugzilla.gnome.org/show_bug.cgi?id=552387 we now have session saving capability in gnome-session. The capplet saving button was disabled since Vincent may not have time to implement it. So I just chipped in whatever I think I can do. Steps to reproduce: 1. launch gnome-session-properties and click on options tab 2. Remember current running application button is made invisible 3. Actual results: Expected results: Does this happen every time? Other information:
Created attachment 130737 [details] [review] patch to provide button to save session on g-s-properties The patch is again trunk as on Mon Mar 16 11:41:41 GMT 2009
The patch is pretty much can be applied to gnome-session 2.25.92 tarball bar the glade file changes.
A similar patch for gnome-session-save is also attached in bug http://bugzilla.gnome.org/show_bug.cgi?id=575546
FWIW, I had a patch like this, and it doesn't work perfectly. Try it :-) Metacity will not save the window placement, eg.
Yeah, you are right. What's missing, ability to specify which workspace ? In fact, I don't even have $HOME/.metacity :(
Created attachment 132554 [details] [review] Improve UI This version improves a bit the UI part, removing the annoying dialog.
(In reply to comment #4) > FWIW, I had a patch like this, and it doesn't work perfectly. Try it :-) > Metacity will not save the window placement, eg. The problem is the same with all applications needing to save their state. The session manager needs to send a SaveYourself request to each client, and it only does in the shutdown phase.
Created attachment 132596 [details] [review] Add support for 2-step session saving in the running phase This new version of patch should do the trick. It adds full support for session saving in the RUNNING phase, in a way similar to what is done in the logout phases. Some pieces of the code could use some refactoring to make it less repetitive, but it already seems to work fine.
Created attachment 132627 [details] [review] The same without saving the session all the time A small update to fix a stupid bug leading to saving the session every time an application sent a SaveYourselfComplete request.
FYI, so far I’m having positive feedback about this patch, included in Debian experimental.
Patch changes UI and adds strings. -> setting gnome 2.28 target instead of 2.26.
Note that the translations already exist for the added strings, since they are exactly the same as the ones in g-session 2.22.
Will this get reviewed for 2.28 at all ?
not urgent enough for 2.28 after talking to the devs -> removing gnome-target
Created attachment 141238 [details] [review] Updated patch for 2.26.2
Created attachment 147095 [details] [review] Updated patch for gnome-session 2.28.0 Gnome-session 2.28 needs a transition to GtkUiManager.
(In reply to comment #14) > not urgent enough for 2.28 after talking to the devs -> removing gnome-target Would be really interesting to try it into 2.30 at least :-) Thanks a lot!
Created attachment 169849 [details] [review] Updated patch for latest master
Will this be fixed some day? Maybe for Gnome3? Thanks
Review of attachment 169849 [details] [review]: I have a various comments with this patch, see below. A general issue is that it's highly similar to the code saving the session at the end of the session, and still the code is mostly duplicated because a bit different. I wonder if we could improve this. ::: gnome-session/gsm-client.h @@ +143,3 @@ +gboolean gsm_client_request_save (GsmClient *client, + guint flags, + GError **error); I'm not a big fan of request_save, as at first read, you can believe it will really save the while client while it will only save the state on the client-side, and gnome-session still has to save something later on. It's also a bit confusing with gsm_client_save(). So maybe call it prepare_save? It might make sense to also indicate in the name that it's only for a running session. So maybe prepare_save_while_running. Don't know. ::: gnome-session/gsm-manager.c @@ +1157,3 @@ +static gboolean +_client_request_save (GsmClient *client, + ClientEndSessionData *data) If we use ClientEndSessionData for this, then maybe it should be renamed to something else since this not about ending the session... @@ +1164,3 @@ + error = NULL; + ret = gsm_client_request_save (client, data->flags, &error); + if (ret) { Any reason to invert the if() compared to _client_end_session()? It makes it harder to compare the two functions. @@ +1168,3 @@ + data->manager->priv->query_clients = g_slist_prepend (data->manager->priv->query_clients, + client); + } else if (error) { Even if error is NULL (why would it be, btw?), we should have the debug. @@ +1200,3 @@ + + g_slist_free (manager->priv->next_query_clients); + manager->priv->next_query_clients = NULL; I'd prefer to have this in a separate function, a bit like what we have for do_phase_end_session_part_2(). Also, it's wrong to have it here since this function can be called because of the timeout, and we don't want to do this if we timed out already. So it should be called from on_client_end_session_response(). Also reading this code again, I think we have a bug, even in the old code: when we do the g_slist_foreach on next_query_clients, we should probably copy next_query_clients to query_clients instead of freeing it. Else, if there are multiple clients in there, we will stop after the first one answers. @@ +1208,3 @@ + g_source_remove (manager->priv->query_timeout_id); + manager->priv->query_timeout_id = 0; + } It might make sense to ensure that query_clients and next_query_clients are reset. We're very paranoid about this in the code. @@ +1210,3 @@ + } + + gsm_session_save (manager->priv->clients, &error); You probably want to call maybe_save_session() here instead. @@ +1302,3 @@ + manager->priv->query_clients = NULL; + + query_save_session_complete (manager); So we have a timeout. Do we really want to save the session in that case? Because to me, it sounds like a failure. But maybe best-effort is okay. In all case, a comment with a rationale for the decision would be good. @@ +1942,2 @@ const char *reason, GsmManager *manager) I'll point out that this function is called "on_client_end_session_response". If we re-use it for live save, then it should be renamed (which implies renaming the signal). @@ +1950,3 @@ g_debug ("GsmManager: Response from end session request: is-ok=%d do-last=%d cancel=%d reason=%s", is_ok, do_last, cancel, reason ? reason :""); + if (manager->priv->phase == GSM_MANAGER_PHASE_RUNNING) { I really don't like how this kind of duplicates the rest of the function, but it's different. I think I'd prefer to have the code in a different function, and I'd make it mimic the current function. @@ +2072,3 @@ +{ + g_debug ("GsmManager: save_request"); + gsm_manager_save_session (manager, NULL); (remove if we remove the save_request signal) @@ +2099,3 @@ + "save-request", + G_CALLBACK (on_xsmp_client_save_request), + manager); (remove if we remove the save_request signal) @@ +3059,3 @@ +gsm_manager_save_session (GsmManager *manager, + GError **error) +{ Since we have a timeout for this, it might make sense to make this dbus method async, so we can return an error later on. @@ +3087,3 @@ + } else { + g_debug ("GsmManager: Nothing to save"); + return FALSE; Why FALSE? It's perfectly fine if there's nothing to save. ::: gnome-session/gsm-xsmp-client.c @@ +508,3 @@ + guint flags, + GError **error) +{ This code is extremely similar to xsmp_end_session(), and yet there are details that are different (no use of a phase2 variable, for example). I suggest to make it closer to xsmp_end_session(). @@ +1042,3 @@ + g_cclosure_marshal_VOID__BOOLEAN, + G_TYPE_NONE, + 1, G_TYPE_BOOLEAN); This signal is added but not used. We could potentially use it in save_yourself_request_callback() (if !shutdown && global), but I'm not even sure we care about it. So either it shouldn't be added, or the code should be completed. ::: gnome-session/org.gnome.SessionManager.xml @@ +257,3 @@ </method> + <method name="SaveSession"> (make it async)
Review of attachment 169849 [details] [review]: ::: gnome-session/gsm-manager.c @@ +1950,3 @@ g_debug ("GsmManager: Response from end session request: is-ok=%d do-last=%d cancel=%d reason=%s", is_ok, do_last, cancel, reason ? reason :""); + if (manager->priv->phase == GSM_MANAGER_PHASE_RUNNING) { Also, we really shouldn't do anything here except run the discard command from the XSMP client if we're after the timeout. I'm actually not even sure how to handle a client that answers after the timeout, since we might even be in another phase... The issue is that if we just ignore it, there will be a session state file on disk -- that's why we should probably call the discard command. But maybe we should keep a list of clients that haven't replied in time, and check the client is in this list?
gnome-session-properties is no longer included in gnome-session.