media galleries config dialog on views

chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/extensions/media_galleries_dialog_views.h"
+
+#include "chrome/browser/ui/views/constrained_window_views.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/views/controls/button/checkbox.h"
+#include "ui/views/controls/button/text_button.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/layout_constants.h"
+#include "ui/views/view.h"
+
+namespace chrome {
+
+typedef MediaGalleriesDialogController::KnownGalleryPermissions
+    GalleryPermissions;
+
+namespace {
+
+// Heading font size correction.

[Peter Kasting, 2012/08/11 23:09:14]

Nit: Can you say more here?  I have no idea what this is about.

[Evan Stade, 2012/08/13 23:10:30]

There are a couple other places which use this pattern and I just copied it. The
documentation for the define states:

// Single define to trigger whether CrOS fonts have BCI on.
// In that case font sizes/deltas should be adjusted.

that's all I know about it.

[Peter Kasting, 2012/08/14 18:44:01]

I'm following up on this separately with the CrOS guys to see what this is
about.

+#if defined(CROS_FONTS_USING_BCI)
+const int kHeadingFontSizeDelta = 0;
+#else
+const int kHeadingFontSizeDelta = 1;
+#endif
+
+const int kContentWidth = 450;
+
+}  // namespace
+
+using views::BoxLayout;
+using views::GridLayout;

[Peter Kasting, 2012/08/11 23:09:14]

Nit: We normally avoid using statements unless they significantly shorten code.

[Evan Stade, 2012/08/13 23:10:30]

Done.

+
+MediaGalleriesDialogViews::MediaGalleriesDialogViews(
+    MediaGalleriesDialogController* controller)
+    : controller_(controller),
+      window_(NULL),
+      contents_(new views::View()),

[Peter Kasting, 2012/08/11 23:09:14]

Who owns this?

[Evan Stade, 2012/08/13 23:10:30]

its parent View (a RootView)

+      confirm_available_(false),
+      accepted_(false) {
+  InitChildViews();
+
+  window_ = new ConstrainedWindowViews(controller->tab_contents(), this);

[Peter Kasting, 2012/08/11 23:09:14]

Who owns this?

[Evan Stade, 2012/08/13 23:10:30]

the TabContents (first arg)

+}
+
+MediaGalleriesDialogViews::~MediaGalleriesDialogViews() {}
+
+void MediaGalleriesDialogViews::InitChildViews() {
+  // Layout.
+  GridLayout* layout = new GridLayout(contents_);
+  layout->SetInsets(views::kPanelVertMargin, views::kPanelHorizMargin,
+                    0, views::kPanelHorizMargin);
+  int column_set_id = 0;
+  views::ColumnSet* columns = layout->AddColumnSet(column_set_id);
+  columns->AddColumn(GridLayout::LEADING,
+                     GridLayout::LEADING,
+                     1,
+                     GridLayout::FIXED,
+                     kContentWidth,
+                     0);
+  contents_->SetLayoutManager(layout);
+
+  // Header text.
+  views::Label* header = new views::Label(controller_->GetHeader());
+  header->SetFont(header->font().DeriveFont(kHeadingFontSizeDelta,
+                                            gfx::Font::BOLD));
+  header->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  layout->StartRow(0, column_set_id);
+  layout->AddView(header);
+
+  // Message text.
+  views::Label* subtext = new views::Label(controller_->GetSubtext());
+  subtext->SetMultiLine(true);
+  subtext->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  layout->StartRowWithPadding(0, column_set_id,
+                              0, views::kRelatedControlVerticalSpacing);
+  layout->AddView(subtext);
+
+  // Checkboxes.
+  checkbox_container_ = new views::View();
+  checkbox_container_->SetLayoutManager(
+      new BoxLayout(BoxLayout::kVertical,
+                    views::kPanelHorizIndentation,
+                    0,
+                    views::kRelatedControlSmallVerticalSpacing));
+  layout->StartRowWithPadding(0, column_set_id,
+                              0, views::kRelatedControlVerticalSpacing);
+  layout->AddView(checkbox_container_);
+
+  const GalleryPermissions& permissions = controller_->permissions();
+  for (GalleryPermissions::const_iterator iter = permissions.begin();
+       iter != permissions.end(); iter++) {

[Peter Kasting, 2012/08/11 23:09:14]

Nit: Preincrement

[Evan Stade, 2012/08/13 23:10:30]

Done.

+    AddOrUpdateGallery(&iter->second.pref_info, iter->second.allowed);
+    if (iter->second.allowed)
+      confirm_available_ = true;
+  }
+
+  // Add Gallery button.
+  add_gallery_ = new views::NativeTextButton(
+      this, l10n_util::GetStringUTF16(IDS_MEDIA_GALLERIES_DIALOG_ADD_GALLERY));
+  layout->StartRowWithPadding(0, column_set_id,
+                              0, views::kRelatedControlVerticalSpacing);
+  layout->AddView(add_gallery_);
+}
+
+void MediaGalleriesDialogViews::UpdateGallery(
+    const MediaGalleryPrefInfo* gallery, bool permitted) {

[Peter Kasting, 2012/08/11 23:09:14]

Nit: One arg per line (2 places)

[Evan Stade, 2012/08/13 23:10:30]

Done.

+  // After adding a new checkbox, we have to update the size of the dialog.
+  if (AddOrUpdateGallery(gallery, permitted))
+    GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());
+}
+
+bool MediaGalleriesDialogViews::AddOrUpdateGallery(
+    const MediaGalleryPrefInfo* gallery, bool permitted) {
+  CheckboxMap::iterator iter = checkbox_map_.find(gallery);
+
+  bool new_checkbox = false;

[Peter Kasting, 2012/08/11 23:09:14]

Nit: How about:

  CheckboxMap::iterator iter = checkbox_map_.find(gallery);
  if (iter != checkbox_map_.end()) {
    iter->second->SetChecked(permitted);
    return false;
  }

  views::Checkbox* checkbox = new views::Checkbox(gallery->display_name);
  ...
  return true;

This reads a little more clearly and avoids temps when possible.

[Evan Stade, 2012/08/13 23:10:30]

Done.

+  views::Checkbox* checkbox = NULL;
+  if (iter != checkbox_map_.end()) {
+    checkbox = iter->second;
+  } else {
+    new_checkbox = true;
+    checkbox = new views::Checkbox(gallery->display_name);
+    checkbox->set_listener(this);
+    checkbox->SetTooltipText(gallery->path.LossyDisplayName());

[Evan Stade, 2012/08/10 21:58:13]

Sailesh: note this (I haven't yet done this for Linux but we decided it was a
good idea)

[sail, 2012/08/10 22:09:04]

Sounds good, will add to Cocoa.

+    checkbox_container_->AddChildView(checkbox);
+    checkbox_map_[gallery] = checkbox;
+  }
+
+  checkbox->SetChecked(permitted);
+  return new_checkbox;
+}
+
+string16 MediaGalleriesDialogViews::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  if (button == ui::DIALOG_BUTTON_OK)

[Peter Kasting, 2012/08/11 23:09:14]

Nit: Most places we just do:

  return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK) ?
      IDS_MEDIA_GALLERIES_DIALOG_CONFIRM : IDS_MEDIA_GALLERIES_DIALOG_CANCEL;

...and don't worry about the hypothetical "other button" case.

[Evan Stade, 2012/08/13 23:10:30]

Done.

+    return l10n_util::GetStringUTF16(IDS_MEDIA_GALLERIES_DIALOG_CONFIRM);
+
+  if (button == ui::DIALOG_BUTTON_CANCEL)
+    return l10n_util::GetStringUTF16(IDS_MEDIA_GALLERIES_DIALOG_CANCEL);
+
+  return string16();
+}
+
+bool MediaGalleriesDialogViews::IsDialogButtonEnabled(
+    ui::DialogButton button) const {
+  return !(button == ui::DIALOG_BUTTON_OK && !confirm_available_);

[Peter Kasting, 2012/08/11 23:09:14]

Nit: Please factor ! through, as nested !s make my brain hurt

[Evan Stade, 2012/08/13 23:10:30]

Done.

+}
+
+bool MediaGalleriesDialogViews::Cancel() {
+  return true;
+}
+
+bool MediaGalleriesDialogViews::Accept() {
+  accepted_ = true;
+  return true;
+}
+
+views::View* MediaGalleriesDialogViews::GetContentsView() {
+  return contents_;
+}
+
+views::Widget* MediaGalleriesDialogViews::GetWidget() {
+  return contents_->GetWidget();
+}
+
+const views::Widget* MediaGalleriesDialogViews::GetWidget() const {
+  return contents_->GetWidget();
+}
+
+void MediaGalleriesDialogViews::DeleteDelegate() {
+  controller_->DialogFinished(accepted_);
+}
+
+void MediaGalleriesDialogViews::ButtonPressed(views::Button* sender,
+                                              const ui::Event& event) {
+  EnableConfirm();
+
+  if (sender == add_gallery_) {
+    controller_->OnAddFolderClicked();
+    return;
+  }
+
+  for (CheckboxMap::iterator iter = checkbox_map_.begin();
+       iter != checkbox_map_.end(); ++iter) {
+    if (iter->second == sender) {

[Peter Kasting, 2012/08/11 23:09:14]

Tiny nit: For consistency with conditional above, (sender == iter->second)

[Evan Stade, 2012/08/13 23:10:30]

Done.

+      controller_->GalleryToggled(
+          iter->first, static_cast<views::Checkbox*>(sender)->checked());
+      return;
+    }
+  }
+
+  NOTREACHED();
+  return;

[Peter Kasting, 2012/08/11 23:09:14]

Nit: No need for return here

[Evan Stade, 2012/08/13 23:10:30]

Done.

+}
+
+void MediaGalleriesDialogViews::EnableConfirm() {

[Peter Kasting, 2012/08/11 23:09:14]

Nit: Any particular reason to factor this out into its own function?  It's only
called once and is incredibly simple.

[Evan Stade, 2012/08/13 23:10:30]

Done. (I originally had it called twice.)

+  confirm_available_ = true;
+  GetWidget()->client_view()->AsDialogClientView()->UpdateDialogButtons();
+}
+
+// MediaGalleriesDialogViewsController -----------------------------------------
+
+// static
+MediaGalleriesDialog* MediaGalleriesDialog::Create(
+    MediaGalleriesDialogController* controller) {
+  return new MediaGalleriesDialogViews(controller);
+}
+
+}  // namespace chrome