Merge lp:~brandontschaefer/unity/fix-711199 into lp:unity

[Brandon Schaefer]

= Problem description =

If no results are returned from a lens there is no message informing the user so.

= The fix =

In DashView::OnSearchFinished a check is made to see if a message should be
shown.

A default message is used if the lens doesn't provide one.

There is also a timer that waits for 150ms then hides the
message, if the search is taking a while.

= Test coverage =

There is an autopilot test for this now!

[Brandon Schaefer (brandontschaefer)]

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

review: Resubmit

[Brandon Schaefer (brandontschaefer)]

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

Need to not use a work around for the start up problem. Looking into removing the call in LensView:
http://bazaar.launchpad.net/~unity-team/unity/trunk/view/head:/plugins/unityshell/src/LensView.cpp#L350

Removing that if statement which calls Search("") and OnSearchFinished is getting emitted with 0 results for ALL lenses, even though they have them. So far testing shows this code can be removed, but want to do more testing by killing the lenses and restarting them.

[Tim Penhey (thumper)]

Why check for
  if (active_lens_view_->lens())
at the start of DashView::OnSearchFinished?
Small style issue, no space before (hints).

plese use:
  nux::color::White
instead of:
  nux::Color(1.0f,1.0f,1.0f,1.0f)

Instead of using += on a std::string, use a string stream.

#include <sstream>

std::stringstream sout;

sout << "<span size='larger' weight='bold'>";
//...
sout << g_variant_get_string(it->second, NULL);
//...
sout << "Sorry, there is nothing that matches your search.";
//...
sout << "</span>";

LOG_DEBUG(logger) << "The no-result-hint is: " << sout.str();

no_results_->SetText(sout.str());

How does the normal results view get shown again?

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

I was checking active_lens_->lens() because there was a problem before where the active home lens would get passed in with out actually having a lens() which would cause a seg fault. That has now been fix/changed so I remove that! (Tested, now no seg fault yay!)

Fixed style problems (there were other style errors; now fixed!), now using nux::color::White, and now markup uses sstream!

As for the normal results view, it is still getting shown how it normally does. The no-result-message is apart of the same layout so when the results == 0 the layout is empty. Then I change the layouts ContentDistribution to nux::MAJOR_POSITION_CENTER, which draws the text in the middle of the dash. When there are > 0 results I change it back to nux::MAJOR_POSITION_START (default), so the new results get draw back where they should. (Instead of the middle!)

[Michal Hruby (mhr3)]

89 + markup << g_variant_get_string(it->second, NULL);

Values in the Hints map are instances of unity::glib::Variant (which have a GVariant* operator), please use the GetString method (which is NULL safe).

93 + markup << "Sorry, there is nothing that matches your search.";

This needs to be marked for translation - just use _("Sorry, ...")

review: Needs Fixing

[Michal Hruby (mhr3)]

Also there could be an issue with hiding the message - currently it's only shown or hidden when the SearchFinishes, which means that this can happen:

If you're searching with a lens/scope that is really slow and is adding results one by one and finishes in let's say 5seconds, you can end up seeing the "Sorry, ..." message for the entire 5 seconds while it's searching and just then the message will disappear.

A solution to this would be to hide the message as soon as the search starts, but that again brings an issue that the search might finish in 10ms with no results, and the message will flicker. A proper solution will probably have to use a timer similar to what the search bar spinner is doing (it starts spinning after ~150ms after the search starts).

[Brandon Schaefer (brandontschaefer)]

review: Approve

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

I also guess you can easily write an autopilot test to check this. Just add a a value for no_results_active_ to the introspection wrapper, and check it with the AP suite ;)

review: Needs Fixing (missing tests)

[Michal Hruby (mhr3)]

https://code.launchpad.net/~brandontschaefer/unity/fix-711199/+merge/89192/comments/198065 - still not done.

[Michal Hruby (mhr3)]

84 + g_source_remove(hide_message_delay_id_);
100 + g_source_remove(hide_message_delay_id_);

You should set the id to 0 after these calls.

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

@Michal opps, I fixed that but forgot to push those changes...like a month ago haha.

[lp:~brandontschaefer/unity/fix-711199]

1907. By Brandon Schaefer on 2012-03-03

trunk

1908. By Brandon Schaefer on 2012-03-03

* Added an autopilot test and removed the manual onew

1909. By Brandon Schaefer on 2012-03-03

* Sleep added after kb.type()

1910. By Brandon Schaefer on 2012-03-03

* Split the autopilot test

1911. By Brandon Schaefer on 2012-03-03

Mixed names up

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

Tests are good, maybe instead of just searching for 'a' you could make it look for a real default application, so it will also indirectly test that lens is working :)

review: Approve

[lp:~brandontschaefer/unity/fix-711199]

1912. By Brandon Schaefer on 2012-03-04

*Changed autopilot search string

1913. By Brandon Schaefer on 2012-03-05

merged trunk

[Michal Hruby (mhr3)]

There is still one/two rare corner cases where the message could be displayed while also results are, but that can be solved in a later merge. Approve from me.

review: Approve

[Alex Launi (alexlauni)]

AP test looks good.

review: Approve

[lp:~brandontschaefer/unity/fix-711199]

1914. By Brandon Schaefer on 2012-03-08

merge trunk

1915. By Brandon Schaefer on 2012-03-09

Removed comments added by me...

[Tim Penhey (thumper)]

review: Approve

=== modified file 'plugins/unityshell/src/DashView.cpp'
--- plugins/unityshell/src/DashView.cpp	2012-02-14 10:18:15 +0000
+++ plugins/unityshell/src/DashView.cpp	2012-03-09 02:13:19 +0000
@@ -82,6 +82,7 @@
   , searching_timeout_id_(0)
   , search_in_progress_(false)
   , activate_on_finish_(false)
+  , hide_message_delay_id_(0)
   , visible_(false)
 {
   renderer_.SetOwner(this);
@@ -106,6 +107,8 @@
 {
   if (searching_timeout_id_)
     g_source_remove (searching_timeout_id_);
+  if (hide_message_delay_id_)
+    g_source_remove(hide_message_delay_id_);
 }
 
 void DashView::SetMonitorOffset(int x, int y)
@@ -374,6 +377,16 @@
   return FALSE;
 }
 
+gboolean DashView::HideResultMessageCb(gpointer data)
+{
+  DashView *self = static_cast<DashView*>(data);
+
+  self->active_lens_view_->HideResultsMessage();
+  self->hide_message_delay_id_ = 0;
+
+  return FALSE;
+}
+
 void DashView::OnSearchChanged(std::string const& search_string)
 {
   LOG_DEBUG(logger) << "Search changed: " << search_string;
@@ -385,9 +398,21 @@
     if (searching_timeout_id_)
     {
       g_source_remove (searching_timeout_id_);
+      searching_timeout_id_ = 0;
     }
+
     // 250ms for the Search method call, rest for the actual search
     searching_timeout_id_ = g_timeout_add (500, &DashView::ResetSearchStateCb, this);
+    
+    
+    if (hide_message_delay_id_)
+    {
+      g_source_remove(hide_message_delay_id_);
+      hide_message_delay_id_ = 0;
+    }
+
+    // 150ms to hide the no reults message if its take a while to return results
+    hide_message_delay_id_ = g_timeout_add (150, &DashView::HideResultMessageCb, this);
   }
 }
 
@@ -450,20 +475,26 @@
 
   search_bar_->can_refine_search = view->can_refine_search();
 
+  if (hide_message_delay_id_)
+  {
+    g_source_remove(hide_message_delay_id_);
+    hide_message_delay_id_ = 0;
+  }
+
   view->QueueDraw();
   QueueDraw();
 }
 
 void DashView::OnSearchFinished(Lens::Hints const& hints)
 {
-  Lens::Hints::const_iterator it;
-  it = hints.find("no-results-hint");
-  
-  if (it != hints.end())
+  if (hide_message_delay_id_)
   {
-    LOG_DEBUG(logger) << "We have no-results-hint: " << g_variant_get_string (it->second, NULL);
+    g_source_remove(hide_message_delay_id_);
+    hide_message_delay_id_ = 0;
   }
 
+  active_lens_view_->CheckNoResults(hints);
+
   std::string search_string = search_bar_->search_string;
   if (active_lens_view_ && active_lens_view_->search_string == search_string)
   {

=== modified file 'plugins/unityshell/src/DashView.h'
--- plugins/unityshell/src/DashView.h	2012-02-13 02:15:31 +0000
+++ plugins/unityshell/src/DashView.h	2012-03-09 02:13:19 +0000
@@ -108,6 +108,7 @@
   nux::Area* KeyNavIteration(nux::KeyNavDirection direction);
 
   static gboolean ResetSearchStateCb(gpointer data);
+  static gboolean HideResultMessageCb(gpointer data);
 
 private:
   UBusManager ubus_manager_;
@@ -136,6 +137,8 @@
   bool search_in_progress_;
   bool activate_on_finish_;
 
+  guint hide_message_delay_id_;
+
   bool visible_;
 };
 

=== 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 02:13:19 +0000
@@ -30,6 +30,8 @@
 #include "UBusWrapper.h"
 #include "PlacesVScrollBar.h"
 
+#include <glib/gi18n-lib.h>
+
 namespace unity
 {
 namespace dash
@@ -120,6 +122,7 @@
   , search_string("")
   , filters_expanded(false)
   , can_refine_search(false)
+  , no_results_active_(false)
   , fix_renderering_id_(0)
 {}
 
@@ -130,6 +133,7 @@
   , can_refine_search(false)
   , lens_(lens)
   , initial_activation_(true)
+  , no_results_active_(false)
   , fix_renderering_id_(0)
 {
   SetupViews(show_filters);
@@ -194,6 +198,10 @@
   scroll_view_->SetLayout(scroll_layout_);
   scroll_view_->SetRightArea(show_filters);
 
+  no_results_ = new nux::StaticCairoText("", NUX_TRACKER_LOCATION);
+  no_results_->SetTextColor(nux::color::White);
+  scroll_layout_->AddView(no_results_, 1, nux::MINOR_POSITION_CENTER, nux::MINOR_SIZE_MATCHCONTENT);
+
   fscroll_view_ = new LensScrollView(new PlacesVScrollBar(NUX_TRACKER_LOCATION),
                                      NUX_TRACKER_LOCATION);
   fscroll_view_->EnableVerticalScrollBar(true);
@@ -362,6 +370,54 @@
   return FALSE;
 }
 
+void LensView::CheckNoResults(Lens::Hints const& hints)
+{
+  gint count = lens_->results()->count();
+
+  if (!count && !no_results_active_)
+  {
+    std::stringstream markup;
+    Lens::Hints::const_iterator it;
+
+    it = hints.find("no-results-hint");
+    markup << "<span size='larger' weight='bold'>";
+
+    if (it != hints.end())
+    {
+      markup << it->second.GetString();
+    }
+    else
+    {
+      markup << _("Sorry, there is nothing that matches your search.");
+    }
+    markup << "</span>";
+
+    LOG_DEBUG(logger) << "The no-result-hint is: " << markup.str();
+
+    scroll_layout_->SetContentDistribution(nux::MAJOR_POSITION_CENTER); 
+
+    no_results_active_ = true;
+    no_results_->SetText(markup.str());
+  }
+  else if (count && no_results_active_)
+  {
+    scroll_layout_->SetContentDistribution(nux::MAJOR_POSITION_START);  
+
+    no_results_active_ = false;
+    no_results_->SetText("");
+  }
+}
+
+void LensView::HideResultsMessage()
+{
+  if (no_results_active_)
+  {
+    scroll_layout_->SetContentDistribution(nux::MAJOR_POSITION_START);  
+    no_results_active_ = false;
+    no_results_->SetText("");
+  }
+}
+
 void LensView::OnGroupExpanded(PlacesGroup* group)
 {
   ResultViewGrid* grid = static_cast<ResultViewGrid*>(group->GetChildView());
@@ -503,7 +559,8 @@
 {
   unity::variant::BuilderWrapper(builder)
     .add("name", lens_->id)
-    .add("lens-name", lens_->name);
+    .add("lens-name", lens_->name)
+    .add("no-results-active", no_results_active_);
 }
 
 

=== modified file 'plugins/unityshell/src/LensView.h'
--- plugins/unityshell/src/LensView.h	2012-02-13 12:55:20 +0000
+++ plugins/unityshell/src/LensView.h	2012-03-09 02:13:19 +0000
@@ -69,6 +69,9 @@
 
   sigc::signal<void, std::string const&> uri_activated;
 
+  void CheckNoResults(Lens::Hints const& hints);
+  void HideResultsMessage();
+
 private:
   void SetupViews(nux::Area* show_filters);
   void SetupCategories();
@@ -101,6 +104,7 @@
   CategoryGroups categories_;
   ResultCounts counts_;
   bool initial_activation_;
+  bool no_results_active_;
 
   nux::HLayout* layout_;
   LensScrollView* scroll_view_;
@@ -108,6 +112,7 @@
   LensScrollView* fscroll_view_;
   nux::VLayout* fscroll_layout_;
   FilterBar* filter_bar_;
+  nux::StaticCairoText* no_results_;
 
   guint fix_renderering_id_;
 

=== modified file 'tests/autopilot/autopilot/tests/test_dash.py'
--- tests/autopilot/autopilot/tests/test_dash.py	2012-03-07 20:14:43 +0000
+++ tests/autopilot/autopilot/tests/test_dash.py	2012-03-09 02:13:19 +0000
@@ -389,3 +389,48 @@
         searchbar = self.dash.get_searchbar()
         self.assertEqual("hello world", searchbar.search_string)
 
+class DashLensResultsTests(AutopilotTestCase):
+    """ Tests results from the lens view """
+
+    def setUp(self):
+        super(DashLensResultsTests, self).setUp()
+        self.dash = Dash()
+
+    def tearDown(self):
+        super(DashLensResultsTests, self).tearDown()
+        self.dash.ensure_hidden()
+    
+    def test_results_message_empty_search(self): 
+        """ This tests a message is not shown when there is no text""" 
+        self.dash.ensure_hidden()
+        self.dash.reveal_application_lens()
+        lens = self.dash.get_current_lens()
+
+        lens.refresh_state() 
+        self.assertFalse(lens.no_results_active)
+
+    def test_results_message(self): 
+        """ This test no mesage will be shown when results are there""" 
+        self.dash.ensure_hidden()
+        self.dash.reveal_application_lens()
+        lens = self.dash.get_current_lens()
+
+        kb = Keyboard();
+        kb.type("Terminal")
+        sleep(1)
+      
+        lens.refresh_state() 
+        self.assertFalse(lens.no_results_active)
+
+    def test_no_results_message(self): 
+        """ This test shows a message will appear in the lens""" 
+        self.dash.ensure_hidden()
+        self.dash.reveal_application_lens()
+        lens = self.dash.get_current_lens()
+
+        kb = Keyboard();
+        kb.type("qwerlkjzvxc")
+        sleep(1)
+
+        lens.refresh_state() 
+        self.assertTrue(lens.no_results_active)