Align panel icons on the right / end.

Align panel icons on the right / end.

BUG=160605
TEST=LauncherModel.AddIndices, LauncherViewTest.AddPanelHidesTabbedBrowser, LauncherViewTest.PanelsHideLast


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170406

[flackr, 2012-11-22 16:13:46]

Hi Steven,

Can you take a look at this CL? This right aligns panel icons and overflows
icons on the left if there is contention for space.

Currently panel icons will never be overflowed so if you have more than your
launcher's space full of panels you'll get weird problems. I'll work on moving
them into the overflow panel too and upload another patch but I suspect it won't
change this patch too much.

Please take a look, thanks!

[stevenjb, 2012-11-26 18:19:20]

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_model.h
File ash/launcher/launcher_model.h (right):

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_model.h...
ash/launcher/launcher_model.h:51: int EndAlignedIndex() const;
"EndAligned" confused me. Maybe "FirstRightAligned"?

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:669: typeb == TYPE_PLATFORM_APP);
nit: one line?

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view_un...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:403: last_visible = added;
We should add something like:
++items_added;
ASSERT_LT(10000, items_added);

To avoid relying on a timeout should the test fail.

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:427: last_panel = AddPanel();
Same here.

[flackr, 2012-11-27 18:40:21]

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_model.h
File ash/launcher/launcher_model.h (right):

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_model.h...
ash/launcher/launcher_model.h:51: int EndAlignedIndex() const;
On 2012/11/26 18:19:21, stevenjb (chromium) wrote:
> "EndAligned" confused me. Maybe "FirstRightAligned"?

This can be bottom aligned if the panel position is set to be on the left or
right. Call it right aligned anyways?

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view.cc...
ash/launcher/launcher_view.cc:669: typeb == TYPE_PLATFORM_APP);
On 2012/11/26 18:19:21, stevenjb (chromium) wrote:
> nit: one line?

Done.

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view_un...
File ash/launcher/launcher_view_unittest.cc (right):

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:403: last_visible = added;
On 2012/11/26 18:19:21, stevenjb (chromium) wrote:
> We should add something like:
> ++items_added;
> ASSERT_LT(10000, items_added);
> 
> To avoid relying on a timeout should the test fail.

Done. And the other instances of this in the file.

https://codereview.chromium.org/11348201/diff/1/ash/launcher/launcher_view_un...
ash/launcher/launcher_view_unittest.cc:427: last_panel = AddPanel();
On 2012/11/26 18:19:21, stevenjb (chromium) wrote:
> Same here.

Done.

[stevenjb, 2012-11-28 00:12:16]

lgtm

[flackr, 2012-11-28 01:31:48]

+sky PTAL, thanks!

[sky, 2012-11-28 05:13:25]

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_model.h
File ash/launcher/launcher_model.h (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_mode...
ash/launcher/launcher_model.h:50: // Returns the index of the first right
aligned item.
The description and name are confusing since first and end are opposites.
Shouldn't the name and description line up? I suggest naming this
FirstPanelIndex() with a description that indicates this returns the index of
the first panel or the index of the item where the first panel would go if there
are no panels.

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:318: AddChildViewAt(overflow_button_,
end_align_index_);
Does the index really matter here?

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:425: // Right aligned icons
How do you know all the panels icons fit?
I don't think this is the right thing to do if is_overflow_mode() is set either.

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view.h
File ash/launcher/launcher_view.h (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.h:248: int end_align_index_;
Why do we need to have this as a member?

[flackr, 2012-11-28 18:07:51]

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_model.h
File ash/launcher/launcher_model.h (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_mode...
ash/launcher/launcher_model.h:50: // Returns the index of the first right
aligned item.
On 2012/11/28 05:13:25, sky wrote:
> The description and name are confusing since first and end are opposites.
> Shouldn't the name and description line up? I suggest naming this
> FirstPanelIndex() with a description that indicates this returns the index of
> the first panel or the index of the item where the first panel would go if
there
> are no panels.

Done.

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:318: AddChildViewAt(overflow_button_,
end_align_index_);
On 2012/11/28 05:13:25, sky wrote:
> Does the index really matter here?

I guess it doesn't. Reverted.

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:425: // Right aligned icons
On 2012/11/28 05:13:25, sky wrote:
> How do you know all the panels icons fit?
> I don't think this is the right thing to do if is_overflow_mode() is set
either.

They may not all fit. There was an open question of how to handle overflow of
panels in crbug.com/162558 but we've decided to do whatever is simplest to
start. I can update this CL to do a simple overflow into the bubble using some
proportion of panels / apps.

Also you're correct, I've moved this after the overflow mode calculation.

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view.h
File ash/launcher/launcher_view.h (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.h:248: int end_align_index_;
On 2012/11/28 05:13:25, sky wrote:
> Why do we need to have this as a member?

I was doing this to get the index corresponding to when the view was updated
from the model, however I see the model should always correspond to the view so
I'll just call to the model.

[sky, 2012-11-28 21:59:39]

LGTM

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:425: // Right aligned icons
On 2012/11/28 18:07:52, flackr wrote:
> On 2012/11/28 05:13:25, sky wrote:
> > How do you know all the panels icons fit?
> > I don't think this is the right thing to do if is_overflow_mode() is set
> either.
> 
> They may not all fit. There was an open question of how to handle overflow of
> panels in crbug.com/162558 but we've decided to do whatever is simplest to
> start. I can update this CL to do a simple overflow into the bubble using some
> proportion of panels / apps.

I'm fine with punting, just add a TODO.

> Also you're correct, I've moved this after the overflow mode calculation.

[flackr, 2012-11-28 23:51:21]

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/11348201/diff/3002/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:425: // Right aligned icons
On 2012/11/28 21:59:39, sky wrote:
> On 2012/11/28 18:07:52, flackr wrote:
> > On 2012/11/28 05:13:25, sky wrote:
> > > How do you know all the panels icons fit?
> > > I don't think this is the right thing to do if is_overflow_mode() is set
> > either.
> > 
> > They may not all fit. There was an open question of how to handle overflow
of
> > panels in crbug.com/162558 but we've decided to do whatever is simplest to
> > start. I can update this CL to do a simple overflow into the bubble using
some
> > proportion of panels / apps.
> 
> I'm fine with punting, just add a TODO.
> 
> > Also you're correct, I've moved this after the overflow mode calculation.
> 

Done.

[commit-bot: I haz the power, 2012-11-28 23:57:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/11348201/18

[commit-bot: I haz the power, 2012-11-30 02:06:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/11348201/18

[commit-bot: I haz the power, 2012-11-30 05:31:01]

Change committed as 170406