Merge lp:~azzar1/unity/fix-850984 into lp:unity

[Andrea Azzarone]

The problem
===
Dash - Missing category separator line in dash

The fix
===

17 - bool found_one = false;
18 -
19 - for (rit = children.rbegin(); rit != children.rend(); ++rit)
20 - {
21 - PlacesGroup* group = static_cast<PlacesGroup*>(*rit);
22 -
23 - if (group->IsVisible())
24 - group->SetDrawSeparator(found_one);
25 -
26 - found_one = group->IsVisible();
27 - }

If the next group is not visibile the separator is not shown in the prev group.
This branch fixes it moving the logic side in another (testable) function. I'd love to
remove the static_cast too but I can't find an easy way right now.

The test
===
Adds a unit test for the logic side.

[Marco Trevisan (Treviño) (3v1n0)]

Overall looks very good to me.

133 + std::list<AbstractPlacesGroup*> groups;
134 +
135 + std::transform(children.begin(), children.end(), std::back_inserter(groups),
136 + [](Area* obj) -> AbstractPlacesGroup*
137 + {
138 + return static_cast<AbstractPlacesGroup*>(obj);
139 + });

I this really much more optimized, than using a normal loop? I mean, it's a nice C++ code, but it could look more cryptic than it really is :)

review: Approve

[Andrea Azzarone (azzar1)]

> Overall looks very good to me.
>
> 133 + std::list<AbstractPlacesGroup*> groups;
> 134 +
> 135 + std::transform(children.begin(), children.end(),
> std::back_inserter(groups),
> 136 + [](Area* obj) -> AbstractPlacesGroup*
> 137 + {
> 138 + return static_cast<AbstractPlacesGroup*>(obj);
> 139 + });
>
> I this really much more optimized, than using a normal loop? I mean, it's a
> nice C++ code, but it could look more cryptic than it really is :)

Not sure, how do you prefer?

=== added file 'plugins/unityshell/src/AbstractPlacesGroup.cpp'
--- plugins/unityshell/src/AbstractPlacesGroup.cpp	1970-01-01 00:00:00 +0000
+++ plugins/unityshell/src/AbstractPlacesGroup.cpp	2012-03-09 22:07:20 +0000
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2012 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Andrea Azzarone <[e-mail]>
+ */
+
+
+#include "AbstractPlacesGroup.h"
+
+namespace unity
+{
+namespace dash
+{
+
+NUX_IMPLEMENT_OBJECT_TYPE(AbstractPlacesGroup);
+
+AbstractPlacesGroup::AbstractPlacesGroup()
+  : nux::View(NUX_TRACKER_LOCATION)
+  , draw_separator(true)
+{
+}
+
+void AbstractPlacesGroup::Draw(nux::GraphicsEngine&, bool)
+{
+}
+
+void AbstractPlacesGroup::DrawContent(nux::GraphicsEngine&, bool)
+{
+}
+
+}
+}

=== added file 'plugins/unityshell/src/AbstractPlacesGroup.h'
--- plugins/unityshell/src/AbstractPlacesGroup.h	1970-01-01 00:00:00 +0000
+++ plugins/unityshell/src/AbstractPlacesGroup.h	2012-03-09 22:07:20 +0000
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2012 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Andrea Azzarone <[e-mail]>
+ */
+
+
+#ifndef UNITYSHELL_ABSTRACTPLACESGROUP_H
+#define UNITYSHELL_ABSTRACTPLACESGROUP_H
+
+#include <Nux/Nux.h>
+#include <Nux/View.h>
+#include <NuxCore/Property.h>
+
+namespace unity
+{
+namespace dash
+{
+
+class AbstractPlacesGroup : public nux::View
+{
+  NUX_DECLARE_OBJECT_TYPE(AbstractPlacesGroup, nux::View);
+public:
+  AbstractPlacesGroup();
+
+  // Properties
+  nux::Property<bool> draw_separator;
+
+protected:
+  void Draw(nux::GraphicsEngine&, bool);
+  void DrawContent(nux::GraphicsEngine&, bool);
+
+};
+
+}
+}
+
+#endif
+

=== modified file 'plugins/unityshell/src/LensView.cpp'
--- plugins/unityshell/src/LensView.cpp	2012-02-21 20:10:05 +0000
+++ plugins/unityshell/src/LensView.cpp	2012-03-09 22:07:20 +0000
@@ -18,6 +18,7 @@
  */
 
 #include "LensView.h"
+#include "LensViewPrivate.h"
 
 #include <boost/lexical_cast.hpp>
 
@@ -345,18 +346,15 @@
 gboolean LensView::FixRenderering(LensView* self)
 {
   std::list<Area*> children = self->scroll_layout_->GetChildren();
-  std::list<Area*>::reverse_iterator rit;
-  bool found_one = false;
-
-  for (rit = children.rbegin(); rit != children.rend(); ++rit)
-  {
-    PlacesGroup* group = static_cast<PlacesGroup*>(*rit);
-
-    if (group->IsVisible())
-      group->SetDrawSeparator(found_one);
-
-    found_one = group->IsVisible();
-  }
+  std::list<AbstractPlacesGroup*>  groups;
+
+  std::transform(children.begin(), children.end(), std::back_inserter(groups),
+    [](Area* obj) -> AbstractPlacesGroup*
+    {
+      return static_cast<AbstractPlacesGroup*>(obj);
+    });
+
+  dash::impl::UpdateDrawSeparators(groups);
 
   self->fix_renderering_id_ = 0;
   return FALSE;

=== added file 'plugins/unityshell/src/LensViewPrivate.cpp'
--- plugins/unityshell/src/LensViewPrivate.cpp	1970-01-01 00:00:00 +0000
+++ plugins/unityshell/src/LensViewPrivate.cpp	2012-03-09 22:07:20 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2012 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Andrea Azzarone <[e-mail]>
+ */
+
+#include "LensViewPrivate.h"
+#include "AbstractPlacesGroup.h"
+
+namespace unity
+{
+namespace dash
+{
+namespace impl
+{
+
+void UpdateDrawSeparators(std::list<AbstractPlacesGroup*> groups)
+{
+  std::list<AbstractPlacesGroup*>::reverse_iterator rit;
+  bool found_one = false;
+
+  for (rit = groups.rbegin(); rit != groups.rend(); ++rit)
+  {
+    if ((*rit)->IsVisible())
+    {
+      (*rit)->draw_separator = found_one;
+      found_one = true;
+    }
+  }
+}
+  
+}
+}
+}
+
+

=== added file 'plugins/unityshell/src/LensViewPrivate.h'
--- plugins/unityshell/src/LensViewPrivate.h	1970-01-01 00:00:00 +0000
+++ plugins/unityshell/src/LensViewPrivate.h	2012-03-09 22:07:20 +0000
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2012 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Andrea Azzarone <[e-mail]>
+ */
+
+#ifndef UNITYSHELL_LENS_VIEW_PRIVATE_H
+#define UNITYSHELL_LENS_VIEW_PRIVATE_H
+
+#include <list>
+
+namespace unity
+{
+namespace dash
+{
+
+class AbstractPlacesGroup;
+
+namespace impl
+{
+
+void UpdateDrawSeparators(std::list<AbstractPlacesGroup*> groups);
+
+} // namespace impl
+} // namespace dash
+} // namespace unity
+
+#endif

=== modified file 'plugins/unityshell/src/PlacesGroup.cpp'
--- plugins/unityshell/src/PlacesGroup.cpp	2012-02-23 05:16:34 +0000
+++ plugins/unityshell/src/PlacesGroup.cpp	2012-03-09 22:07:20 +0000
@@ -106,14 +106,13 @@
 NUX_IMPLEMENT_OBJECT_TYPE(PlacesGroup);
 
 PlacesGroup::PlacesGroup()
-  : View(NUX_TRACKER_LOCATION),
+  : AbstractPlacesGroup(),
     _child_view(nullptr),
     _focus_layer(nullptr),
     _idle_id(0),
     _is_expanded(true),
     _n_visible_items_in_unexpand_mode(0),
-    _n_total_items(0),
-    _draw_sep(true)
+    _n_total_items(0)
 {
   SetAcceptKeyNavFocusOnMouseDown(false);
   SetAcceptKeyNavFocusOnMouseEnter(false);
@@ -380,7 +379,7 @@
   graphics_engine.GetRenderStates().SetBlend(true);
   graphics_engine.GetRenderStates().SetPremultipliedBlend(nux::SRC_OVER);
 
-  if (_draw_sep)
+  if (draw_separator)
   {
     nux::Color col(0.15f, 0.15f, 0.15f, 0.15f);
 
@@ -488,14 +487,6 @@
   return _header_layout->GetGeometry().height;
 }
 
-void
-PlacesGroup::SetDrawSeparator(bool draw_it)
-{
-  _draw_sep = draw_it;
-
-  QueueDraw();
-}
-
 bool PlacesGroup::HeaderHasKeyFocus() const
 {
   return (_header_view && _header_view->HasKeyFocus());

=== modified file 'plugins/unityshell/src/PlacesGroup.h'
--- plugins/unityshell/src/PlacesGroup.h	2012-02-13 17:09:05 +0000
+++ plugins/unityshell/src/PlacesGroup.h	2012-03-09 22:07:20 +0000
@@ -27,6 +27,7 @@
 
 #include <sigc++/sigc++.h>
 
+#include "AbstractPlacesGroup.h"
 #include "IconTexture.h"
 #include "Introspectable.h"
 #include "StaticCairoText.h"
@@ -40,9 +41,9 @@
 namespace unity
 {
 
-class PlacesGroup : public nux::View , public debug::Introspectable
+class PlacesGroup : public dash::AbstractPlacesGroup, public debug::Introspectable
 {
-  NUX_DECLARE_OBJECT_TYPE(PlacesGroup, nux::View);
+  NUX_DECLARE_OBJECT_TYPE(PlacesGroup, dash::AbstractPlacesGroup);
 public:
 
   PlacesGroup();
@@ -70,8 +71,6 @@
   bool HeaderIsFocusable() const;
   nux::View* GetHeaderFocusableView() const;
 
-  void SetDrawSeparator(bool draw_it);
-
   sigc::signal<void, PlacesGroup*> expanded;
 
 protected:

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2012-03-08 13:25:07 +0000
+++ tests/CMakeLists.txt	2012-03-09 22:07:20 +0000
@@ -123,8 +123,11 @@
                  test_main_xless.cpp
                  test_grabhandle.cpp
                  test_unityshell_private.cpp
+                 test_lensview_impl.cpp
                  ${UNITY_SRC}/AbstractLauncherIcon.h
                  ${UNITY_SRC}/AbstractLauncherIcon.cpp
+                 ${UNITY_SRC}/AbstractPlacesGroup.cpp
+                 ${UNITY_SRC}/AbstractPlacesGroup.h
                  ${UNITY_SRC}/AbstractShortcutHint.h
                  ${UNITY_SRC}/Animator.cpp
                  ${UNITY_SRC}/Animator.h
@@ -140,6 +143,8 @@
                  ${UNITY_SRC}/IconTextureSource.cpp
                  ${UNITY_SRC}/LauncherModel.cpp
                  ${UNITY_SRC}/LauncherModel.h
+                 ${UNITY_SRC}/LensViewPrivate.cpp
+                 ${UNITY_SRC}/LensViewPrivate.h
                  ${UNITY_SRC}/FavoriteStorePrivate.cpp
                  ${UNITY_SRC}/FavoriteStorePrivate.h
                  ${UNITY_SRC}/MockLauncherIcon.h

=== added file 'tests/test_lensview_impl.cpp'
--- tests/test_lensview_impl.cpp	1970-01-01 00:00:00 +0000
+++ tests/test_lensview_impl.cpp	2012-03-09 22:07:20 +0000
@@ -0,0 +1,125 @@
+/*
+ * Copyright (C) 2012 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Andrea Azzarone <[e-mail]>
+ */
+
+#include <list>
+#include <vector>
+
+#include <gmock/gmock.h>
+
+#include "LensViewPrivate.h"
+#include "AbstractPlacesGroup.h"
+
+using namespace unity;
+using namespace testing;
+
+namespace
+{
+
+const int NUM_GROUPS = 4;
+
+class TestPlacesGroupImpl : public Test
+{
+public:
+  TestPlacesGroupImpl()
+  {
+    for (int i=0; i<NUM_GROUPS; ++i)
+      groups_vector.push_back(new dash::AbstractPlacesGroup);
+  }
+
+  ~TestPlacesGroupImpl()
+  {
+    for (int i=0; i<NUM_GROUPS; ++i)
+      delete groups_vector[i];
+  }
+
+  std::list<dash::AbstractPlacesGroup*> GetList()
+  {
+    std::list<dash::AbstractPlacesGroup*> ret;
+    std::copy(groups_vector.begin(), groups_vector.end(), std::back_inserter(ret));
+    return ret;
+  }
+
+  std::vector<dash::AbstractPlacesGroup*> groups_vector;
+};
+
+
+TEST_F(TestPlacesGroupImpl, TestUpdateDrawSeparatorsEmpty)
+{
+  std::list<dash::AbstractPlacesGroup*> groups;
+
+  // Just to make sure it doesn't crash.
+  dash::impl::UpdateDrawSeparators(groups);
+}
+
+TEST_F(TestPlacesGroupImpl, TestUpdateDrawSeparatorsAllVisible)
+{
+  groups_vector[0]->SetVisible(true);
+  groups_vector[1]->SetVisible(true);
+  groups_vector[2]->SetVisible(true);
+  groups_vector[3]->SetVisible(true);
+
+  dash::impl::UpdateDrawSeparators(GetList());
+
+  EXPECT_TRUE(groups_vector[0]->draw_separator);
+  EXPECT_TRUE(groups_vector[1]->draw_separator);
+  EXPECT_TRUE(groups_vector[2]->draw_separator);
+  EXPECT_FALSE(groups_vector[3]->draw_separator);
+}
+
+TEST_F(TestPlacesGroupImpl, TestUpdateDrawSeparatorsLastInvisible)
+{
+  groups_vector[0]->SetVisible(true);
+  groups_vector[1]->SetVisible(true);
+  groups_vector[2]->SetVisible(true);
+  groups_vector[3]->SetVisible(false);
+
+  dash::impl::UpdateDrawSeparators(GetList());
+
+  EXPECT_TRUE(groups_vector[0]->draw_separator);
+  EXPECT_TRUE(groups_vector[1]->draw_separator);
+  EXPECT_FALSE(groups_vector[2]->draw_separator);
+}
+
+TEST_F(TestPlacesGroupImpl, TestUpdateDrawSeparatorsLastTwoInvisible)
+{
+  groups_vector[0]->SetVisible(true);
+  groups_vector[1]->SetVisible(true);
+  groups_vector[2]->SetVisible(false);
+  groups_vector[3]->SetVisible(false);
+
+  dash::impl::UpdateDrawSeparators(GetList());
+
+  EXPECT_TRUE(groups_vector[0]->draw_separator);
+  EXPECT_FALSE(groups_vector[1]->draw_separator);
+}
+
+TEST_F(TestPlacesGroupImpl, TestUpdateDrawSeparators)
+{
+  groups_vector[0]->SetVisible(true);
+  groups_vector[1]->SetVisible(false);
+  groups_vector[2]->SetVisible(false);
+  groups_vector[3]->SetVisible(true);
+
+  dash::impl::UpdateDrawSeparators(GetList());
+
+  EXPECT_TRUE(groups_vector[0]->draw_separator);
+  EXPECT_FALSE(groups_vector[3]->draw_separator);
+}
+
+}
+