Created attachment 140936 [details] [review]
0001-dash-Make-recent-docs-display-two-columns.patch

A couple things we discussed:

1) The document list currently overflows, but it should stop at the bottom edge of the workspaces area.
2) We need the documents to be draggable. Places items need to be draggable too, which is possibly related.
3) Need more explanation comments in _getPreferredWidth(), getPreferredHeight(), and _allocate()
4) Possibly need to move out documents code to a different file to not make dash.js a new monster file :).

Created attachment 141018 [details] [review]
dnd: Centralize drag actor positioning code, add new method doDragActivate

We had multiple copies of the code to position a drag actor given a particular
source.  Instead, just put it inside dnd.js.

Second, rather than test for GenericDisplay in various places, add a new
special method doDragActivate, which drag recipients can look to invoke
when accepting a drop.

Created attachment 141019 [details] [review]
dash: Make recent docs display two columns

The design has smaller icons in two columns.  Add a new
custom display to docDisplay for it.

Clean up some of the texture cache handling for recent URIs so
it's not size-dependent, since the dash size is now different
from the default GenericDisplay size.

I think keeping the explicit checks for the class types in workspaces.js is cleaner, and is more in line with the other check for the drop source being an instance of WindowClone that is there. It basically allows workspaces to be concerned with what they can accept for the drop items and what happens when they accept the drop item and avoids the need of additional methods in other classes.

If you think that adding doDragActivate() or some other method (e.g. shellWorkspaceLaunch() which was suggested by Owen on IRC) is better, here are a few things that I noticed:

1) Dragging items to workspaces now switches to that workspace. It should stay in the overview mode instead. You only need to call
this.launch() in doDragActivate() in genericDisplay.js and in appDisplay.js, and not call this.emit("activated") to fix this.

2) Workspaces stopped accepting documents with this patch. This is probably because doDragActivate() is not present in DocDisplayItem, and source.doDragActivate returns false. Not sure what is the best way to fix this so that the code picks up on the inheritance relationship between DocDisplayItem and GenericDisplayItem.

Minor comments:
 const AppDisplay = imports.ui.appDisplay;
 const DND = imports.ui.dnd;
-const GenericDisplay = imports.ui.genericDisplay;

 const AppDisplay would no longer be needed either.

 // Draggable interface - FIXME deduplicate with GenericDisplay

Can remove the FIXME since the duplication is gone.

+ // If the user dragged from the source, then position
than

(In reply to comment #5)
> I think keeping the explicit checks for the class types in workspaces.js is
> cleaner, and is more in line with the other check for the drop source being an
> instance of WindowClone that is there. It basically allows workspaces to be
> concerned with what they can accept for the drop items and what happens when
> they accept the drop item and avoids the need of additional methods in other
> classes.

One of these things needs to know about the other.  The .shellWorkspaceLaunch approach on sources means each source says where it can be dropped and what happens, where calling instanceof has it in the destination.  I think the .shellWorkspaceLaunch is slightly cleaner because it combines "allow drop in this context" with "what to do when dropped".

> 1) Dragging items to workspaces now switches to that workspace. It should stay
> in the overview mode instead. You only need to call
> this.launch() in doDragActivate() in genericDisplay.js and in appDisplay.js,
> and not call this.emit("activated") to fix this.

Fixed, thanks.

> 2) Workspaces stopped accepting documents with this patch. This is probably
> because doDragActivate() is not present in DocDisplayItem, and
> source.doDragActivate returns false. Not sure what is the best way to fix this
> so that the code picks up on the inheritance relationship between
> DocDisplayItem and GenericDisplayItem.

Fixed, thanks.

Created attachment 141186 [details] [review]
dnd: Centralize drag actor positioning code, use shellWorkspaceLaunch for workspaces

We had multiple copies of the code to position a drag actor given a particular
source.  Instead, just put it inside dnd.js.

Second, rather than test for GenericDisplay/WellDisplayItem etc.,
in various places, add a new method on each source "shellWorkspaceLaunch"
which both marks the item as being droppable on a workspace, and is
called by the workspaces code to launch the item.

Created attachment 141187 [details] [review]
dash: Make recent docs display two columns

The design has smaller icons in two columns.  Add a new
custom display to docDisplay for it.

Clean up some of the texture cache handling for recent URIs so
it's not size-dependent, since the dash size is now different
from the default GenericDisplay size.

Created attachment 141188 [details] [review]
appDisplay: Don't leave overview for drag&drop of an app

The design says we stay in the overview for these situations.

Created attachment 141189 [details] [review]
places: Implement drag&drop for overview

Add drag and drop.  We need to be able to recreate the icon texture,
so instead of passing it directly into PlaceDisplay, pass
a factory function which knows how to create a new texture.

All the comments are pretty minor. Overall everything looks and works well!

Patch 1

>  I think the
> .shellWorkspaceLaunch is slightly cleaner because it combines "allow drop in
> this context" with "what to do when dropped".

Ok, this argument convinced me :).

         this.appInfo.launch();
     },
 
-    // Draggable interface - FIXME deduplicate with GenericDisplay
+    shellWorkspaceLaunch : function() {
+        this.appInfo.launch();
+    },

This should be this.launch() to be in agreement with what we do for AppDisplayItem and not to duplicate functionality.

+++ b/js/ui/docDisplay.js
@@ -83,9 +83,15 @@ DocDisplayItem.prototype = {
         }
     },
 
+    //// Drag and Drop ////
+
+    shellWorkspaceLaunch: function() {
+        this._info.launch();
+    },

This should be this.launch() because this._info is not defined.

---------------------------------------------------------------------------
Patch 2

All the comments you added to _getPreferredWidth(), _getPreferredHeight(), and _allocate() functions are really helpful!

-function DocManager(size) {
- this._init(size);
+function DocManager() {
+ this._init();
}

DocManager.prototype = {
_init: function(iconSize) {
- this._iconSize = iconSize;

Remove iconSize from _init: function() too.

+const DocInfo = imports.misc.docInfo;
Not used.

+const DEFAULT_SPACING = 4;
Not used.

+function DashDocDisplay(docInfo) {
Should be named DashDocDisplayItem for consistency with other individual items.

+function DashDocsArea() {
Should be named DashDocDisplay.

+ // We use two columns maximum. Just take the min and natural size of the
+ // first two items, even though strictly speaking it's not correct.
+ // In practice the dash gets a fixed width anyways.

Is it not correct because these particular items will be placed one under the other?
Explanation of why it's not correct in the comment would help.

+ // Loop over the children, going vertically down first. When we run
+ // out of vertical space (our y variable is bigger than box.y2, switch
+ // to the second column.

Need a closing parenthesis after box.y2

+ * Removes all references added by shell_texture_cache_load_thumbnail() function
+ * created for the given URI.

Maybe it should be this instead:
* Removes all references created by shell_texture_cache_load_thumbnail() function
* for the given URI.

+ * Removes all references added by shell_texture_cache_load_recent_thumbnail() function
+ * created for the URI associated with the given @info.

Maybe it should be this instead:
+ * Removes all references created by shell_texture_cache_load_recent_thumbnail() function
+ * for the URI associated with the given @info.

Perhaps you should ask Owen to take a quick look at the two C functions.

---------------------------------------------------------------------------
Patch 3

The commit message is misleading. You are not actually changing functionality with this patch, but rather just getting rid of the 'activated' signal. You can also call Main.overview.hide() in _handleActivated() because it already has other functionality not associated with dnd, such as switching to an existing window. However, it looks like this will actually make it less consistent with what's done in places.js, so for greater consistency you can rename _handleActivated() to onActivate() and not move Main.overview.hide() back there.

The commit message can be:
------------------------------------------------
Don't use 'activated' signal in WellDisplayItem

We can call Main.overview.hide() right away when we handle an application being activated by clicking on it in the applications well. We don't need to use 'activated' signal.
-----------------------------------------------

You should also remove
Signals.addSignalMethods(WellDisplayItem.prototype);

-----------------------------------------------------------------------------
Patch 4

Looks and works well.

+function PlaceDisplay(name, iconFactory, onActivate) {

This is not part of this patch, but the name PlaceDisplay is not consistent with the naming in other places. I have been using *DisplayItem to call a single item and *Display to call the area that displays such items.

(In reply to comment #11)

> ---------------------------------------------------------------------------
> Patch 3 [details]
> 
> The commit message is misleading. You are not actually changing functionality
> with this patch, but rather just getting rid of the 'activated' signal. 

It is changing functionality, drag and drop (what was called .launch() before) no longer switches out of the overview.

> You can
> also call Main.overview.hide() in _handleActivated() because it already has
> other functionality not associated with dnd, such as switching to an existing
> window. 

I cleaned this up a bit and fixed another bug.

Still to do on this bug:

* mouseover previews

Further bugs will be open for later designs.