Merge lp:~attente/unity-settings-daemon/gnome-key-grabber into lp:unity-settings-daemon

Proposed by William Hua on 2014-02-12
Status: Merged
Approved by: Sebastien Bacher on 2014-02-18
Approved revision: 4019
Merged at revision: 4015
Proposed branch: lp:~attente/unity-settings-daemon/gnome-key-grabber
Merge into: lp:unity-settings-daemon
Diff against target: 114 lines (+20/-11)
2 files modified
debian/control (+1/-0)
plugins/media-keys/gsd-media-keys-manager.c (+19/-11)
To merge this branch: bzr merge lp:~attente/unity-settings-daemon/gnome-key-grabber
Reviewer Review Type Date Requested Status
Sebastien Bacher Approve on 2014-02-18
PS Jenkins bot (community) continuous-integration Approve on 2014-02-17
Unity Settings Daemon Development Team 2014-02-12 Pending
Review via email: mp+206033@code.launchpad.net

Commit message

Revert the legacy key grabber.

Description of the change

(This branch depends on https://code.launchpad.net/~attente/unity/gnome-key-grabber/+merge/202755 being released)

Revert the legacy key grabber.

To post a comment you must log in.
Dimitri John Ledkov (xnox) wrote :

Looks good, once a sensible version of unity with matching change is known, unity-settings-daemon needs "Breaks: unity (<< FIRST_VERSION_WITH_KEY_GRABS)"

Sebastien Bacher (seb128) wrote :

Thanks for the work!

There were some discussions last week, on #ubutu-desktop about that (see the irclog if you are interested into the details), turns out that some of the flavors/sessions would like to keep using the legacy grabber.

It looks like keeping the codepath for the LTS should be reasonable low cost (feel free to comment on that if you disagree, we might have overlooked details/issues), could you change the merge to keep the code but make it used only out of GNOME and Unity?

review: Needs Fixing
4016. By William Hua on 2014-02-14

Update debian/changelog.

4017. By William Hua on 2014-02-17

Try again.

4018. By William Hua on 2014-02-17

Ubuntu has key grabber.

4019. By William Hua on 2014-02-17

Breaks pre-key-grabber Unity.

William Hua (attente) wrote :

Ok, thanks for clarifying, I guess I misunderstood before because I thought u-s-d would only be available under Unity, and other DEs would be using the less patched g-s-d.

Sebastien Bacher (seb128) wrote :

You didn't misunderstood, we though it would be the case, but it turned out that some flavors/session feel like they are closer from unity (they use indicators, they don't want the new GtkHeaderBars, they want menus) ... so we are revisiting the approch and going to make those sessions be variant of Unity rather than GNOME.

Thanks for updating it

One small comment, we usually do check for XDG_CURRENT_DESKTOP=GNOME|Unity, is there any reason to look at DESKTOP_SESSION there?

review: Needs Information
William Hua (attente) wrote :

So, the code in that file originally used DESKTOP_SESSION. But if we use XDG_CURRENT_DESKTOP, then there is no way to differentiate between GNOME Shell, GNOME Classic, and the two GNOME Fallback sessions. These are the values for each DE:

[Desktop: DESKTOP_SESSION, XDG_CURRENT_DESKTOP]
GNOME Shell: gnome, GNOME
GNOME Classic: gnome-classic, GNOME
GNOME Flashback (Compiz): gnome-fallback-compiz, GNOME
GNOME Flashback (Metacity): gnome-fallback, GNOME
Unity: ubuntu, Unity

So the question is if GNOME Classic and GNOME Fallback should use the legacy key grabber. If yes, we should use DESKTOP_SESSION, if not, we can use XDG_CURRENT_DESKTOP.

Sebastien Bacher (seb128) wrote :

that makes sense, thanks for the explanation!

review: Approve
Sebastien Bacher (seb128) wrote :

Works fine here, thanks

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2014-02-07 14:57:09 +0000
3+++ debian/control 2014-02-17 16:37:48 +0000
4@@ -61,6 +61,7 @@
5 gnome-session (<< 3.9.90-0ubuntu6),
6 gnome-screensaver (<< 2.28.0),
7 gnome-color-manager (<< 3.0),
8+ unity (<< 7.1.2+14.04.20140214.1-0ubuntu1),
9 unity-greeter (<< 0.2.1-0ubuntu1),
10 indicator-datetime (<< 12.10.3daily13.03.26)
11 Suggests: x11-xserver-utils,
12
13=== modified file 'plugins/media-keys/gsd-media-keys-manager.c'
14--- plugins/media-keys/gsd-media-keys-manager.c 2013-11-13 02:03:10 +0000
15+++ plugins/media-keys/gsd-media-keys-manager.c 2014-02-17 16:37:48 +0000
16@@ -741,8 +741,8 @@
17 }
18
19 static gboolean
20-grab_media_key_unity (MediaKey *key,
21- GsdMediaKeysManager *manager)
22+grab_media_key_legacy (MediaKey *key,
23+ GsdMediaKeysManager *manager)
24 {
25 char *tmp;
26 gboolean need_flush;
27@@ -832,7 +832,7 @@
28 if (!manager->priv->have_legacy_keygrabber)
29 grab_media_key (key, manager);
30 else {
31- if (grab_media_key_unity (key, manager))
32+ if (grab_media_key_legacy (key, manager))
33 need_flush = TRUE;
34 }
35 break;
36@@ -997,7 +997,7 @@
37 g_ptr_array_add (manager->priv->keys, key);
38
39 if (manager->priv->have_legacy_keygrabber)
40- grab_media_key_unity (key, manager);
41+ grab_media_key_legacy (key, manager);
42 }
43
44 static void
45@@ -1038,7 +1038,7 @@
46 g_ptr_array_add (manager->priv->keys, key);
47
48 if (manager->priv->have_legacy_keygrabber)
49- grab_media_key_unity (key, manager);
50+ grab_media_key_legacy (key, manager);
51 }
52 g_strfreev (custom_paths);
53
54@@ -2123,8 +2123,9 @@
55 GVariant *sources;
56 gint i, n;
57
58- if (!manager->priv->have_legacy_keygrabber)
59- return;
60+ if (g_strcmp0 (g_getenv ("DESKTOP_SESSION"), "ubuntu") != 0)
61+ if (!manager->priv->have_legacy_keygrabber)
62+ return;
63
64 settings = g_settings_new (GNOME_DESKTOP_INPUT_SOURCES_DIR);
65 sources = g_settings_get_value (settings, KEY_INPUT_SOURCES);
66@@ -2755,13 +2756,20 @@
67 init_kbd (manager);
68 }
69
70+static gboolean
71+session_has_key_grabber (void)
72+{
73+ const gchar *session = g_getenv ("DESKTOP_SESSION");
74+ return g_strcmp0 (session, "gnome") == 0 || g_strcmp0 (session, "ubuntu") == 0;
75+}
76+
77 static void
78 on_shell_appeared (GDBusConnection *connection,
79 const char *name,
80 const char *name_owner,
81 gpointer user_data)
82 {
83- if (g_strcmp0 (g_getenv ("DESKTOP_SESSION"), "gnome") != 0)
84+ if (!session_has_key_grabber ())
85 return;
86
87 GsdMediaKeysManager *manager = user_data;
88@@ -2787,7 +2795,7 @@
89 const char *name,
90 gpointer user_data)
91 {
92- if (g_strcmp0 (g_getenv ("DESKTOP_SESSION"), "gnome") != 0)
93+ if (!session_has_key_grabber ())
94 return;
95
96 GsdMediaKeysManager *manager = user_data;
97@@ -2807,7 +2815,7 @@
98 GsdMediaKeysManager *manager = user_data;
99 GSList *l;
100
101- if (g_strcmp0 (g_getenv ("DESKTOP_SESSION"), "gnome") == 0)
102+ if (session_has_key_grabber ())
103 return;
104
105 manager->priv->have_legacy_keygrabber = TRUE;
106@@ -2844,7 +2852,7 @@
107 {
108 GsdMediaKeysManager *manager = user_data;
109
110- if (g_strcmp0 (g_getenv ("DESKTOP_SESSION"), "gnome") == 0)
111+ if (session_has_key_grabber ())
112 return;
113
114 manager->priv->have_legacy_keygrabber = FALSE;

Subscribers

People subscribed via source and target branches