Created attachment 236794 [details] [review]
appDisplay: Remove categories

We will eventually display some categories as app folders, but the
existing code doesn't help with the implementation, so remove it.

Created attachment 236795 [details] [review]
appDisplay: Merge AppIcon and AppWellIcon

AppIcon is just a tiny wrapper around BaseIcon, which does not add
anything over using BaseIcon directly, so merge its code with
AppWellIcon. As the concept of the "app well" has not been used
since well before 3.0, use AppIcon as name for the merged class.

Created attachment 236796 [details] [review]
appDisplay: Merge AllAppDisplay and ViewByCategories

With categories removed, the separation between AllAppDisplay and
ViewByCategories no longer makes sense. Also use this opportunity
to rename the outdated AllAppDisplay to AppDisplay; it will
eventually be used to manage different views.

Created attachment 236797 [details] [review]
appDisplay: Remove unused variable

Created attachment 236798 [details] [review]
messageTray: Move makeCloseButton() into Util

The function is useful for other modules as well, so move it to a
more generic place.

Created attachment 236799 [details] [review]
appDisplay: Make _loadCategory() a utility function

When we bring back categories as folders, this method will be
useful outside of AppDisplay.

Created attachment 236800 [details] [review]
appDisplay: Make scrolling in AlphabeticalView optional

Folders will reuse AlphabeticalView to display a category, but we
only want scrolling in the primary view, so add a parameter to
make scrolling optional.

Created attachment 236801 [details] [review]
appDisplay: Set a maximum column number

In particular on widescreen monitors, the number of items in a row
can be overwhelming, so limit the number of columns and center the
view.

Created attachment 236802 [details] [review]
appDisplay: Add intermediate stack widget to hold folder popups

AlphabeticalView will be updated to allow app folders in addition
to app launchers. Folders will pop up in the existing view, so add
an additional stack widget into the hierarchy which popups will use
as parent.

Created attachment 236803 [details] [review]
appDisplay: Add new FolderIcon class

FolderIcons will appear in the primary app view along AppIcons, but
represent a group of applications rather than a single application.

Created attachment 236804 [details] [review]
appDisplay: Add AlphabeticalView.addFolder() method

Adjust AlphabeticalView to be able to hold both apps and folders,
and add a addFolder() method to insert a folder.

Created attachment 236805 [details] [review]
appDisplay: Create AppFolderIcons for selected categories

App folders are intended for grouping some applications, not to
assign a category to every single application, so we will only
create folders for a selected subset of the existing categories.
Software/Alacarte will eventually allow to create/modify those
folders, so store the setting in GSettings so that it can be
shared with those applications.

Created attachment 236806 [details] [review]
appDisplay: Make folder popups "view-modal"

While a group is open, we want to block events on items underneath,
but still allow interaction with components outside the app display
as well as scrolling of the view as a whole.

Created attachment 236807 [details] [review]
shell-app-usage: Remove unused parameter

Created attachment 236808 [details] [review]
appDisplay: Add additional view showing frequently used apps

According to the design mockups, the app picker should follow the
recent view pattern as used by applications, where the user is first
offered a subset of applications he/she is likely to start, and only
then allow switching to the full set of installed applications.
So implement the ability to manage several views in AppDisplay and add
FrequentView as additional view, which uses the existing ShellAppUsage
to display a list of frequently used applications.

Review of attachment 236795 [details] [review]:

Yes, but for 3.10 we can probably kill the whole BaseIcon class, as the only users are AppDisplay and Wanda (and the latter should be updated for the search design changes)

Review of attachment 236797 [details] [review]:

Yes.

Review of attachment 236794 [details] [review]:

I personally like categories, but if this is the design, then ok.

Review of attachment 236796 [details] [review]:

Yes.

(Although AppDisplay is still a bad name, AppPage/AppView would be better...)

Review of attachment 236798 [details] [review]:

We have a close button in WindowOverlay too. Merge it?

Review of attachment 236799 [details] [review]:

One minor nit.

::: js/ui/appDisplay.js
@@ +30,3 @@
 
+// Recursively load a GMenuTreeDirectory; we could put this in ShellAppSystem too
+function _loadCategory(dir, view) {

Why the underscore?

Review of attachment 236800 [details] [review]:

I'm not convinced on this one. If you remove scrolling, most of AlphabeticalView code (panning, moving for offsets) ceases to be useful. I would use a different class for folders instead.

Review of attachment 236797 [details] [review]:

Uh oh, appSystem is unused too.

Review of attachment 236801 [details] [review]:

Yes.

(In reply to comment #19)
> (Although AppDisplay is still a bad name, AppPage/AppView would be better...)

I agree that AppView is nicer, but that would leave us with WorkspacesDisplay, AppView and SearchResults as view classes - AppDisplay is at least consistent with WorkspacesDisplay ...

The version that I'm running remembers the frequent / all state - if I'm in all and close the overview, when I reopen the app view I find myself in the all apps view again.

We should probably start with frequent apps every time you enter the apps view.

Review of attachment 236802 [details] [review]:

::: js/ui/appDisplay.js
@@ +62,3 @@
         this._allApps = [];
 
         let box = new St.BoxLayout({ vertical: true });

Why do we have this box layout at all?

(In reply to comment #21)
> +function _loadCategory(dir, view) {
> 
> Why the underscore?

As an indication that it is private to this module. I don't have any problem with removing it if you deem it better though ...

(In reply to comment #26)
> We should probably start with frequent apps every time you enter the apps view.

I don't mind either way, but the design page mentions that the switch should be persistent.

(In reply to comment #27)
>          let box = new St.BoxLayout({ vertical: true });
> 
> Why do we have this box layout at all?

Because only Scrollables can be added to a ScrollView, and StBoxLayout is the only widget we have that implements the interface ...

Review of attachment 236803 [details] [review]:

::: js/ui/appDisplay.js
@@ +358,3 @@
+            function(popup, isOpen) {
+                if (!isOpen)
+                if (!this.actor.mapped && this._popup)

Isn't this recursive from notify::checked?

@@ +390,3 @@
+
+        this._boxPointer.actor.bind_property('opacity', closeButton, 'opacity',
+

Wouldn't it be easier to add the close button to the box pointer, ie reverse the relation between actor and _boxPointer?

@@ +404,3 @@
+        this.actor.show();
+        this._boxPointer.setArrowOrigin(this._source.actor.x + this._source.actor.width / 2);
+

No slide?
It has a definite origin, which according to the designs is the distinction between fade and fade+slide.

Review of attachment 236804 [details] [review]:

::: js/ui/appDisplay.js
@@ +116,2 @@
         let appIcon = new AppIcon(app);
+        let pos = Util.insertSorted(this._allItems, app, this._compareItems);

While we're here, we can fix this horrible quadratic behavior: addApp is only called by loadCategory(), so make it insert at the end, and sort when everything is loaded.

Review of attachment 236805 [details] [review]:

This is a stretch of the menu spec really, and the proper way to do it would be to have gnome-applications.menu with just those two categories, but that would break apps-menu, so ok.

(What's "Sundry", btw?)

Review of attachment 236806 [details] [review]:

Uhm... GrabHelper? Or even PopupMenuManager, for keynav between folders?

Review of attachment 236807 [details] [review]:

You could remove context too...

Review of attachment 236808 [details] [review]:

::: js/ui/appDisplay.js
@@ +234,3 @@
+        // the allocation, so rather than clipping away eventual overflow, we
+        // use an unscrollable ScrollView with hidden scrollbars to nicely
+                                             columnLimit: 6 });

This needs changing after IRC discussion.

(In reply to comment #31)
> ::: js/ui/appDisplay.js
> @@ +358,3 @@
> +            function(popup, isOpen) {
> +                if (!isOpen)
> +                if (!this.actor.mapped && this._popup)
> 
> Isn't this recursive from notify::checked?

Uhm, not really? 'notify::checked' is about opening/closing the menu when the button state changes, 'open-state-changed' is about unchecking the button if the popup was closed by other means (close button etc)


> Wouldn't it be easier to add the close button to the box pointer, ie reverse
> the relation between actor and _boxPointer?

Not sure what you mean - _boxPointer.actor is an StBin, so it doesn't allow for additional children (if you mean to put view.actor and close button into an StWidget and add that to _boxPointer.bin, we'd end up with a mispositioned close button)

Review of attachment 236801 [details] [review]:

You should use MAX_COLUMNS for the frequent apps as well.

(In reply to comment #36)
> Review of attachment 236808 [details] [review]:
> 
> ::: js/ui/appDisplay.js
> @@ +234,3 @@
> +        // the allocation, so rather than clipping away eventual overflow, we
> +        // use an unscrollable ScrollView with hidden scrollbars to nicely
> +                                             columnLimit: 6 });
> 
> This needs changing after IRC discussion.

Uh ... I should read all the comments first ...

Review of attachment 236801 [details] [review]:

Back to acn ...

(In reply to comment #33)
> This is a stretch of the menu spec really, and the proper way to do it would be
> to have gnome-applications.menu with just those two categories, but that would
> break apps-menu, so ok.

Right, the idea here is to allow using the new and traditional representations side-by-side.

(In reply to comment #33)
> (What's "Sundry", btw?)

http://git.gnome.org/browse/gnome-menus/commit/?id=36d5d699d7d4193a1b3d84777566466326f78b19

(In reply to comment #34)
> Uhm... GrabHelper? Or even PopupMenuManager, for keynav between folders?

I originally used GrabHelper, but it's not a good fit (without adding a bunch of new API): it doesn't allow to allow events for a parent and not its children (scroll view and items), or to only take a "grab" for part of the hierarchy (app display, leaving dash etc. working normally). Regarding keynav, I see folders as being much closer to e.g. summary items than menus, so I think we want to require closing a folder first before moving to the next one.

(In reply to comment #35)
> You could remove context too...

Technically it's not unused, we just use the same context everywhere. Still, might come in handy at some point, so leaving it for now.

Review of attachment 236804 [details] [review]:

::: js/ui/appDisplay.js
@@ +179,3 @@
         let aligns = [ Clutter.ActorAlign.START, Clutter.ActorAlign.END ];
+        let n = 0;
+        for (let i = 0; i < this._allItems.length; i++) {

Can you merge some of these fixes back into the "Add FolderIcon" patch? Seems strange to introduce a method and then change up control flow.

Review of attachment 236807 [details] [review]:

I have some bugs elsewhere to remove context.

(In reply to comment #45)
> Review of attachment 236804 [details] [review]:
> 
> ::: js/ui/appDisplay.js
> @@ +179,3 @@
>          let aligns = [ Clutter.ActorAlign.START, Clutter.ActorAlign.END ];
> +        let n = 0;
> +        for (let i = 0; i < this._allItems.length; i++) {
> 
> Can you merge some of these fixes back into the "Add FolderIcon" patch? Seems
> strange to introduce a method and then change up control flow.

It makes sense to not add the special case for _allApps.length == 0 in the first place, but I think the other changes are in the correct place (rename _allApps to _allItems, account for items that are not apps).

(In reply to comment #43)
> (In reply to comment #34)
> > Uhm... GrabHelper? Or even PopupMenuManager, for keynav between folders?
> 
> I originally used GrabHelper, but it's not a good fit (without adding a bunch
> of new API): it doesn't allow to allow events for a parent and not its children
> (scroll view and items), or to only take a "grab" for part of the hierarchy
> (app display, leaving dash etc. working normally). Regarding keynav, I see
> folders as being much closer to e.g. summary items than menus, so I think we
> want to require closing a folder first before moving to the next one.

And that choice is unclear to me. We have the bubble metaphor all around the shell, but it's always modal.
It's inconsistent.

(All other comments pretty much ok)

(In reply to comment #42)
> (In reply to comment #33)
> > (What's "Sundry", btw?)
> 
> http://git.gnome.org/browse/gnome-menus/commit/?id=36d5d699d7d4193a1b3d84777566466326f78b19

I know, I saw the category. Now, it might be I'm not a native speaker, but it doesn't sound like a good user visible name...

(In reply to comment #48)
> And that choice is unclear to me. We have the bubble metaphor all around the
> shell, but it's always modal.
> It's inconsistent.

Yes, and maybe we should consider to adjust the styling to make the distinction more clear. My original proposal used actual popup bubbles (e.g. a PopupMenu controlled by GrabHelper), but depending on the number of apps in the folder you could end up with scrolling inside the menu. The designers were pretty shocked at the sight of double scrollbars, thus the design of moving the folder content inside the main scroll view.
(There's a point where I actually got the intention of the mockup wrong - Jakub intended the folder content to push the other content aside (similar to expanders/shell submenus), but the "popup look" fooled me)

Created attachment 236842 [details] [review]
appDisplay: Separate app loading from filling the grid

At the moment when loading the applications, each app is inserted
at its correct (alphabetical) position. Avoid this overhead by
loading all apps first, then sort them once and fill the grid with
the sorted actors.


(In reply to comment #32)
> Review of attachment 236804 [details] [review]:
> 
> ::: js/ui/appDisplay.js
> @@ +116,2 @@
>          let appIcon = new AppIcon(app);
> +        let pos = Util.insertSorted(this._allItems, app, this._compareItems);
> 
> While we're here, we can fix this horrible quadratic behavior: addApp is only
> called by loadCategory(), so make it insert at the end, and sort when
> everything is loaded.

Yeah, that's a valid point. (Not reattaching the whole bunch, rebased patches are in the branch)

Ok, I went through the branch again, it's mostly acn for me, but:

1) I still believe the non scrollable alphabetical view deserves its own class, rather than a scrollable parameter.
Or maybe FolderView should inherit from a base AppView, shared with the current AlphabeticalView, if you want to keep the AppIcon creation and sorting.

2) Setting checked from open-state-changed (emitted from popup()/popdown()) causes a notify::checked, which triggers another popup()/popdown(). The second one sees the same state and stops, but it's still recursive.
It would look cleaner to me to have toggle() on FolderPopup, and connect clicked instead of notify::checked

(In reply to comment #51)
> 1) I still believe the non scrollable alphabetical view deserves its own class,
> rather than a scrollable parameter.
> Or maybe FolderView should inherit from a base AppView, shared with the current
> AlphabeticalView, if you want to keep the AppIcon creation and sorting.

Yes, if we split this, we want a common base class. Currently on it.


> 2) [...]
> It would look cleaner to me to have toggle() on FolderPopup, and connect
> clicked instead of notify::checked

Sure, can do.

Created attachment 236854 [details] [review]
appDisplay: Make AlphabeticalView an abstract base class of AllView

We are going to introduce app folders, which will also present an
alphabetical list of applications, but use a different UI. In
preparation for that, split out the item logic into an abstract
base class.


> 1) I still believe the non scrollable alphabetical view deserves its own class,
> rather than a scrollable parameter.

Here we are then. (I'll update the branch in a minute).

Created attachment 236857 [details] [review]
appDisplay: Add new FolderIcon class

> 2) [...]
> It would look cleaner to me to have toggle() on FolderPopup, and connect
> clicked instead of notify::checked

... and here.

Review of attachment 236842 [details] [review]:

So, this one was a yes.

Review of attachment 236854 [details] [review]:

::: js/ui/appDisplay.js
@@ +77,3 @@
+    _compareItems: function(a, b) {
+        throw new Error('Not implemented');
+

These three can be shared, right?

Review of attachment 236857 [details] [review]:

Much better.

(In reply to comment #56)
> Review of attachment 236854 [details] [review]:
> 
> ::: js/ui/appDisplay.js
> @@ +77,3 @@
> +    _compareItems: function(a, b) {
> +        throw new Error('Not implemented');
> +
> 
> These three can be shared, right?

Ah, sorry, I saw now that you change the implementation in a later patch. Acn then.

OK, thanks for the reviews!

Attachment 236794 [details] pushed as 77d8ff3 - appDisplay: Remove categories
Attachment 236795 [details] pushed as 2658754 - appDisplay: Merge AppIcon and AppWellIcon
Attachment 236796 [details] pushed as c9e2f16 - appDisplay: Merge AllAppDisplay and ViewByCategories
Attachment 236798 [details] pushed as f21b820 - messageTray: Move makeCloseButton() into Util
Attachment 236799 [details] pushed as 9758029 - appDisplay: Make _loadCategory() a utility function
Attachment 236801 [details] pushed as 2fbd9b9 - appDisplay: Set a maximum column number
Attachment 236802 [details] pushed as 25d8deb - appDisplay: Add intermediate stack widget to hold folder popups
Attachment 236805 [details] pushed as e3c9c46 - appDisplay: Create AppFolderIcons for selected categories
Attachment 236806 [details] pushed as 6628214 - appDisplay: Make folder popups "view-modal"
Attachment 236807 [details] pushed as 10e16cc - shell-app-usage: Remove unused parameter
Attachment 236808 [details] pushed as b75fc2b - appDisplay: Add additional view showing frequently used apps
Attachment 236842 [details] pushed as 65f9649 - appDisplay: Separate app loading from filling the grid
Attachment 236854 [details] pushed as 426b227 - appDisplay: Make AlphabeticalView an abstract base class of AllView
Attachment 236857 [details] pushed as 6e3e2d9 - appDisplay: Add new FolderIcon class

*** Bug 638271 has been marked as a duplicate of this bug. ***

Comment 26 filed as bug 694349.