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

Proposed by William Hua
Status: Merged
Approved by: Sebastien Bacher
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
PS Jenkins bot (community) continuous-integration Approve
Unity Settings Daemon Development Team 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.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
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)"

Revision history for this message
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

Update debian/changelog.

4017. By William Hua

Try again.

4018. By William Hua

Ubuntu has key grabber.

4019. By William Hua

Breaks pre-key-grabber Unity.

Revision history for this message
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.

Revision history for this message
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
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
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.

Revision history for this message
Sebastien Bacher (seb128) wrote :

that makes sense, thanks for the explanation!

review: Approve
Revision history for this message
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