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

Proposed by Shih-Yuan Lee on 2014-12-10
Status: Needs review
Proposed branch: lp:~fourdollars/unity-greeter/add-hidpi-support
Merge into: lp:unity-greeter
Diff against target: 84 lines (+33/-12)
1 file modified
src/unity-greeter.vala (+33/-12)
To merge this branch: bzr merge lp:~fourdollars/unity-greeter/add-hidpi-support
Reviewer Review Type Date Requested Status
Robert Ancell 2014-12-10 Needs Information on 2015-02-11
Shih-Yuan Lee (community) Needs Information on 2015-01-14
PS Jenkins bot (community) continuous-integration Needs Fixing on 2015-01-07
Review via email: mp+244266@code.launchpad.net

Commit message

Add HiDPI display support.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :

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)
1468. By Launchpad Translations on behalf of unity-greeter-team on 2014-12-11

Launchpad automatic translations update.

Robert Ancell (robert-ancell) wrote :

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) wrote :

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

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) wrote :

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.

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

Launchpad automatic translations update.

Robert Ancell (robert-ancell) wrote :

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.

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) wrote :
review: Needs Fixing (continuous-integration)
Shih-Yuan Lee (fourdollars) wrote :

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) wrote :

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
Shih-Yuan Lee (fourdollars) wrote :

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) wrote :

> 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) wrote :

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

Robert Ancell (robert-ancell) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

- 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.

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

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

Robert Ancell (robert-ancell) wrote :

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) wrote :

@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) wrote :

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

Shih-Yuan Lee (fourdollars) wrote :

@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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/unity-greeter.vala'
2--- src/unity-greeter.vala 2014-12-06 01:56:56 +0000
3+++ src/unity-greeter.vala 2015-02-13 07:27:53 +0000
4@@ -35,7 +35,7 @@
5
6 private Cairo.XlibSurface background_surface;
7
8- private SettingsDaemon settings_daemon;
9+ private static SettingsDaemon settings_daemon;
10
11 public bool orca_needs_kick;
12 private MainWindow main_window;
13@@ -74,12 +74,6 @@
14 if (!connected && !test_mode)
15 Posix.exit (Posix.EXIT_FAILURE);
16
17- if (!test_mode)
18- {
19- settings_daemon = new SettingsDaemon ();
20- settings_daemon.start ();
21- }
22-
23 var state_dir = Path.build_filename (Environment.get_user_cache_dir (), "unity-greeter");
24 DirUtils.create_with_parents (state_dir, 0775);
25
26@@ -487,7 +481,6 @@
27 warning ("Error starting the at-spi registry: %s", e.message);
28 }
29
30- Gtk.init (ref args);
31 Ido.init ();
32
33 log_timer = new Timer ();
34@@ -495,10 +488,6 @@
35
36 debug ("Starting unity-greeter %s UID=%d LANG=%s", Config.VERSION, (int) Posix.getuid (), Environment.get_variable ("LANG"));
37
38- /* Set the cursor to not be the crap default */
39- debug ("Setting cursor");
40- Gdk.get_default_root_window ().set_cursor (new Gdk.Cursor (Gdk.CursorType.LEFT_PTR));
41-
42 bool do_show_version = false;
43 bool do_test_mode = false;
44 OptionEntry versionOption = { "version", 'v', 0, OptionArg.NONE, ref do_show_version,
45@@ -535,7 +524,39 @@
46 }
47
48 if (do_test_mode)
49+ {
50 debug ("Running in test mode");
51+ }
52+ else
53+ {
54+ settings_daemon = new SettingsDaemon ();
55+ settings_daemon.start ();
56+
57+ var builder = new StringBuilder ();
58+ builder.append ("xwininfo -root -tree");
59+ builder.append ("| awk '{print $1}'");
60+ builder.append ("| grep ^0x");
61+ builder.append ("| while read id; do xprop -id $id; done");
62+ builder.append ("| grep _XSETTINGS_SETTINGS > /dev/null");
63+
64+ for (int i = 0; i < 3 && Posix.system(builder.str) != 0; i++)
65+ {
66+ var loop = new MainLoop ();
67+ var time = new TimeoutSource (1000);
68+ time.set_callback (() => {
69+ loop.quit ();
70+ return false;
71+ });
72+ time.attach (loop.get_context ());
73+ loop.run ();
74+ }
75+ }
76+
77+ Gtk.init (ref args);
78+
79+ /* Set the cursor to not be the crap default */
80+ debug ("Setting cursor");
81+ Gdk.get_default_root_window ().set_cursor (new Gdk.Cursor (Gdk.CursorType.LEFT_PTR));
82
83 /* Set GTK+ settings */
84 debug ("Setting GTK+ settings");

Subscribers

People subscribed via source and target branches