Merge lp:~mhr3/libunity/dbus-activation-fixes into lp:libunity

[Michal Hruby]

Fixes an issue where a search which didn't change the search key could finish before the search that was actually searching.

The problem manifested itself where a DBus-activated lens daemon would return a result set with model-seqnum=0. This was the case because all lenses call during initialization queue_search_changed() which creates a LensSearch instance with empty search string, this generates empty string search-key and therefore the actual Search method just returned because the search key didn't change from the last search.

The fix for this is to properly wait for the last search to finish before returning from the non-changed one.

Added test to detect this issue.

[Michal Hruby (mhr3)]

This fixes the problem where DBus-activated lens daemon would return result set with model-seqnum=0. This was the case because all lenses call during initialization queue_search_changed() which creates a LensSearch instance with empty search string, this generates empty string search-key and therefore the actual Search method just returned because the search key didn't change from the last search.

The fix for this is to properly wait for the last search to finish before returning from the non-changed one.

[Mikkel Kamstrup Erlandsen (kamstrup)]

Looking good and tests are passing. Awesome work dude!

review: Approve

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-libunity/46/console reported an error when processing this lp:~mhr3/libunity/dbus-activation-fixes branch.
Not merging it.

[Didier Roche-Tolomelli (didrocks)]

This one is due to https://code.launchpad.net/~mhr3/libunity/test-tool/+merge/91027 failing. sorry, I need to fix this race (should be easy, just have to take the time :))

=== modified file 'src/unity-lens-private.vala'
--- src/unity-lens-private.vala	2012-01-09 17:40:45 +0000
+++ src/unity-lens-private.vala	2012-01-31 18:37:22 +0000
@@ -516,7 +516,7 @@
     // wait for the results from all scopes - yield will wait for
     // search.callback to resume execution, and that will be called once all
     // scopes finish search (thanks to closure magic)
-    yield;
+    if (num_scopes > 0) yield;
 
     result.insert ("model-seqnum", new Variant.uint64 (_results_model.get_seqnum ()));
 
@@ -572,7 +572,7 @@
     // wait for the results from all scopes - yield will wait for
     // search.callback to resume execution, and that will be called once all
     // scopes finish search (thanks to closure magic)
-    yield;
+    if (num_scopes > 0) yield;
 
     var result = new HashTable<string, Variant> (str_hash, str_equal);
     result.insert ("model-seqnum", new Variant.uint64 (_global_results_model.get_seqnum ()));

=== modified file 'src/unity-lens-search.vala'
--- src/unity-lens-search.vala	2011-12-09 09:23:25 +0000
+++ src/unity-lens-search.vala	2012-01-31 18:37:22 +0000
@@ -49,6 +49,18 @@
             results_model:results_model);
   }
 
+  private bool got_finished = false;
+
+  construct
+  {
+    this.finished.connect (() => { got_finished = true; });
+  }
+
+  internal bool was_finished ()
+  {
+    return got_finished;
+  }
+
   public bool equals (LensSearch? other)
   {
     if (other == null ||

=== modified file 'src/unity-lens-tools.vala'
--- src/unity-lens-tools.vala	2012-01-05 10:29:50 +0000
+++ src/unity-lens-tools.vala	2012-01-31 18:37:22 +0000
@@ -203,7 +203,8 @@
     }
     else
     {
-      warning (@"Could not find row to remove: $(provider.get_string (iter, 0))");
+      /* this can easily happen now that we support custom merge strategies */
+      debug (@"Could not find row to remove: $(provider.get_string (iter, 0))");
     }
   }
 

=== modified file 'src/unity-scope-private.vala'
--- src/unity-scope-private.vala	2012-01-17 20:38:42 +0000
+++ src/unity-scope-private.vala	2012-01-31 18:37:22 +0000
@@ -395,6 +395,9 @@
       search_internal.callback ();
     });
 
+    LensSearch? last_search = null;
+    ulong ls_sig_id = 0;
+
     string search_key = get_search_key (s, search_type);
     if ((search_key != null && search_key != search_keys[search_type]) ||
         (search_key == null && !s.equals (_owner.get_last_search (search_type))))
@@ -406,8 +409,28 @@
     }
     else
     {
-      // returning empty HashTable in this case (search_key didn't change)
-      return result;
+      // search key didn't change, but the last search might not have finished
+      // yet, let's wait for it if that's the case
+      last_search = _owner.get_last_search (search_type);
+      if (last_search != null && !last_search.was_finished ())
+      {
+        // wait for the last search to finish
+        ls_sig_id = last_search.finished.connect (() =>
+        {
+          s.finished ();
+        });
+
+        // update view_type if necessary, because schedule_search_changed
+        // might be waiting for it
+        ViewType current_view = is_global_search ?
+          ViewType.HOME_VIEW : ViewType.LENS_VIEW;
+        if (view_type != current_view) set_view_type (current_view);
+      }
+      else
+      {
+        // returning empty HashTable in this case (search_key didn't change)
+        return result;
+      }
     }
 
     // now we have a Cancellable associated with this search
@@ -425,6 +448,7 @@
     yield;
     // we just got finished signal, or the cancellable was cancelled
 
+    if (ls_sig_id != 0) SignalHandler.disconnect (last_search, ls_sig_id);
     SignalHandler.disconnect (s, sig_id);
     cancellable.disconnect (canc_id);
 
@@ -441,6 +465,7 @@
       string search_string, HashTable<string, Variant> hints) throws Error
   {
     HashTable<string, Variant> result;
+
     result = yield search_internal (search_string, hints, SearchType.DEFAULT);
 
     return result;
@@ -450,6 +475,7 @@
       string search_string, HashTable<string, Variant> hints) throws Error
   {
     HashTable<string, Variant> result;
+
     result = yield search_internal (search_string, hints, SearchType.GLOBAL);
 
     return result;

=== modified file 'test/vala/test-lens.vala'
--- test/vala/test-lens.vala	2012-01-20 11:29:46 +0000
+++ test/vala/test-lens.vala	2012-01-31 18:37:22 +0000
@@ -47,6 +47,7 @@
     Test.add_data_func ("/Unit/LocalScope/SearchChanged", test_local_scope_search);
     Test.add_data_func ("/Unit/LocalScope/MergeStrategy", test_merge_strategy);
     Test.add_data_func ("/Unit/Lens/ReturnAfterScopeFinish", test_lens_return_after_scope_finish);
+    Test.add_data_func ("/Unit/Lens/SuccessiveSearches", test_lens_successive_searches);
     Test.add_data_func ("/Unit/Lens/TwoSearches", test_lens_two_searches);
     Test.add_data_func ("/Unit/Lens/ModelSync", test_lens_model_sync);
     Test.add_data_func ("/Unit/Lens/ReplyHint", test_lens_reply_hint);
@@ -237,6 +238,7 @@
     /* Since test cases are not completely isolated we need 
      * to instantate the default merge strategy when done */
     var old_merge_strategy = exported_lens.merge_strategy;
+    local_scope.results_model.clear ();
     
     var merge_strategy = new TestMergeStrategy ();
     exported_lens.merge_strategy = merge_strategy;
@@ -296,6 +298,51 @@
     SignalHandler.disconnect (local_scope, sig_id);
   }
 
+  public static void test_lens_successive_searches ()
+  {
+    assert (local_scope != null);
+
+    var ml = new MainLoop ();
+    bool got_search_changed = false;
+    bool finish_called = false;
+
+    ulong sig_id = local_scope.search_changed.connect ((lens_search) =>
+    {
+      got_search_changed = true;
+      Timeout.add (750, () =>
+      {
+        lens_search.results_model.clear ();
+        lens_search.results_model.append ("", "", 0, "", "", "", "");
+        lens_search.finished ();
+        finish_called = true;
+        return false;
+      });
+    });
+
+    // we want to make sure the Search DBus call doesn't return before we
+    // call finished on the LensSearch instance
+    Variant? result1 = null;
+    Variant? result2 = null;
+    call_lens_search ("successive-searches", (result) =>
+    {
+      result1 = result;
+    });
+    // and another search with the same search string, it shouldn't return first
+    call_lens_search ("successive-searches", (result) =>
+    {
+      result2 = result;
+      ml.quit ();
+    });
+    
+    run_with_timeout (ml, 5000);
+
+    assert (got_search_changed == true);
+    assert (finish_called == true);
+    assert (result1.equal (result2));
+
+    SignalHandler.disconnect (local_scope, sig_id);
+  }
+
   public static void test_lens_two_searches ()
   {
     assert (local_scope != null);
@@ -460,7 +507,6 @@
   {
     assert (local_scope != null);
 
-    bool got_search_changed = false;
     local_scope.sources.add_option ("id1", "Source1", null);
     local_scope.sources.add_option ("id2", "Source2", null);
 

=== modified file 'test/vala/test-remote-scope.vala'
--- test/vala/test-remote-scope.vala	2012-01-20 11:29:46 +0000
+++ test/vala/test-remote-scope.vala	2012-01-31 18:37:22 +0000
@@ -28,6 +28,7 @@
     Test.add_data_func ("/Integration/Scope/Export", test_scope_export);
     Test.add_data_func ("/Integration/Scope/Search", test_scope_search);
     Test.add_data_func ("/Integration/Scope/Search2", test_scope_search2);
+    Test.add_data_func ("/Integration/Scope/SuccessiveSearches", test_scope_successive_searches);
     Test.add_data_func ("/Integration/Scope/TwoSearches", test_scope_two_searches);
     Test.add_data_func ("/Integration/Scope/ModelSync", test_scope_model_sync);
     Test.add_data_func ("/Integration/Scope/ReplyHint", test_scope_reply_hint);
@@ -113,6 +114,39 @@
     bus.flush_sync ();
   }
 
+  public static void test_scope_successive_searches ()
+  {
+    // we expect that the lens will call the search method
+    assert (exported_scope != null);
+
+    var ml = new MainLoop ();
+    bool got_search_changed = false;
+
+    ulong sig_id = exported_scope.search_changed.connect ((lens_search, search_type, cancellable) =>
+    {
+      assert (lens_search.search_string == "successive-searches");
+      got_search_changed = true;
+      // wait a while to emit the finished signal
+      Timeout.add (500, () =>
+      {
+        lens_search.finished ();
+        ml.quit ();
+        return false;
+      });
+    });
+
+    Timeout.add (2000, () => { ml.quit (); return false; });
+    ml.run ();
+
+    // earlier searches need to cancelled if there's a newer search
+    assert (got_search_changed == true);
+
+    SignalHandler.disconnect (exported_scope, sig_id);
+
+    var bus = Bus.get_sync (BusType.SESSION);
+    bus.flush_sync ();
+  }
+
   public static void test_scope_two_searches ()
   {
     // we expect that the lens will call the search method