Merge lp:~fourdollars/unity-greeter/add-hidpi-support into lp:unity-greeter

[Shih-Yuan Lee]

Add HiDPI display support.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1468
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~fourdollars/unity-greeter/add-hidpi-support/+merge/244266/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/unity-greeter-ci/55/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-greeter-vivid-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-greeter-vivid-armhf-ci/3

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-greeter-ci/55/rebuild

review: Needs Fixing (continuous-integration)

[lp:~fourdollars/unity-greeter/add-hidpi-support]

1468. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-11

Launchpad automatic translations update.

[Robert Ancell (robert-ancell)]

Can't this just be completed using the existing GDK API instead of using X calls?

i.e.

var use_hidpi = false;
var screen = Gdk.Screen.get_default ()
for (var i = 0; i < screen.get_n_monitors (); i++)
{
    var width_mm = screen.get_monitor_width_mm (i);
    var height_mm = screen.get_monitor_height_mm (i);
    var ppi = ...
    if (ppp ...)
        use_hidpi = true;
}

review: Needs Fixing

[Shih-Yuan Lee (fourdollars)]

No, this step needs to be done before GDK initialized.

[lp:~fourdollars/unity-greeter/add-hidpi-support]

1469. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-12

Launchpad automatic translations update.

1470. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-13

Launchpad automatic translations update.

1471. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-14

Launchpad automatic translations update.

[Shih-Yuan Lee (fourdollars)]

BTW, by the internal design of GTK+3/GDK, unity-settings-daemon/gnome-settings-daemon should be started before any GTK+3/GDK based application.
Because the xsettings plugin will bring up a property called '_XSETTINGS_SETTINGS', and every GTK+3/GDK based application will look for this property to set the proper scale factor when it starts.
This scale factor will be changed with "gsettings get org.gnome.desktop.interface scaling-factor".
The current design of unity-greeter will execute Gtk.init() or Gdk.init() before unity-settings-daemon starts, so the scale factor is already set as 1 by default because it can not find the X property '_XSETTINGS_SETTINGS'.
If we can execute unity-settings-daemon before Gtk.init()/Gdk.init() and make sure the X property '_XSETTINGS_SETTINGS' is already there, we may not need this patch.

[lp:~fourdollars/unity-greeter/add-hidpi-support]

1472. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-15

Launchpad automatic translations update.

[Robert Ancell (robert-ancell)]

That seems like a better solution. We should start unity-settings-daemon before we initialize GTK+ because all sorts of behaviour might change when it starts.

[lp:~fourdollars/unity-greeter/add-hidpi-support]

1473. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-16

Launchpad automatic translations update.

1474. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-17

Launchpad automatic translations update.

1475. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-18

Launchpad automatic translations update.

1476. By Robert Ancell on 2014-12-18

Ensure non-selected prompt boxes don't overlap adjacent boxes. This fixes a bug where you couldn't select usernames

1477. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-19

Launchpad automatic translations update.

1478. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-20

Launchpad automatic translations update.

1479. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-21

Launchpad automatic translations update.

1480. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-22

Launchpad automatic translations update.

1481. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-23

Launchpad automatic translations update.

1482. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-24

Launchpad automatic translations update.

1483. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-25

Launchpad automatic translations update.

1484. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-26

Launchpad automatic translations update.

1485. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-27

Launchpad automatic translations update.

1486. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-28

Launchpad automatic translations update.

1487. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-29

Launchpad automatic translations update.

1488. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-30

Launchpad automatic translations update.

1489. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-31

Launchpad automatic translations update.

1490. By Launchpad Translations on behalf of unity-greeter-team on 2015-01-01

Launchpad automatic translations update.

1491. By Launchpad Translations on behalf of unity-greeter-team on 2015-01-02

Launchpad automatic translations update.

1492. By Launchpad Translations on behalf of unity-greeter-team on 2015-01-03

Launchpad automatic translations update.

1493. By Launchpad Translations on behalf of unity-greeter-team on 2015-01-04

Launchpad automatic translations update.

1494. By Launchpad Translations on behalf of unity-greeter-team on 2015-01-05

Launchpad automatic translations update.

1495. By Launchpad Translations on behalf of unity-greeter-team on 2015-01-06

Launchpad automatic translations update.

1496. By Launchpad Translations on behalf of unity-greeter-team on 2015-01-07

Launchpad automatic translations update.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1497
http://jenkins.qa.ubuntu.com/job/unity-greeter-ci/56/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-greeter-vivid-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-greeter-vivid-armhf-ci/4/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-greeter-ci/56/rebuild

review: Needs Fixing (continuous-integration)

[Shih-Yuan Lee (fourdollars)]

Hi,

Could you help to review my revision?
I saw the failed of jenkins, but it doesn't seem relative to my change.

review: Needs Information

[Robert Ancell (robert-ancell)]

I don't understand the purpose of:

var builder = new StringBuilder ();
builder.append ("xwininfo -root -tree");
builder.append ("| awk '{print $1}'");
builder.append ("| grep ^0x");
builder.append ("| while read id; do xprop -id $id; done");
builder.append ("| grep _XSETTINGS_SETTINGS > /dev/null");

Why do we need to do this?

review: Needs Information

[Robert Ancell (robert-ancell)]

Does this obsolete https://code.launchpad.net/~woodrow-shen/unity-greeter/fix-hidpi-support/+merge/242046?

[Shih-Yuan Lee (fourdollars)]

Making https://code.launchpad.net/~woodrow-shen/unity-greeter/fix-hidpi-support/+merge/242046 obsolete should be OK.
It is up to Robert.

The StringBuilder is to create commands combo to check if the xsettings plugin of unity-settings-daemon is ready or not.

[Robert Ancell (robert-ancell)]

> The StringBuilder is to create commands combo to check if the xsettings plugin
> of unity-settings-daemon is ready or not.

That seems really hacky - how does Unity shell tell if the xsettings plugin is active? We should use the same method,

[Shih-Yuan Lee (fourdollars)]

So far as I know, Unity shell does nothing for the xsettings plugin.

[Robert Ancell (robert-ancell)]

Looking at unity-settings-daemon/gnome-settings-daemon/main.c

static void
name_acquired_handler (GDBusConnection *connection,
                       const gchar *name,
                       gpointer user_data)
{
        GDBusProxy *proxy;

        proxy = gnome_settings_session_get_session_proxy ();
#ifdef HAVE_IBUS
        set_legacy_ibus_env_vars (proxy);
#endif
        start_settings_manager ();
        register_with_gnome_session (proxy);
        watch_for_term_signal (manager);
}

And in unity-greeter/src/settings-daemon.vala:

[DBus (name="org.gnome.SessionManager")]
public class SessionManagerInterface : Object
{
    public bool session_is_active { get { return true; } }
    public string session_name { get { return "ubuntu"; } }
    public uint32 inhibited_actions { get { return 0; } }
}

So I think you should be able to add a register_client method and wait for "gnome-settings-daemon" to connect.

[Robert Ancell (robert-ancell)]

I think for Unity shell it's probably managed in gnome-session (i.e. the shell doesn't start until gnome-session has gnome-settings-daemon connect).

[Shih-Yuan Lee (fourdollars)]

Even unity-settings-daemon/gnome-settings-daemon is connected, it doesn't mean the xsettings plugin is ready.
Is there any way to check if the xsettings plugin is ready directly?

[Robert Ancell (robert-ancell)]

I don't know of a method for this, but:
- Does this mean there's a race condition if you start a GTK+ application in Unity before the xsettings plugin is ready? And we only see this in the greeter because we start quickly?
- Do we need to wait for the plugin? Shouldn't GTK+ detect when this setting appears/changes and adjust appropriately?

[Shih-Yuan Lee (fourdollars)]

- Yes, there is a race condition. We also see this issue in another bug #1382291.
- GTK+ will detect _XSETTINGS_SETTINGS in the first function call relative to the graphics. If it can not find _XSETTINGS_SETTINGS, it will use 1 as the scale factor by default when the scale factor is actually 2 in _XSETTINGS_SETTINGS.

[lp:~fourdollars/unity-greeter/add-hidpi-support]

1497. By Shih-Yuan Lee on 2015-02-13

Waiting for the xsettings plugin of unity-settings-daemon. (LP: #1286878)

[Robert Ancell (robert-ancell)]

The problem with this branch is the hack calling xwininfo from the the greeter before starting. This is risky because:
- We're calling a relatively complicated shell script from inside an executable (very hard to understand)
- We're blocking startup for this to complete (if something goes wrong we have no UI)

The changes without this hack will be accepted, then it might be easier to do a second MP to solve the race condition.

[Sebastien Bacher (seb128)]

@Shih-Yuan Lee, are we sure about the "the xsettings needs to be set before GTK starts"?

I just did that test
- start a wmaker session
- run gedit
- run unity-settings-daemon
- change the scaling-factor gsettings key

gedit scaled fine, obviously it was started before having a settings manager...

(that's on vivid)

[Sebastien Bacher (seb128)]

should that be rejected? the greeter correctly scale now in vivid with unity-settings-daemon

[Shih-Yuan Lee (fourdollars)]

@seb128,

I believe there is some improvement for the GTK+2/GTK+3 in vivid so we may not need this in vivid.

Unmerged revisions

1497. By Shih-Yuan Lee on 2015-02-13

Waiting for the xsettings plugin of unity-settings-daemon. (LP: #1286878)

=== modified file 'src/unity-greeter.vala'
--- src/unity-greeter.vala	2014-12-06 01:56:56 +0000
+++ src/unity-greeter.vala	2015-02-13 07:27:53 +0000
@@ -35,7 +35,7 @@
 
     private Cairo.XlibSurface background_surface;
 
-    private SettingsDaemon settings_daemon;
+    private static SettingsDaemon settings_daemon;
 
     public bool orca_needs_kick;
     private MainWindow main_window;
@@ -74,12 +74,6 @@
         if (!connected && !test_mode)
             Posix.exit (Posix.EXIT_FAILURE);
 
-        if (!test_mode)
-        {
-            settings_daemon = new SettingsDaemon ();
-            settings_daemon.start ();
-        }
-
         var state_dir = Path.build_filename (Environment.get_user_cache_dir (), "unity-greeter");
         DirUtils.create_with_parents (state_dir, 0775);
         
@@ -487,7 +481,6 @@
             warning ("Error starting the at-spi registry: %s", e.message);
         }
 
-        Gtk.init (ref args);
         Ido.init ();
 
         log_timer = new Timer ();
@@ -495,10 +488,6 @@
 
         debug ("Starting unity-greeter %s UID=%d LANG=%s", Config.VERSION, (int) Posix.getuid (), Environment.get_variable ("LANG"));
 
-        /* Set the cursor to not be the crap default */
-        debug ("Setting cursor");
-        Gdk.get_default_root_window ().set_cursor (new Gdk.Cursor (Gdk.CursorType.LEFT_PTR));
-
         bool do_show_version = false;
         bool do_test_mode = false;
         OptionEntry versionOption = { "version", 'v', 0, OptionArg.NONE, ref do_show_version,
@@ -535,7 +524,39 @@
         }
 
         if (do_test_mode)
+        {
             debug ("Running in test mode");
+        }
+        else
+        {
+            settings_daemon = new SettingsDaemon ();
+            settings_daemon.start ();
+
+            var builder = new StringBuilder ();
+            builder.append ("xwininfo -root -tree");
+            builder.append ("| awk '{print $1}'"); 
+            builder.append ("| grep ^0x");
+            builder.append ("| while read id; do xprop -id $id; done");
+            builder.append ("| grep _XSETTINGS_SETTINGS > /dev/null");
+
+            for (int i = 0; i < 3 && Posix.system(builder.str) != 0; i++)
+            {
+                var loop = new MainLoop ();
+                var time = new TimeoutSource (1000);
+                time.set_callback (() => {
+                    loop.quit ();
+                    return false;
+                });
+                time.attach (loop.get_context ());
+                loop.run ();
+            }
+        }
+
+        Gtk.init (ref args);
+
+        /* Set the cursor to not be the crap default */
+        debug ("Setting cursor");
+        Gdk.get_default_root_window ().set_cursor (new Gdk.Cursor (Gdk.CursorType.LEFT_PTR));
 
         /* Set GTK+ settings */
         debug ("Setting GTK+ settings");