Code review comment for lp://qastaging/~azzar1/unity/filters-fixes

Another branch that really shouldn't have mixed style changes with real changes.

Use const& params when passing objects around, even smart pointers.

  void FilterAllButton::SetFilter(Filter::Ptr const& filter)

When would this method be called?

I know that you just moved the code, but FilterBasicButton.cpp,
FilterBasicButton::ComputeContentSize, should just set the cached_geometry_
inside the if statement, and reuse "geo".

Is there a reason we aren't using the cached_geometry_ in the Draw method?
Seems weird to cache it and not use it. Gah... and GetGeometry() is called
multiple times in Draw.

+1 on adding the missing & to const std::string. Extra brownie points for
making them "std::string const&" :-)

Inside FilterExpanderLabel::SetContents, you change the behaviour slightly.
Is this ever called more than once for any given object?

By calling SinkReference you are changing it from being unowned, to just
having a reference count of 1. When it is added to the layout, it gets a
reference count of 2. Normally when the layout is destroyed, it would
dereference. This is how the old code path handled the cleanup. The
SetContents method adds the unref, but doesn't remove the old contents from
the layout. However all of this is moot if this method should only ever be
called once.

Why add a SpaceLayout that isn't added to anything?

Also, we really shouldn't be adding Reference/UnReference/SinkReference calls
anywhere. Ideally you'd use a nux::ObjectPtr<xxx> to handle this.

Hmm... things becoming clearer.

You have a bug in your removal of the space layout.
  if (space_ and space_->IsChildOf(space_))
should be
  if (space_ and space_->IsChildOf(layout_))

> layout_->AddView(space_, 1);
I hate magic numbers in code, what does the "1" do?

If you changed the space_ and contents_ members to be nux::ObjectPtr wrappers,
you wouldn't need the the UnReference/SinkReference calls.

The SetContent method could then use the Adopt method to take ownership.

/me sighs

> nux::View* FilterFactory::WidgetForFilter(Filter::Ptr filter)

Please pass the filter by const&

This code will blow up if the filter type is not understood by trying to call
a method on a dynamic_cast of a nullptr.

Firstly, don't use nux::View* for the temporary. Use a FilterWidget* and call
it, oh maybe, widget. You also don't need to static_cast up an inheritance
hierarchy, you can just assign. Also, by doing this, you don't need the
dynamic_cast either.

  FilterWidget* widget = nullptr;
  if (filter_type == renderer_type_check_options)
  {
    widget = new FilterGenre(NUX_TRACKER_LOCATION);
  }
  else if (filter_type == renderer_type_ratings)
  {
    widget = new FilterRatingsWidget(NUX_TRACKER_LOCATION);
  }
  else if (filter_type == renderer_type_multirange)
  {
    widget = new FilterMultiRange(NUX_TRACKER_LOCATION);
  }
  else if (filter_type == renderer_type_radio_options)
  {
    widget = new FilterGenre(NUX_TRACKER_LOCATION);
  }
  else
  {
    LOG_WARNING(logger) << "Do not understand filter of type \""
                        << filter_type
                        << "\"";
  }

  if (widget)
    widget->SetFilter(filter);

  return widget;

> FilterFactory.h

A virtual destructor shows intent of the object being inherited from. The
filter factory will never be inherited from, so it doesn't need a virtual
destructor. In fact I don't think there is any need for a destructor at all.

The header doesn't work stand-alone as it references Filter::Ptr, which isn't
included in the header. The dash:: prefix for the Filter::Ptr can be removed
since you added the namespace for it.

> FilterGenreButton.cpp

Why have overrides for the Draw, DrawContent, and PostDraw methods that just
call up? Since the methods are virtual, you can delete them. They all have
implementations and are not pure virtual.

> FilterMultiRangeButton.cpp

I'm pretty sure I changed the GetGeometry() method to return a const&, so this
can be stored in a temporary and used for repeated GetGeometry() calls.

How about we store active_ and normal_ in a unique_ptr?

  std::string name = "10";
should be
  std::string name("10");

The first calls the std::string(const char*) constructor to convert "10" into
a std::string, and then uses the copy constructor. The second just calls the
right constuctor.

> FilterMultiRangeButton.h

Can you change the enum types please?

typedef enum {
  MULTI_RANGE_SIDE_LEFT,
  MULTI_RANGE_SIDE_RIGHT,
  MULTI_RANGE_CENTER
} MultiRangeSide;

becomes:

enum class MultiRangeSide
{
  LEFT,
  RIGHT,
  CENTER
};

Then the values must be accessed using the scope operator,
ie MultiRangeSide::LEFT

const& params as a general comment.
virtual methods that just call parent implementations shold be deleted
I'm fairly sure that std::unique_ptr is in <memory>

typedef std::unique_ptr<nux::CairoWrapper> NuxCairoPtr;
NuxCairoPtr active_;
NuxCairoPtr normal_;

> FilterMultiRangeWidget.cpp

  for (auto it : filter_->options())
    OnOptionAdded(it);

This makes me queezy. filter_->options() returns a copy of the content.
This is creating a temporary vectory of filter options. The for loop then
iterates over that. Now the lifetime of the temporary object is until the
next sequence point. What I'm not entirely clear on is when that sequence
point is. After a bit of research, I found this:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2243.html

6.5.4 The range-based for statement

Add the following new section:

1 The range-based for statement

for ( for-range-declaration : expression ) statement

is equivalent to

{
     auto && __range = ( expression );

     for ( auto __begin = std::Range<_RangeT>::begin(__range),
                  __end = std::Range<_RangeT>::end(__range);
          __begin != __end;
          ++__begin )
     {
         for-range-declaration = *__begin;
         statement
     }
}

This seems to indicate that the temporary returned by the expression,
in this case filter_->options(), is valid for the duration of the for loop.

for(auto it : buttons_) should have a space after "for" and before "("

  for (auto it : buttons_)
  {
    FilterMultiRangeButton* button = (it);

should just be

  for (auto button : buttons_)
  {

in all the places in the file.

> FilterRatingsButton.cpp and .h

all the CairoWrappers should be kept in smart pointers.

The rest of the changes appear to relate to one or other of the comments
above, so I won't go over them again.