GNOME Bugzilla – Bug 693171
Add window-list in classic mode
Last modified: 2013-02-05 19:11:16 UTC
As for legacy-colors, it would be good to have this in 3.7.5 and continue work on master (it is rather big though, so I'd certainly understand if it only gets review post-release)
Created attachment 235175 [details] [review] window-list: New extension
Created attachment 235176 [details] [review] Factor out WindowTitle
Created attachment 235177 [details] [review] Add option for grouping windows by application
Created attachment 235178 [details] [review] Add a small preference UI
Review of attachment 235175 [details] [review]: ::: extensions/window-list/extension.js @@ +142,3 @@ + })); + this._itemAddedId = + Main.messageTray.connect('summary-item-added', source-added @@ +148,3 @@ + + _itemAdded: function(tray, item) { + item.source._windowListDestroyId = item.source.connect('destroy', Lang.bind(this, this._itemRemoved)); Do we really need this? Can't we just export the number of sources from message tray? Also, does it account for disabled sources? @@ +167,3 @@ + + _onDestroy: function() { + Main.messageTray.disconnect(this._itemAddedId); And all the windowListDestroyId @@ +282,3 @@ + for (let i = 0; i < numWorkspaces; i++) { + let workspace = global.screen.get_workspace_by_index(i); + if (workspace._windowAddedId) Heh, if the workspace is already connected, you can skip the disconnect+riconnect. Especially since this signal might be called often with dynamic workspaces. Also, windowAddedId/windowRemovedId probably needs namespacing, or a wrapper object + a hash table. @@ +303,3 @@ + + if (Main.keyboard.actor) + Main.keyboard.actor.anchor_y = 0; It is probably better to call layoutManager.hideKeyboard() here, you don't know what else might be affecting the anchor point.
Review of attachment 235176 [details] [review]: Ok
Review of attachment 235177 [details] [review]: ::: extensions/window-list/extension.js @@ +16,3 @@ + NEVER: 0, + AUTO: 1, /* TODO: implement */ +const Convenience = Me.imports.convenience; Do we need to have AUTO if not implemented? @@ +167,3 @@ + this._multiWindowTitle.add(new St.Label({ text: app.get_name() })); + + Lang.bind(this, this._updateIconGeometry)); Using one menuManager per app, instead of a global one, breaks key nav. @@ +411,3 @@ + this._settings = Convenience.getSettings(); + this._settings.connect('changed::grouping-mode', + Lang.bind(this, this._groupingModeChanged)); Disconnect the signal, or run_dispose() the settings object when destroyed.
Review of attachment 235178 [details] [review]: ::: extensions/window-list/prefs.js @@ +44,3 @@ + let currentMode = this._settings.get_string('grouping-mode'); + let range = this._settings.get_range('grouping-mode'); + let modes = range.get_child_value(1).get_variant().deep_unpack(); Can't you do range.deep_unpack()[1].deep_unpack()?
Created attachment 235181 [details] [review] window-list: New extension (In reply to comment #5) > + _itemAdded: function(tray, item) { > + item.source._windowListDestroyId = item.source.connect('destroy', > Lang.bind(this, this._itemRemoved)); > > Do we really need this? Can't we just export the number of sources from message > tray? Sure, if we also add a 'source-number-changed' signal (or just 'source-removed'); however it's something that's not interesting for anything in core, so it's cheating :-) > Also, does it account for disabled sources? No, but it doesn't look like it should? 'source-added' is only emitted for sources that are added to the tray (e.g. enabled sources) ...
Created attachment 235182 [details] [review] Factor out WindowTitle (just reattaching to maintain patch order)
Created attachment 235183 [details] [review] Add option for grouping windows by application (In reply to comment #7) > Review of attachment 235177 [details] [review]: > > ::: extensions/window-list/extension.js > @@ +16,3 @@ > + NEVER: 0, > + AUTO: 1, /* TODO: implement */ > +const Convenience = Me.imports.convenience; > > Do we need to have AUTO if not implemented? I mean to implement it for 3.8, but I can split it out and keep it locally if it bothers you a lot. > @@ +167,3 @@ > + this._multiWindowTitle.add(new St.Label({ text: app.get_name() })); > + > + Lang.bind(this, this._updateIconGeometry)); > > Using one menuManager per app, instead of a global one, breaks key nav. (keynav was actually not implemented, I've added that now in the first patch) Using a global menuManager is correct for a menu bar, however the window list contains buttons (of which some open a menu on click). Requiring the menu to be closed before navigating to other window buttons feels very much correct to me - it's also consistent with keynav in the message tray. > Disconnect the signal, or run_dispose() the settings object when destroyed. OK.
Created attachment 235184 [details] [review] Add a small preference UI (In reply to comment #8) > Can't you do range.deep_unpack()[1].deep_unpack()? Indeed.
(In reply to comment #9) > Created an attachment (id=235181) [details] [review] > window-list: New extension > > (In reply to comment #5) > > + _itemAdded: function(tray, item) { > > + item.source._windowListDestroyId = item.source.connect('destroy', > > Lang.bind(this, this._itemRemoved)); > > > > Do we really need this? Can't we just export the number of sources from message > > tray? > > Sure, if we also add a 'source-number-changed' signal (or just > 'source-removed'); however it's something that's not interesting for anything > in core, so it's cheating :-) So? It would not be the first time we add API for the benefit of extensions, and these are the official extensions, they deserve an higher standard. > > > Also, does it account for disabled sources? > > No, but it doesn't look like it should? 'source-added' is only emitted for > sources that are added to the tray (e.g. enabled sources) ... Yes, but as you pointed out, there is no source-removed, so it would be buggy if you get a source and then disable notifications from that app in the panel. (In reply to comment #11) > Created an attachment (id=235183) [details] [review] > Add option for grouping windows by application > > (In reply to comment #7) > > Review of attachment 235177 [details] [review] [details]: > > > > ::: extensions/window-list/extension.js > > @@ +16,3 @@ > > + NEVER: 0, > > + AUTO: 1, /* TODO: implement */ > > +const Convenience = Me.imports.convenience; > > > > Do we need to have AUTO if not implemented? > > I mean to implement it for 3.8, but I can split it out and keep it locally if > it bothers you a lot. Bah, it just seems confusing that you let the user select it if it's not implemented. Also, you check for != ALWAYS and != NEVER, so AUTO results in a completely empty tray.
(In reply to comment #13) > > No, but it doesn't look like it should? 'source-added' is only emitted for > > sources that are added to the tray (e.g. enabled sources) ... > > Yes, but as you pointed out, there is no source-removed, so it would be buggy > if you get a source and then disable notifications from that app in the panel. The source actor is destroyed when it is removed from the tray, so source::destroy is more or less an alias for messageTray::source-removed (at least that's what the code suggests, in testing existing sources were not removed at all when disabling notifications, but that's supposedly just a bug). Anyway, I'll add an explicit 'source-removed' signal ...
Created attachment 235245 [details] [review] window-list: New extension Use new 'source-removed' signal instead of summary item actor's destroy signal
Created attachment 235246 [details] [review] Add option for grouping windows by application Remove AUTO for now.
Created attachment 235247 [details] [review] Add a small preference UI Dto.
Created attachment 235248 [details] [review] window-list: Add classic mode styling Classic mode uses a distinct visual style, support this by shipping a dedicated stylesheet.
(In reply to comment #14) > > The source actor is destroyed when it is removed from the tray, so > source::destroy is more or less an alias for messageTray::source-removed (at > least that's what the code suggests, in testing existing sources were not > removed at all when disabling notifications, but that's supposedly just a bug). Ugh really? I recall testing it when I wrote the notification filtering patches, and it worked...
Review of attachment 235245 [details] [review]: LGTM
Review of attachment 235246 [details] [review]: Yes
Review of attachment 235247 [details] [review]: Yes.
Review of attachment 235248 [details] [review]: Ok, just a minor style nit. ::: extensions/window-list/classic.css @@ +6,3 @@ + } + + .bottom-panel .window-button > StWidget { Wrong indentation?
Attachment 235245 [details] pushed as b843058 - window-list: New extension Attachment 235248 [details] pushed as a1c938d - window-list: Add classic mode styling