GNOME Bugzilla – Bug 682109
Kill mode-switch
Last modified: 2012-10-10 00:39:02 UTC
Patch series to remove mode-switching from the overview. More details here: https://live.gnome.org/GnomeShell/Design/AppPickerRefresh
Created attachment 221626 [details] [review] viewSelector: Use a fixed set of view tabs Design calls for views being accessible by other means than the current tab system, so we have no longer a need for the public viewTab API. Move the initialization of tabs to the viewSelector and make viewSelector.addViewTab() private.
Created attachment 221627 [details] [review] overview: Move the entry out of the SearchTab into the overview The entry should be positioned in the center of the overview. This makes that its position can’t be set in the viewSelector without making things overly complicated. Therefore we move the entry to the overview.
Created attachment 221628 [details] [review] dash: Make the favRemoveTarget always visible We’ll be repurposing the favRemoveTarget, which calls for it the be permanently visibe. The favRemoveTarget used to be added to the dash when needed and removed again when it wasn’t. This made that it always appeared at the bottom of the dash. Now that we always show it, we also need to explicitly define it to be at the bottom of the dash.
Created attachment 221629 [details] [review] dash: Refactor favRemoveTarget to showAppsIcon In the new designs, we no longer need favRemoveTarget. As it shares a lot of its functionality with the new showAppsIcon, we refactor and restyle it accordingly.
Created attachment 221630 [details] [review] viewSelector: Hook up the dash's showApps button We pass the dash’s showApps button to the viewSelector, and we connect it to the showing and hiding of the appsView. This is necessary because there are different mechanisms for switching the views, and it has to stay in sync with the button’s state.
Created attachment 221631 [details] [review] viewSelector: Remove tab title bar All the functionality provided by the tab titles has been replaced, so the title bar can be removed without loss of functionality.
Created attachment 221632 [details] [review] viewSelector: Remove shortcut to switch between tabs We no longer have tabs, so it no longer makes sense to keep the shortcut.
Created attachment 221633 [details] [review] overview: Center the dash vertically
Created attachment 221634 [details] [review] viewSelector: Merge SearchTab into the ViewSelector Tabs used to provide an abstraction for a page and the control used to activate it. As the latter has now been replaced with external controls handled directly in the viewSelector, the abstraction itself doesn't make much sense anymore. In preparation of replacing it, move the search handling provided by the SearchTab directly in the viewSelector.
Created attachment 221635 [details] [review] viewSelector: Replace tab system with pages The old tab system is no longer a good fit for the current design, so replace most of the abstractions with a simpler page system.
Review of attachment 221626 [details] [review]: ::: js/ui/overview.js @@ -211,2 @@ // Load remote search providers provided by applications RemoteSearch.loadRemoteSearchProviders(Lang.bind(this, this.addSearchProvider)); It doesn't make sense to initialize build-in providers in viewSelector and leave remote providers here - just move this as well.
Review of attachment 221627 [details] [review]: Mmmh, some weird junk in the commit message ... ::: js/ui/viewSelector.js @@ +127,2 @@ this._searchResults = new SearchDisplay.SearchResults(this._searchSystem, this._openSearchSystem); + this.parent(new St.Bin(), this._searchResults.actor, _("Search"), 'edit-find'); The first parameter could use a comment - a simple new St.Bin() /* dummy */ is probably enough
Review of attachment 221628 [details] [review]: LGTM
Review of attachment 221626 [details] [review]: Or don't move anything. Search providers are not tabs.
Review of attachment 221629 [details] [review]: ::: data/theme/gnome-shell.css @@ +864,3 @@ .search-result-content:focus > .overview-icon, +.show-apps:focus > .overview-icon, +.contact:selected, Mmmh? Rebase gone wrong?
Review of attachment 221627 [details] [review]: The junk is MIME headers. git format-patch will put it there if there's any bytes not in the ASCII range in the message.
Review of attachment 221630 [details] [review]: Looks good!
Review of attachment 221631 [details] [review]: Less code, good!
Review of attachment 221632 [details] [review]: Yeah, makes sense.
Review of attachment 221633 [details] [review]: ::: js/ui/overview.js @@ +510,3 @@ + let viewY = searchY + searchHeight + this._spacing; + let viewWidth = primary.width - dashWidth - this._spacing; + let viewHeight = contentHeight - this._spacing - viewY; Mmh, "viewY" in the height calculation assumes that the primary monitor is also the top monitor. Admittedly having the primary monitor below other monitors doesn't really work for other reasons as well, and the same assumption is already present in the current code (see contentY/contentHeight above), but still ...
Review of attachment 221634 [details] [review]: LGTM
Review of attachment 221635 [details] [review]: Looks mostly good - in addition to the comments below, there are also some left-overs in the CSS. Please remove those as well. ::: js/ui/viewSelector.js @@ +168,3 @@ + x_fill: true, + y_fill: true, + style_class: 'view-tab-page' }); view-tab-page? ;-) @@ +327,3 @@ })); } + this._showPage(this._searchPage); So you are switching to the search page here (as the old code did) ... @@ +413,3 @@ this._searchResults.doSearch(text); + this._showPage(this._searchPage); ... and here - one of those should go! Having it in _doSearch() looks right to me ...
Created attachment 221647 [details] [review] perf: Update to latest overlay changes Removing tabs from the overview broke the perf test for the application view; update test to the new method of bringing it up.
I've been testing the work in progress for this. It's really good, and you seem to have all the bases covered. One small thing - I'd suggest making the search box a bit wider, since it looks a bit constrained right now. It's about 280px wide now, compared with 320px in the mockups.
Created attachment 221659 [details] [review] viewSelector: Allow to start keynav using the down arrow As the search entry acts like it had keyboard focus, it seems logical to use the down arrow to move focus to the active page. (as suggested by Jon at guadec)
Created attachment 221660 [details] [review] viewSelector: Allow to close the application view with escape With the latest changes to the overview, the application view is now clearly on a different level compared to the window picker. For that reason it now makes sense to close it on Escape rather than hiding the overview directly, as we do for search.
Minor l10n note: could you replace "Show Apps" string with "Show Applications"? Much appreciated!
Created attachment 221798 [details] [review] viewSelector: Use a fixed set of view tabs Design calls for views being accessible by other means than the current tab system, so we have no longer a need for the public viewTab API. Move the initialization of tabs to the viewSelector and make viewSelector.addViewTab() private.
Created attachment 221799 [details] [review] overview: Move the entry out of the SearchTab into the overview The entry should be positioned in the center of the overview. This makes that its position can’t be set in the viewSelector without making things overly complicated. Therefore we move the entry to the overview.
Created attachment 221800 [details] [review] dash: Make the favRemoveTarget always visible We’ll be repurposing the favRemoveTarget, which calls for it the be permanently visibe. The favRemoveTarget used to be added to the dash when needed and removed again when it wasn’t. This made that it always appeared at the bottom of the dash. Now that we always show it, we also need to explicitly define it to be at the bottom of the dash.
Created attachment 221801 [details] [review] dash: Refactor favRemoveTarget to showAppsIcon In the new designs, we no longer need favRemoveTarget. As it shares a lot of its functionality with the new showAppsIcon, we refactor and restyle it accordingly.
Created attachment 221802 [details] [review] viewSelector: Hook up the dash's showApps button We pass the dash’s showApps button to the viewSelector, and we connect it to the showing and hiding of the appsView. This is necessary because there are different mechanisms for switching the views, and it has to stay in sync with the button’s state.
Created attachment 221803 [details] [review] viewSelector: Remove tab title bar All the functionality provided by the tab titles has been replaced, so the title bar can be removed without loss of functionality.
Created attachment 221804 [details] [review] viewSelector: Remove shortcut to switch between tabs We no longer have tabs, so it no longer makes sense to keep the shortcut.
Created attachment 221805 [details] [review] overview: Center the dash vertically
Created attachment 221806 [details] [review] viewSelector: Merge SearchTab into the ViewSelector Tabs used to provide an abstraction for a page and the control used to activate it. As the latter has now been replaced with external controls handled directly in the viewSelector, the abstraction itself doesn't make much sense anymore. In preparation of replacing it, move the search handling provided by the SearchTab directly in the viewSelector.
Created attachment 221807 [details] [review] viewSelector: Replace tab system with pages The old tab system is no longer a good fit for the current design, so replace most of the abstractions with a simpler page system.
Created attachment 221808 [details] [review] dash: Increase the searchEntry width to 320 pixels
Created attachment 221809 [details] [review] overview: Increase the searchEntry width to 320 pixels
Review of attachment 221798 [details] [review]: Sure
Review of attachment 221799 [details] [review]: Needs rebasing onto master
Review of attachment 221800 [details] [review]: Still good.
Review of attachment 221801 [details] [review]: ::: data/theme/gnome-shell.css @@ +864,3 @@ .search-result-content:focus > .overview-icon, +.show-apps:focus > .overview-icon, +.contact:selected, Contacts search has been removed a while ago, so don't reintroduce .contact here
Review of attachment 221802 [details] [review]: Yup
Review of attachment 221803 [details] [review]: Yup
Review of attachment 221804 [details] [review]: LGTM
Review of attachment 221805 [details] [review]: Previous comment still applies
Review of attachment 221806 [details] [review]: Please rebase onto master
Created attachment 221812 [details] [review] dash: Refactor favRemoveTarget to showAppsIcon In the new designs, we no longer need favRemoveTarget. As it shares a lot of its functionality with the new showAppsIcon, we refactor and restyle it accordingly.
Review of attachment 221807 [details] [review]: LGTM
Review of attachment 221812 [details] [review]: Looks good, thanks!
Review of attachment 221809 [details] [review]: Sure.
Created attachment 221815 [details] [review] overview: Move the entry out of the SearchTab into the overview The entry should be positioned in the center of the overview. This makes that its position can’t be set in the viewSelector without making things overly complicated. Therefore we move the entry to the overview.
Created attachment 221816 [details] [review] viewSelector: Merge SearchTab into the ViewSelector Tabs used to provide an abstraction for a page and the control used to activate it. As the latter has now been replaced with external controls handled directly in the viewSelector, the abstraction itself doesn't make much sense anymore. In preparation of replacing it, move the search handling provided by the SearchTab directly in the viewSelector.
Review of attachment 221815 [details] [review]: ::: js/ui/viewSelector.js @@ +126,3 @@ this._iconClickedId = 0; this._searchResults = new SearchDisplay.SearchResults(this._searchSystem, this._openSearchSystem); The open search system has been removed, so the above doesn't apply against master now.
Attachment 221798 [details] pushed as b5dc78a - viewSelector: Use a fixed set of view tabs Attachment 221800 [details] pushed as 2ee6f49 - dash: Make the favRemoveTarget always visible Attachment 221802 [details] pushed as 816a29d - viewSelector: Hook up the dash's showApps button Attachment 221803 [details] pushed as dff6735 - viewSelector: Remove tab title bar Attachment 221804 [details] pushed as 7d12f47 - viewSelector: Remove shortcut to switch between tabs Attachment 221805 [details] pushed as 0959adc - overview: Center the dash vertically Attachment 221807 [details] pushed as 8c40ded - viewSelector: Replace tab system with pages Attachment 221809 [details] pushed as f347aba - overview: Increase the searchEntry width to 320 pixels Attachment 221812 [details] pushed as 27fbe4c - dash: Refactor favRemoveTarget to showAppsIcon Attachment 221815 [details] pushed as 0ea2d98 - overview: Move the entry out of the SearchTab into the overview Attachment 221816 [details] pushed as c267a7a - viewSelector: Merge SearchTab into the ViewSelector
Congratulations, Joost! This is great - thanks for all your work.
Review of attachment 221647 [details] [review]: People still use the perf framework? I'm shocked.
Review of attachment 221659 [details] [review]: Sure.
Review of attachment 221660 [details] [review]: Sure.
Attachment 221647 [details] pushed as 37da87d - perf: Update to latest overlay changes Attachment 221659 [details] pushed as 776b71b - viewSelector: Allow to start keynav using the down arrow Attachment 221660 [details] pushed as ee0102e - viewSelector: Allow to close the application view with escape
Having a centered dash is slightly more cumbersome than keeping the dash at a fixed vertical offset from the top. With the centered dash, pinned applications change location on-screen depending on how many applications are running, so you can't develop any muscle memory for launching apps.