Merge lp:~3v1n0/unity/launcher-primary-indicators-fix into lp:unity

[Marco Trevisan (Treviño)]

BamfLauncherIcon: implement WindowsOnViewport to get only windows on visible VP

This function doesn't take account of the monitors.
Rewritten also BamfLauncherIcon::Windows and ::WindowsForMonitor
to remove code duplication, using the new GetWindows function to perform
filtering via the WindowFilterMask.

UNBLOCK as requested by didrocks

[Jason Smith (jassmith)]

looks good, works good, I like it

review: Approve

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

Please make the manual test follow the test template (manual-tests/TEST_TEMPLATE.txt). Specifically:

Tests need to be divided with a HR.
Tests need to have three stages: Setup, Actions, Expected Results.
For all three sections, steps should be bullet points, and as simple and explicit as possible.

For example, the first test should be written as:
<snip>
Test Launcher Icon with Windows in multiple monitors
----------------------------------------------------
This test shows how the launcher should draw its iucons when there are
application windows in multiple monitors.

Setup:
 * Run CCSM and ensure that the unity option in "Experimental" -> "Launcher Monitors" is set to "All Desktops"

Actions:
 * Open two GEdit windows on monitor 1, workspace 1.
 * Open one GEdit window on monitor 2, workspace 1.
 * Focus one of the two gedit windows on monitor one.

Expected Results:
 * GEdit launcher icon on monitor 1 must have:
   * Two pips on the left of the icon.
   * A filled-in arrow on the right of the icon.
 * GEDit launcher icon on monitor 2 must have:
   * One pip on the left of the icon
   * One hollow arrow on the right of the icon.
</snip>

...Although you will need to make that correct (I can't understand your test as it is, so I made most of that up).

Create a separate test that describes what should happen when you move workspaces.

Remember: tests should be small and simple.

review: Needs Fixing

[Tim Penhey (thumper)]

I'd like to see the manual tests become autopilot tests.

[Thomi Richards (thomir-deactivatedaccount)]

I'd also like to see these made into autopilot tests. The last thing I want to see is a bunch more manual tests that we need to slog through every release.

Ideally I'd like to see the tests not repeat the actions / expected results sections, but this is good enough to be run.

I'll approve this now, on the understanding that someone (ideally not me) turns these into autopilot tests. The only reason we're adding manual tests now is that we're so close to a release.

review: Approve

[Unity Merger (unity-merger)]

Attempt to merge into lp:unity failed due to conflicts:

text conflict in manual-tests/Launcher.txt

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

> Ideally I'd like to see the tests not repeat the actions / expected results
> sections, but this is good enough to be run.

Yes, but we should also avoid to re-write the same actions for performing different tests.
IMHO we should just support something like "stages"... I.e.

TEST foo

Stage 1
Actions...
Expected results...

Stage 2
Actions...
Expected results...

.

So it will be easier to reference a single failure, but it would still avoid to re-define new tests only because there are actions dependents on the previous ones.

=== modified file 'manual-tests/Launcher.txt'
--- manual-tests/Launcher.txt	2012-03-23 02:34:18 +0000
+++ manual-tests/Launcher.txt	2012-03-23 10:29:18 +0000
@@ -190,6 +190,160 @@
     from gnome-control-center -> Monitor should update the launcher position.
 
 
+Test Multiple Launchers with Windows in Multiple Monitors
+---------------------------------------------------------
+This test shows how the launchers should draw their icons when there are
+application windows in multiple monitors.
+
+Setup:
+ * Run CCSM and ensure that the unity option in "Experimental" ->
+   "Launcher Monitors" is set to "All Desktops"
+
+Actions:
+ * Open 2 Gedit windows on monitor 1, workspace 1.
+ * Open 1 Gedit window on monitor 2, workspace 1.
+ * Focus one of the two gedit windows on monitor 1.
+
+Expected Results:
+ * Gedit launcher icon on monitor 1 must have:
+   * Two pips on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+ * Gedit launcher icon on monitor 2 must have:
+   * One empty arrow on the left of the icon.
+   * Nothing on the right of the icon.
+
+
+Test Multiple Launchers with Windows in Multiple Monitors and Workspaces
+------------------------------------------------------------------------
+This test shows how the launchers should draw their icons when there are
+application windows in multiple monitors spreaded in multiple workspaces.
+
+Setup:
+ * Run CCSM and ensure that the unity option in "Experimental" ->
+   "Launcher Monitors" is set to "All Desktops"
+
+Actions:
+ * Open 1 Gedit window on monitor 1, workspace 1.
+ * Open 2 Gedit windows on monitor 2, workspace 1.
+ * Open 2 Gedit windows on monitor 1, workspace 2.
+ * Open 1 Gedit window on monitor 2, workspace 3.
+ * Move to workspace 1.
+ * Focus the gedit window on monitor 1.
+
+Expected Results:
+ * Gedit launcher icon on monitor 1 must have:
+   * A filled-in arrow on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+ * Gedit launcher icon on monitor 2 must have:
+   * Two pips on the left of the icon.
+   * Nothing on the right of the icon.
+
+Actions:
+ * Move to workspace 2.
+ * Focus one of the two gedit windows on monitor 1.
+
+Expected Results:
+ * Gedit launcher icon on monitor 1 must have:
+   * Two pips on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+ * Gedit launcher icon on monitor 2 must have:
+   * An empty arrow on the left of the icon.
+   * Nothing on the right of the icon.
+
+Actions:
+ * Move to workspace 3.
+ * Focus the gedit window on monitor 2.
+
+Expected Results:
+ * Gedit launcher icon on monitor 1 must have:
+   * An empty arrow on the left of the icon.
+   * Nothing on the right of the icon.
+ * Gedit launcher icon on monitor 2 must have:
+   * A filled-in arrow on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+
+Actions:
+ * Move to workspace 4.
+
+Expected Results:
+ * Gedit launcher icon on monitor 1 must have:
+   * An empty arrow on the left of the icon.
+   * Nothing on the right of the icon.
+ * Gedit launcher icon on monitor 2 must have:
+   * An empty arrow on the left of the icon.
+   * Nothing on the right of the icon.
+
+Test Single Launcher with Windows in Multiple Monitors
+------------------------------------------------------
+This test shows how the launcher should draw its icons when there are
+application windows in multiple monitors.
+
+Setup:
+ * Run CCSM and ensure that the unity option in "Experimental" ->
+   "Launcher Monitors" is set to "Primary Desktop"
+
+Actions:
+ * Open 2 Gedit windows on monitor 1, workspace 1.
+ * Open 1 Gedit window on monitor 2, workspace 1.
+ * Focus one of the two gedit windows on monitor 1.
+
+Expected Results:
+ * The launcher is placed only on primary monitor
+ * Gedit launcher icon must have:
+   * Three pips on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+
+
+Test Single Launcher with Windows in Multiple Monitors and Workspaces
+---------------------------------------------------------------------
+This test shows how the launcher should draw its icons when there are
+application windows in multiple monitors spreaded in multiple workspaces.
+
+Setup:
+ * Run CCSM and ensure that the unity option in "Experimental" ->
+   "Launcher Monitors" is set to "Primary Desktop"
+
+Actions:
+ * Open 1 Gedit window on monitor 1, workspace 1.
+ * Open 2 Gedit windows on monitor 2, workspace 1.
+ * Open 2 Gedit windows on monitor 1, workspace 2.
+ * Open 1 Gedit window on monitor 2, workspace 3.
+ * Move to workspace 1.
+ * Focus the gedit window on monitor 1.
+
+Expected Results:
+ * The launcher is shown only on primary monitor
+ * Gedit launcher icon must have:
+   * Three pips on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+
+Actions:
+ * Move to workspace 2.
+ * Focus one of the two gedit windows on monitor 1.
+
+Expected Results:
+ * Gedit launcher icon on monitor 1 must have:
+   * Two pips on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+
+Actions:
+ * Move to workspace 3.
+ * Focus the gedit window on monitor 2.
+
+Expected Results:
+ * Gedit launcher icon on monitor 2 must have:
+   * A filled-in arrow on the left of the icon.
+   * A filled-in arrow on the right of the icon.
+
+Actions:
+ * Move to workspace 4.
+
+Expected Results:
+ * Gedit launcher icon on monitor 1 must have:
+   * An empty arrow on the left of the icon.
+   * Nothing on the right of the icon.
+
+
 Panel appearance with overlays
 ------------------------------
 Setup:

=== modified file 'plugins/unityshell/src/AbstractLauncherIcon.h'
--- plugins/unityshell/src/AbstractLauncherIcon.h	2012-03-14 06:24:18 +0000
+++ plugins/unityshell/src/AbstractLauncherIcon.h	2012-03-23 10:29:18 +0000
@@ -147,7 +147,9 @@
 
   virtual std::vector<Window> WindowsForMonitor(int monitor) = 0;
 
-  virtual std::string NameForWindow (Window window) = 0;
+  virtual std::vector<Window> WindowsOnViewport() = 0;
+
+  virtual std::string NameForWindow(Window window) = 0;
 
   virtual const bool WindowVisibleOnMonitor(int monitor) = 0;
 

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.cpp'
--- plugins/unityshell/src/BamfLauncherIcon.cpp	2012-03-21 12:31:11 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.cpp	2012-03-23 10:29:18 +0000
@@ -287,55 +287,71 @@
     ubus_server_send_message(ubus_server_get_default(), UBUS_LAUNCHER_ACTION_DONE, nullptr);
 }
 
+std::vector<Window> BamfLauncherIcon::GetWindows(WindowFilterMask filter, int monitor)
+{
+  std::vector<Window> results;
+  GList* children, *l;
+  WindowManager *wm = WindowManager::Default();
+
+  monitor = (filter & WindowFilter::ON_ALL_MONITORS) ? -1 : monitor;
+  bool mapped = (filter & WindowFilter::MAPPED);
+  bool user_visible = (filter & WindowFilter::USER_VISIBLE);
+  bool current_desktop = (filter & WindowFilter::ON_CURRENT_DESKTOP);
+
+  children = bamf_view_get_children(BAMF_VIEW(_bamf_app.RawPtr()));
+  for (l = children; l; l = l->next)
+  {
+    if (!BAMF_IS_WINDOW(l->data))
+      continue;
+  
+    auto window = static_cast<BamfWindow*>(l->data);
+    auto view = static_cast<BamfView*>(l->data);
+
+    if ((monitor >= 0 && bamf_window_get_monitor(window) == monitor) || monitor < 0)
+    {
+      if ((user_visible && bamf_view_user_visible(view)) || !user_visible)
+      {
+        guint32 xid = bamf_window_get_xid(window);
+
+        if ((mapped && wm->IsWindowMapped(xid)) || !mapped)
+        {
+          if ((current_desktop && wm->IsWindowOnCurrentDesktop(xid)) || !current_desktop)
+          {
+            results.push_back(xid);
+          }
+        }
+      }
+    }
+  }
+
+  g_list_free(children);
+  return results;
+}
+
 std::vector<Window> BamfLauncherIcon::Windows()
 {
-  std::vector<Window> results;
-  GList* children, *l;
-  WindowManager *wm = WindowManager::Default();
-
-  children = bamf_view_get_children(BAMF_VIEW(_bamf_app.RawPtr()));
-  for (l = children; l; l = l->next)
-  {
-    if (!BAMF_IS_WINDOW(l->data))
-      continue;
-
-    Window xid = bamf_window_get_xid(static_cast<BamfWindow*>(l->data));
-
-    if (wm->IsWindowMapped(xid))
-    {
-      results.push_back(xid);
-    }
-  }
-
-  g_list_free(children);
-  return results;
+  return GetWindows(WindowFilter::MAPPED|WindowFilter::ON_ALL_MONITORS);
+}
+
+std::vector<Window> BamfLauncherIcon::WindowsOnViewport()
+{
+  WindowFilterMask filter;
+  filter |= WindowFilter::MAPPED;
+  filter |= WindowFilter::USER_VISIBLE;
+  filter |= WindowFilter::ON_CURRENT_DESKTOP;
+  filter |= WindowFilter::ON_ALL_MONITORS;
+
+  return GetWindows(filter);
 }
 
 std::vector<Window> BamfLauncherIcon::WindowsForMonitor(int monitor)
 {
-  std::vector<Window> results;
-  GList* children, *l;
-  WindowManager *wm = WindowManager::Default();
-
-  children = bamf_view_get_children(BAMF_VIEW(_bamf_app.RawPtr()));
-  for (l = children; l; l = l->next)
-  {
-    if (!BAMF_IS_WINDOW(l->data))
-      continue;
-
-    auto window = static_cast<BamfWindow*>(l->data);
-    if (bamf_window_get_monitor(window) == monitor)
-    {
-      guint32 xid = bamf_window_get_xid(window);
-      bool user_visible = bamf_view_user_visible(reinterpret_cast<BamfView*>(window));
-
-      if (user_visible && wm->IsWindowMapped(xid) && wm->IsWindowOnCurrentDesktop(xid))
-        results.push_back(xid);
-    }
-  }
-
-  g_list_free(children);
-  return results;
+  WindowFilterMask filter;
+  filter |= WindowFilter::MAPPED;
+  filter |= WindowFilter::USER_VISIBLE;
+  filter |= WindowFilter::ON_CURRENT_DESKTOP;
+
+  return GetWindows(filter, monitor);
 }
 
 std::string BamfLauncherIcon::NameForWindow(Window window)

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.h'
--- plugins/unityshell/src/BamfLauncherIcon.h	2012-03-14 06:24:18 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.h	2012-03-23 10:29:18 +0000
@@ -61,6 +61,7 @@
   virtual unsigned long long SwitcherPriority();
 
   std::vector<Window> Windows();
+  std::vector<Window> WindowsOnViewport();
   std::vector<Window> WindowsForMonitor(int monitor);
   std::string NameForWindow(Window window);
 
@@ -87,6 +88,15 @@
   std::string GetName() const;
 
 private:
+  typedef unsigned long int WindowFilterMask;
+  enum WindowFilter
+  {
+    MAPPED = (1 << 0),
+    USER_VISIBLE = (1 << 1),
+    ON_CURRENT_DESKTOP = (1 << 2),
+    ON_ALL_MONITORS = (1 << 3),
+  };
+
   void EnsureWindowState();
   void EnsureMenuItemsReady();
   void UpdateDesktopFile();
@@ -103,6 +113,7 @@
 
   bool OwnsWindow(Window w) const;
 
+  std::vector<Window> GetWindows(WindowFilterMask filter, int monitor = -1);
   const std::set<std::string>& GetSupportedTypes();
 
 

=== modified file 'plugins/unityshell/src/Launcher.cpp'
--- plugins/unityshell/src/Launcher.cpp	2012-03-23 02:22:04 +0000
+++ plugins/unityshell/src/Launcher.cpp	2012-03-23 10:29:18 +0000
@@ -939,7 +939,7 @@
   else
   {
     if (options()->show_for_all)
-      arg.window_indicators = std::max<int> (icon->Windows().size(), 1);
+      arg.window_indicators = std::max<int> (icon->WindowsOnViewport().size(), 1);
     else
       arg.window_indicators = std::max<int> (icon->WindowsForMonitor(monitor).size(), 1);
   }

=== modified file 'plugins/unityshell/src/LauncherIcon.h'
--- plugins/unityshell/src/LauncherIcon.h	2012-03-14 06:24:18 +0000
+++ plugins/unityshell/src/LauncherIcon.h	2012-03-23 10:29:18 +0000
@@ -102,11 +102,13 @@
 
   int SortPriority();
 
-  virtual std::vector<Window> Windows () { return std::vector<Window> (); }
-
-  virtual std::vector<Window> WindowsForMonitor (int monitor) { return std::vector<Window> (); }
-
-  virtual std::string NameForWindow (Window window) { return std::string(); }
+  virtual std::vector<Window> Windows() { return std::vector<Window> (); }
+
+  virtual std::vector<Window> WindowsOnViewport() { return std::vector<Window> (); }
+
+  virtual std::vector<Window> WindowsForMonitor(int monitor) { return std::vector<Window> (); }
+
+  virtual std::string NameForWindow(Window window) { return std::string(); }
 
   const bool WindowVisibleOnMonitor(int monitor);
 

=== modified file 'plugins/unityshell/src/MockLauncherIcon.h'
--- plugins/unityshell/src/MockLauncherIcon.h	2012-03-14 06:24:18 +0000
+++ plugins/unityshell/src/MockLauncherIcon.h	2012-03-23 10:29:18 +0000
@@ -81,6 +81,22 @@
     return result;
   }
 
+  std::vector<Window> WindowsOnViewport ()
+  {
+    std::vector<Window> result;
+
+    result.push_back ((100 << 16) + 200);
+    result.push_back ((500 << 16) + 200);
+    result.push_back ((300 << 16) + 200);
+    result.push_back ((200 << 16) + 200);
+    result.push_back ((300 << 16) + 200);
+    result.push_back ((100 << 16) + 200);
+    result.push_back ((300 << 16) + 200);
+    result.push_back ((600 << 16) + 200);
+
+    return result;
+  }
+
   std::vector<Window> WindowsForMonitor (int monitor)
   {
     std::vector<Window> result;