Merge lp:~townsend/libertine/fix-focus-tracking into lp:libertine
- fix-focus-tracking
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Larry Price |
Approved revision: | 303 |
Merged at revision: | 300 |
Proposed branch: | lp:~townsend/libertine/fix-focus-tracking |
Merge into: | lp:libertine |
Diff against target: |
304 lines (+132/-63) 5 files modified
CMakeLists.txt (+0/-1) debian/control (+0/-1) pasted/CMakeLists.txt (+1/-1) pasted/pasted.cpp (+111/-57) pasted/pasted.h (+20/-3) |
To merge this branch: | bzr merge lp:~townsend/libertine/fix-focus-tracking |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Libertine CI Bot | continuous-integration | Approve | |
Larry Price | Approve | ||
Review via email: mp+305135@code.launchpad.net |
Commit message
pasted: Switch to using an X event based model with a worker thread instead of the QTimer polling method. This avoids races and missed focus status.
Description of the change
Libertine CI Bot (libertine-ci-bot) wrote : | # |
Libertine CI Bot (libertine-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:300
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Larry Price (larryprice) wrote : | # |
few easy inlines - i'll try to verify that it still works in the morning
Christopher Townsend (townsend) wrote : | # |
Thanks for reviewing. I addressed your question and will make some changes.
Libertine CI Bot (libertine-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:301
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Libertine CI Bot (libertine-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:302
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Larry Price (larryprice) wrote : | # |
some inline suggestions. wrapping up an sbuild of the arm version now to test on frieza.
- 303. By Christopher Townsend
-
Changes and fixes based on feedback.
Larry Price (larryprice) wrote : | # |
lgtm! very nice updates
Libertine CI Bot (libertine-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:303
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Preview Diff
1 | === modified file 'CMakeLists.txt' |
2 | --- CMakeLists.txt 2016-09-06 12:36:47 +0000 |
3 | +++ CMakeLists.txt 2016-09-08 18:47:27 +0000 |
4 | @@ -20,7 +20,6 @@ |
5 | find_package(Qt5Gui REQUIRED) |
6 | find_package(Qt5Quick REQUIRED) |
7 | find_package(Qt5Widgets REQUIRED) |
8 | -find_package(Qt5X11Extras REQUIRED) |
9 | find_package(Gettext REQUIRED) |
10 | find_package(Intltool REQUIRED) |
11 | find_package(X11 REQUIRED) |
12 | |
13 | === modified file 'debian/control' |
14 | --- debian/control 2016-09-06 12:36:47 +0000 |
15 | +++ debian/control 2016-09-08 18:47:27 +0000 |
16 | @@ -13,7 +13,6 @@ |
17 | libgirepository1.0-dev, |
18 | libglib2.0-dev, |
19 | libgtest-dev, |
20 | - libqt5x11extras5-dev, |
21 | libx11-dev, |
22 | lsb-release, |
23 | python3-apt, |
24 | |
25 | === modified file 'pasted/CMakeLists.txt' |
26 | --- pasted/CMakeLists.txt 2016-08-10 12:23:31 +0000 |
27 | +++ pasted/CMakeLists.txt 2016-09-08 18:47:27 +0000 |
28 | @@ -6,5 +6,5 @@ |
29 | |
30 | qt5_use_modules(pasted DBus) |
31 | |
32 | -target_link_libraries(pasted Qt5::Core Qt5::Gui Qt5::Widgets Qt5::X11Extras content-hub X11) |
33 | +target_link_libraries(pasted Qt5::Core Qt5::Gui Qt5::Widgets content-hub X11) |
34 | install(TARGETS pasted RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}) |
35 | |
36 | === modified file 'pasted/pasted.cpp' |
37 | --- pasted/pasted.cpp 2016-09-01 20:01:52 +0000 |
38 | +++ pasted/pasted.cpp 2016-09-08 18:47:27 +0000 |
39 | @@ -19,12 +19,102 @@ |
40 | |
41 | #include "pasted.h" |
42 | |
43 | +#include <QDBusInterface> |
44 | #include <QDebug> |
45 | +#include <QThread> |
46 | |
47 | -#include <QX11Info> |
48 | #include <X11/Xatom.h> |
49 | |
50 | |
51 | +namespace |
52 | +{ |
53 | + |
54 | +constexpr auto MIR_WM_PERSISTENT_ID = "_MIR_WM_PERSISTENT_ID"; |
55 | +constexpr auto UNITY_FOCUSINFO_SERVICE = "com.canonical.Unity.FocusInfo"; |
56 | +constexpr auto UNITY_FOCUSINFO_PATH = "/com/canonical/Unity/FocusInfo"; |
57 | +constexpr auto UNITY_FOCUSINFO_INTERFACE = "com.canonical.Unity.FocusInfo"; |
58 | +constexpr auto UNITY_FOCUSINFO_METHOD = "isSurfaceFocused"; |
59 | + |
60 | + |
61 | +static QString getPersistentSurfaceId() |
62 | +{ |
63 | + Display *dpy = XOpenDisplay(NULL); |
64 | + Atom prop = XInternAtom(dpy, MIR_WM_PERSISTENT_ID, 0), |
65 | + type; // unused |
66 | + int form, // unused |
67 | + status; |
68 | + unsigned long remain, // unused |
69 | + len; // unused |
70 | + unsigned char *data = nullptr; |
71 | + QString persistentSurfaceId; |
72 | + |
73 | + status = XGetWindowProperty(dpy, XDefaultRootWindow(dpy), prop, 0, 1024, 0, |
74 | + XA_STRING, &type, &form, &len, &remain, &data); |
75 | + |
76 | + if (status) |
77 | + { |
78 | + qDebug() << "Failure retrieving the persistentSurfaceID!"; |
79 | + } |
80 | + else |
81 | + { |
82 | + persistentSurfaceId = (const char *)data; |
83 | + } |
84 | + |
85 | + XCloseDisplay(dpy); |
86 | + XFree(data); |
87 | + |
88 | + return persistentSurfaceId; |
89 | +} |
90 | + |
91 | +} //anonymous namespace |
92 | + |
93 | + |
94 | +void XEventWorker:: |
95 | +checkForAppFocus() |
96 | +{ |
97 | + bool hasFocus = false; |
98 | + XEvent event; |
99 | + |
100 | + QDBusInterface *unityFocus = new QDBusInterface(UNITY_FOCUSINFO_SERVICE, |
101 | + UNITY_FOCUSINFO_PATH, |
102 | + UNITY_FOCUSINFO_INTERFACE, |
103 | + QDBusConnection::sessionBus(), |
104 | + this); |
105 | + |
106 | + QString surfaceId = getPersistentSurfaceId(); |
107 | + |
108 | + QDBusReply<bool> isFocused = unityFocus->call(UNITY_FOCUSINFO_METHOD, surfaceId); |
109 | + |
110 | + if (isFocused == true) |
111 | + { |
112 | + focusChanged(); |
113 | + hasFocus = true; |
114 | + } |
115 | + |
116 | + Display *dpy = XOpenDisplay(NULL); |
117 | + XSelectInput(dpy, XDefaultRootWindow(dpy), FocusChangeMask); |
118 | + |
119 | + while (1) |
120 | + { |
121 | + XNextEvent(dpy, &event); |
122 | + |
123 | + isFocused = unityFocus->call(UNITY_FOCUSINFO_METHOD, surfaceId); |
124 | + |
125 | + if (hasFocus == false && isFocused == true) |
126 | + { |
127 | + qDebug() << "Surface is focused"; |
128 | + focusChanged(); |
129 | + hasFocus = true; |
130 | + } |
131 | + else if (hasFocus == true && isFocused == false) |
132 | + { |
133 | + qDebug() << "Surface lost focus"; |
134 | + hasFocus = false; |
135 | + } |
136 | + } |
137 | +} |
138 | + |
139 | + |
140 | Pasted:: |
141 | Pasted(int argc, char** argv) |
142 | : QApplication(argc, argv) |
143 | @@ -32,15 +122,9 @@ |
144 | , content_hub_(cuc::Hub::Client::instance()) |
145 | , mimeDataX_(new QMimeData) |
146 | , lastMimeData_() |
147 | -, rootWindowHasFocus_(false) |
148 | -, firstSeenWindow_(None) |
149 | { |
150 | setApplicationName("pasted"); |
151 | |
152 | - QTimer *timer = new QTimer(this); |
153 | - connect(timer, &QTimer::timeout, this, &Pasted::checkForAppFocus); |
154 | - timer->start(400); |
155 | - |
156 | connect(clipboard_, &QClipboard::dataChanged, this, &Pasted::handleXClipboard); |
157 | } |
158 | |
159 | @@ -81,36 +165,6 @@ |
160 | } |
161 | |
162 | |
163 | -void Pasted:: |
164 | -checkForAppFocus() |
165 | -{ |
166 | - Window w; |
167 | - int revert_to; // unused |
168 | - |
169 | - XGetInputFocus(QX11Info::display(), &w, &revert_to); |
170 | - |
171 | - if (firstSeenWindow_ == None && w != None) |
172 | - { |
173 | - firstSeenWindow_ = w; |
174 | - } |
175 | - |
176 | - if (w == None && rootWindowHasFocus_ == true) |
177 | - { |
178 | - qDebug() << "Xmir lost focus"; |
179 | - rootWindowHasFocus_ = false; |
180 | - } |
181 | - else if ((w == PointerRoot || |
182 | - (w && firstSeenWindow_ == PointerRoot)) |
183 | - && rootWindowHasFocus_ == false) |
184 | - { |
185 | - qDebug() << "Xmir gained focus"; |
186 | - rootWindowHasFocus_ = true; |
187 | - setPersistentSurfaceId(); |
188 | - handleContentHubPasteboard(); |
189 | - } |
190 | -} |
191 | - |
192 | - |
193 | bool Pasted:: |
194 | compareMimeData(const QMimeData *a, const QMimeData *b) |
195 | { |
196 | @@ -180,29 +234,19 @@ |
197 | { |
198 | if (persistentSurfaceId_.isEmpty()) |
199 | { |
200 | - Atom prop = XInternAtom(QX11Info::display(), "_MIR_WM_PERSISTENT_ID", 0), |
201 | - type; // unused |
202 | - int form, // unused |
203 | - status; |
204 | - unsigned long remain, // unused |
205 | - len; // unused |
206 | - unsigned char *data; |
207 | - status = XGetWindowProperty(QX11Info::display(), XDefaultRootWindow(QX11Info::display()), prop, |
208 | - 0, 1024, 0, XA_STRING, &type, &form, &len, &remain, &data); |
209 | - |
210 | - if (status) |
211 | - { |
212 | - qDebug() << "Failure retrieving the persistentSurfaceID!"; |
213 | - } |
214 | - else |
215 | - { |
216 | - qDebug() << "Setting persistentSurfaceId"; |
217 | - persistentSurfaceId_ = (const char *)data; |
218 | - } |
219 | + persistentSurfaceId_ = getPersistentSurfaceId(); |
220 | } |
221 | } |
222 | |
223 | |
224 | +void Pasted:: |
225 | +appFocused() |
226 | +{ |
227 | + setPersistentSurfaceId(); |
228 | + handleContentHubPasteboard(); |
229 | +} |
230 | + |
231 | + |
232 | int |
233 | main(int argc, char* argv[]) |
234 | { |
235 | @@ -210,5 +254,15 @@ |
236 | |
237 | Pasted pasted(argc, argv); |
238 | |
239 | - pasted.exec(); |
240 | + QThread t; |
241 | + XEventWorker worker; |
242 | + |
243 | + worker.moveToThread(&t); |
244 | + |
245 | + QObject::connect(&worker, &XEventWorker::focusChanged, &pasted, &Pasted::appFocused); |
246 | + QObject::connect(&t, &QThread::started, &worker, &XEventWorker::checkForAppFocus); |
247 | + |
248 | + t.start(); |
249 | + |
250 | + return pasted.exec(); |
251 | } |
252 | |
253 | === modified file 'pasted/pasted.h' |
254 | --- pasted/pasted.h 2016-08-29 15:33:05 +0000 |
255 | +++ pasted/pasted.h 2016-09-08 18:47:27 +0000 |
256 | @@ -33,6 +33,23 @@ |
257 | namespace cuc = com::ubuntu::content; |
258 | |
259 | |
260 | +class XEventWorker |
261 | +: public QObject |
262 | +{ |
263 | + Q_OBJECT |
264 | + |
265 | + public: |
266 | + XEventWorker() = default; |
267 | + virtual ~XEventWorker() = default; |
268 | + |
269 | + signals: |
270 | + void focusChanged(); |
271 | + |
272 | + public slots: |
273 | + void checkForAppFocus(); |
274 | +}; |
275 | + |
276 | + |
277 | class Pasted |
278 | : public QApplication |
279 | { |
280 | @@ -42,6 +59,9 @@ |
281 | Pasted(int argc, char** argv); |
282 | virtual ~Pasted() = default; |
283 | |
284 | + public slots: |
285 | + void appFocused(); |
286 | + |
287 | private: |
288 | void updateLastMimeData(const QMimeData *source); |
289 | void updateXMimeData(const QMimeData *source); |
290 | @@ -53,15 +73,12 @@ |
291 | |
292 | private slots: |
293 | void handleXClipboard(); |
294 | - void checkForAppFocus(); |
295 | |
296 | private: |
297 | QClipboard *clipboard_; |
298 | cuc::Hub *content_hub_; |
299 | QMimeData *mimeDataX_; |
300 | std::unique_ptr<QMimeData> lastMimeData_; |
301 | - bool rootWindowHasFocus_; |
302 | - Window firstSeenWindow_; |
303 | QString persistentSurfaceId_; |
304 | }; |
305 |
FAILED: Continuous integration, rev:300 /jenkins. canonical. com/libertine/ job/lp- libertine- ci/120/ /jenkins. canonical. com/libertine/ job/build/ 332 /jenkins. canonical. com/libertine/ job/test- 0-autopkgtest/ label=amd64, release= vivid+overlay, testname= default/ 265 /jenkins. canonical. com/libertine/ job/test- 0-autopkgtest/ label=amd64, release= xenial+ overlay, testname= default/ 265/console /jenkins. canonical. com/libertine/ job/test- 0-autopkgtest/ label=amd64, release= yakkety, testname= default/ 265 /jenkins. canonical. com/libertine/ job/test- 0-autopkgtest/ label=i386, release= vivid+overlay, testname= default/ 265 /jenkins. canonical. com/libertine/ job/test- 0-autopkgtest/ label=i386, release= xenial+ overlay, testname= default/ 265 /jenkins. canonical. com/libertine/ job/test- 0-autopkgtest/ label=i386, release= yakkety, testname= default/ 265 /jenkins. canonical. com/libertine/ job/lp- generic- update- mp/252/ console /jenkins. canonical. com/libertine/ job/build- 0-fetch/ 334 /jenkins. canonical. com/libertine/ job/build- 1-sourcepkg/ release= vivid+overlay/ 317 /jenkins. canonical. com/libertine/ job/build- 1-sourcepkg/ release= xenial+ overlay/ 317 /jenkins. canonical. com/libertine/ job/build- 1-sourcepkg/ release= yakkety/ 317 /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=amd64, release= vivid+overlay/ 316 /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=amd64, release= vivid+overlay/ 316/artifact/ output/ *zip*/output. zip /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=amd64, release= xenial+ overlay/ 316 /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=amd64, release= xenial+ overlay/ 316/artifact/ output/ *zip*/output. zip /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=amd64, release= yakkety/ 316 /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=amd64, release= yakkety/ 316/artifact/ output/ *zip*/output. zip /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=i386, release= vivid+overlay/ 316 /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=i386, release= vivid+overlay/ 316/artifact/ output/ *zip*/output. zip /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=i386, release= xenial+ overlay/ 316 /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=i386, release= xenial+ overlay/ 316/artifact/ output/ *zip*/output. zip /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=i386, release= yakkety/ 316 /jenkins. canonical. com/libertine/ job/build- 2-binpkg/ arch=i386, release= yakkety/ 316/artifact/ output/ *zip*/output. zip
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild: /jenkins. canonical. com/libertine/ job/lp- libertine- ci/120/ rebuild
https:/