Code review comment for lp:~mandel/unity/generic-payment-preview

> You have some text conflicts in dash/DashView.cpp & UnityCore/Preview.cpp
>
> 27 + * Copyright (C) 2011-2012 Canonical Ltd
>
> 41 + * Authored by: Neil Jagdish Patel <email address hidden>
> 42 + * Michal Hruby <email address hidden>
>
> Please update all headers to include 2013 and your contact details
>
Sorry, I yy those lines, fixed.

> 277 {
> 278 +
> 279 if (!preview_displaying_)
>
> extra line.

No problem.

>
> 407 +const std::string MusicPaymentPreview::DATA_INFOHINT_ID =
> "album_purchase_preview";
> 408 +const std::string MusicPaymentPreview::DATA_PASSWORD_KEY =
> "password";
> 409 +const std::string MusicPaymentPreview::CHANGE_PAYMENT_ACTION =
> "change_payment_method";
> 410 +const std::string MusicPaymentPreview::FORGOT_PASSWORD_ACTION =
> "forgot_password";
> 411 +const std::string MusicPaymentPreview::CANCEL_PURCHASE_ACTION =
> "cancel_purchase";
> 412 +const std::string MusicPaymentPreview::PURCHASE_ALBUM_ACTION =
> "purchase_album";
>
> Is there a reason why these are scoped to MusicPaymentPreview class? Can you
> put them in global namespace?
>

Is there a reason for that,I mean, it can be done but it will be nice to be able to access them for the tests.

> 936 + std::map<std::string, nux::ObjectPtr<nux::AbstractButton>>
> sorted_buttons_;
>
> What is sorted about a map ? ;)
>
> 701 + actions_layout->AddView(
> 702 +
> sorted_buttons_[MusicPaymentPreview::CHANGE_PAYMENT_ACTION].GetPointer(),
> 703 + 1, nux::MINOR_POSITION_START, nux::MINOR_SIZE_FULL,
> 704 + 100.0f, nux::NUX_LAYOUT_END);
> 705 + actions_layout->AddView(
> 706 +
> sorted_buttons_[MusicPaymentPreview::FORGOT_PASSWORD_ACTION].GetPointer(),
> 707 + 1, nux::MINOR_POSITION_START, nux::MINOR_SIZE_FULL,
> 708 + 100.0f, nux::NUX_LAYOUT_END);
>
> 728 + buttons_data_layout->AddView(sorted_buttons_[MusicPaymentPreview::C
> ANCEL_PURCHASE_ACTION].GetPointer(),
> 729 + 1, nux::MINOR_POSITION_CENTER, nux::MINOR_SIZE_FULL,
> 100.0f,
> 730 + nux::NUX_LAYOUT_END);
> 731 + buttons_data_layout->AddView(sorted_buttons_[MusicPaymentPreview::P
> URCHASE_ALBUM_ACTION].GetPointer(),
> 732 + 1, nux::MINOR_POSITION_CENTER, nux::MINOR_SIZE_FULL,
> 100.0f,
> 733 + nux::NUX_LAYOUT_END);
>
> should check if sorted_buttons_ contains the actions before adding them.
>

No problem,

> 784 + dash::Preview::InfoHintPtrList hints =
> preview_model_->GetInfoHints();
> 785 + GVariant *preview_data = NULL;
> 786 + dash::Preview::InfoHintPtr data_info_hint_ = NULL;
> 787 + if (!hints.empty())
> 788 + {
> 789 + for (dash::Preview::InfoHintPtr info_hint : hints)
> 790 + {
> 791 + if (info_hint->id == MusicPaymentPreview::DATA_INFOHINT_ID)
> 792 + {
> 793 + preview_data = info_hint->value;
> 794 + }
> 795 + if (preview_data != NULL)
> 796 + {
> 797 + error_message_ = GetErrorMessage(preview_data);
> 798 + }
> 799 + }
> 800 + }
>
> 1. data_info_hint_ doesn't seem to be used.

Deleted

> 2. Don't need to check if the hints are empty if you're iterating over them.

True

> 3. dash::Preview::InfoHintPtr const& info_hint. even though it's a pointer,
> you're still constructing the wrapper.

Well since is no needed this problem does not longer exist.

> 4. Not sure what this is doing, but i might be missing something. looping over
> the info hints. if it is a speicifc id, then set the preview data and get an
> error message; otherwise continue looping, but dont update the data, even
> though you are continuing to do a GetErrorMessage for it?
>
>
> 765 + // set the tab ordering
> 766 + SetFirstInTabOrder(password_entry_->text_entry());
> 767 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::CANCEL_PURCH
> ASE_ACTION].GetPointer());
> 768 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::PURCHASE_ALB
> UM_ACTION].GetPointer());
> 769 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::CHANGE_PAYME
> NT_ACTION].GetPointer());
> 770 + SetLastInTabOrder(sorted_buttons_[MusicPaymentPreview::FORGOT_PASSW
> ORD_ACTION].GetPointer());
>
> Why are you setting the tab order in inside PreLayoutManagement? Shouldn't it
> be done only once when you create the views?
>
> 1431 + virtual void PreLayoutManagement() = 0;
>

We don't want any other developer to be able to create an instance of the PaymentPreview.

> This is a virtual method in nux::Area I think. Why are you making it pure
> virtual in PaymentPreview?
>
> 1735 +
> 1736 +
>
> extra lines
>
> 1778 +using namespace testing;
>
> can you move this to after the includes please.

Easy fixes.