Created attachment 167023 [details] [review]
[appDisplay] Factor out WellGrid/AppIcon

All mockups now use a representation for documents/places similar to
the one used for applications. Rename AppIcon to BaseIcon and move its
code together with WellGrid out of appDisplay to stress their general
usefulness.

Comment on attachment 167023 [details] [review]
[appDisplay] Factor out WellGrid/AppIcon

>Rename AppIcon to BaseIcon and move its
>code together with WellGrid out of appDisplay to stress their general
>usefulness.

Sigh. "All of this has happened before and it will happen again" :-}

>     getDragActor: function() {
>-        return this.app.create_icon_texture(APPICON_SIZE);
>+        return new Clutter.Clone({ source: this.getDragActorSource() });
>     },

for the message tray source/notification changes, I ended up moving ICON_SIZE into Source.prototype so I could just refer to "this.ICON_SIZE" in subclasses. That might be nicer than using Clutter.Clone here (which may run into problems if we ever have situations where, eg, you can drag out of the overview, causing the original icon to be destroyed).

OTOH...

>-        this._clipWidth = AppDisplay.APPICON_SIZE / 2;
>+        this._clipWidth = IconGrid.ICON_SIZE / 2;

the changes here (panel.js) make the code even more wrong than it was before... blah. And, oh, look a few lines down in the diff context:

>         this._spinner = new AnimatedIcon('process-working.png', 24);

here we hardcode a number that coincidentally happens to be the same as the divided-by-two value of the constant we're stealing from another module.

It would be nice to get rid of IconGrid.ICON_SIZE altogether and replace it with proper CSS instead.

Created attachment 168381 [details] [review]
[appDisplay] Factor out WellGrid/AppIcon

(In reply to comment #2)
> That might be nicer than using Clutter.Clone here (which may run into problems
> if we ever have situations where, eg, you can drag out of the overview, causing
> the original icon to be destroyed).

Right.


> OTOH...
> 
> >-        this._clipWidth = AppDisplay.APPICON_SIZE / 2;
> >+        this._clipWidth = IconGrid.ICON_SIZE / 2;
> 
> the changes here (panel.js) make the code even more wrong than it was before...
> blah. And, oh, look a few lines down in the diff context:
> 
> >         this._spinner = new AnimatedIcon('process-working.png', 24);
> 
> here we hardcode a number that coincidentally happens to be the same as the
> divided-by-two value of the constant we're stealing from another module.

Hmm - it also happens to be the same as the existing PANEL_ICON_SIZE constant. I updated the patch to use that constant instead - it doesn't really make sense to tie the size of panel icons to the size of icons in the overview.


> It would be nice to get rid of IconGrid.ICON_SIZE altogether and replace it
> with proper CSS instead.

Not sure about that - I kept ICON_SIZE for setting a default value. It may be overwritten from CSS, and it is no longer used outside of IconGrid.

Created attachment 169322 [details] [review]
[baseIcon] Allow setting the icon size from code

The current design features a sidebar where the icon size shrinks
automatically to fit more items. Add BaseIcon.setIconSize() to
support this.

I'd like to squash this patch with the previous one before committing.

Created attachment 169323 [details] [review]
[baseIcon] Tie the icon's width to its height

Both current design and new mockups use square icons. Currently this
is implemented by setting a fixed width and height in the CSS, which
does not work for the growing/shrinking items in the new sidebar.
By moving the assumption of square items into the code, the size will
adjust according to icon size, spacing, padding and font size if not
explicitly overwritten in the CSS.

Another small patch which could be squashed with the main patch.

Comment on attachment 169322 [details] [review]
[baseIcon] Allow setting the icon size from code

Hm... but if the code calls setIconSize() explicitly, you don't want a later style-changed emission to overwrite it, right? (In theory, if we ever support changing the theme at run time like gtk does, this would result in style-changed being emitted. In the sorter term, the way the code is written now, calling setIconSize and then adding it to a container will give you different results than adding it to the container and then calling setIconSize.)

Comment on attachment 169323 [details] [review]
[baseIcon] Tie the icon's width to its height

doesn't this cause the queue_relayout-during-allocation warning?

(In reply to comment #7)
> (From update of attachment 169323 [details] [review])
> doesn't this cause the queue_relayout-during-allocation warning?

It doesn't, but that might be a clutter bug.

(In reply to comment #6)
> (From update of attachment 169322 [details] [review])
> Hm... but if the code calls setIconSize() explicitly, you don't want a later
> style-changed emission to overwrite it, right?

Yes. The code assumes that the icon-size property is not set for icons which are scaled explicitly, but I'm unsure what would be a good way to enforce it. Maybe some _sizeExplicitlySet property which is set when setIconSize() is called and disables the style property?

Created attachment 170299 [details] [review]
[appDisplay] Factor out WellGrid/AppIcon

Rebased to master - code updates in IconGrid according to the review in bug 622446; I assume the patch won't need another review, feel free to disagree :)

Created attachment 170307 [details] [review]
[appDisplay] Factor out WellGrid/AppIcon

Sorry for the noise, the last rebase was with a previous patch version.

Created attachment 170827 [details] [review]
[baseIcon] Allow setting the icon size from code

Updated code to allow setting the size manually xor from CSS.

Created attachment 170828 [details] [review]
[baseIcon] Tie the icon's width to its height

Rebased on the previous patch.

Comment on attachment 170828 [details] [review]
[baseIcon] Tie the icon's width to its height

>+        this.actor.connect('queue-relayout',
>+                           Lang.bind(this, this._adjustWidth));

ok, I was confused before when I said this would cause the queue-relayout warning, since you're essentially calling queue-relayout from inside queue-relayout, not from inside the layouting itself.

But looking at clutter_actor_queue_relayout(), there's another problem here; it only emits the signal if a relayout isn't already queued. So if you do:

    this.actor.something_that_changes_its_height_request_to_10();
    this.actor.something_else_that_changes_its_height_request_to_15();

then your signal handler will be called after the first call, but not after the second, so you'll end up with this.actor.height == 15 and this.actor.width == 10.

I think the right way to do this is to use a ShellGenericContainer or the like, and enforce height==width at request time.

Created attachment 170851 [details] [review]
[baseIcon] Tie the icon's width to its height

(In reply to comment #14)
> I think the right way to do this is to use a ShellGenericContainer or the like,
> and enforce height==width at request time.

Makes sense.

Comment on attachment 170851 [details] [review]
[baseIcon] Tie the icon's width to its height

>+        let iconSize = availHeight - this._spacing - labelMinHeight;

You ought to use labelNatHeight here if availHeight >= preferredHeight. Failing that, if you're always going to use labelMinHeight here, you should use labelMinHeight to calculate the natural_size in getPreferredHeight too.

(feel free to commit with that change if it's simple.)

Attachment 170307 [details] pushed as 8c1bf34 - [appDisplay] Factor out WellGrid/AppIcon
Attachment 170827 [details] pushed as 01f4dc6 - [baseIcon] Allow setting the icon size from code
Attachment 170851 [details] pushed as 9708b68 - [baseIcon] Tie the icon's width to its height