Merge lp:~3v1n0/unity/hud-padding-fixes into lp:unity

[Marco Trevisan (Treviño)]

Hud: fixed the paddings and geometries, remove the icon glitches

I've fixed all the paddings and geometries to match design (and the dash, then), plus I've fixed a size mismatch that caused some drawing glitches of the icon.

HUD before the fix: http://i.imgur.com/b7BO0.png | http://i.imgur.com/9vpmv.png
HUD after the fix: http://i.imgur.com/wDxPO.png | http://i.imgur.com/kSLch.png

The HUD padding was not correct and this caused it not to match the dash geometries, especially when using a locked launcher.

I've fixed all the paddings and geometries to match design (and the dash, then), plus I've fixed a size mismatch that caused some drawing glitches of the icon.

HUD before the fix: http://i.imgur.com/b7BO0.png | http://i.imgur.com/9vpmv.png
HUD after the fix: http://i.imgur.com/wDxPO.png | http://i.imgur.com/kSLch.png

[Andrea Cimitan (cimi)]

review: Approve (design)

[Andrea Cimitan (cimi)]

review: Needs Resubmitting

[Andrea Cimitan (cimi)]

review: Abstain

[Andrea Cimitan (cimi)]

how can I cancel my review? :) The hud visuals seems fine, but you're affecting the launcher, so this seems leading to regressions. But I didn't test on my pc to prove them

[Andrea Cimitan (cimi)]

Looking closely it seems you're right, there's 1px gap between the hud and the launcher. I should not do reviews on sunday morning - I will compile and test tomorrow

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

> how can I cancel my review? :) The hud visuals seems fine, but you're
> affecting the launcher, so this seems leading to regressions.

I'm not modifying the launcher, I've modified the default parameter used by the HUD to set the launcher size.
But that parameter was wrong (maybe it has been put there as workaround to make the HUD to be close to the dash geometries).

> Looking closely it seems you're right, there's 1px gap between the hud and the launcher.

That's what I meant. Compare my two screenshots, this is totally wrong.

[Thomi Richards (thomir-deactivatedaccount)]

Needs a test.

Should be pretty easy to construct an autopilot test that changes the launcher to different sizes, reveals the hud, and checks that it's X position is correct?

Cheers,

review: Needs Fixing

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

Thomi, this is a pure visual change, so tests are not needed.

Of course it's possible to integrate some checks in AP, but this shouldn't be required.

[Tim Penhey (thumper)]

I'm OK with these changes. I was talking with Thomi about the tests. While it is desirable to have some AP tests around the layout, which we should be able to do relatively easily, I don't want to change the goal posts during the last four weeks of the cycle, so this can land without tests due to the visual nature of the change.

In the future (i.e. next cycle), I expect that we'll start to add some AP tests around layout.

review: Approve

[Andrea Cimitan (cimi)]

review: Approve (design)

=== modified file 'plugins/unityshell/src/HudController.cpp'
--- plugins/unityshell/src/HudController.cpp	2012-03-26 13:12:08 +0000
+++ plugins/unityshell/src/HudController.cpp	2012-04-01 01:00:26 +0000
@@ -39,7 +39,7 @@
 }
 
 Controller::Controller()
-  : launcher_width(65)
+  : launcher_width(64)
   , launcher_locked_out(false)
   , multiple_launchers(true)
   , hud_service_("com.canonical.hud", "/com/canonical/hud")

=== modified file 'plugins/unityshell/src/HudIcon.cpp'
--- plugins/unityshell/src/HudIcon.cpp	2012-03-27 14:10:40 +0000
+++ plugins/unityshell/src/HudIcon.cpp	2012-04-01 01:00:26 +0000
@@ -19,9 +19,13 @@
 
 #include "HudIcon.h"
 #include "NuxCore/Logger.h"
+#include "config.h"
+
 namespace
 {
   nux::logging::Logger logger("unity.hud.icon");
+  const unsigned int tile_margin = 4;
+  const unsigned int minimum_width = 64;
 }
 
 namespace unity
@@ -29,23 +33,13 @@
 namespace hud
 {
 
-Icon::Icon(nux::BaseTexture* texture, guint width, guint height)
-  : unity::IconTexture(texture, width, height)
-{
-  Init();
-  icon_renderer_.SetTargetSize(54, 46, 0);
-}
-
-Icon::Icon(std::string const& icon_name, unsigned int size, bool defer_icon_loading)
-  : unity::IconTexture(icon_name, size, defer_icon_loading)
-{
-  Init();
-}
-
-void Icon::Init()
-{
-  SetMinimumWidth(66);
-  SetMinimumHeight(66);
+Icon::Icon(std::string const& icon_name, unsigned int size)
+  : IconTexture(icon_name, size, true)
+{
+  int tile_size = size + tile_margin * 2;
+  SetMinimumWidth(minimum_width);
+  SetMinimumHeight(tile_size);
+  icon_renderer_.SetTargetSize(tile_size, size, 0);
   background_.Adopt(nux::CreateTexture2DFromFile(PKGDATADIR"/launcher_icon_back_54.png", -1, true));
   gloss_.Adopt(nux::CreateTexture2DFromFile(PKGDATADIR"/launcher_icon_shine_54.png", -1, true));
   edge_.Adopt(nux::CreateTexture2DFromFile(PKGDATADIR"/launcher_icon_edge_54.png", -1,  true));
@@ -80,7 +74,6 @@
 
 
   auto toplevel = GetToplevel();
-  icon_renderer_.SetTargetSize(54, 46, 0);
   icon_renderer_.PreprocessIcons(args, toplevel->GetGeometry());
   icon_renderer_.RenderIcon(GfxContext, arg, toplevel->GetGeometry(), toplevel->GetGeometry());
 }

=== modified file 'plugins/unityshell/src/HudIcon.h'
--- plugins/unityshell/src/HudIcon.h	2012-03-27 14:10:40 +0000
+++ plugins/unityshell/src/HudIcon.h	2012-04-01 01:00:26 +0000
@@ -21,22 +21,6 @@
 #ifndef HUDICON_H
 #define HUDICON_H
 
-#include <set>
-#include <string>
-
-#include "config.h"
-
-#include <Nux/Nux.h>
-#include <Nux/BaseWindow.h>
-#include <NuxCore/Math/MathInc.h>
-
-#include <sigc++/trackable.h>
-#include <sigc++/signal.h>
-#include <sigc++/functors/ptr_fun.h>
-#include <sigc++/functors/mem_fun.h>
-
-#include <gtk/gtk.h>
-
 #include "IconTexture.h"
 #include "HudIconTextureSource.h"
 #include "IconRenderer.h"
@@ -51,15 +35,14 @@
 {
 public:
   typedef nux::ObjectPtr<IconTexture> Ptr;
-  Icon(nux::BaseTexture* texture, guint width, guint height);
-  Icon(std::string const& icon_name, unsigned int size, bool defer_icon_loading = false);
+  Icon(std::string const& icon_name, unsigned int size);
 
 protected:
   void Draw(nux::GraphicsEngine& GfxContext, bool force_draw);
-  void Init();
 
   std::string GetName() const;
 
+private:
   nux::ObjectPtr<nux::BaseTexture> background_;
   nux::ObjectPtr<nux::BaseTexture> gloss_;
   nux::ObjectPtr<nux::BaseTexture> edge_;

=== modified file 'plugins/unityshell/src/HudView.cpp'
--- plugins/unityshell/src/HudView.cpp	2012-03-29 21:23:51 +0000
+++ plugins/unityshell/src/HudView.cpp	2012-04-01 01:00:26 +0000
@@ -20,19 +20,11 @@
 
 #include <math.h>
 
-#include <gio/gdesktopappinfo.h>
 #include <glib/gi18n-lib.h>
-#include <gtk/gtk.h>
-#include <Nux/Button.h>
-#include <iostream>
-#include <sstream>
-
 #include <NuxCore/Logger.h>
 #include <UnityCore/GLibWrapper.h>
-#include <UnityCore/RadioOptionFilter.h>
 #include <UnityCore/Variant.h>
 #include <Nux/HLayout.h>
-#include <Nux/LayeredLayout.h>
 
 #include <NuxCore/Logger.h>
 #include "HudButton.h"
@@ -47,10 +39,19 @@
 namespace
 {
 nux::logging::Logger logger("unity.hud.view");
-int icon_size = 42;
+const int icon_size = 46;
 const std::string default_text = _("Type your command");
 const int grow_anim_length = 90 * 1000;
 const int pause_before_grow_length = 32 * 1000;
+
+const int default_width = 1024;
+const int default_height = 276;
+const int content_width = 941;
+
+const int top_padding = 11;
+const int bottom_padding = 9;
+const int left_padding = 11;
+const int right_padding = 0;
 }
 
 NUX_IMPLEMENT_OBJECT_TYPE(View);
@@ -185,8 +186,7 @@
   LOG_DEBUG(logger) << "content_geo: " << content_geo_.width << "x" << content_geo_.height;
 
   layout_->SetMinimumWidth(content_geo_.width);
-  layout_->SetMaximumWidth(content_geo_.width);
-  layout_->SetMaximumHeight(content_geo_.height);
+  layout_->SetMaximumSize(content_geo_.width, content_geo_.height);
 
   QueueDraw();
 }
@@ -258,7 +258,7 @@
     button->is_rounded = (query == --(queries.end())) ? true : false;
     button->fake_focused = (query == (queries.begin())) ? true : false;
 
-    button->SetMinimumWidth(941);
+    button->SetMinimumWidth(content_width);
     found_items++;
   }
   if (found_items)
@@ -285,14 +285,13 @@
 
   if (show_embedded_icon_)
   {
-    layout_->AddLayout(icon_layout_.GetPointer(), 0, nux::MINOR_POSITION_TOP,
-                       nux::MINOR_SIZE_MATCHCONTENT, 100.0f, nux::LayoutPosition::NUX_LAYOUT_BEGIN);
-
+    layout_->AddView(icon_.GetPointer(), 0, nux::MINOR_POSITION_LEFT,
+                     nux::MINOR_SIZE_FULL, 100.0f, nux::LayoutPosition::NUX_LAYOUT_BEGIN);
     AddChild(icon_.GetPointer());
   }
   else
   {
-    layout_->RemoveChildObject(static_cast<nux::Area*>(icon_layout_.GetPointer()));
+    layout_->RemoveChildObject(icon_.GetPointer());
     RemoveChild(icon_.GetPointer());
   }
 
@@ -306,13 +305,12 @@
 {
   //FIXME - remove magic values, replace with scalable text depending on DPI
   // requires smarter font settings really...
-  int width, height = 0;
-  width = 1024;
-  height = 276;
+  int width = default_width;
+  int height = default_height;
 
   if (!show_embedded_icon_)
   {
-    width -= icon_layout_->GetGeometry().width;
+    width -= icon_->GetGeometry().width;
   }
 
   LOG_DEBUG (logger) << "best fit is, " << width << ", " << height;
@@ -338,15 +336,6 @@
   absolute_window_geometry_ = absolute_geo;
 }
 
-namespace
-{
-  const int top_spacing = 11;
-  const int content_width = 941;
-  const int icon_vertical_margin = 5;
-  const int spacing_between_icon_and_content = 8;
-  const int bottom_padding = 10;
-}
-
 void View::SetupViews()
 {
   dash::Style& style = dash::Style::Instance();
@@ -354,27 +343,19 @@
   nux::VLayout* super_layout = new nux::VLayout(); 
   layout_ = new nux::HLayout();
   {
-    // fill icon layout with icon
-    icon_ = new Icon("", icon_size, true);
-    icon_layout_ = new nux::VLayout();
+    // fill layout with icon
+    icon_ = new Icon("", icon_size);
     {
       AddChild(icon_.GetPointer());
-      icon_layout_->SetVerticalExternalMargin(icon_vertical_margin);
-      icon_layout_->AddView(icon_.GetPointer(), 0, nux::MINOR_POSITION_LEFT, nux::MINOR_SIZE_FULL);
-      layout_->AddLayout(icon_layout_.GetPointer(), 0, nux::MINOR_POSITION_TOP, nux::MINOR_SIZE_MATCHCONTENT);
+      layout_->AddView(icon_.GetPointer(), 0, nux::MINOR_POSITION_LEFT, nux::MINOR_SIZE_FULL);
     }
 
-    // add padding to layout between icon and content
-    layout_->AddLayout(new nux::SpaceLayout(spacing_between_icon_and_content,
-                                            spacing_between_icon_and_content,
-                                            spacing_between_icon_and_content,
-                                            spacing_between_icon_and_content), 0);
-
     // fill the content layout
     content_layout_ = new nux::VLayout();
     {
-      // add the top spacing
-      content_layout_->AddLayout(new nux::SpaceLayout(top_spacing,top_spacing,top_spacing,top_spacing), 0);
+      // Set the layout paddings
+      content_layout_->SetLeftAndRightPadding(left_padding, right_padding);
+      content_layout_->SetTopAndBottomPadding(top_padding, bottom_padding);
 
       // add the search bar to the composite
       search_bar_ = new unity::SearchBar(true);
@@ -389,10 +370,6 @@
       button_views_->SetMaximumWidth(content_width);
 
       content_layout_->AddLayout(button_views_.GetPointer(), 1, nux::MINOR_POSITION_LEFT);
-      content_layout_->AddLayout(new nux::SpaceLayout(bottom_padding,
-                                                      bottom_padding,
-                                                      bottom_padding,
-                                                      bottom_padding), 0);
     }
 
     layout_->AddLayout(content_layout_.GetPointer(), 1, nux::MINOR_POSITION_TOP);

=== modified file 'plugins/unityshell/src/HudView.h'
--- plugins/unityshell/src/HudView.h	2012-03-25 10:14:37 +0000
+++ plugins/unityshell/src/HudView.h	2012-04-01 01:00:26 +0000
@@ -21,14 +21,9 @@
 
 #include <string>
 
-#include <NuxGraphics/GraphicsEngine.h>
 #include <Nux/Nux.h>
-#include <Nux/PaintLayer.h>
 #include <Nux/View.h>
 #include <Nux/VLayout.h>
-#include <StaticCairoText.h>
-
-#include <glib.h>
 
 #include <UnityCore/Hud.h>
 #include "Introspectable.h"
@@ -105,7 +100,6 @@
   //FIXME - replace with dash search bar once modifications to dash search bar land
   SearchBar::Ptr search_bar_;
   Icon::Ptr icon_;
-  nux::ObjectPtr<nux::Layout> icon_layout_;
   bool visible_;
 
   Hud::Queries queries_;

=== modified file 'plugins/unityshell/src/IconTexture.cpp'
--- plugins/unityshell/src/IconTexture.cpp	2012-03-27 14:10:40 +0000
+++ plugins/unityshell/src/IconTexture.cpp	2012-04-01 01:00:26 +0000
@@ -81,6 +81,12 @@
   _icon_name = icon_name;
   _size = size;
 
+  if (_size == 0)
+  {
+    _texture_cached = nullptr;
+    return;
+  }
+
   LoadIcon();
 }
 
@@ -95,7 +101,7 @@
   LOG_DEBUG(logger) << "LoadIcon called (" << _icon_name << ") - loading: " << _loading;
   static const char* const DEFAULT_GICON = ". GThemedIcon text-x-preview";
 
-  if (_loading)
+  if (_loading || _size == 0)
     return;
 
   _loading = true;