Merge lp:~ris/unity-2d/fix-692444 into lp:unity-2d/3.0

Proposed by Robert Sajdok
Status: Merged
Merge reported by: Florian Boucault
Merged at revision: not available
Proposed branch: lp:~ris/unity-2d/fix-692444
Merge into: lp:unity-2d/3.0
Diff against target: 327 lines (+148/-62)
7 files modified
launcher/UnityApplications/CMakeLists.txt (+2/-0)
launcher/UnityApplications/launcherapplication.cpp (+2/-56)
launcher/UnityApplications/launcherapplication.h (+0/-2)
launcher/UnityApplications/launcherutility.cpp (+58/-0)
launcher/UnityApplications/launcherutility.h (+16/-0)
launcher/UnityApplications/trash.cpp (+66/-2)
launcher/UnityApplications/trash.h (+4/-2)
To merge this branch: bzr merge lp:~ris/unity-2d/fix-692444
Reviewer Review Type Date Requested Status
Florian Boucault (community) code Needs Fixing
Review via email: mp+57944@code.launchpad.net

Description of the change

[launcher] Match trash nautilus window with the trash item in the launcher.

To post a comment you must log in.
Florian Boucault (fboucault) wrote :

Thanks a lot for the work Robert. The technique you are using (similar to the one proposed for Unity 3D) looks pretty good. Unfortunately it does not work on my computer. I suspect this is due to the fact that I am running Ubuntu in French and the window name returned by wnck_window_get_name might be translated to "Corbeille" instead of "Trash".

I still have to do a deeper code review.

review: Needs Fixing (functional)
Florian Boucault (fboucault) wrote :

I confirm this previous statement: it fails because the window name is translated. But that's not the only reason, a pointer to a QString is created and is then passed to QString::compare which should fail:

54 + QString *windowName = new QString(wnck_window_get_name(wnckWindow));
55 +
56 + int found = QString::compare("Trash", windowName, Qt::CaseSensitive);

The following should work better:

QString windowName = QString(wnck_window_get_name(wnckWindow);

review: Needs Fixing (code)
Florian Boucault (fboucault) wrote :

Code in Trash::running() and Trash::show() is mostly duplicated. Code duplication could be avoided by creating a utility method that returns the first BamfWindow corresponding to the trash if it exists:

BamfWindow* firstWindow();

review: Needs Fixing (code)
Florian Boucault (fboucault) wrote :

Duplicating LauncherApplication::showWindow with Trash::showWindow is something that should be avoided as well. Same for LauncherApplication::moveViewportToWindow. One could think of a separate utility class/namespace where these 2 would go but for now it is just simpler to call LauncherApplication::showWindow directly from Trash's code.

review: Needs Fixing (code)
Robert Sajdok (ris) wrote :

> I confirm this previous statement: it fails because the window name is
> translated. But that's not the only reason, a pointer to a QString is created
> and is then passed to QString::compare which should fail:
>
> 54 + QString *windowName = new QString(wnck_window_get_name(wnckWindow));
> 55 +
> 56 + int found = QString::compare("Trash", windowName,
> Qt::CaseSensitive);
>
>
> The following should work better:
>
> QString windowName = QString(wnck_window_get_name(wnckWindow);

Try this version it should work fine.

Robert Sajdok (ris) wrote :

> Code in Trash::running() and Trash::show() is mostly duplicated. Code
> duplication could be avoided by creating a utility method that returns the
> first BamfWindow corresponding to the trash if it exists:
>
> BamfWindow* firstWindow();

Yes I know, but please note that the method "show ()" does not stop after finding the first window: "Trash". This method "show ()" shows all the windows, whose name is "Trash"

Robert Sajdok (ris) wrote :

> Duplicating LauncherApplication::showWindow with Trash::showWindow is
> something that should be avoided as well. Same for
> LauncherApplication::moveViewportToWindow. One could think of a separate
> utility class/namespace where these 2 would go but for now it is just simpler
> to call LauncherApplication::showWindow directly from Trash's code.

Done.

Florian Boucault (fboucault) wrote :

Thanks a lot for the fixes Robert. Sorry for having taken so long to reply. The Ubuntu Developer Summit and related meetings took all my attention away.

I added more fixes to your code in lp:~fboucault/unity-2d/trash_singleton
If you can have a look and check that you are happy with that, we should merge it.

Robert Sajdok (ris) wrote :

> Thanks a lot for the fixes Robert. Sorry for having taken so long to reply.
> The Ubuntu Developer Summit and related meetings took all my attention away.
No problem. I understand.

> I added more fixes to your code in lp:~fboucault/unity-2d/trash_singleton
> If you can have a look and check that you are happy with that, we should merge
> it.
I looked at it. It looks good. You can merge :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/UnityApplications/CMakeLists.txt'
2--- launcher/UnityApplications/CMakeLists.txt 2011-04-27 16:10:32 +0000
3+++ launcher/UnityApplications/CMakeLists.txt 2011-05-02 11:09:31 +0000
4@@ -20,6 +20,7 @@
5 launcherapplicationslistdbus.cpp
6 launcherdevice.cpp
7 launcherdeviceslist.cpp
8+ launcherutility.cpp
9 placeentry.cpp
10 place.cpp
11 launcherplaceslist.cpp
12@@ -38,6 +39,7 @@
13 launcherapplicationslistdbus.h
14 launcherdevice.h
15 launcherdeviceslist.h
16+ launcherutility.h
17 placeentry.h
18 place.h
19 launcherplaceslist.h
20
21=== modified file 'launcher/UnityApplications/launcherapplication.cpp'
22--- launcher/UnityApplications/launcherapplication.cpp 2011-04-28 13:09:25 +0000
23+++ launcher/UnityApplications/launcherapplication.cpp 2011-05-02 11:09:31 +0000
24@@ -31,13 +31,10 @@
25 via QML’s import statement (`import Unity2d 1.0`), so there is no need to
26 set it again here.
27 */
28-extern "C" {
29-#include <libwnck/libwnck.h>
30-#include <libwnck/util.h>
31-}
32
33 #include "launcherapplication.h"
34 #include "launchermenu.h"
35+#include "launcherutility.h"
36 #include "bamf-matcher.h"
37 #include "bamf-indicator.h"
38
39@@ -710,58 +707,7 @@
40 wnck_screen_force_update(screen);
41
42 WnckWindow* window = wnck_window_get(important->xid());
43- showWindow(window);
44-}
45-
46-void
47-LauncherApplication::showWindow(WnckWindow* window)
48-{
49- WnckWorkspace* workspace = wnck_window_get_workspace(window);
50-
51- /* Using X.h's CurrentTime (= 0) */
52- wnck_workspace_activate(workspace, CurrentTime);
53-
54- /* If the workspace contains a viewport then move the viewport so
55- that the window is visible.
56- Compiz for example uses only one workspace with a desktop larger
57- than the screen size which means that a viewport is used to
58- determine what part of the desktop is visible.
59-
60- Reference:
61- http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#largedesks
62- */
63- if (wnck_workspace_is_virtual(workspace)) {
64- moveViewportToWindow(window);
65- }
66-
67- /* Using X.h's CurrentTime (= 0) */
68- wnck_window_activate(window, CurrentTime);
69-}
70-
71-void
72-LauncherApplication::moveViewportToWindow(WnckWindow* window)
73-{
74- WnckWorkspace* workspace = wnck_window_get_workspace(window);
75- WnckScreen* screen = wnck_window_get_screen(window);
76-
77- int screen_width = wnck_screen_get_width(screen);
78- int screen_height = wnck_screen_get_height(screen);
79- int viewport_x = wnck_workspace_get_viewport_x(workspace);
80- int viewport_y = wnck_workspace_get_viewport_y(workspace);
81-
82- int window_x, window_y, window_width, window_height;
83- wnck_window_get_geometry(window, &window_x, &window_y,
84- &window_width, &window_height);
85-
86- /* Compute the row and column of the "virtual workspace" that contains
87- the window. A "virtual workspace" is a portion of the desktop of the
88- size of the screen.
89- */
90- int viewport_column = (viewport_x + window_x) / screen_width;
91- int viewport_row = (viewport_y + window_y) / screen_height;
92-
93- wnck_screen_move_viewport(screen, viewport_column * screen_width,
94- viewport_row * screen_height);
95+ LauncherUtility::showWindow(window);
96 }
97
98 void
99
100=== modified file 'launcher/UnityApplications/launcherapplication.h'
101--- launcher/UnityApplications/launcherapplication.h 2011-04-28 06:59:39 +0000
102+++ launcher/UnityApplications/launcherapplication.h 2011-05-02 11:09:31 +0000
103@@ -108,8 +108,6 @@
104
105 Q_INVOKABLE virtual void createMenuActions();
106
107- static void showWindow(WnckWindow* window);
108- static void moveViewportToWindow(WnckWindow* window);
109 void updateOverlaysState(QMap<QString, QVariant> properties);
110
111 Q_SIGNALS:
112
113=== added file 'launcher/UnityApplications/launcherutility.cpp'
114--- launcher/UnityApplications/launcherutility.cpp 1970-01-01 00:00:00 +0000
115+++ launcher/UnityApplications/launcherutility.cpp 2011-05-02 11:09:31 +0000
116@@ -0,0 +1,58 @@
117+
118+extern "C" {
119+#include <libsn/sn.h>
120+}
121+
122+#include "launcherutility.h"
123+
124+void
125+LauncherUtility::moveViewportToWindow(WnckWindow* window)
126+{
127+ WnckWorkspace* workspace = wnck_window_get_workspace(window);
128+ WnckScreen* screen = wnck_window_get_screen(window);
129+
130+ int screen_width = wnck_screen_get_width(screen);
131+ int screen_height = wnck_screen_get_height(screen);
132+ int viewport_x = wnck_workspace_get_viewport_x(workspace);
133+ int viewport_y = wnck_workspace_get_viewport_y(workspace);
134+
135+ int window_x, window_y, window_width, window_height;
136+ wnck_window_get_geometry(window, &window_x, &window_y,
137+ &window_width, &window_height);
138+
139+ /* Compute the row and column of the "virtual workspace" that contains
140+ the window. A "virtual workspace" is a portion of the desktop of the
141+ size of the screen.
142+ */
143+ int viewport_column = (viewport_x + window_x) / screen_width;
144+ int viewport_row = (viewport_y + window_y) / screen_height;
145+
146+ wnck_screen_move_viewport(screen, viewport_column * screen_width,
147+ viewport_row * screen_height);
148+}
149+
150+void
151+LauncherUtility::showWindow(WnckWindow* window)
152+{
153+ WnckWorkspace* workspace = wnck_window_get_workspace(window);
154+
155+ /* Using X.h's CurrentTime (= 0) */
156+ wnck_workspace_activate(workspace, CurrentTime);
157+
158+ /* If the workspace contains a viewport then move the viewport so
159+ that the window is visible.
160+ Compiz for example uses only one workspace with a desktop larger
161+ than the screen size which means that a viewport is used to
162+ determine what part of the desktop is visible.
163+
164+ Reference:
165+ http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#largedesks
166+ */
167+ if (wnck_workspace_is_virtual(workspace)) {
168+ moveViewportToWindow(window);
169+ }
170+
171+ /* Using X.h's CurrentTime (= 0) */
172+ wnck_window_activate(window, CurrentTime);
173+}
174+
175
176=== added file 'launcher/UnityApplications/launcherutility.h'
177--- launcher/UnityApplications/launcherutility.h 1970-01-01 00:00:00 +0000
178+++ launcher/UnityApplications/launcherutility.h 2011-05-02 11:09:31 +0000
179@@ -0,0 +1,16 @@
180+
181+#ifndef LAUNCHERUTILITY_H
182+#define LAUNCHERUTILITY_H
183+
184+#include <gio/gdesktopappinfo.h>
185+#include <libwnck/libwnck.h>
186+
187+class LauncherUtility {
188+
189+public:
190+
191+ static void showWindow(WnckWindow* window);
192+ static void moveViewportToWindow(WnckWindow* window);
193+};
194+
195+#endif // LAUNCHERUTILITY_H
196
197=== modified file 'launcher/UnityApplications/trash.cpp'
198--- launcher/UnityApplications/trash.cpp 2011-04-28 13:09:25 +0000
199+++ launcher/UnityApplications/trash.cpp 2011-05-02 11:09:31 +0000
200@@ -19,6 +19,11 @@
201
202 #include "trash.h"
203 #include "launchermenu.h"
204+#include "launcherutility.h"
205+
206+#include "bamf-application.h"
207+#include "bamf-window.h"
208+#include "bamf-matcher.h"
209
210 #include "config.h"
211
212@@ -28,6 +33,7 @@
213
214 #include <QAction>
215
216+
217 #define TRASH_URI "trash://"
218
219 Trash::Trash()
220@@ -53,7 +59,61 @@
221 bool
222 Trash::running() const
223 {
224- return false;
225+ BamfMatcher& matcher = BamfMatcher::get_default();
226+ QScopedPointer<BamfApplicationList> running_applications(matcher.running_applications());
227+ BamfApplication* bamfApplication;
228+
229+ for(int i=0; i<running_applications->size(); i++) {
230+ bamfApplication = running_applications->at(i);
231+
232+ QScopedPointer<BamfWindowList> windowApplications(bamfApplication->windows());
233+
234+ for (int j=0; j < windowApplications->size(); j++) {
235+
236+ BamfWindow *bamfWindow = windowApplications->at(j);
237+
238+ WnckWindow* wnckWindow = wnck_window_get(bamfWindow->xid());
239+ QString windowName = QString(wnck_window_get_name(wnckWindow));
240+
241+ int found = QString::compare(u2dTr("Trash", "nautilus"), windowName, Qt::CaseSensitive);
242+
243+ if (found == 0) {
244+
245+ return true;
246+ }
247+ }
248+ }
249+
250+ return false;
251+}
252+
253+void
254+Trash::show()
255+{
256+ BamfMatcher& matcher = BamfMatcher::get_default();
257+ QScopedPointer<BamfApplicationList> running_applications(matcher.running_applications());
258+ BamfApplication* bamfApplication;
259+
260+ for(int i=0; i<running_applications->size(); i++) {
261+ bamfApplication = running_applications->at(i);
262+
263+ QScopedPointer<BamfWindowList> windowApplications(bamfApplication->windows());
264+
265+ for (int j=0; j < windowApplications->size(); j++) {
266+
267+ BamfWindow *bamfWindow = windowApplications->at(j);
268+
269+ WnckWindow* wnckWindow = wnck_window_get(bamfWindow->xid());
270+ QString windowName = QString(wnck_window_get_name(wnckWindow));
271+
272+ int found = QString::compare(u2dTr("Trash", "nautilus"), windowName, Qt::CaseSensitive);
273+
274+ if (found == 0) {
275+ LauncherUtility::showWindow(wnckWindow);
276+ }
277+ }
278+ }
279+
280 }
281
282 int
283@@ -91,7 +151,11 @@
284 void
285 Trash::activate()
286 {
287- open();
288+ if (running()) {
289+ show();
290+ } else {
291+ open();
292+ }
293 }
294
295 void
296
297=== modified file 'launcher/UnityApplications/trash.h'
298--- launcher/UnityApplications/trash.h 2011-03-31 08:12:28 +0000
299+++ launcher/UnityApplications/trash.h 2011-05-02 11:09:31 +0000
300@@ -20,9 +20,10 @@
301 #ifndef TRASH_H
302 #define TRASH_H
303
304-#include <gio/gio.h>
305+#include <libwnck/libwnck.h>
306
307 #include "launcheritem.h"
308+#include "launcherutility.h"
309
310 #include <QAbstractListModel>
311 #include <QMetaType>
312@@ -58,6 +59,7 @@
313
314 private Q_SLOTS:
315 void onEmptyTriggered();
316+ void show();
317
318 private:
319 void open() const;
320@@ -65,7 +67,7 @@
321 int count() const;
322
323 static void recursive_delete(GFile* dir);
324-
325+
326 GFile* m_trash;
327 };
328

Subscribers

People subscribed via source and target branches