Created attachment 178233 [details] [review]
app-display: Expose BaseIcon params in AppWellIcon

AppWellIcon is used both in the dash and view selector. As the dash
requires manual sizing, it is not possible to set the icon size used
in the view selector in the CSS, but icons will use the default size
(unless set manually as in the dash).

Expose the params parameter of BaseIcon and enable manual resizing
only for AppWellIcons in the dash.

Created attachment 178234 [details] [review]
app-view: Use larger icons

According to the mockups[0], launcher icons in the app view should
be larger. Leave icons in search results at their current size,
until the designers decide what's appropriate there.

[0] http://live.gnome.org/GnomeShell/Design/Whiteboards/Launchers

Before these patches get pushed, I'd like to say that the referenced mockups are not finished, as the same designers admit, and have a problem with independent software not providing x-large icons.
The proposed solutions are not viable, because they would remove the association between a program and its brand, the application icon. Also, they risk incurring into nasty trademark issues - think of a game called "World of Chaos"...
So I'd rather we stick with current 48x48 icons, that even if don't look very good when upscaled, are better than 256x256.
(This goes in pair with the opposition to "drop categories", of which this mockup is the result)

As I said in bug 636655 I tried something similar and giant icons as click targets (buttons) simply feel odd on non touch screens.

There ought to be a better solution than this ...

Whatever the outcome is, I'd still like to get the first patch in - it doesn't make sense to have manual icon sizing outside the dash.

Review of attachment 178233 [details] [review]:

Looks good.

The basic problem I see is that for years, the key icon size used for application authors in GNOME 2 has been 48x48; it's been fine to rely on downscaling for other sizes.

I think we need to give the authors time to transition before we make their app icon look like blurry crap, basically.

But this is something to punt back to design; tagging ui-review.

Comment on attachment 178233 [details] [review]
app-display: Expose BaseIcon params in AppWellIcon

Attachment 178233 [details] pushed as 38fb51a - app-display: Expose BaseIcon params in AppWellIcon

Created attachment 180522 [details] [review]
app-view: Use larger icons

Adjust running-indicator.svg to fit the new icon size.

Created attachment 180523 [details] [review]
app-icon: Use a maximum size for drag actor

The large launcher icons in the application view look rather odd
when dragged around, so make sure that drag actors for app icons
don't exceed a default icon size of 48.

Looks much better! No issues here.

Looks much better and matches the mockup well.  Let's do it and get started on filing bugs for better icons.  Thanks!

Created attachment 180603 [details] [review]
dash: Emit a signal on icon size changes

As elements in the dash are scaled to accommodate a growing number
of items, the icon size used may end up rather small. In that case,
dragging items to the dash which are significantly larger than items
in the dash is getting clumsy, so it makes sense for some components
to synchronize the size of drag actors with the currently used icon
size in the dash. To enable other components to do this, emit a signal
on icon size changes.

Created attachment 180604 [details] [review]
overview: Make the dash public

In order to enable components other than Overview to make use of
the newly added Dash::icon-size-changed signal, make the dash a
public property of the overview.

Created attachment 180605 [details] [review]
app-display: Use the dash's icon size for dragged icons

As the dash is one of the primary drop targets for dragging items
from the application view, it's reasonable in this case to synchronize
the size of drag actors with the icon size used by the dash, especially
with the large launcher size now used in the application view.

This behavior was suggested by Jon yesterday, and seems like a better approach than setting a maximum size of drag actors.

Review of attachment 180603 [details] [review]:

Looks good

Review of attachment 180604 [details] [review]:

Sure

Review of attachment 180605 [details] [review]:

::: js/ui/appDisplay.js
@@ +42,3 @@
+        // Main.overview not having initialized yet - in other places
+        // we use show()/hide() in that case, but having to chain up
+        // appDisplay.show()->viewByCategories.show()->alphabeticalView.show()

If we're inserting hacks all over the place, then we really should just have a public init function so that main.js does

 overview = new Overview.Overview();
 overview.init();

Of course, then we'd need to figure out what gets done in the constructor and what gets done in init() - and we might need to propagate init() functions down to other components, but we can't just do crude hacks ad infinitum for a core part of our structure - we need to know how we solve this cleanly.

@@ +57,3 @@
         this.actor.set_policy(Gtk.PolicyType.NEVER, Gtk.PolicyType.AUTOMATIC);
+        this.actor.connect('destroy', Lang.bind(this,
+            function() {

The inline function here rather than an onDestroy handler bothers me a bit, not sure why - probably partly consistency with all the other places, and partly it seems more about the entire object and less about the actor.

@@ +85,3 @@
+                    let item = this._grid.getItemAtIndex(i);
+                    if (item._delegate)
+                        item._delegate.setDragIconSize(this._dashIconSize);

So, why do we do this? (and I guess why do we need the signal in dash?, and why do we need the hack for setup?) - wouldn't it be far easier and more efficient to extract the size when we begin the drag? If you don't want appIcon to know about the dash, then you could do when creating an icon

 icon.connect('pre-drag', function() {
    icon.setDragIconSize(Main.overview.dash.iconSize)
 });

But since appIcon is right in this source code file anyways, it doesn't seem too evil to make it just access Main.overview.dash.iconSize itself.

@@ +563,3 @@
+        let iconSize = this._dragIconSize > 0 ? this._dragIconSize
+                                              : this.icon.iconSize;
+        return this.app.create_icon_texture(iconSize);

I'm not sure why, but what I see is that the snap-back animation scales the icon down when you snap back - it looks like if the app-display icon size is 96 and the dash is 24, then during the animation back the icon gets scaled down to 8 pixels high or something along those lines. I don't think the icon has to scale back up in the animation, it could stay 24 pixels high, but it shouldn't scale *down*.

Review of attachment 180522 [details] [review]:

Will be interesting to see if and how quickly people fix their icons...

(In reply to comment #19)
> Review of attachment 180522 [details] [review]:
> 
> Will be interesting to see if and how quickly people fix their icons...

Or whether that happens at all ...

Created attachment 180697 [details] [review]
dash: Make iconSize property public

As elements in the dash are scaled to accommodate a growing number
of items, the icon size used may end up rather small. In that case,
dragging items to the dash which are significantly larger than items
in the dash is getting clumsy, so it makes sense for some components
to synchronize the size of drag actors with the currently used icon
size in the dash. To enable other components to do this, make the icon
size a public property.

Following the suggestion in comment 18, the drag actor size will be set directly from the dash's icon size property instead of using the ::icon-size-changed signal.

(In reply to comment #17)

Commit message's body changed to:

In order to enable components other than Overview to read the
dash's iconSize, make the dash a public property of the overview.

Created attachment 180701 [details] [review]
app-icon: Use the dash's icon size for dragged icons

As the dash is one of the primary drop targets for dragging application
launchers, it's reasonable to use the dash icon size for app icons'
drag actors, especially with the larger size now used in the application
view.

(In reply to comment #18)
> @@ +42,3 @@
> +        // Main.overview not having initialized yet - in other places
> +        // we use show()/hide() in that case, but having to chain up
> +        // appDisplay.show()->viewByCategories.show()->alphabeticalView.show()
> 
> If we're inserting hacks all over the place, then we really should just have a
> public init function

The hack is no longer needed in this patch, so let's handle this in a different bug - it's a really nice cleanup (especially in the view selector), I'll file it tomorrow after writing proper commit messages.


> @@ +57,3 @@
> The inline function here rather than an onDestroy handler bothers me a bit, not
> sure why - probably partly consistency with all the other places, and partly it
> seems more about the entire object and less about the actor.

No longer in the patch, but I don't quite understand the second concern - doing the object's cleanup work when its actor is destroyed is a pattern which makes sense to me and which we use all over the place.



> @@ +85,3 @@
> +                    let item = this._grid.getItemAtIndex(i);
> +                    if (item._delegate)
> +                        item._delegate.setDragIconSize(this._dashIconSize);
> 
> So, why do we do this?

The idea was that AppWellIcon is used both in the app view and in the dash, and we only want to adjust the drag actor for the former. Of course, as the drag actors in the dash are using the dash's icon size pretty much by definition, this idea is stupid.

AppWellIcon is used for application search results as well, so with the attached patch the drag actors' icon size is adjusted there as well (can't really think of a good reason why we wouldn't want this ...)

> (and I guess why do we need the signal in dash?, and why
> do we need the hack for setup?)

Yeah ...


> But since appIcon is right in this source code file anyways, it doesn't seem
> too evil to make it just access Main.overview.dash.iconSize itself.

Doing this now.


> @@ +563,3 @@
> +        let iconSize = this._dragIconSize > 0 ? this._dragIconSize
> +                                              : this.icon.iconSize;
> +        return this.app.create_icon_texture(iconSize);
> 
> I'm not sure why, but what I see is that the snap-back animation scales the
> icon down when you snap back - it looks like if the app-display icon size is 96
> and the dash is 24, then during the animation back the icon gets scaled down to
> 8 pixels high or something along those lines. I don't think the icon has to
> scale back up in the animation, it could stay 24 pixels high, but it shouldn't
> scale *down*.

Still happening - I'm pretty sure it's caused by the snap back code in dnd.js assuming that the drag actor's size matches the size of the original source actor (first if block in _getRestoreLocation()). I'll take a closer look sometimes this weekend.

(In reply to comment #23)

> > @@ +57,3 @@
> > The inline function here rather than an onDestroy handler bothers me a bit, not
> > sure why - probably partly consistency with all the other places, and partly it
> > seems more about the entire object and less about the actor.
> 
> No longer in the patch, but I don't quite understand the second concern - doing
> the object's cleanup work when its actor is destroyed is a pattern which makes
> sense to me and which we use all over the place.

What I'm saying is that

 this.actor.connect('destroy', Lang.bind(this, this._onDestroy);

reads to me as "our standard pattern for cleanups on a delegate class". While 

 this.actor.connect('destroy', Lang.bind(this, function() {
    /* stuff */
 });

Reads to me as "do some random stuff when the actor is destroyed". Not a huge concern, just giving my impression.

> > @@ +563,3 @@
> > +        let iconSize = this._dragIconSize > 0 ? this._dragIconSize
> > +                                              : this.icon.iconSize;
> > +        return this.app.create_icon_texture(iconSize);
> > 
> > I'm not sure why, but what I see is that the snap-back animation scales the
> > icon down when you snap back - it looks like if the app-display icon size is 96
> > and the dash is 24, then during the animation back the icon gets scaled down to
> > 8 pixels high or something along those lines. I don't think the icon has to
> > scale back up in the animation, it could stay 24 pixels high, but it shouldn't
> > scale *down*.
> 
> Still happening - I'm pretty sure it's caused by the snap back code in dnd.js
> assuming that the drag actor's size matches the size of the original source
> actor (first if block in _getRestoreLocation()). I'll take a closer look
> sometimes this weekend.

Yeah,  it's almost certainly a dnd.js fix not something in this patch.

Review of attachment 180697 [details] [review]:

Looks good

Review of attachment 180701 [details] [review]:

A little simpler this way!

(If we ever need to have some case where we want to drag at the original size, we can just add a 'dragIconMatchesDash' property on appIcon)

Attachment 180522 [details] pushed as aad3560 - app-view: Use larger icons
Attachment 180604 [details] pushed as 2f3e47b - overview: Make the dash public
Attachment 180697 [details] pushed as 1d77914 - dash: Make iconSize property public
Attachment 180701 [details] pushed as b0efe68 - app-icon: Use the dash's icon size for dragged icons