Merge lp:~vanvugt/unity/fix-742544-trunk into lp:unity

[Daniel van Vugt]

Fixed: Launcher/dash ignoring primary monitor setting. (LP: #742544)

The primary monitor according to Unity is now chosen in this order:
  1. If there is only one monitor, then it's obvious.
  2. Primary monitor as set in the Displays control panel (Xrandr).
  3. Primary monitor as defined by xorg option TwinViewXineramaInfoOrder
     used by NVIDIA.
  4. Laptop screen, if one can be found.
  5. Left-most monitor.

[Andrea Azzarone (azzar1)]

Of course the "preview" in display settings panel should use the same algorithm. I will do it fixing Bug #875644.

[Daniel van Vugt (vanvugt)]

Work in progress... needs fixing for systems that don't support Xrandr and use Xinerama instead (NVIDIA). See comment #98 of bug 742544.

[lp:~vanvugt/unity/fix-742544-trunk]

1724. By Daniel van Vugt on 2011-11-04

Fix incorrect parsing of Option lines that don't start with spaces.

[Daniel van Vugt (vanvugt)]

Note that in the 5 steps described in this fix, steps 1-4 are already built into gdk_screen_get_primary_monitor().

If we are willing to remove step 5 (which I maintained for compatibility with the oneiric default behaviour), then most of UScreen::Refresh() can be deleted and replaced by: primary_ = gdk_screen_get_primary_monitor(screen).

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

Is this still a WIP? In that case, please, set the branch review status accordingly.
Thanks.

[Daniel van Vugt (vanvugt)]

Not WIP. The branch is ready for review and merging as is. And has been tested (from my PPA) by various users in the comments of bug 742544.

[Sam Spilsbury (smspillaz)]

18 +static bool
19 +XorgOptionDefined(const char *name)
20 +{
21 + bool found = false;
22 + FILE *file = fopen("/etc/X11/xorg.conf", "r");
23 + if (file != NULL)
24 + {
25 + char line[512];
26 + while (!found && fgets(line, sizeof(line), file) != NULL)
27 + {
28 + char option[128];
29 + if (sscanf(line, "Option%*[ \t]\"%127[^\"]\"", option) == 1 ||
30 + sscanf(line, "%*[ \t]Option%*[ \t]\"%127[^\"]\"", option) == 1)
31 + found = !strcmp(option, name);
32 + }
33 + fclose(file);
34 + }
35 + return found;
36 +}

That kind of scares me.

Is there a separate library that we can use to parse the xorg.conf or can you use nvidia-settings directly to do this ?

Probably best to add a unit test for stuff like this. I can ask tim if we can get someone else to do this.

[Daniel van Vugt (vanvugt)]

Scary, maybe. But well tested by many nvidia users already who use ppa:vanvugt/unity if you read the bug.

Also, calling nvidia-settings would be MUCH slower than this quick file scan.

I personally vote for my previous suggestion: Remove step 5 and then all the ugly changes can be deleted and replaced by:
primary_ = gdk_screen_get_primary_monitor(screen).

[Sam Spilsbury (smspillaz)]

I prefer option 5. There are probably other driver edge cases that gdk handles.

Also prefer to use nux::logging::Logger instead of g_print in unity. nux::logging::Logger allows filtering of error messages by domains etc and is safer than print* because it uses iostreams.

You can do that with something like:

namespace
{
    nux::logging::Logger logger ("unity.screen");
}

LOG_INFO (logger) << "foo!";

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

"I prefer option 5."

Do you mean you agree with me and prefer removing step 5? This bug is _caused_ by step 5. :)

[Sam Spilsbury (smspillaz)]

Yes, I prefer removing step 5 and going with primary_ = gdk_screen_get_primary_monitor(screen)

[Daniel van Vugt (vanvugt)]

And if we do remove step 5 and revert to a simple:
primary_ = gdk_screen_get_primary_monitor(screen)
then the logging can be removed completely.

P.S. The glib functions do have domains, not to mention being faster and more elegant than nux. But anything is ;)...
http://developer.gnome.org/glib/2.30/glib-Message-Logging.html

Unmerged revisions

1724. By Daniel van Vugt on 2011-11-04

Fix incorrect parsing of Option lines that don't start with spaces.

1723. By Daniel van Vugt on 2011-11-04

Added detection of NVIDIA/TwinViewXinerama primary monitor settings.
The primary monitor according to Unity is now chosen in this order:
  1. If there is only one monitor, then it's obvious.
  2. Primary monitor as set in the Displays control panel (Xrandr).
  3. Primary monitor as defined by xorg option TwinViewXineramaInfoOrder
     used by NVIDIA.
  4. Laptop screen, if one can be found.
  5. Left-most monitor.
(LP: #742544)

1722. By Daniel van Vugt on 2011-10-16

Fixed: Launcher ignoring primary monitor setting. (LP: #742544)

The primary monitor according to Unity is now chosen in this order:
  1. Primary monitor if set in the Displays control panel (XRandR).
  2. Laptop screen, if one can be found.
  3. Left-most monitor.

=== modified file 'plugins/unityshell/src/UScreen.cpp'
--- plugins/unityshell/src/UScreen.cpp	2011-09-23 23:53:13 +0000
+++ plugins/unityshell/src/UScreen.cpp	2011-11-04 10:04:24 +0000
@@ -17,6 +17,9 @@
  */
 
 #include "UScreen.h"
+#include <X11/extensions/Xrandr.h>
+
+#define UPREFIX "UScreen: "
 
 static UScreen* _default_screen = NULL;
 
@@ -89,21 +92,41 @@
   return FALSE;
 }
 
+static bool
+XorgOptionDefined(const char *name)
+{
+  bool found = false;
+  FILE *file = fopen("/etc/X11/xorg.conf", "r");
+  if (file != NULL)
+  {
+    char line[512];
+    while (!found && fgets(line, sizeof(line), file) != NULL)
+    {
+      char option[128];
+      if (sscanf(line, "Option%*[ \t]\"%127[^\"]\"", option) == 1 ||
+          sscanf(line, "%*[ \t]Option%*[ \t]\"%127[^\"]\"", option) == 1)
+        found = !strcmp(option, name);
+    }
+    fclose(file);
+  }
+  return found;
+}
+
 void
 UScreen::Refresh()
 {
-  GdkScreen*    screen;
-  nux::Geometry last_geo(0, 0, 1, 1);
-
-  screen = gdk_screen_get_default();
-
-  _monitors.erase(_monitors.begin(), _monitors.end());
-
-  g_print("\nScreen geometry changed:\n");
+  GdkScreen* screen = gdk_screen_get_default();
+
+  _monitors.clear();
+
+  g_print("\n" UPREFIX "Screen geometry changed:\n");
 
   int lowest_x = std::numeric_limits<int>::max();
   int highest_y = std::numeric_limits<int>::min();
-  for (int i = 0; i < gdk_screen_get_n_monitors(screen); i++)
+  int left_most = 0;
+  int laptop_monitor = -1;
+  int nmonitors = gdk_screen_get_n_monitors(screen);
+  for (int i = 0; i < nmonitors; i++)
   {
     GdkRectangle rect = { 0 };
 
@@ -111,24 +134,71 @@
 
     nux::Geometry geo(rect.x, rect.y, rect.width, rect.height);
 
-    // Check for mirrored displays
-    if (geo == last_geo)
-      continue;
-    last_geo = geo;
-
     _monitors.push_back(geo);
 
     if (geo.x < lowest_x || (geo.x == lowest_x && geo.y > highest_y))
     {
       lowest_x = geo.x;
       highest_y = geo.y;
-      primary_ = i;
-    }
-
-    g_print("   %dx%dx%dx%d\n", geo.x, geo.y, geo.width, geo.height);
-  }
-
-  g_print("\n");
+      left_most = i;
+    }
+
+    gchar *name = gdk_screen_get_monitor_plug_name(screen, i);
+    g_print(UPREFIX "   #%d: %s %dx%d +%d+%d\n",
+            i,
+            name == NULL ? "?" : name,
+            geo.width, geo.height,
+            geo.x, geo.y);
+    if (name)
+    {
+      if (g_ascii_strncasecmp(name, "LVDS", 4) == 0)
+        laptop_monitor = i;
+      g_free(name);
+    }
+  }
+
+  int gdk_primary = gdk_screen_get_primary_monitor(screen);
+  if (nmonitors <= 1)
+  {
+    primary_ = 0;
+    g_print(UPREFIX "Only one monitor.\n");
+  }
+  else if (gdk_primary > 0)
+  {
+    primary_ = gdk_primary;
+    g_print(UPREFIX "Primary GDK monitor is unambiguously #%d.\n", primary_);
+  }
+  else // Primary setting is ambiguous. Need to check using other methods...
+  {
+    Display *dpy = nux::GetGraphicsDisplay()->GetX11Display();
+    RROutput rr_primary = XRRGetOutputPrimary(dpy, DefaultRootWindow(dpy));
+    if (rr_primary != None)
+    {
+      primary_ = 0;
+      g_print(UPREFIX "Primary monitor is #0 according to GDK and Xrandr.\n");
+    }
+    else
+    {
+      g_print(UPREFIX "No primary monitor has been configured via Xrandr.\n");
+      if (XorgOptionDefined("TwinViewXineramaInfoOrder"))
+      {
+        primary_ = gdk_primary;
+        g_print(UPREFIX "Primary monitor is #%d according to GDK and "
+                        "confirmed by TwinViewXineramaInfoOrder.\n", primary_);
+      }
+      else if (laptop_monitor >= 0)
+      {
+        primary_ = laptop_monitor;
+        g_print(UPREFIX "Using laptop LVDS display: #%d.\n", primary_);
+      }
+      else
+      {
+        primary_ = left_most;
+        g_print(UPREFIX "Using left-most monitor: #%d.\n", primary_);
+      }
+    }
+  }
+  g_print(UPREFIX "Primary monitor for Unity is #%d.\n\n", primary_);
 
   changed.emit(primary_, _monitors);
 }