> 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)

Done.

> 
> When would this method be called?

Every time the associated *Widget got another filter.

> 
> 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".

Done.

> 
> 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.

We're caching cached_geometry_ to not invalidate active_ and normal_ (?) every single time.
But I think that it's legal to use cached_geometry_ in the Draw method too... Maybe we can
ask Jay.

> 
> +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.

Because SetVisible doesn't work on a nux::layout ;)

> Is this ever called more than once for any given object?

It should be called just the first time.

> 
> 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?

It was necessary to add an extra padding during the collapse state. I've found a 
cleaner solution ;)

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

Done ;)

> 
> 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?
> 

No longer needed. I've just removed space_;

> 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.

Done.

> 
> /me sighs
> 
> > nux::View* FilterFactory::WidgetForFilter(Filter::Ptr filter)
> 
> Please pass the filter by const&

Done.

> 
> 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.

Done.

> 
> 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;

Done but i've added a dynamic_cast in the return statement. If you don't like it here, i've to move the dynamic_cast in FilterBar... 

> 
> > 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.

Done

> 
> > 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.

Done.

> 
> > 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.

Done.

> 
> How about we store active_ and normal_ in a unique_ptr?
> 

Done.

>   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.

Done.

> 
> > 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

Done.

> 
> 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_;

Done.

> 
> > 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.

So I've not changed it... is it ok?

> 
> 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.

Done.
> 
> > 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.

Done.