Merge lp://qastaging/~azzar1/unity/filters-fixes into lp://qastaging/unity

Did you make sure this doesn't change the way the dbus signals are sent to lenses? (from looking at the changes it shouldn't though)

> Did you make sure this doesn't change the way the dbus signals are sent to
> lenses? (from looking at the changes it shouldn't though)

There's a TestRatingsFilter (or something like that) and it's ok after the changes.

The changes too UnityCore/RatingsFilter.cpp makes sure that after calling ::Clear the ratings is really setted to 0 (how happens with the other filters). The canges make sure that setting the ratings too 0 (or calling ::Clear) the filtering property is "setted" (quotation marks because it'a read only property) to false.

*The changes to

Too many mistakes :/

> Did you make sure this doesn't change the way the dbus signals are sent to
> lenses? (from looking at the changes it shouldn't though)

There's a TestRatingsFilter (or something like that) and it's ok after the changes.

The changes to UnityCore/RatingsFilter.cpp make sure that after calling ::Clear the ratings property is really setted to 0 (how happens with the other filters). And the canges make sure that setting the ratings property to 0 (or calling ::Clear) the filtering property is "setted" (quotation marks because it'a read only property) to false.

> - There is already a merge proposal [2] but it has too many conflicts so I've created my own patch with
> a quite different solution.

How is your solution different? Is there a good reason not to start with the existing proposal that gord had reviewed?

> > - There is already a merge proposal [2] but it has too many conflicts so
> I've created my own patch with
> > a quite different solution.
>
> How is your solution different? Is there a good reason not to start with the
> existing proposal that gord had reviewed?

It's quite similar. The existing proposal uses a base class with an all button to remove duplicate in all the *widget.cpp files. My branch uses an all button class that is added using nux layout to the widget.

P.S. There was too many conflicts. I asked him to resolve them but I didn't get an answer.

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

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

The only change is to make the FilterWidget inherit from nux::View, then the FilterExpanderLabel inherits from FilterWidget. This will remove the multiple inheritance, and the need for the dynamic_cast in the factory method.

> The only change is to make the FilterWidget inherit from nux::View, then the
> FilterExpanderLabel inherits from FilterWidget. This will remove the multiple
> inheritance, and the need for the dynamic_cast in the factory method.

Done.

Works well with latest libunity.