Merge lp:~fkrull/unity-2d/compiz-integration into lp:unity-2d/3.0

Proposed by Felix Krull
Status: Merged
Merge reported by: Florian Boucault
Merged at revision: not available
Proposed branch: lp:~fkrull/unity-2d/compiz-integration
Merge into: lp:unity-2d/3.0
Diff against target: 168 lines (+76/-34)
3 files modified
launcher/UnityApplications/launcherapplication.cpp (+34/-12)
launcher/UnityApplications/workspaces.cpp (+20/-9)
launcher/app/launcherview.cpp (+22/-13)
To merge this branch: bzr merge lp:~fkrull/unity-2d/compiz-integration
Reviewer Review Type Date Requested Status
Florian Boucault (community) code Approve
Review via email: mp+57896@code.launchpad.net

Description of the change

For the workspaces and window overviews, trigger Compiz's Expo and Scale plugins if Compiz is running. If Compiz isn't running, said plugins aren't enabled or the D-Bus plugin isn't enabled, fall back to using unity-2d-spread.

See also https://bugs.launchpad.net/unity-2d/+bug/760674

How to test:
Log into Unity 2D. Run `compiz --replace`. Using ccsm, make sure that the Expo, Scale and D-Bus plugins for Compiz are enabled. Trigger the workspace or window overview.

To post a comment you must log in.
534. By Felix Krull

Also invoke Expo plugin on Super+S keypress.

Revision history for this message
Florian Boucault (fboucault) wrote :

Although the code looks generally ok, I am unable to test it for a lack of D-Bus service 'org.freedesktop.compiz'. I carefully activated Expo, Scale and D-Bus plugins in ccsm. Any tips?

Revision history for this message
Florian Boucault (fboucault) wrote :

Note there is a conflict with trunk.

review: Needs Fixing
Revision history for this message
Felix Krull (fkrull) wrote :

I can't really reproduce your problem regarding D-Bus. Having just enabled the plugin, I can see that service just fine (in qdbusviewer). Anyway, how would I resolve that conflict with trunk? I tried to rebase, but couldn't push that -- obviously, really. Or would I just pull and merge from trunk?

Revision history for this message
Olivier Tilloy (osomon) wrote :

Felix, you’ll need to merge the latest trunk into your branch and solve the conflict(s) there. Once this is done, commit and push to lp:~fkrull/unity-2d/compiz-integration. Thanks!

535. By Felix Krull

Merged trunk (r557).

536. By Felix Krull

Merged r560 from trunk.

Revision history for this message
Felix Krull (fkrull) wrote :

I've resolved any conflicts, but I noticed another problem with the code: Activating the window overview for an application doesn't work correctly using the Super+<Number> shortcut keys.

a) The overview doesn't appear as soon as you press the number button (and toggles quickly when kept pressed), but only when you release it. Also, one has to keep the keys pressed for a time (half a second, maybe), otherwise nothing happens.
b) It only affects the keyboard shortcuts. Activating the overview by clicking on an icon in the launcher works perfectly. Also, holding Super and then clicking an icon works as well.
c) It only affects the window overview with Compiz's Scale plugin. Switching to the application when it is not focused works, as does activating the unity-2d-spread 2D overview, *as does activating Compiz's Expo plugin*.

I have honestly no idea what this is about. I can only guess that there is some weird quirk or bug with the Scale plugin. I don't think it's a bug in my code, because I can't imagine where that would come from. Anyway, with that in mind, you might or might not want to actually merge this; I think it's OK apart from this one problem.

(Do I need to select resubmit here or anything?)

Revision history for this message
Olivier Tilloy (osomon) wrote :

No need for a resubmit or any additional interaction on your part Felix. Thanks for your work, we’ll make sure to look into it soon.

Revision history for this message
Florian Boucault (fboucault) wrote :

After resolving some new conflicts that arose with the latest trunk I performed some functional testing and I must say that it works really well! Even the Super+<Number> shortcut keys :)

I will finish checking a bit more the code and then it's good to go. Well done Felix!

review: Approve (functional)
Revision history for this message
Florian Boucault (fboucault) wrote :

Merge with trunk was done in branch: lp:~fboucault/unity-2d/compiz-integration

Revision history for this message
Florian Boucault (fboucault) wrote :

Felix, I think the code is good. I added one simplification for which I would love to have your feedback on. It's in the last revision of lp:~fboucault/unity-2d/compiz-integration

If that change is ok with you then I suggest we merge the whole thing in trunk.

review: Approve (code)
Revision history for this message
Felix Krull (fkrull) wrote :

Yeah, sure, no problem with that ;) I'm not really familiar with Qt, so I just guessed there; your variant is certainly simpler.

The number shortcuts consistently don't work as I described both on my main system and in a clean Natty VM. If it works for you, did you do anything weird or do you have a different Compiz or something?

Revision history for this message
Florian Boucault (fboucault) wrote :

Thanks Felix, it's now merged in trunk and will be part of tomorrow's daily build.

I don't think I have any strange Compiz version or configuration. In case that was not clear, I am using my laptop's keyboard with no external keyboard connected and pressing the number keys that are above the letters.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/UnityApplications/launcherapplication.cpp'
2--- launcher/UnityApplications/launcherapplication.cpp 2011-04-28 13:09:25 +0000
3+++ launcher/UnityApplications/launcherapplication.cpp 2011-05-14 15:59:28 +0000
4@@ -50,6 +50,7 @@
5 #include <debug_p.h>
6
7 // Qt
8+#include <Qt>
9 #include <QDebug>
10 #include <QAction>
11 #include <QDBusInterface>
12@@ -57,6 +58,7 @@
13 #include <QFile>
14 #include <QFileSystemWatcher>
15 #include <QScopedPointer>
16+#include <QX11Info>
17
18 extern "C" {
19 #include <libsn/sn.h>
20@@ -767,21 +769,41 @@
21 void
22 LauncherApplication::spread(bool showAllWorkspaces)
23 {
24- QDBusInterface iface("com.canonical.Unity2d.Spread", "/Spread",
25- "com.canonical.Unity2d.Spread");
26- QDBusReply<bool> isShown = iface.call("IsShown");
27- if (isShown.isValid()) {
28- if (isShown.value() == true) {
29- iface.asyncCall("FilterByApplication", m_application->desktop_file());
30- } else {
31- if (showAllWorkspaces) {
32- iface.asyncCall("ShowAllWorkspaces", m_application->desktop_file());
33+ QDBusInterface compiz("org.freedesktop.compiz",
34+ "/org/freedesktop/compiz/scale/screen0/initiate_all_key",
35+ "org.freedesktop.compiz");
36+
37+ if (compiz.isValid()) {
38+ Qt::HANDLE root = QX11Info::appRootWindow();
39+ BamfUintList* xids = m_application->xids();
40+ QString match = "";
41+ for (int i = 0; i < xids->size(); i++) {
42+ uint xid = xids->at(i);
43+ match.append("xid=");
44+ QVariant tmp(xid);
45+ match.append(tmp.toString());
46+ if (i < xids->size() - 1) {
47+ match.append(" | ");
48+ }
49+ }
50+ compiz.asyncCall("activate", "root", static_cast<int>(root), "match", match);
51+ } else {
52+ QDBusInterface spread("com.canonical.Unity2d.Spread", "/Spread",
53+ "com.canonical.Unity2d.Spread");
54+ QDBusReply<bool> isShown = spread.call("IsShown");
55+ if (isShown.isValid()) {
56+ if (isShown.value() == true) {
57+ spread.asyncCall("FilterByApplication", m_application->desktop_file());
58 } else {
59- iface.asyncCall("ShowCurrentWorkspace", m_application->desktop_file());
60+ if (showAllWorkspaces) {
61+ spread.asyncCall("ShowAllWorkspaces", m_application->desktop_file());
62+ } else {
63+ spread.asyncCall("ShowCurrentWorkspace", m_application->desktop_file());
64+ }
65 }
66+ } else {
67+ UQ_WARNING << "Failed to get property IsShown on com.canonical.Unity2d.Spread";
68 }
69- } else {
70- UQ_WARNING << "Failed to get property IsShown on com.canonical.Unity2d.Spread";
71 }
72 }
73
74
75=== modified file 'launcher/UnityApplications/workspaces.cpp'
76--- launcher/UnityApplications/workspaces.cpp 2011-04-28 13:09:25 +0000
77+++ launcher/UnityApplications/workspaces.cpp 2011-05-14 15:59:28 +0000
78@@ -23,6 +23,8 @@
79
80 #include <QDBusInterface>
81 #include <QDBusReply>
82+#include <Qt>
83+#include <QX11Info>
84
85 // libunity-2d
86 #include <unity2dtr.h>
87@@ -85,17 +87,26 @@
88 void
89 Workspaces::activate()
90 {
91- QDBusInterface iface("com.canonical.Unity2d.Spread", "/Spread",
92- "com.canonical.Unity2d.Spread");
93- QDBusReply<bool> isShown = iface.call("IsShown");
94- if (isShown.isValid()) {
95- if (isShown.value() == true) {
96- iface.asyncCall("FilterByApplication", QString());
97+ QDBusInterface compiz("org.freedesktop.compiz",
98+ "/org/freedesktop/compiz/expo/screen0/expo_key",
99+ "org.freedesktop.compiz");
100+
101+ if (compiz.isValid()) {
102+ Qt::HANDLE root = QX11Info::appRootWindow();
103+ compiz.asyncCall("activate", "root", static_cast<int>(root));
104+ } else {
105+ QDBusInterface spread("com.canonical.Unity2d.Spread", "/Spread",
106+ "com.canonical.Unity2d.Spread");
107+ QDBusReply<bool> isShown = spread.call("IsShown");
108+ if (isShown.isValid()) {
109+ if (isShown.value() == true) {
110+ spread.asyncCall("FilterByApplication", QString());
111+ } else {
112+ spread.asyncCall("ShowAllWorkspaces", QString());
113+ }
114 } else {
115- iface.asyncCall("ShowAllWorkspaces", QString());
116+ UQ_WARNING << "Failed to get property IsShown on com.canonical.Unity2d.Spread";
117 }
118- } else {
119- UQ_WARNING << "Failed to get property IsShown on com.canonical.Unity2d.Spread";
120 }
121 }
122
123
124=== modified file 'launcher/app/launcherview.cpp'
125--- launcher/app/launcherview.cpp 2011-05-03 18:34:05 +0000
126+++ launcher/app/launcherview.cpp 2011-05-14 15:59:28 +0000
127@@ -344,19 +344,28 @@
128 void
129 LauncherView::showWorkspaceSwitcher()
130 {
131- QDBusInterface spreadInterface(SPREAD_DBUS_SERVICE, SPREAD_DBUS_PATH, SPREAD_DBUS_INTERFACE);
132-
133- /* Here we only show the spread, if it's hidden.
134- However on Super+s the spread should exit if it's already running.
135- That is done directly in spread/Workspaces.qml because the spread
136- fully grabs the keyboard, so it's the only place where Super+s can
137- be handled while the spread is active */
138- QDBusReply<bool> isShown = spreadInterface.call("IsShown");
139- if (isShown.isValid()) {
140- if (isShown.value() == false) {
141- spreadInterface.asyncCall("ShowAllWorkspaces", QString());
142+ QDBusInterface compiz("org.freedesktop.compiz",
143+ "/org/freedesktop/compiz/expo/screen0/expo_key",
144+ "org.freedesktop.compiz");
145+
146+ if (compiz.isValid()) {
147+ Qt::HANDLE root = QX11Info::appRootWindow();
148+ compiz.asyncCall("activate", "root", static_cast<int>(root));
149+ } else {
150+ QDBusInterface spreadInterface(SPREAD_DBUS_SERVICE, SPREAD_DBUS_PATH, SPREAD_DBUS_INTERFACE);
151+
152+ /* Here we only show the spread, if it's hidden.
153+ However on Super+s the spread should exit if it's already running.
154+ That is done directly in spread/Workspaces.qml because the spread
155+ fully grabs the keyboard, so it's the only place where Super+s can
156+ be handled while the spread is active */
157+ QDBusReply<bool> isShown = spreadInterface.call("IsShown");
158+ if (isShown.isValid()) {
159+ if (isShown.value() == false) {
160+ spreadInterface.asyncCall("ShowAllWorkspaces", QString());
161+ }
162+ } else {
163+ UQ_WARNING << "Failed to get property IsShown on" << SPREAD_DBUS_SERVICE;
164 }
165- } else {
166- UQ_WARNING << "Failed to get property IsShown on" << SPREAD_DBUS_SERVICE;
167 }
168 }

Subscribers

People subscribed via source and target branches