GNOME Bugzilla – Bug 702818
Move the session menu into a bubble
Last modified: 2013-06-26 19:20:44 UTC
Created attachment 247445 [details] mockup There are some issues with the existing session menu. First, it looks kinda bad. It seems like it's hanging around there, but it doesn't really know what to do with itself. Second, when it expands down it requires that the buttons below move down with it. This kind of movement is awkward and looks a bit weird. Third, it's current position makes the "dialog" tall and unwieldy when you add things like messages for finger print readers or authentication errors. My proposal is to move the menu into a popup bubble, which can be accessed from a gear button menu that is next to the Sign In button. A mockup is attached.
sounds good to me, what about the spinner that shows up there now? should we crossfade between them?
(In reply to comment #1) > ... what about the spinner that shows up there now? should we > crossfade between them? That would be good. The main thing here is to ensure that the spinner is in the same position as the gear icon.
Created attachment 247593 [details] [review] loginDialog: Move the session list to a PopupMenu -- First pass, without the spinner integration.
Review of attachment 247593 [details] [review]: Hi thanks for looking into this. I was planning to as well, but since you've picked it up I'll do the review half. ::: data/theme/gnome-shell.css @@ +2350,3 @@ +.login-dialog-session-list-button StIcon { + icon-size: 16px; This should probably say 1.5em or something instead, so it scales better on hidpi screens. @@ +2355,2 @@ .login-dialog-session-list-button { + color: #e6e6e6; So one thing is, the prelight isn't very contrasty. I guess because it's such a small icon. it might make sense to use a darker starting color, not sure. ::: js/gdm/loginDialog.js @@ +348,1 @@ Name: 'SessionList', maybe we should rename it to SessionMenu? @@ +366,3 @@ + let subtitle = new PopupMenu.PopupMenuItem(_("Session"), { style_class: 'popup-subtitle-menu-item', + reactive: false }); + this._menu.addMenuItem(subtitle); I'd probably call this a "header" instead of a subtitle, but doesn't really matter. @@ +383,3 @@ this._button.reactive = sensitive; this._button.can_focus = sensitive; + this._menu.close(BoxPointer.PopupAnimation.NONE); I sort of worry if we close the menu here we may end up in some situation where the menu gets closed out from under the user. But I'd say lets cross that bridge if we ever come to it. @@ +422,2 @@ + item.connect('activate', Lang.bind(this, function() { + this.setSessionItem(id); This should be setActiveSession not setSessionItem right? Selecting a session doesn't seem to work at the moment.
Created attachment 247731 [details] [review] loginDialog: Move the session list to a PopupMenu There are some issues with the existing session menu. First, it looks kinda bad. It seems like it's hanging around there, but it doesn't really know what to do with itself. Second, when it expands down it requires that the buttons below move down with it. This kind of movement is awkward and looks a bit weird. Third, it's current position makes the "dialog" tall and unwieldy when you add things like messages for finger print readers or authentication errors. This commit moves the session list to a menu behind a button to address the above problems. Small updates to patch by Ray Strode.
Doing the crossfade is sort of hard since the spinner is done at the modal dialog level. I'll post a patch to make the login dialog not depend on modal dialog anymore.
Created attachment 247759 [details] [review] ui: move AnimatedIcon out of panel.js The class is generally useful, so it only makes sense in panel.js for historical reasons. Because other parts of the code are using it, though, problems are cropping up that require a workaround like: placeSpinner: function(...) { /* This is here because of recursive imports */ const Panel = imports.ui.panel; Panel.AnimatedIcon(spinnerIcon, WORK_SPINNER_ICON_SIZE); ... } This commit moves AnimatedIcon to its own file so we can drop that workaround.
Created attachment 247760 [details] [review] loginDialog: clean up unused imports
Created attachment 247761 [details] [review] loginDialog: drop use of modal dialog The login screen is no longer even remotely dialog-like, so using ModalDialog is pretty weird. It also makes it difficult to put the session list in the same place as the spinner. This commit moves loginDialog away from using modal dialog.
Created attachment 247762 [details] [review] loginDialog: Move the session list to a PopupMenu There are some issues with the existing session menu. First, it looks kinda bad. It seems like it's hanging around there, but it doesn't really know what to do with itself. Second, when it expands down it requires that the buttons below move down with it. This kind of movement is awkward and looks a bit weird. Third, it's current position makes the "dialog" tall and unwieldy when you add things like messages for finger print readers or authentication errors. This commit moves the session list to a menu behind a button to address the above problems. Small updates to patch by Ray Strode.
Created attachment 247763 [details] [review] loginDialog: make spinner and session menu button share position They never need to be shown at the same time, and the design has the UI fade between them. This commit implements that.
Review of attachment 247759 [details] [review]: OK.
Review of attachment 247760 [details] [review]: ::: js/gdm/loginDialog.js @@ +21,3 @@ const AccountsService = imports.gi.AccountsService; const Clutter = imports.gi.Clutter; const CtrlAltTab = imports.ui.ctrlAltTab; This should be in the lower section. @@ +28,3 @@ const Meta = imports.gi.Meta; const Lang = imports.lang; const Pango = imports.gi.Pango; Pango is also unused. @@ +29,3 @@ const Lang = imports.lang; const Pango = imports.gi.Pango; const Realmd = imports.gdm.realmd; Lower section. @@ +33,3 @@ const Shell = imports.gi.Shell; const St = imports.gi.St; const Gdm = imports.gi.Gdm; Should be alphabetically sorted.
Review of attachment 247761 [details] [review]: ::: js/gdm/loginDialog.js @@ +526,3 @@ this._userSelectionBox = new St.BoxLayout({ style_class: 'login-dialog-user-selection-box', vertical: true }); + this._userSelectionBox.add_constraint(new Clutter.AlignConstraint({ source: this.actor, What is this needed for? Can't you just use x_align: Clutter.ActorAlign.MIDDLE ? @@ +544,3 @@ this._promptBox = new St.BoxLayout({ style_class: 'login-dialog-prompt-layout', vertical: true }); + this._promptBox.add_constraint(new Clutter.AlignConstraint({ source: this.actor, same @@ -615,3 @@ this._logoBin = new St.Bin({ style_class: 'login-dialog-logo-bin', y_expand: true }); this._logoBin.set_y_align(Clutter.ActorAlign.END); - this.backgroundStack.add_actor(this._logoBin); This means we can remove backgroundStack from ModalDialog, right? @@ +629,3 @@ + align_axis: Clutter.AlignAxis.Y_AXIS, + factor: 1.0 })); + this.actor.add_child(this._logoBin); I'd rather have this be part of the screen shield than the login dialog. I don't like the idea of having the login dialog stretch the length of the screen. @@ +820,3 @@ + this.cancel(); + })); + global.stage.connect('captured-event', ugh. Why can't you use a key-press handler on the group, and have the key event bubble from the entry naturally? If we need to go with a captured event (please don't, guh), then we need to disconnect this signal to prevent it from leaking after you hit Cancel.
Review of attachment 247762 [details] [review]: ::: js/gdm/loginDialog.js @@ +722,3 @@ this._buttonBox.visible = true; + if (this._sessionList.actor.get_parent()) + this._buttonBox.remove_child(this._sessionList.actor); ugh @@ +761,3 @@ + this._buttonBox.add(this._sessionList.actor, + { expand: false, + x_align: St.Align.END }); To have these two crossfaded, I'd add both the spinner and session menu button to a bin-like actor that you put in place. That would be a lot easier to handle, I think.
Review of attachment 247763 [details] [review]: ::: js/gdm/loginDialog.js @@ +623,3 @@ }, _setWorking: function(working) { showModifier(SPINNER); showModifier(SESSION_LIST); showModifier(NONE); ? I don't really like "if we're working, hide the session menu, and then when we become unworking, don't show it again" @@ +770,3 @@ + this._workSpinner.actor.add_constraint(new Clutter.BindConstraint({ source: this._sessionList.actor, + coordinate: Clutter.BindCoordinate.ALL })); Yeah, I prefer the bin.
(In reply to comment #14) > Review of attachment 247761 [details] [review]: > > ::: js/gdm/loginDialog.js > @@ +526,3 @@ > this._userSelectionBox = new St.BoxLayout({ style_class: > 'login-dialog-user-selection-box', > vertical: true }); > + this._userSelectionBox.add_constraint(new Clutter.AlignConstraint({ > source: this.actor, > > What is this needed for? Can't you just use x_align: Clutter.ActorAlign.MIDDLE We talked about this a bit on irc: <halfline> out of curiosity, why don't you like constraints ? <halfline> they have the appeal in my mind of describing very concisely what the intention of the interaction between two actors are <Jasper> halfline, ideally, they'd work fine <Jasper> halfline, in practice, there's *a lot* of complications that mean they're suboptimal. <Jasper> We've had relayout loops and full stage redraws when using constraints before. <halfline> i see <halfline> thing is, there's this other bug where if a pam prompt message shows up that's longer than the buttons, then the buttons spread out <halfline> since there's no visible box around them that looks wrong <halfline> so ultimately i want to decouple the actors from each other <halfline> i don't want them really acting like they're in the same box layout <halfline> and instead just describe how they sit relative to each other <drago01> Jasper: that depends on the actor hierachy thoough <drago01> Jasper: contraints take away a lot of complexity in cases where they work <Jasper> halfline, hm, OK. > This means we can remove backgroundStack from ModalDialog, right? Indeed. I'd also like to consolidate the login dialog and unlock dialog code, then we can get rid of placeSpinner from ModalDialog as well (we can do that in a different bug though) > I'd rather have this be part of the screen shield than the login dialog. > > I don't like the idea of having the login dialog stretch the length of the > screen. I assume given our discussion above, you're okay with treating the login dialog as full screen conceptually now. I'd rather keep it in login dialog, because: 1) it's already there, and that's orthogonal to this bug 2) it's only ever shown in the login screen, so having in the login screen specific code sort of makes sense > > @@ +820,3 @@ > + this.cancel(); > + })); > + global.stage.connect('captured-event', > > ugh. Why can't you use a key-press handler on the group, and have the key event > bubble from the entry naturally? Yea, that's clearly better. will do.
Comment on attachment 247759 [details] [review] ui: move AnimatedIcon out of panel.js Attachment 247759 [details] pushed as aa6b633 - ui: move AnimatedIcon out of panel.js
Created attachment 247768 [details] [review] loginDialog: clean up import lines There are a few unused imports, and some import lines out of order. This commit tries to clean it all up.
Review of attachment 247768 [details] [review]: OK.
(In reply to comment #16) > Review of attachment 247763 [details] [review]: > > ::: js/gdm/loginDialog.js > @@ +623,3 @@ > }, > > _setWorking: function(working) { > > showModifier(SPINNER); > showModifier(SESSION_LIST); > showModifier(NONE); > > ? > > I don't really like "if we're working, hide the session menu, and then when we > become unworking, don't show it again" I kind of understand your point. It's reasonable to allow the user to select a session even after stopping the spinner. So you're proposing reworking the code to be in terms of features like "spinner", "session list" and having a way to turn them on and off. The thing is, they aren't really comparable items. The only thing they have in common is that they occupy the same space and different points of the operation. So I feel like the abstraction should be on the operation, not the on features themselves. Fixing the else clause to reshow the session list makes sense to me though. > @@ +770,3 @@ > > + this._workSpinner.actor.add_constraint(new Clutter.BindConstraint({ > source: this._sessionList.actor, > + > coordinate: Clutter.BindCoordinate.ALL })); > > Yeah, I prefer the bin. I assume since you're okay with constraints now following our discussion above, that this is okay now too.
Comment on attachment 247768 [details] [review] loginDialog: clean up import lines Attachment 247768 [details] pushed as 048d5dc - loginDialog: clean up import lines
Created attachment 247773 [details] [review] loginDialog: drop use of modal dialog The login screen is no longer even remotely dialog-like, so using ModalDialog is pretty weird. It also makes it difficult to put the session list in the same place as the spinner. This commit moves loginDialog away from using modal dialog.
Created attachment 247774 [details] [review] loginDialog: Move the session list to a PopupMenu There are some issues with the existing session menu. First, it looks kinda bad. It seems like it's hanging around there, but it doesn't really know what to do with itself. Second, when it expands down it requires that the buttons below move down with it. This kind of movement is awkward and looks a bit weird. Third, it's current position makes the "dialog" tall and unwieldy when you add things like messages for finger print readers or authentication errors. This commit moves the session list to a menu behind a button to address the above problems. Small updates to patch by Ray Strode.
Created attachment 247775 [details] [review] loginDialog: make spinner and session menu button share position They never need to be shown at the same time, and the design has the UI fade between them. This commit implements that.
(In reply to comment #21) > (In reply to comment #16) > The thing is, they aren't really comparable items. The only thing they have in > common is that they occupy the same space and different points of the > operation. So I feel like the abstraction should be on the operation, not the > on features themselves. I think setWorking, as an API, is bad. It says "please show the spinner", completely ignoring the fact that there's also the session list there. It's possible we should have "spinnerShowing" and "sessionListShowing" and if both are showing, we choose one to prioritize -- let's say the spinner. I don't like embedding random logic about which element to show in setWorking.
Review of attachment 247773 [details] [review]: OK.
Review of attachment 247774 [details] [review]: ::: js/gdm/loginDialog.js @@ +387,3 @@ + item.connect('activate', Lang.bind(this, function() { + this.setActiveSession(id); + this._menu.close(); This shouldn't be needed? A PopupMenu connects to the item's 'activate' signal manually and closes it then. @@ +732,3 @@ this._buttonBox.visible = true; + if (this._sessionList.actor.get_parent()) + this._buttonBox.remove_child(this._sessionList.actor); As I said... ugh.
Review of attachment 247774 [details] [review]: ::: js/gdm/loginDialog.js @@ +19,3 @@ */ +const Atk = imports.gi.Atk; Also, duplicate import? I'm quite sure gjs will complain about duplicated consts. Did you not test this?
Review of attachment 247775 [details] [review]: You didn't implement the bin actor that I thought you would? ::: js/gdm/loginDialog.js @@ +639,3 @@ Tweener.removeTweens(this._workSpinner.actor); if (working) { + if (this._sessionList.actor.opacity > 0) Why the check? @@ +655,3 @@ }); } else { + if (this._sessionList.actor.opacity == 0 && Why the check? @@ +727,3 @@ transition: 'easeOutQuad' }); + if ((this._user && !this._user.is_logged_in()) || this._verifyingUser) { Factor this out into this._shouldShowSessionList() or something. Also, no brackets.
Comment on attachment 247773 [details] [review] loginDialog: drop use of modal dialog Attachment 247773 [details] pushed as ea02380 - loginDialog: drop use of modal dialog
(In reply to comment #28) > Review of attachment 247774 [details] [review]: > > ::: js/gdm/loginDialog.js > @@ +387,3 @@ > + item.connect('activate', Lang.bind(this, function() { > + this.setActiveSession(id); > + this._menu.close(); > > This shouldn't be needed? A PopupMenu connects to the item's 'activate' signal > manually and closes it then. Nope, trying it, apparently not. If you knew that now, why did you add it in the first place? :-) > @@ +732,3 @@ > this._buttonBox.visible = true; > + if (this._sessionList.actor.get_parent()) > + this._buttonBox.remove_child(this._sessionList.actor); > > As I said... ugh. Yea, it's not super pretty, but what's the alternative? we don't want it to get destroyed and we don't want to have to rebuild the list every time, right?
(In reply to comment #32) > Nope, trying it, apparently not. If you knew that now, why did you add it in > the first place? :-) Huh, given how I didn't even get the method name right, I figured it was something you added. Guess not. I'd want to debug this, though. See PopupMenu._connectItemSignals -- it adds the 'activate' handler that closes the menu. > Yea, it's not super pretty, but what's the alternative? we don't want it to get > destroyed and we don't want to have to rebuild the list every time, right? I think I'd be happier with this._buttonBox.contains(this._sessionList.actor) instead of the get_parent().
(In reply to comment #33) > (In reply to comment #32) > > Nope, trying it, apparently not. If you knew that now, why did you add it in > > the first place? :-) > > I'd want to debug this, though. See > PopupMenu._connectItemSignals -- it adds the 'activate' handler that closes the > menu. I think we're miscommunicating because of double negatives. To be clear, we're both saying that call is completely unnecessary and it works fine with out it. Nothing needs to be debugged.
(In reply to comment #33) > I think I'd be happier with this._buttonBox.contains(this._sessionList.actor) > instead of the get_parent(). Slightly less efficient, but not enough to matter in practice, I guess. sure.
Thinking about this more, we can just use remove_all_children instead of destroy_all_children. All other children will get destroyed when they're references get replaced.
Created attachment 247830 [details] [review] loginDialog: Move the session list to a PopupMenu There are some issues with the existing session menu. First, it looks kinda bad. It seems like it's hanging around there, but it doesn't really know what to do with itself. Second, when it expands down it requires that the buttons below move down with it. This kind of movement is awkward and looks a bit weird. Third, it's current position makes the "dialog" tall and unwieldy when you add things like messages for finger print readers or authentication errors. This commit moves the session list to a menu behind a button to address the above problems. Some updates to patch by Ray Strode.
Created attachment 247831 [details] [review] loginDialog: make spinner and session menu button share position They never need to be shown at the same time, and the design has the UI fade between them. This commit implements that.
Review of attachment 247830 [details] [review]: Code looks good, one minor comment. ::: js/gdm/loginDialog.js @@ +293,1 @@ const SessionList = new Lang.Class({ I think renaming to SessionMenu or SessionMenuButton would be a good idea.
Comment on attachment 247830 [details] [review] loginDialog: Move the session list to a PopupMenu okay pushed with that change Attachment 247830 [details] pushed as 8d7649e - loginDialog: Move the session list to a PopupMenu
Created attachment 247853 [details] [review] loginDialog: make spinner and session menu button share position They never need to be shown at the same time, and the design has the UI fade between them. This commit implements that.
Created attachment 247854 [details] [review] loginDialog: make spinner and session menu button share position They never need to be shown at the same time, and the design has the UI fade between them. This commit implements that.
Review of attachment 247853 [details] [review]: Yeah, I like this better.
okay pushed. Attachment 247854 [details] pushed as f7568b6 - loginDialog: make spinner and session menu button share position