GNOME Bugzilla – Bug 683156
Allow changing the session mode at runtime
Last modified: 2012-09-04 21:57:24 UTC
WIP patches, nowhere near finished, only here to show gcampax what my ideas were. They work, so that's that.
Created attachment 223139 [details] [review] panel: Clean up panel construction code
Created attachment 223140 [details] [review] sessionMode: Allow changing the session mode at runtime
Review of attachment 223140 [details] [review]: The only SessionMode bit this patch handles is panel, and does it wrong. To me, this is just not what we need, but I'll let the others decide. (In complete fairness, it handles extensions too. IMHO, that's questionable, as it prevents extensions from getting into the screen lock notification box, or extending the screen lock with additional content) ::: js/ui/panel.js @@ +1143,3 @@ + if (parent) { + // Unparent the indicator. + parent.remove_child(actor); This will break hard. St is not made to keep live actors off the stage, and crashes if you do so. I don't want to risk that. ::: js/ui/sessionMode.js @@ +35,3 @@ + right: ['lockScreen'] + }, + }, You need some way to move the user-menu to the left and mark it locked (or have another user widget that is shown only in the lock-screen)
Created attachment 223158 [details] [review] st-bin: Don't allocate a hidden actor
Created attachment 223159 [details] [review] sessionMode: Allow changing the session mode at runtime Fixed both of the issues you described, and also hooked up showCalendarEvents and allowSettings. hasOverview/hasRunDialog/hasWorkspaces coming soon.
Created attachment 223172 [details] [review] st-widget: Unset hover when setting track_hover to FALSE
Created attachment 223173 [details] [review] sessionMode: Allow changing the session mode at runtime Since we eventually want to add a system for changing the top panel contents depending on the current state of the shell, let's use the "session mode" feature for this, and add a mechanism for updating the session mode at runtime. Add support for every key besides the two functional keys, and make all the components update automatically when the session mode is changed. Add a new lock-screen mode, and make the lock screen change to this when locked. This is a much less WIP patch. This is something I feel comfortable landing.
Created attachment 223174 [details] [review] userMenu: Make the user menu insensitive in the lock screen And show a lock icon as well.
Created attachment 223175 [details] [review] userMenu: Don't update the presence icon immediately If we don't freeze the presence icon, we can end up in a place where we'll be updating the icon before we fade out the panel indicators when coming back from the lock screen.
Review of attachment 223139 [details] [review]: Is there anything different here from my patch in bug 682546, worth the authorship change?
Review of attachment 223158 [details] [review]: Makes sense
Review of attachment 223172 [details] [review]: Sure
Review of attachment 223139 [details] [review]: No. I couldn't apply it for some reason, so I rewrote it.
Review of attachment 223173 [details] [review]: This turned out to be a lot nicer than I thought. Some comments inline, but mostly good. ::: js/ui/main.js @@ +228,1 @@ sessionMode.createSession(); While you're there, can you split this functional bit in some kind of sessionAgents: ['network', 'polkit', 'autorun', 'automount', ...] with a defined interface (constructor, .activate(), .deactivate() could do) @@ +239,2 @@ + sessionMode.connect('update', _sessionUpdated); + _sessionUpdated(); You removed the call to sessionMode.forceUpdated(), why? ::: js/ui/popupMenu.js @@ +872,3 @@ this._settingsActions = { }; + + Main.sessionMode.connect('updated', Lang.bind(this, this._sessionUpdated)); Get the signal id and disconnect it in destroy() ::: js/ui/screenShield.js @@ +584,3 @@ this._lockScreenGroup.fixed_position_set = false; + this._lockScreenGroup.scale_x = 1; + this._lockScreenGroup.scale_y = 1; Wrong patch for this. @@ +672,2 @@ this.emit('lock-status-changed', false); + Main.sessionMode.popMode('lock-screen'); .lock() and .unlock() are called by logind, they must be idempotent. ::: js/ui/sessionMode.js @@ +30,3 @@ + 'lock-screen': { + hasOverview: false, + showCalendarEvents: true, true? We don't even have a dateMenu in the lock screen... @@ +36,3 @@ + hasRunDialog: false, + hasWorkspaces: false, + extraStylesheet: null, You should probably drop this extraStylesheet thing, it's unused since the merge of gdm.css @@ +42,3 @@ + right: ['lockScreen'] + }, + }, 1) Should we have a "canPush" boolean? 2) MessageTray is still using lock-status-changed, right? @@ +101,3 @@ + popMode: function(mode) { + if (this.currentMode != mode || this._modeStack.length === 1) + throw new Error("invalid pop"); "Invalid SessionMode.popMode", at least. (Yes, we have backtraces most of time, but a clear error message is still nice)
Review of attachment 223174 [details] [review]: ::: js/ui/panelMenu.js @@ +118,3 @@ + setSensitive: function(sensitive) { + this.actor.reactive = sensitive; + this.actor.can_focus = sensitive; I wonder if this is a problem for a11y (although if can_focus is true, you can tab to the menu and open it...) ::: js/ui/sessionMode.js @@ +37,3 @@ hasWorkspaces: false, extraStylesheet: null, + isLocked: true, isLocked is quite generic, maybe userMenuLocked?
Review of attachment 223175 [details] [review]: It's a bit overengineered, but ok.
(In reply to comment #14) > Review of attachment 223173 [details] [review]: > > This turned out to be a lot nicer than I thought. I knew it would work. I think it's a pretty good solution. > ::: js/ui/main.js > @@ +228,1 @@ > sessionMode.createSession(); > > While you're there, can you split this functional bit in some kind of > sessionAgents: ['network', 'polkit', 'autorun', 'automount', ...] > with a defined interface (constructor, .activate(), .deactivate() could do) Good idea. > @@ +239,2 @@ > + sessionMode.connect('update', _sessionUpdated); > + _sessionUpdated(); > > You removed the call to sessionMode.forceUpdated(), why? I originally had the plan that we'd force an update so we wouldn't have to do something on construction. The problem comes when you build things that connect to the 'update' signal in response to an 'update' signal, like the user menu. It will never get the 'update' signal, so I just made it so that we explicitly call _sessionUpdated everywhere. > @@ +672,2 @@ > this.emit('lock-status-changed', false); > + Main.sessionMode.popMode('lock-screen'); > > .lock() and .unlock() are called by logind, they must be idempotent. What do you want me to do here instead? > ::: js/ui/sessionMode.js > @@ +30,3 @@ > + 'lock-screen': { > + hasOverview: false, > + showCalendarEvents: true, > > true? > We don't even have a dateMenu in the lock screen... Whoops. > @@ +36,3 @@ > + hasRunDialog: false, > + hasWorkspaces: false, > + extraStylesheet: null, > > You should probably drop this extraStylesheet thing, it's unused since the > merge of gdm.css Yeah, I heavily considered it, since then we can drop the loadTheme on session update too (which prevents reading the CSS twice on init). Will do. > @@ +42,3 @@ > + right: ['lockScreen'] > + }, > + }, > > 1) Should we have a "canPush" boolean? What would it do? Prevent a pushModal? How? Why? > 2) MessageTray is still using lock-status-changed, right? Yep. I'll fix that. automountManager, shellDBus and layout are using them too. layout can be ported; I'm not sure about the other ones. > @@ +101,3 @@ > + popMode: function(mode) { > + if (this.currentMode != mode || this._modeStack.length === 1) > + throw new Error("invalid pop"); > > "Invalid SessionMode.popMode", at least. > (Yes, we have backtraces most of time, but a clear error message is still nice) OK.
(In reply to comment #17) > (In reply to comment #14) > > Review of attachment 223173 [details] [review] [details]: > > @@ +239,2 @@ > > + sessionMode.connect('update', _sessionUpdated); > > + _sessionUpdated(); > > > > You removed the call to sessionMode.forceUpdated(), why? > > I originally had the plan that we'd force an update so we wouldn't have to do > something on construction. The problem comes when you build things that connect > to the 'update' signal in response to an 'update' signal, like the user menu. > It will never get the 'update' signal, so I just made it so that we explicitly > call _sessionUpdated everywhere. Ok, fine > > @@ +672,2 @@ > > this.emit('lock-status-changed', false); > > + Main.sessionMode.popMode('lock-screen'); > > > > .lock() and .unlock() are called by logind, they must be idempotent. > > What do you want me to do here instead? Have a _inLockScreenMode boolean on the screenshield, or reuse _isLocked. In other words, make sure that .unlock() never calls popMode if the mode is not the right one. > [...] > > @@ +42,3 @@ > > + right: ['lockScreen'] > > + }, > > + }, > > > > 1) Should we have a "canPush" boolean? > > What would it do? Prevent a pushModal? How? Why? Well, it would make the code self-explanatory, and would clarify that some modes are meant for shell setup, and some are transient during normal operation. > > 2) MessageTray is still using lock-status-changed, right? > > Yep. I'll fix that. automountManager, shellDBus and layout are using them too. > layout can be ported; I'm not sure about the other ones. > AutomountManager should be handled like polkit and network, in the sessionAgents. Layout only needs that to force the panel even if a fullscreen window is shown. Add a "ignoreFullscreen" boolean to the mode if you want, or let it depend on screenshield. ShellDBus is just exporting the screenshield status as legacy screensaver, so it needs to talk to screenshield directly.
(In reply to comment #14) > Review of attachment 223173 [details] [review]: > While you're there, can you split this functional bit in some kind of > sessionAgents: ['network', 'polkit', 'autorun', 'automount', ...] > with a defined interface (constructor, .activate(), .deactivate() could do) Just did this. You are *not* going to like it. Just saying.
Attachment 223158 [details] pushed as 50f8ae6 - st-bin: Don't allocate a hidden actor Attachment 223172 [details] pushed as 79bfea5 - st-widget: Unset hover when setting track_hover to FALSE
Created attachment 223239 [details] [review] sessionMode: Remove extraStylesheet It's been unused ever since the gdm merge.
Created attachment 223240 [details] [review] sessionMode: Allow changing the session mode at runtime Since we eventually want to add a system for changing the top panel contents depending on the current state of the shell, let's use the "session mode" feature for this, and add a mechanism for updating the session mode at runtime. Add support for every key besides the two functional keys, and make all the components update automatically when the session mode is changed. Add a new lock-screen mode, and make the lock screen change to this when locked.
Created attachment 223241 [details] [review] calendar: Launch the calendar server with DBus autostart The supposed reason for launching the calendar server in a peculiar way was so that the process would be killed when the Shell was killed, but that didn't actually work. Launch the calendar server through auto-start, and persist all throughout the session.
Created attachment 223242 [details] [review] main: Remove some cruft Main.hotCorners has been empty for a long, long time.
Created attachment 223243 [details] [review] Rearchitect the Shell to have a components system Components are pieces of the shell code that can be added/removed at runtime, like extension, but are tied more directly to a session mode. The session polkit agent, the network agent, autorun/automount, are all components. What is wrong with me this is way too overengineered, Giovanni is going to hate my guts argh why did I waste a full day on this.
Created attachment 223244 [details] [review] keyringPrompt: Turn into a component
Created attachment 223245 [details] [review] userMenu: Make the user menu insensitive in the lock screen And show a lock icon as well.
Created attachment 223246 [details] [review] userMenu: Don't update the presence icon immediately If we don't freeze the presence icon, we can end up in a place where we'll be updating the icon before we fade out the panel indicators when coming back from the lock screen.
Last two userMenu patches are untouched from before. (In reply to comment #15) > Review of attachment 223174 [details] [review]: > > ::: js/ui/panelMenu.js > @@ +118,3 @@ > + setSensitive: function(sensitive) { > + this.actor.reactive = sensitive; > + this.actor.can_focus = sensitive; > > I wonder if this is a problem for a11y > (although if can_focus is true, you can tab to the menu and open it...) Right, that was my feeling. I'll let someone more experienced with a11y try and help out with this. > ::: js/ui/sessionMode.js > @@ +37,3 @@ > hasWorkspaces: false, > extraStylesheet: null, > + isLocked: true, > > isLocked is quite generic, maybe userMenuLocked? I like isLocked. I think it makes sense. Nothing big should depend on it, just minor UI tweaks that need to care. For an alternate approach, I could make a "fake" user menu actor that has no menu, and always shows the pending-changes icon. That might be a bit easier to manage. (In reply to comment #18) > > What would it do? Prevent a pushModal? How? Why? > > Well, it would make the code self-explanatory, and would clarify that some > modes are meant for shell setup, and some are transient during normal > operation. I chose not to do this. I don't think that we want to have "some are for shell setup, and some are transient". > > > 2) MessageTray is still using lock-status-changed, right? MessageTray is still using lock-status-changed. Actually, writing this up right now, I realized that since the automount component is disabled in the lock screen, we don't even need Notification.showInLockScreen or any of that machinery. And I also started on the mess that is the components system. Oh boy, what a can of worms we're opening here. It's a closed system, tied to the session mode system. Not the biggest fan of my implementation, as it was hacked out brute force with tons of distractions for the day, but I like the idea. Open questions: * Where should we go with this, in terms of session modes or components? Should we make the overview a session mode, given that it needs to have the app menu hidden, and also disable certain kinds of key bindings, and fiddle with things in the same way the session mode needs to? * Should we make more things components? I have a WIP patch to make the extension system a component, but it's fairly ugly. * There's a lot of subtle similarities between some things and I'm curious if we should standardize some interfaces. Components, extensions, panel applets, message tray notifications/sources all have this enable/disable feature. We should see if we can standardize on a set of extension points, like those above, and open them to the world. * Also related: should we ditch the components system, and basically make an internal extensions system? Cinnamon (yes, I know) has an applets system that I think is fairly nice, and very similar to the session mode panel definitions (that's why I said something about GSettings during the panel construction cleanup patch)?
Review of attachment 223243 [details] [review]: You don't say overengineered, you say extensible. And yes, I like it. It follows the gnome-settings-daemon pattern, it is nice to read and it keeps each part self contained. PS: ShellPolkitAuthenticationAgent should be on the kill list, followed by ShellNetworkAgent once we transition to libsecret. ::: js/ui/components/__init__.js @@ +3,3 @@ +const Main = imports.ui.main; + +const Components = new Lang.Class({ ComponentManager is probably better, I think. @@ +17,3 @@ + let newEnabledComponents = Main.sessionMode.components; + + newEnabledComponents.filter(Lang.bind(this, function(name) { Can we have a big // KEEP THIS SORTED in sessionMode.js, and take advantage of that to avoid O(N^2)? @@ +27,3 @@ + })).forEach(Lang.bind(this, function(name) { + this._disableComponent(name); + })); this._enabledComponents = newEnabledComponents ::: js/ui/autorunManager.js @@ +142,3 @@ const AutorunManager = new Lang.Class({ Name: 'AutorunManager', + Component: 'autorun', Uh? ::: js/ui/main.js @@ +160,3 @@ notificationDaemon = new NotificationDaemon.NotificationDaemon(); windowAttentionHandler = new WindowAttentionHandler.WindowAttentionHandler(); + components = new Components.Components(); I wonder if there is anything else here that can be componentified. ::: js/ui/sessionMode.js @@ +52,3 @@ hasRunDialog: false, hasWorkspaces: false, + components: [], You want at least networkAgent (IIRC you don't have a dialog for wifi password in the initial setup tool). polkitAgent would be nice too. Ah, and it's called polkitAuthenticationAgent, if you go by module name. @@ +70,3 @@ createUnlockDialog: Main.createSessionUnlockDialog, + components: ['networkAgent', 'polkitAgent', 'telepathyClient', + 'recorder', 'autorun', 'automount'], 'autorunManager' and 'automountManager'
Review of attachment 223244 [details] [review]: Am I reading it wrong, or previously it was started unconditionally? Why only in session mode now? (For example, isn't the keyring needed in initial setup?) ::: js/ui/sessionMode.js @@ +70,3 @@ createUnlockDialog: Main.createSessionUnlockDialog, components: ['networkAgent', 'polkitAgent', 'telepathyClient', + 'keyring', 'recorder', 'autorun', 'automount'], As in the previous patches, keyringPrompt.
Review of attachment 223239 [details] [review]: Sure.
Review of attachment 223240 [details] [review]: Good to commit, just a small nit. ::: js/ui/sessionMode.js @@ +46,1 @@ 'initial-setup': { hasOverview: false, Given that initial-setup is extensible and plugins can in principle be made fullscreen, you need hasWindows here.
Review of attachment 223241 [details] [review]: To be honest, it did work: every time the shell closes the stdin pipe (by exit(), signal or execve()+O_CLOEXEC), the calendar server would exit. But this is way cleaner. ::: js/ui/calendar.js @@ +209,3 @@ + g_name: 'org.gnome.Shell.CalendarServer', + g_object_path: '/org/gnome/Shell/CalendarServer', + g_flags: Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES }); This still screams for GDBus 2...
Review of attachment 223242 [details] [review]: Asd, sure.
Review of attachment 223245 [details] [review]: Ok
Review of attachment 223240 [details] [review]: ::: js/ui/popupMenu.js @@ +1177,3 @@ this.emit('destroy'); + + Main.sessionMode.disconnect('updated', this._sessionUpdatedId); ... disconnect takes a signal id only.
Review of attachment 223246 [details] [review]: -
(In reply to comment #29) > Last two userMenu patches are untouched from before. > > (In reply to comment #15) > > Review of attachment 223174 [details] [review] [details]: > > > > ::: js/ui/panelMenu.js > > @@ +118,3 @@ > > + setSensitive: function(sensitive) { > > + this.actor.reactive = sensitive; > > + this.actor.can_focus = sensitive; > > > > I wonder if this is a problem for a11y > > (although if can_focus is true, you can tab to the menu and open it...) > > Right, that was my feeling. I'll let someone more experienced with a11y try and > help out with this. > > > ::: js/ui/sessionMode.js > > @@ +37,3 @@ > > hasWorkspaces: false, > > extraStylesheet: null, > > + isLocked: true, > > > > isLocked is quite generic, maybe userMenuLocked? > > I like isLocked. I think it makes sense. Nothing big should depend on it, just > minor UI tweaks that need to care. For an alternate approach, I could make a > "fake" user menu actor that has no menu, and always shows the pending-changes > icon. That might be a bit easier to manage. > > (In reply to comment #18) > > > What would it do? Prevent a pushModal? How? Why? > > > > Well, it would make the code self-explanatory, and would clarify that some > > modes are meant for shell setup, and some are transient during normal > > operation. > > I chose not to do this. I don't think that we want to have "some are for shell > setup, and some are transient". > > > > > 2) MessageTray is still using lock-status-changed, right? > > MessageTray is still using lock-status-changed. Actually, writing this up right > now, I realized that since the automount component is disabled in the lock > screen, we don't even need Notification.showInLockScreen or any of that > machinery. Notification.showWhenLocked can be ditched when bug 682544 lands (hint hint). Source.showInLockScreen can be removed if disabling autorun destroys its source, but it will come back in 3.8 with the notification g-c-c panel. > > And I also started on the mess that is the components system. Oh boy, what a > can of worms we're opening here. It's a closed system, tied to the session mode > system. Not the biggest fan of my implementation, as it was hacked out brute > force with tons of distractions for the day, but I like the idea. > > Open questions: > > * Where should we go with this, in terms of session modes or components? > Should we make the overview a session mode, given that it needs to have the app > menu hidden, and also disable certain kinds of key bindings, and fiddle with > things in the same way the session mode needs to? No, or if you prefer, not now. We can think about in 3.7.1, for now it works just fine. (As for keybindings, consider that bug 613543 is changing where the policy lives, if Florian pushes his updated patches) > * Should we make more things components? I have a WIP patch to make the > extension system a component, but it's fairly ugly. Why is that? You already have .enableAllExtensions() / .disableAllExtensions() in the session mode patch. Note: .disable() is not destroy, you don't need to unload all extensions or do anything more than .disable()-ing each of them. (In general, you must not unload any extension ever, as a GType might get unregistered, and then bad things would happen) > * There's a lot of subtle similarities between some things and I'm curious if > we should standardize some interfaces. Components, extensions, panel applets, > message tray notifications/sources all have this enable/disable feature. We > should see if we can standardize on a set of extension points, like those > above, and open them to the world. > > * Also related: should we ditch the components system, and basically make an > internal extensions system? Cinnamon (yes, I know) has an applets system that I > think is fairly nice, and very similar to the session mode panel definitions > (that's why I said something about GSettings during the panel construction > cleanup patch)? Extensions are about the packaging system, mostly (zip file, metadata, etc.). We can think of requiring each extension to have const Component = new Lang.Class({ ... }) (or const Extension) but it's probably not needed yet. The more things we move over to be components, the better IMHO. Not only it opens up for forks, it also cleans up the various interdependencies in the current shell code. But really, components need to be self-contained. If you write a class that depends on three other modules in js/ui/components, then it's no longer a component
Review of attachment 223240 [details] [review]: ::: js/ui/sessionMode.js @@ +46,1 @@ 'initial-setup': { hasOverview: false, No, I don't. We inherit from 'user', so that should mean that we get hasWindows: true by default, right?
Review of attachment 223241 [details] [review]: ::: js/ui/calendar.js @@ +209,3 @@ + g_name: 'org.gnome.Shell.CalendarServer', + g_object_path: '/org/gnome/Shell/CalendarServer', + g_flags: Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES }); We'll get to that for 3.8.
(In reply to comment #40) > Review of attachment 223240 [details] [review]: > > ::: js/ui/sessionMode.js > @@ +46,1 @@ > 'initial-setup': { hasOverview: false, > > No, I don't. We inherit from 'user', so that should mean that we get > hasWindows: true by default, right? Oh right, the broken Param.parse thing. Ok then.
Attachment 223239 [details] pushed as 3a1f623 - sessionMode: Remove extraStylesheet Attachment 223241 [details] pushed as cb9062f - calendar: Launch the calendar server with DBus autostart Attachment 223242 [details] pushed as b257029 - main: Remove some cruft
Review of attachment 223243 [details] [review]: ::: js/ui/components/__init__.js @@ +17,3 @@ + let newEnabledComponents = Main.sessionMode.components; + + newEnabledComponents.filter(Lang.bind(this, function(name) { I don't see how sorting will help us. ::: js/ui/autorunManager.js @@ +142,3 @@ const AutorunManager = new Lang.Class({ Name: 'AutorunManager', + Component: 'autorun', Remnants from an earlier approach. ::: js/ui/main.js @@ +160,3 @@ notificationDaemon = new NotificationDaemon.NotificationDaemon(); windowAttentionHandler = new WindowAttentionHandler.WindowAttentionHandler(); + components = new Components.Components(); I figured to go conversative for now. We can always do more when we want to. ::: js/ui/sessionMode.js @@ +52,3 @@ hasRunDialog: false, hasWorkspaces: false, + components: [], If polkit shows up at all during initial-setup, it's a bug, IMO. (Do you know the password for the gnome-initial-setup user account?) @@ +70,3 @@ createUnlockDialog: Main.createSessionUnlockDialog, + components: ['networkAgent', 'polkitAgent', 'telepathyClient', + 'recorder', 'autorun', 'automount'], Splinter doesn't show it, but I renamed the files. Unless you want me to rename these back. I'm sort of fond of the short names, free of suffixes like 'manager', 'client', 'agent'.
Created attachment 223411 [details] [review] sessionMode: Allow changing the session mode at runtime Since we eventually want to add a system for changing the top panel contents depending on the current state of the shell, let's use the "session mode" feature for this, and add a mechanism for updating the session mode at runtime. Add support for every key besides the two functional keys, and make all the components update automatically when the session mode is changed. Add a new lock-screen mode, and make the lock screen change to this when locked. There's one bug in here that I can't find. It's just console spew, so it's not harmful to land, but the panel changes to have an St.Bin wrapper breaks hiding the app menu when entering the overview: (lt-gnome-shell-real:28828): Clutter-WARNING **: ./clutter-actor.c:9670: Actor '<appMenu>[<StBin>:0x1c06930]' tried to allocate a size of -12.00 x 27.00 and more like it.
Created attachment 223412 [details] [review] Rearchitect the Shell to have a components system Components are pieces of the shell code that can be added/removed at runtime, like extension, but are tied more directly to a session mode. The session polkit agent, the network agent, autorun/automount, are all components. What is wrong with me this is way too overengineered, Giovanni is going to hate my guts argh why did I waste a full day on this.
Created attachment 223413 [details] [review] userMenu: Make the user menu insensitive in the lock screen And show a lock icon as well.
Created attachment 223414 [details] [review] userMenu: Don't update the presence icon immediately If we don't freeze the presence icon, we can end up in a place where we'll be updating the icon before we fade out the panel indicators when coming back from the lock screen.
Created attachment 223415 [details] [review] sessionMode: Reindent This puts all the parameters at the same indent level, which makes the file much easier to read.
Created attachment 223416 [details] [review] sessionMode: Inherit from a more restrictive session mode by default It makes more sense to define session modes in terms of what you're adding to the bare shell, not in terms of what you're taking away from the user session.
Review of attachment 223412 [details] [review]: Yes, I hate you with the bottom of my hearth, but please reword that commit message ;) ::: js/ui/components/__init__.js @@ +27,3 @@ + })).forEach(Lang.bind(this, function(name) { + this._disableComponent(name); + })); And yes, sorting would help us. How: 1) Have the two lists, call them old and new 2) Pick an element of each 3) If the elements match, advance both lists 4) Pick the lowest element 5) If it comes from old, disable it 6) If it comes from new, enable it 7) Advance the list it comes from 8) Continue until one of the list is empty 9) Continue with the other list, disabling or enabling as appropriate This is simple set merging, and it is O(N) for sorted lists, compared to O(N^2) that you wrote. Btw, my assumption was that you atomically changed _enabledComponents at the end of the process. If you just push the name in _enableComponent you lose the ordering, so this no longer works.
(In reply to comment #45) > Created an attachment (id=223411) [details] [review] > [...] > > There's one bug in here that I can't find. It's just console spew, so it's > not harmful to land, but the panel changes to have an St.Bin wrapper breaks > hiding the app menu when entering the overview: > > (lt-gnome-shell-real:28828): Clutter-WARNING **: ./clutter-actor.c:9670: Actor > '<appMenu>[<StBin>:0x1c06930]' tried to allocate a size of -12.00 x 27.00 > > and more like it. That's a regression from "st-bin: Don't allocate a hidden actor", because st-bin gets an empty allocation but still forwards it to the hidden actor, and clutter emits the warning before checking if the actor is visible. Do you remember what prompted you to create this patch initially? I reverted it and saw no difference.
Review of attachment 223411 [details] [review]: Let's do this.
Review of attachment 223413 [details] [review]: Ok then.
Review of attachment 223414 [details] [review]: .
Review of attachment 223415 [details] [review]: We should have done this before
Review of attachment 223416 [details] [review]: Ok
Created attachment 223450 [details] [review] VolumeMenu: read output and input if PulseAudio is already ready when constructing Previously we would only read the default sink and default source when the connection to PulseAudio succeded. This worked because all VolumeMenu users where initialized synchronously during shell load. With the recent session mode changes though, the lock screen menu is created on demand, and when it loads PA is already connected, so it doesn't update the sliders.
Created attachment 223451 [details] [review] Panel: move the _panelContainer down to PanelMenu Panel already forces each item to be a PanelMenu.Button, so it's better to have the latter handle the bin container too, instead of attaching a private property that might collide with internal usage by the indicator.
Review of attachment 223412 [details] [review]: Wait a second... what happened to Main.screenShield.showDialog()? Or if you prefer, what makes us show the GDM login dialog now?
Review of attachment 223244 [details] [review]: ::: js/ui/keyringPrompt.js @@ +219,3 @@ + }, + + disable: function() { You need to unregister here, or you get warnings when you enable again,
Created attachment 223454 [details] [review] Components: use linear set merging for computing changes Take advantage from sorting component names in the session mode and avoid the quadratic behavior of continuously scanning both lists for matches. This is what I had in mind for sorting. Btw, this is seriously needed. There is some bug in your code, and components get disabled multiple times (noticed on top of my branch which has separate modes for lock-screen and unlock-dialog)
Created attachment 223455 [details] [review] ScreenShield: use session mode to handle the GDM login dialog Have main.js call .showDialog() when going back from the lock-screen, instead of using the return value of createUnlockDialog to know if the dialog was persistent. _keepDialog is still used as LoginDialog cannot really be destroyed, and cancelling it does not destroy it. Your previous code assumed that from lock screen you could only go to the unlock dialog, which is not true in the idle gdm case. This is an attempt to fix that, while generally cleaning the code around it.
Review of attachment 223411 [details] [review]: ::: js/ui/main.js @@ +145,3 @@ +function _sessionUpdated() { + Meta.keybindings_set_custom_handler('panel-run-dialog', sessionMode.hasRunDialog ? openRunDialog : null); Uh oh. Testing reveals that Meta.keybindings_set_custom_handler does not (allow-none) for its handler function.
(In reply to comment #52) > That's a regression from "st-bin: Don't allocate a hidden actor", because > st-bin gets an empty allocation but still forwards it to the hidden actor, and > clutter emits the warning before checking if the actor is visible. > Do you remember what prompted you to create this patch initially? I reverted it > and saw no difference. When I hide the container, I got a bit of padding/spacing/whatever where a supposedly hidden menu item should be. (In reply to comment #64) > Review of attachment 223411 [details] [review]: > > ::: js/ui/main.js > @@ +145,3 @@ > > +function _sessionUpdated() { > + Meta.keybindings_set_custom_handler('panel-run-dialog', > sessionMode.hasRunDialog ? openRunDialog : null); > > Uh oh. Testing reveals that > Meta.keybindings_set_custom_handler does not (allow-none) for its handler > function. Whoops, I forgot I had a local patch for that.
Review of attachment 223454 [details] [review]: I don't see the point of this. Yes, it's O(N^2), but like N is 5 in the worst case. If this is a bottleneck, you must have a GPU from like 2151 (and can I borrow that time machine?). It also makes the code much less readable and more confusing.
Comment on attachment 223244 [details] [review] keyringPrompt: Turn into a component Whoops, I forgot to obsolete this one.
Review of attachment 223455 [details] [review]: I eventually want the goal of making sessionMode effectively JSON (so no function/class references), so I thought about turning the shield/dialog into a component, but didn't get very far. This is fine for now.
Review of attachment 223451 [details] [review]: Yeah, this makes sense.
(In reply to comment #66) > Review of attachment 223454 [details] [review]: > > I don't see the point of this. Yes, it's O(N^2), but like N is 5 in the worst > case. If this is a bottleneck, you must have a GPU from like 2151 (and can I > borrow that time machine?). It also makes the code much less readable and more > confusing. Actually, I find it a lot cleaner. If you want, my code follows a standard pattern, yours reuses .forEach and .filter in a confusing way, and relies on side effects. It's not just a matter of performance (although when all else equals, getting the complexity right is a good thing). Plus, my code works, yours doesn't (enable and disable are not paired, in my testing)
(In reply to comment #70) > Actually, I find it a lot cleaner. If you want, my code follows a standard > pattern, yours reuses .forEach and .filter in a confusing way, and relies on > side effects. It's not just a matter of performance (although when all else > equals, getting the complexity right is a good thing). Side effects? > Plus, my code works, yours doesn't (enable and disable are not paired, in my > testing) Oh, whoops, I never removed things from _enableComponents, do I...
(In reply to comment #70) > Actually, I find it a lot cleaner. If you want, my code follows a standard > pattern, yours reuses .forEach and .filter in a confusing way, and relies on > side effects. It's not just a matter of performance (although when all else > equals, getting the complexity right is a good thing). > > Plus, my code works, yours doesn't (enable and disable are not paired, in my > testing) The other thing is that it pretty much matches the code in extensionSystem.js, which is a big plus to me.
Attachment 223411 [details] pushed as ca2e09f - sessionMode: Allow changing the session mode at runtime Attachment 223412 [details] pushed as 2a800e4 - Rearchitect the Shell to have a components system Attachment 223413 [details] pushed as ec01f5d - userMenu: Make the user menu insensitive in the lock screen Attachment 223414 [details] pushed as f1ca96b - userMenu: Don't update the presence icon immediately Attachment 223415 [details] pushed as 59e2710 - sessionMode: Reindent Attachment 223416 [details] pushed as 16e92a7 - sessionMode: Inherit from a more restrictive session mode by default Feel free to push the other patches now, Giovanni.
Attachment 223450 [details] pushed as 3f5edf7 - VolumeMenu: read output and input if PulseAudio is already ready when constructing Attachment 223451 [details] pushed as 7c244b0 - Panel: move the _panelContainer down to PanelMenu Attachment 223455 [details] pushed as 92d8d65 - ScreenShield: use session mode to handle the GDM login dialog Ok, those were the last approved patches on this bug. Let's wait for the inevitable regressions...