Move the version number in App Info Dialog to be next to app's name

Move the version number in App Info Dialog to be next to app's name.

In the case where there is no 'view in webstore' link, place the version number on the following line. In the case where there is no version _or_ 'view in webstore' link, allow the title to take up multiple lines.

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

[sashab, 2014-05-13 23:57:18]

I have also opened crbug.com/373053 which happens as a result of this change. I
will fix it in a separate review.

[sashab, 2014-05-15 03:32:47]

I have fixed crbug.com/373053 and this review now contains that fix.

[calamity, 2014-05-15 06:31:08]

I'm concerned that relying so much on the subtle interactions between all these
GridView flex options is going to be confusing in future.

As discussed, it should be possible to make everything a BoxLayout except for
the 'App Name + Version' view which requires the app name to flex.

The plan to simplify the hierarchy is to make the top level a vertical box
layout. The AppIconAndInfo is a horizontal box layout.

For the app info do:
if (bookmark_app) {
  app_info_view = Label();
  app_info_view->SetMultiline(true);
} else {
  app_info_view = MulticomponentAppInfoView(
    name_label, version_label, webstore_link);
}

MulticomponentAppInfoView consists of a grid layout which contains rows with:
-A resizable column and a fixed column
-A single filling column

[sashab, 2014-05-18 23:59:26]

Switched to using BoxLayout with cross-axis alignment from
https://codereview.chromium.org/284753002/ as discussed. Please re-review.

[calamity, 2014-05-19 01:48:58]

lgtm w/ nits.

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:47:
namespace {
nit: Blank line after namespace.

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:59:
AddChildView(foo);
s/foo/container_view/

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:105: };
It feels like these two classes should be rolled into one. At the very least, it
seems strange to have a VerticallyCenteredStack class when it could be
implemented as a local variable. I suppose the other option is to push the
vertical stack logic into the callsite.

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:315:
app_info_panel->AddChildView(app_name_label);
Following on from the suggestion above:

if (!app_version_label && ! view_in_store_link_) {
  app_name_label->SetMultiLine(true);
  app_info_panel->AddChildView(app_name_label);
} else {
  views::View* vertical_container = new views::View();
  views::BoxLayout* layout =
      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0);
  layout->set_main_axis_alignment(views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER);
  vertical_container->SetLayoutManager(layout);

  /** All those other cases, adding views directly to vertical_container **/
}

[sashab, 2014-05-19 03:16:01]

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:47:
namespace {
On 2014/05/19 01:48:58, calamity wrote:
> nit: Blank line after namespace.

Done.

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:59:
AddChildView(foo);
On 2014/05/19 01:48:58, calamity wrote:
> s/foo/container_view/

Done.

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:105: };
On 2014/05/19 01:48:58, calamity wrote:
> It feels like these two classes should be rolled into one. At the very least,
it
> seems strange to have a VerticallyCenteredStack class when it could be
> implemented as a local variable. I suppose the other option is to push the
> vertical stack logic into the callsite.

Put the logic in the callsite, as further suggested below.

https://codereview.chromium.org/274443005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:315:
app_info_panel->AddChildView(app_name_label);
On 2014/05/19 01:48:58, calamity wrote:
> Following on from the suggestion above:
> 
> if (!app_version_label && ! view_in_store_link_) {
>   app_name_label->SetMultiLine(true);
>   app_info_panel->AddChildView(app_name_label);
> } else {
>   views::View* vertical_container = new views::View();
>   views::BoxLayout* layout =
>       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0);
>  
layout->set_main_axis_alignment(views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER);
>   vertical_container->SetLayoutManager(layout);
> 
>   /** All those other cases, adding views directly to vertical_container **/
> }

Nice idea!! This makes it really neat actually. Done.

[benwells, 2014-05-19 04:30:16]

https://codereview.chromium.org/274443005/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/274443005/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:185:
AppInfoSummaryTab::AppInfoSummaryTab(gfx::NativeWindow parent_window,
This constructor is getting a bit out of control, I think it is > 150 lines.
It's also strange how some of it is actually in AppNameAndVersion's constructor.

So, some comments:
1. We should split it up into subfunctions which are each understandable and
doing some clear part of the layout.

If it was me I'd start with something like:
CreateSummaryControls
CreateLaunchOptionControl
CreateButtons
LayoutSummary
LayoutDetails

This would require a few more instance variables.

2. The class AppNameAndVersion feels like it should be a function, not a class.
Is there any reason why it is a class?

[sashab, 2014-05-19 22:02:17]

If you don't agree with the latest patchset, I'm happy to roll back to when the
AppInfoSummaryPanel was a part of the tab view.

https://codereview.chromium.org/274443005/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/274443005/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:185:
AppInfoSummaryTab::AppInfoSummaryTab(gfx::NativeWindow parent_window,
On 2014/05/19 04:30:17, benwells wrote:
> This constructor is getting a bit out of control, I think it is > 150 lines.
> It's also strange how some of it is actually in AppNameAndVersion's
constructor.
> 
> So, some comments:
> 1. We should split it up into subfunctions which are each understandable and
> doing some clear part of the layout.
> 
> If it was me I'd start with something like:
> CreateSummaryControls
> CreateLaunchOptionControl
> CreateButtons
> LayoutSummary
> LayoutDetails
> 
> This would require a few more instance variables.
> 
> 2. The class AppNameAndVersion feels like it should be a function, not a
class.
> Is there any reason why it is a class?

1. Sounds good to me, thanks for this feedback

2. Views made sense to me as subclasses because they are like modular, movable
components of the UI. If you look at
chrome/browser/ui/views/create_application_shortcut_view.cc, they have a custom
'AppInfoView' for the panel at the top of the dialog with the app's info (in
fact, this might make sense in our case for the 'summary' are at the top of the
dialog). If they are each in a separate class they can be easily moved
around/reused in other places and work as nice components of the 'views'
hierarchy.
I think this argument is a bit weaker if there are no custom methods or
listeners on the class, so in this case I'm happy either way.
I've changed the AppNameAndVersion view to a function.

https://codereview.chromium.org/274443005/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:185:
AppInfoSummaryTab::AppInfoSummaryTab(gfx::NativeWindow parent_window,
On second thoughts, with this now in mind, I've moved all the common logic for
the top area into its own view. I think this design is more consistent with
create_application_shortcut_view, for example, and it means the new summary view
is more modular (clear dependencies & can move or be re-used elsewhere).

[benwells, 2014-05-19 22:21:37]

lgtm++

This is so looking great, the code is much easier to follow and review!

I think the encapsulation works like this because you're encapsulating a piece
of the dialog, whereas before you were just encapsulating a relatively simple
layout algorithm that was only used sometimes in a piece of the dialog. i.e.

https://codereview.chromium.org/274443005/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/274443005/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:82: const
extensions::Extension* app_;
Hmmm .... can you file a bug on yourself to make your dialog resilient to
extensions being uninstalled (e.g. via sync) while your dialog is open?

[sashab, 2014-05-19 22:54:30]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-05-19 22:55:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/274443005/180001

[sashab, 2014-05-19 23:37:57]

Thanks :) Bug is at crbug.com/374999.


On Tue, May 20, 2014 at 8:21 AM, <benwells@chromium.org> wrote:

> lgtm++
>
> This is so looking great, the code is much easier to follow and review!
>
> I think the encapsulation works like this because you're encapsulating a
> piece
> of the dialog, whereas before you were just encapsulating a relatively
> simple
> layout algorithm that was only used sometimes in a piece of the dialog.
> i.e.
>
>
> https://codereview.chromium.org/274443005/diff/180001/
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
> File
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
> (right):
>
> https://codereview.chromium.org/274443005/diff/180001/
> chrome/browser/ui/views/apps/app_info_dialog/app_info_
> summary_tab.cc#newcode82
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:82:
> const extensions::Extension* app_;
> Hmmm .... can you file a bug on yourself to make your dialog resilient
> to extensions being uninstalled (e.g. via sync) while your dialog is
> open?
>
> https://codereview.chromium.org/274443005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-05-20 02:22:08]

Change committed as 271537