GNOME Bugzilla – Bug 659021
user-menu: should restore last presence when starting
Last modified: 2011-10-07 05:30:34 UTC
- Log in : your presence is unavailable - Change it to online - Log off - Log in : your presence is unavailable The Shell should restore the presence I had when I logged off and restore it.
*** Bug 660500 has been marked as a duplicate of this bug. ***
Created attachment 198456 [details] [review] user-menu: Restore previous IM presence on startup Move the saved user-set presence to GSettings, so it is preserved between logins.
Created attachment 198457 [details] [review] user-menu: Restore previous session presence at startup Save the session presence in GSettings and restore it on startup.
Review of attachment 198457 [details] [review]: ::: js/ui/userMenu.js @@ +275,3 @@ + if (this._settings.get_int('saved-im-presence') != presence) + return; + this._imPresenceRestored = true; This code is a bit hairy. Let me get this straight: _IMStatusChanged is called when Tp says our "most available status" is changed. We get that on startup, and then try to set it with the value with gsettings. This method could be called in the meantime before our set happens, and we need to ignore it? (also, why isn't this in the previous patch?)
Review of attachment 198456 [details] [review]: ::: js/ui/userMenu.js @@ +159,3 @@ Lang.bind(this, this._changeIMStatus)); + this._settings = new Gio.Settings({ schema: SHELL_SCHEMA }); global.settings? @@ +180,3 @@ + } else { + this._setComboboxPresence(savedPresence); + s = this._statusForPresence(savedPresence); Don't use "s". Rename it to "status".
Created attachment 198467 [details] [review] user-menu: Restore previous IM presence on startup
Created attachment 198468 [details] [review] user-menu: Restore previous session presence at startup (In reply to comment #4) > Review of attachment 198457 [details] [review]: > > ::: js/ui/userMenu.js > @@ +275,3 @@ > + if (this._settings.get_int('saved-im-presence') != presence) > + return; > + this._imPresenceRestored = true; > > This code is a bit hairy. Let me get this straight: > > [...] > > (also, why isn't this in the previous patch?) It actually is (in the callback to tp_proxy_prepare_async()), the snippet here is a left-over from a previous iteration; removed.
Review of attachment 198468 [details] [review]: ::: js/ui/userMenu.js @@ +357,3 @@ _sessionStatusChanged: function(sessionPresence, sessionStatus) { + if (!this._imPresenceRestored) + return; Is there ever a case where this could happen?
Review of attachment 198467 [details] [review]: I'm not entirely sure GSettings is the most appropriate way to have persistence, but go for it. ::: js/ui/userMenu.js @@ +171,2 @@ function(mgr) { let [presence, s, msg] = mgr.get_most_available_presence(); You've fixed this locally, right? @@ +326,3 @@ getIMPresenceForSessionStatus: function(sessionStatus) { if (sessionStatus == GnomeSession.PresenceStatus.AVAILABLE) + return global.settings.get_int('saved-im-presence'); This isn't obvious at first glance. A comment would be nice.
(In reply to comment #8) > Review of attachment 198468 [details] [review]: > > ::: js/ui/userMenu.js > @@ +357,3 @@ > _sessionStatusChanged: function(sessionPresence, sessionStatus) { > + if (!this._imPresenceRestored) > + return; > > Is there ever a case where this could happen? It shouldn't, but given that: - changing the im status may change the session status - changing the session status may change the im status it is important that the statuses are restored in a fixed order (im before session), so I'd rather play it safe (keep in mind that we now restore both on login and shell restart). If indeed something changes the session status before we have finished restoring im status, we will pick it up later (notice the explicit call in _IMStatusChanged).
(In reply to comment #9) > Review of attachment 198467 [details] [review]: > > I'm not entirely sure GSettings is the most appropriate way to have > persistence, but go for it. Neither am I, but it still seems nicer than writing it to disk ... > ::: js/ui/userMenu.js > @@ +171,2 @@ > function(mgr) { > let [presence, s, msg] = mgr.get_most_available_presence(); > > You've fixed this locally, right? Yes. > @@ +326,3 @@ > getIMPresenceForSessionStatus: function(sessionStatus) { > if (sessionStatus == GnomeSession.PresenceStatus.AVAILABLE) > + return global.settings.get_int('saved-im-presence'); > > This isn't obvious at first glance. A comment would be nice. Because it's using GSettings now? Anyway, how about: // Restore the last user-set presence when coming back from // BUSY/IDLE (otherwise the last user-set presence matches // the current one)
> Because it's using GSettings now? I just don't think the logic was clear in the first place. I was thinking something like: // GnomeSession's presence settings has less choices than Telepathy's. // We mark the user as "Available" for one of the extended choices, // and save the IM status separately -- the other choices match up // perfectly. And the first is ACN now. Go ahead.
Attachment 198467 [details] pushed as 39c5d23 - user-menu: Restore previous IM presence on startup Attachment 198468 [details] pushed as 2947b92 - user-menu: Restore previous session presence at startup (In reply to comment #12) > I just don't think the logic was clear in the first place. I was thinking > something like: > > // GnomeSession's presence settings has less choices than Telepathy's. > // We mark the user as "Available" for one of the extended choices, > // and save the IM status separately -- the other choices match up > // perfectly. I see how a comment is necessary ;-) Pushed with the comment I suggested - the function is not about translating session status to IM status, but about the policy how the session status affects IM status (for instance the IM status restored when the session status is "available" may very well be "busy" if that's what the user had chosen explicitly before).