After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 575544 - gnome-session-properties needs button to save session
gnome-session-properties needs button to save session
Status: RESOLVED OBSOLETE
Product: gnome-session
Classification: Core
Component: gnome-session-properties
git master
Other All
: Normal major
: ---
Assigned To: Session Maintainers
Session Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-03-16 11:40 UTC by Ghee Teo
Modified: 2014-03-05 11:39 UTC
See Also:
GNOME target: ---
GNOME version: 2.25/2.26


Attachments
patch to provide button to save session on g-s-properties (8.69 KB, patch)
2009-03-16 11:42 UTC, Ghee Teo
none Details | Review
Improve UI (11.23 KB, patch)
2009-04-12 15:40 UTC, Josselin Mouette
none Details | Review
Add support for 2-step session saving in the running phase (21.62 KB, patch)
2009-04-13 14:52 UTC, Josselin Mouette
none Details | Review
The same without saving the session all the time (21.60 KB, patch)
2009-04-14 08:15 UTC, Josselin Mouette
none Details | Review
Updated patch for 2.26.2 (21.51 KB, patch)
2009-08-20 11:26 UTC, Josselin Mouette
none Details | Review
Updated patch for gnome-session 2.28.0 (21.24 KB, patch)
2009-11-06 13:07 UTC, Josselin Mouette
none Details | Review
Updated patch for latest master (20.92 KB, patch)
2010-09-09 12:46 UTC, Priit Laes (IRC: plaes)
needs-work Details | Review

Description Ghee Teo 2009-03-16 11:40:17 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:
Comment 1 Ghee Teo 2009-03-16 11:42:15 UTC
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
Comment 2 Ghee Teo 2009-03-16 11:43:40 UTC
The patch is pretty much can be applied to gnome-session 2.25.92 tarball bar the glade file changes.
Comment 3 Ghee Teo 2009-03-16 11:59:28 UTC
A similar patch for gnome-session-save is also attached in bug http://bugzilla.gnome.org/show_bug.cgi?id=575546
Comment 4 Vincent Untz 2009-03-16 12:35:11 UTC
FWIW, I had a patch like this, and it doesn't work perfectly. Try it :-) Metacity will not save the window placement, eg.
Comment 5 Ghee Teo 2009-03-16 16:12:08 UTC
Yeah, you are right. What's missing, ability to specify which workspace ?
In fact, I don't even have $HOME/.metacity :(
Comment 6 Josselin Mouette 2009-04-12 15:40:26 UTC
Created attachment 132554 [details] [review]
Improve UI

This version improves a bit the UI part, removing the annoying dialog.
Comment 7 Josselin Mouette 2009-04-12 16:26:28 UTC
(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.
Comment 8 Josselin Mouette 2009-04-13 14:52:51 UTC
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.
Comment 9 Josselin Mouette 2009-04-14 08:15:11 UTC
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.
Comment 10 Josselin Mouette 2009-04-27 15:09:38 UTC
FYI, so far I’m having positive feedback about this patch, included in Debian experimental.
Comment 11 André Klapper 2009-06-18 09:18:20 UTC
Patch changes UI and adds strings. -> setting gnome 2.28 target instead of 2.26.
Comment 12 Josselin Mouette 2009-06-18 10:50:17 UTC
Note that the translations already exist for the added strings, since they are exactly the same as the ones in g-session 2.22.
Comment 13 Gilles Dartiguelongue 2009-07-19 20:31:08 UTC
Will this get reviewed for 2.28 at all ?
Comment 14 André Klapper 2009-08-17 10:51:08 UTC
not urgent enough for 2.28 after talking to the devs -> removing gnome-target
Comment 15 Josselin Mouette 2009-08-20 11:26:43 UTC
Created attachment 141238 [details] [review]
Updated patch for 2.26.2
Comment 16 Josselin Mouette 2009-11-06 13:07:00 UTC
Created attachment 147095 [details] [review]
Updated patch for gnome-session 2.28.0

Gnome-session 2.28 needs a transition to GtkUiManager.
Comment 17 Pacho Ramos 2010-01-06 15:27:03 UTC
(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!
Comment 18 Priit Laes (IRC: plaes) 2010-09-09 12:46:29 UTC
Created attachment 169849 [details] [review]
Updated patch for latest master
Comment 19 Pacho Ramos 2010-10-12 16:45:58 UTC
Will this be fixed some day? Maybe for Gnome3?

Thanks
Comment 20 Vincent Untz 2010-11-29 23:44:18 UTC
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)
Comment 21 Vincent Untz 2010-11-29 23:55:59 UTC
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?
Comment 22 Matthias Clasen 2014-03-05 11:39:09 UTC
gnome-session-properties is no longer included in gnome-session.