GNOME Bugzilla – Bug 740142
loginDialog: only emit session-activated on user action
Last modified: 2018-02-19 18:45:38 UTC
Right now we emit session-activated any time the bullet moves in the session menu. That includes at start up when picking an item arbitrarily, and any time GDM reports the session was read from the user's account settings. session-activated informs GDM about the newly selected session, so emitting it in response to GDM reporting a session is a bad idea (it's not only pointless, but it can least to oscillations) This commit changes the code to only emit session-activated when the user explicitly activates a session item from the gear menu.
Created attachment 290737 [details] [review] loginDialog: only emit session-activated on user action
Review of attachment 290737 [details] [review]: ::: js/gdm/loginDialog.js @@ -351,3 @@ - if (!this._activeSessionId) - this.setActiveSession(id); Why was this removed?
(In reply to comment #2) > Review of attachment 290737 [details] [review]: > > ::: js/gdm/loginDialog.js > @@ -351,3 @@ > > - if (!this._activeSessionId) > - this.setActiveSession(id); > > Why was this removed? If we don't know what the user's selected session is, we shouldn't show a bullet pointing at some random one that won't get used.
to be clear, this does mean in some rare cases there will be no bullet at all in the menu. Until the user picks one. I think that's the right behavior, but also, in practice it won't happen for any case but smartcards, since we query for the username up front and load the user up front in all other cases.
Created attachment 299538 [details] [review] loginDialog: Don't arbitrarily pick a random session for the user gdm should be in charge of telling us the default session, and it should pick one for the user if she doesn't already have one.
Created attachment 299539 [details] [review] loginDialog: Better handle setting the active session We currently use the setActiveSession method to both mark a menu item as selected, and also tell gdm about the current session the user selected. Since gdm is ultimately in charge of the state, we should decouple this and simply ask gdm to set the session, and have the menu item reflect what gdm thinks is the current session. This prevents state getting mismatched and oscillations from happening, where we get in a loop of constantly telling gdm what the session is.
Review of attachment 299538 [details] [review]: obviously, like this one.
Review of attachment 299539 [details] [review]: This is better than what I came up with, and it seems to work as well in testing. It does incur a roundtrip through GDM, but that's probably a good thing.
Attachment 299538 [details] pushed as 6d40cb9 - loginDialog: Don't arbitrarily pick a random session for the user Attachment 299539 [details] pushed as 681861c - loginDialog: Better handle setting the active session
(In reply to Ray Strode [halfline] from comment #8) > it seems to work as well in testing. Apparently I suck at testing. I don't know if I typed make install in the wrong ssh window or what, but this patch break session changing.
So there are two problems: setActiveSession: function(sessionId) { - this.emit('session-activated', this._activeSessionId); + this.emit('session-activated', sessionId); }, and the roundtrip through GDM isn't instant, because GDM is blocking waiting for the user's password. normally, the messages don't get processed until after the user enters their password.
So I think we should revert this for now, since we're so late in the release cycle and it's clear what the best way forward is. We could do the original patch instead, but at this point, I think it's probably better to be conservative and go back to 3.15.91 behavior.
Please revert back to 3.15.91. Sorry about this.
Created attachment 299732 [details] [review] Revert "loginDialog: Better handle setting the active session" This reverts commit 681861c8c76cb96ab2a6199df1153202a5c9e292.
Created attachment 299733 [details] [review] Revert "loginDialog: Don't arbitrarily pick a random session for the user" This reverts commit 6d40cb98e7e7a24f66662f0a06ecf8e93ed5dea2.
Attachment 299732 [details] pushed as ee360d8 - Revert "loginDialog: Better handle setting the active session" Attachment 299733 [details] pushed as e5270cb - Revert "loginDialog: Don't arbitrarily pick a random session for the user"
moved patch to gitlab: https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/33