Merge lp:~mandel/unity/fix-static-cairo-text into lp:unity

[Manuel de la Peña]

Fixes #1071327 by ensuring that the PreLayoutManagement method does not call SetBaseSize unless is really needed else (if it is called) the method will be called again making the nux layout get into an infinite loop.

Tests can be ran doing ./tests/test-gtest --gtest_filter=*TestStaticCairoText* from the build dir. In order to run the launcher tooltip tests please do ./tests/test-gtest --gtest_filter=*Tooltip* and run the launcher to assert that tooltips work as expected.

[Nick Dedekind (nick-dedekind)]

LGTM.
Tests provided succeed.

review: Approve

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

Waiting for a proper fix in nux, this is fine.

review: Approve

[Brandon Schaefer (brandontschaefer)]

This causes a regression in the tooltips...

http://i.imgur.com/cVZUb.jpg

review: Needs Fixing

[Tim Penhey (thumper)]

https://code.launchpad.net/~thumper/unity/revert-r2960/+merge/138896 reverting this change until tooltips fixed.

[Tim Penhey (thumper)]

A test should be added to work out why tooltip rendering broke, and make the tests pass.

review: Needs Fixing

[Manuel de la Peña (mandel)]

Sorry, I did not check that the launcher would work correctly and I just focused on the Dash. Is a very simple fix will push asap (The min height of the label has to be the one returned by extend).

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

Fix works ok and test passes, but since you're there... Could you please get rid of the _labelText variable (you can just use the StaticCairoText::GetText where needed)? It looks like a duplication to me.

150 + Settings* settings;

No need a pointer here... Just add Settings settings;

[Brandon Schaefer (brandontschaefer)]

Looks good to me now. Tooltip renders correctly.

review: Approve

[Nick Dedekind (nick-dedekind)]

57 + text.SetSetterFunction([this](std::string newText)
58 + {
59 + this->_tooltip_text->SetText(newText);
60 + return true;
61 + }
62 + );c

You can't just return true; otherwise the property changed signal gets emitted every time you call set. It should only return true if the value of the text has changed, otherwise false.

48 + _tooltip_text = new nux::StaticCairoText(TEXT("Unity"), NUX_TRACKER_LOCATION);
271 + EXPECT_NE(new_tip, tooltip->_tooltip_text->GetText());

Don't know why tool-tips are being initialised to TEXT("Unity"). They should be blank by default.
And then you can change the test to:
271 + EXPECT_EQ("", tooltip->_tooltip_text->GetText());

Otherwise, LGTM.
Builds OK for me, runs and passes test.

review: Approve

[Nick Dedekind (nick-dedekind)]

review: Needs Fixing

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

Thanks for the property fixes, it looks ok now.

review: Approve

=== modified file 'launcher/LauncherIcon.cpp'
--- launcher/LauncherIcon.cpp	2012-12-03 15:34:23 +0000
+++ launcher/LauncherIcon.cpp	2012-12-11 15:48:21 +0000
@@ -147,7 +147,7 @@
   _tooltip = new Tooltip();
   AddChild(_tooltip.GetPointer());
 
-  _tooltip->SetText(tooltip_text());
+  _tooltip->text = tooltip_text();
 }
 
 void LauncherIcon::LoadQuicklist()
@@ -475,7 +475,7 @@
   {
     target = escaped;
     if (_tooltip)
-      _tooltip->SetText(target);
+      _tooltip->text = target;
     result = true;
   }
 
@@ -520,7 +520,7 @@
 
   if (!_tooltip)
     LoadTooltip();
-  _tooltip->SetText(tooltip_text());
+  _tooltip->text = tooltip_text();
   _tooltip->ShowTooltipWithTipAt(tip_x, tip_y);
   _tooltip->ShowWindow(!tooltip_text().empty());
   tooltip_visible.emit(_tooltip);

=== modified file 'launcher/Tooltip.cpp'
--- launcher/Tooltip.cpp	2012-11-06 18:19:09 +0000
+++ launcher/Tooltip.cpp	2012-12-11 15:48:21 +0000
@@ -47,7 +47,6 @@
 Tooltip::Tooltip() :
   _anchorX(0),
   _anchorY(0),
-  _labelText(TEXT("Unity")),
   _cairo_text_has_changed(true)
 {
   _hlayout = new nux::HLayout(TEXT(""), NUX_TRACKER_LOCATION);
@@ -61,7 +60,7 @@
 
   _vlayout->AddLayout(_top_space, 0);
 
-  _tooltip_text = new nux::StaticCairoText(_labelText, NUX_TRACKER_LOCATION);
+  _tooltip_text = new nux::StaticCairoText(TEXT(""), NUX_TRACKER_LOCATION);
   _tooltip_text->SetTextAlignment(nux::StaticCairoText::AlignState::NUX_ALIGN_CENTRE);
   _tooltip_text->SetTextVerticalAlignment(nux::StaticCairoText::AlignState::NUX_ALIGN_CENTRE);
   _tooltip_text->SetMinimumWidth(MINIMUM_TEXT_WIDTH);
@@ -69,6 +68,22 @@
   _tooltip_text->sigTextChanged.connect(sigc::mem_fun(this, &Tooltip::RecvCairoTextChanged));
   _tooltip_text->sigFontChanged.connect(sigc::mem_fun(this, &Tooltip::RecvCairoTextChanged));
 
+  // getter and setter for the property
+  text.SetSetterFunction([this](std::string newText)
+    {
+      if(_tooltip_text->GetText() == newText)
+        return false;
+
+      _tooltip_text->SetText(newText);
+      return true;
+    }
+  );
+  text.SetGetterFunction([this]()
+    {
+      return _tooltip_text->GetText();
+    }
+  );
+
   _vlayout->AddView(_tooltip_text.GetPointer(), 1, nux::MINOR_POSITION_CENTER, nux::MINOR_SIZE_FULL);
 
   _vlayout->AddLayout(_bottom_space, 0);
@@ -129,6 +144,7 @@
   }
 
   _tooltip_text->SetMinimumWidth(text_min_width);
+  _tooltip_text->SetMinimumHeight(text_height);
 
   if (text_height < ANCHOR_HEIGHT)
   {
@@ -539,17 +555,6 @@
 {
 }
 
-void Tooltip::SetText(std::string const& text)
-{
-  if (_labelText == text)
-    return;
-
-  _labelText = text;
-  _tooltip_text->SetText(_labelText);
-
-  QueueRelayout();
-}
-
 // Introspection
 
 std::string Tooltip::GetName() const
@@ -560,7 +565,7 @@
 void Tooltip::AddProperties(GVariantBuilder* builder)
 {
   variant::BuilderWrapper(builder)
-    .add("text", _labelText)
+    .add("text", text)
     .add("x", GetBaseX())
     .add("y", GetBaseY())
     .add("width", GetBaseWidth())

=== modified file 'launcher/Tooltip.h'
--- launcher/Tooltip.h	2012-07-09 13:34:41 +0000
+++ launcher/Tooltip.h	2012-12-11 15:48:21 +0000
@@ -39,11 +39,11 @@
 public:
   Tooltip();
 
+  nux::RWProperty<std::string> text;
+
   void Draw(nux::GraphicsEngine& gfxContext, bool forceDraw);
   void DrawContent(nux::GraphicsEngine& gfxContext, bool forceDraw);
 
-  void SetText(std::string const& text);
-
   void ShowTooltipWithTipAt(int anchor_tip_x, int anchor_tip_y);
 
   // Introspection
@@ -52,11 +52,15 @@
 
   virtual nux::Area* FindAreaUnderMouse(const nux::Point& mouse_position, nux::NuxEventType event_type);
 
-private:
+protected:
+  // protected to simplify testing
+  nux::ObjectPtr<nux::StaticCairoText> _tooltip_text;
+
   void RecvCairoTextChanged(nux::StaticCairoText* cairo_text);
 
   void PreLayoutManagement();
 
+private:
   long PostLayoutManagement(long layoutResult);
 
   void PositionChildLayout(float offsetX,
@@ -69,9 +73,6 @@
 
   int                   _anchorX;
   int                   _anchorY;
-  std::string           _labelText;
-
-  nux::ObjectPtr<nux::StaticCairoText> _tooltip_text;
 
   nux::HLayout* _hlayout;
   nux::VLayout* _vlayout;

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2012-12-11 02:33:06 +0000
+++ tests/CMakeLists.txt	2012-12-11 15:48:21 +0000
@@ -219,6 +219,7 @@
                  test_launcher_hide_machine.cpp
                  test_launcher_hover_machine.cpp
                  test_launcher_icon.cpp
+                 test_launcher_tooltip.cpp
                  test_keyboard_util.cpp
                  test_panel_style.cpp
                  test_panel_menu_view.cpp

=== added file 'tests/test_launcher_tooltip.cpp'
--- tests/test_launcher_tooltip.cpp	1970-01-01 00:00:00 +0000
+++ tests/test_launcher_tooltip.cpp	2012-12-11 15:48:21 +0000
@@ -0,0 +1,109 @@
+/*
+ * Copyright 2012 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser 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 warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the applicable version of the GNU Lesser General Public
+ * License for more details.
+ *
+ * You should have received a copy of both the GNU Lesser General Public
+ * License version 3 along with this program.  If not, see
+ * <http://www.gnu.org/licenses/>
+ *
+ * Authored by: Manuel de la Pena <[e-mail]>
+ *
+ */
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "unity-shared/StaticCairoText.h"
+#include "launcher/Tooltip.h"
+
+#include <unity-shared/UnitySettings.h>
+#include "test_utils.h"
+
+namespace unity
+{
+
+class MockStaticCairoText : public nux::StaticCairoText
+{
+public:
+  MOCK_METHOD1(SetMinimumWidth, void(int));
+  MOCK_METHOD1(SetMinimumHeight, void(int));
+
+  MockStaticCairoText(std::string const& text):StaticCairoText(text) {}
+
+}; // StaticCairoText
+
+class TooltipMock : public Tooltip
+{
+public:
+
+  TooltipMock() : Tooltip()
+  {
+    // change the text and reconnect it as it should
+    std::string old_text = _tooltip_text->GetText();
+    _tooltip_text = new MockStaticCairoText(old_text);
+    _tooltip_text->SetTextAlignment(
+      nux::StaticCairoText::AlignState::NUX_ALIGN_CENTRE);
+    _tooltip_text->SetTextVerticalAlignment(
+      nux::StaticCairoText::AlignState::NUX_ALIGN_CENTRE);
+
+    _tooltip_text->sigTextChanged.connect(
+      sigc::mem_fun(this, &TooltipMock::RecvCairoTextChanged));
+    _tooltip_text->sigFontChanged.connect(
+      sigc::mem_fun(this, &TooltipMock::RecvCairoTextChanged));
+  }
+
+  using Tooltip::_tooltip_text;
+  using Tooltip::RecvCairoTextChanged;
+  using Tooltip::PreLayoutManagement;
+
+}; // TooltipMock
+
+
+class TestTooltip : public ::testing::Test
+{
+protected:
+  TestTooltip() : Test()
+  {
+    Settings settings;
+    tooltip = new TooltipMock();
+  }
+
+  nux::ObjectPtr<TooltipMock> tooltip;
+}; // TestTooltip
+
+TEST_F(TestTooltip, StaticCairoTextCorrectSize)
+{
+  int text_width;
+  int text_height;
+  tooltip->_tooltip_text->GetTextExtents(text_width, text_height);
+
+  // do assert that the methods are called with at least the min size provided
+  // by the GetTextExtents method
+  MockStaticCairoText* text = dynamic_cast<MockStaticCairoText*>(
+    tooltip->_tooltip_text.GetPointer());
+
+  EXPECT_CALL(*text, SetMinimumWidth(testing::Ge(text_width)));
+  EXPECT_CALL(*text, SetMinimumHeight(testing::Ge(text_height)));
+
+  tooltip->PreLayoutManagement();
+}
+
+TEST_F(TestTooltip, TestSetTooltipText)
+{
+  std::string new_tip = "My tooltip";
+  EXPECT_EQ("", tooltip->_tooltip_text->GetText());
+
+  tooltip->text.Set(new_tip);
+  EXPECT_EQ(new_tip, tooltip->_tooltip_text->GetText());
+}
+
+} // unity

=== modified file 'tests/test_static_cairo_text.cpp'
--- tests/test_static_cairo_text.cpp	2012-12-10 00:32:30 +0000
+++ tests/test_static_cairo_text.cpp	2012-12-11 15:48:21 +0000
@@ -33,17 +33,33 @@
 class MockStaticCairoText : public nux::StaticCairoText
 {
 public:
+  MOCK_METHOD2(SetBaseSize, void(int, int));
+
   MockStaticCairoText():StaticCairoText("") {}
 
   using StaticCairoText::GetTextureStartIndices;
   using StaticCairoText::GetTextureEndIndices;
-};
-
-TEST(TestStaticCairoText, TextTextureSize)
-{
+  using StaticCairoText::PreLayoutManagement;
+};
+
+class TestStaticCairoText : public ::testing::Test
+{
+
+protected:
+  TestStaticCairoText() : Test()
+  {
+    text = new MockStaticCairoText();
+  }
+
+  nux::ObjectPtr<MockStaticCairoText> text;
+
+};
+
+TEST_F(TestStaticCairoText, TextTextureSize)
+{
+  EXPECT_CALL(*text.GetPointer(), SetBaseSize(_, _)).Times(AnyNumber());
   // Test multi-texture stitching support.
 
-  nux::ObjectPtr<MockStaticCairoText> text(new MockStaticCairoText());
   text->SetLines(-2000);
   text->SetMaximumWidth(100);
 
@@ -70,4 +86,16 @@
   }
 }
 
+TEST_F(TestStaticCairoText, TextPreLayoutManagementMultipleCalls)
+{
+  EXPECT_CALL(*text.GetPointer(), SetBaseSize(_, _)).Times(2);
+  text->PreLayoutManagement();
+  
+  // the first prelayout methods should have called set base size and therefore
+  // we should not call it again
+
+  EXPECT_CALL(*text.GetPointer(), SetBaseSize(_, _)).Times(0);
+   text->PreLayoutManagement();
+}
+
 }

=== modified file 'unity-shared/StaticCairoText.cpp'
--- unity-shared/StaticCairoText.cpp	2012-12-10 00:32:30 +0000
+++ unity-shared/StaticCairoText.cpp	2012-12-11 15:48:21 +0000
@@ -229,12 +229,16 @@
 void StaticCairoText::PreLayoutManagement()
 {
   Geometry geo = GetGeometry();
-  pimpl->pre_layout_size_.width = geo.width;
-  pimpl->pre_layout_size_.height = geo.height;
-
-  SetBaseSize(pimpl->cached_extent_.width,
-              pimpl->cached_extent_.height);
-
+
+  if(pimpl->pre_layout_size_.width != geo.width
+    || pimpl->pre_layout_size_.height != geo.height)
+  {
+    pimpl->pre_layout_size_.width = geo.width;
+    pimpl->pre_layout_size_.height = geo.height;
+
+    SetBaseSize(pimpl->cached_extent_.width,
+                pimpl->cached_extent_.height);
+  }
   if (pimpl->textures2D_.empty())
   {
     pimpl->UpdateTexture();