Merge lp:~3v1n0/bamf/libreoffice-fixes into lp:bamf/0.4
- libreoffice-fixes
- Merge into 0.4
Status: | Merged | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Approved by: | Mikkel Kamstrup Erlandsen | ||||||||||||||||
Approved revision: | 443 | ||||||||||||||||
Merged at revision: | 424 | ||||||||||||||||
Proposed branch: | lp:~3v1n0/bamf/libreoffice-fixes | ||||||||||||||||
Merge into: | lp:bamf/0.4 | ||||||||||||||||
Diff against target: |
805 lines (+327/-147) 8 files modified
configure.in (+1/-1) src/bamf-application.c (+15/-2) src/bamf-legacy-screen.c (+31/-0) src/bamf-legacy-screen.h (+2/-0) src/bamf-legacy-window.c (+35/-7) src/bamf-legacy-window.h (+2/-0) src/bamf-matcher.c (+234/-136) src/bamf-view.c (+7/-1) |
||||||||||||||||
To merge this branch: | bzr merge lp:~3v1n0/bamf/libreoffice-fixes | ||||||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Mikkel Kamstrup Erlandsen (community) | Approve | ||
Review via email: mp+85395@code.launchpad.net |
Commit message
Fixed LibreOffice and OpenOffice compatibility, remapping windows to their real type
Description of the change
When a window can be mapped as a LibreOffice / OpenOffice application we look for the right hint for the given window, we set that hint by default (if valid) and we connect to the window's name-changed event.
So, every time the window name changes, we check again for the window type and if it has changed (i.e. we moved from writer to start-center), then we simulate a window open/close event that cause the window to be re-mapped to
the proper application.
This works very well and allows to change the application type on run-time.
Also, every recognized libreoffice window that doesn't match any main application, is now considered as a "Standard" libreoffice application window, using the libreoffice-
This allows to correctly match not only the start center, but also dialog windows such as the restore window that shows up after a crash.
I also tried to introduce matching based on window class names (using libreoffice-writer, libreoffice-calc ... values), but since these parameters get updated only after the window name change, this check could be confusing (also if theoretically more secure).
The splash screen and the toolbars are now ignored, and so don't respect the rules above.
I've also included some optimizations, fixes (memleaks) and code cleanup.
Mikkel Kamstrup Erlandsen (kamstrup) wrote : | # |
- 436. By Marco Trevisan (Treviño)
-
BamfApplication: don't leak the icon string
- 437. By Marco Trevisan (Treviño)
-
Configure.in: set glib-2.0 >= 2.28 to support g_list_free_full
- 438. By Marco Trevisan (Treviño)
-
BamfMatcher: use finalize function, instead of dispose.
- 439. By Marco Trevisan (Treviño)
-
BamfView: emit a g_critical message when unregistering a DBus object
- 440. By Marco Trevisan (Treviño)
-
BamfLegacyScreen: added docs to bamf_legacy_
screen_ inject_ window
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> Firstly - I LOVE you for getting rid of all the 'key = g_new (gint, 1);' that
> just gross in so many ways :-)
Yes, I'm sure that there are also some other leaks around, but I've not read everything yet. :)
Now for some comments:
> The function you're updating here, bamf_applicatio
> seems to leak self->priv->icon right at the end if it's set before we assign
> it. Can you plug that while you're here?
Done. Free'd also on dispose.
> 30 void
> 31 +bamf_legacy_
>
> It's not totally clear to me what this function is intended to do. Can you add
> a short docstring? (I assume most ppl wont bother to dig out of the revision
> history like me ;-))
Done.
> 94 +void
> 95 +bamf_legacy_
>
> Same as the last new function :-)
Done.
> 101 + g_object_weak_ref (G_OBJECT (self), (GWeakNotify)
> handle_
> 102 + GUINT_TO_POINTER (xid));
>
> It's not very clear what the magic behind this weak ref is, could you add an
> explanatory comment?
Ok.
> 373 + g_list_free_full (possible_apps, g_free);
>
> The introduction of g_list_free_full() is nice, but requires glib >=2.28.
> Please add that check to configure.in.
Done.
> 423 + if (!g_hash_
> 424 + {
> 425 + g_hash_table_insert (registered_pids, key, g_strdup (window_hint));
> 426 + }
>
> Only updating if we don't already know the pid seems like it could cause
> things to go askew? If that's not the case I think this logic requires a
> comment in the code.
Well, this code was already there and I just kept the old behavior alive. However it seems to work well, also because generally a pid is always connected to just one application, except particular cases like this one (libreoffice or chromium).
However, I'll study more on this and eventually I'll change this code.
> 628 static void
> 629 +bamf_matcher_
>
> This function does not look like it's safe to run N times on the same object.
> That is the contract of dispose(). Fx.
Right, moving to finalize().
> The commit message for this chunk mentions that you log a g_critical()when you
> replace an object. I don't see that critical being logged - is that
> intentional?
Ehehe... Really it was there, but I don't know why I've not committed it.
Re-added! ;)
- 441. By Marco Trevisan (Treviño)
-
BamfLegacyWindow: added docs to bamf_legacy_
window_ reopen - 442. By Marco Trevisan (Treviño)
-
BamfMatcher: correctly match the libreoffice transient windows.
We'll use the class name for matching them. This allows to be even more precise.
Mikkel Kamstrup Erlandsen (kamstrup) wrote : | # |
Ok, looking good now. Awesome work! Just one line to fix before I can approve this:
773 + object_
You mean
+ object_
:-)
- 443. By Marco Trevisan (Treviño)
-
BamfMatcher: fixing typo s/dispose/finalize/
Mikkel Kamstrup Erlandsen (kamstrup) wrote : | # |
I am happy! :-D
Preview Diff
1 | === modified file 'configure.in' |
2 | --- configure.in 2011-11-28 10:17:05 +0000 |
3 | +++ configure.in 2011-12-15 11:35:15 +0000 |
4 | @@ -56,7 +56,7 @@ |
5 | # |
6 | # glib |
7 | # |
8 | -PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.16.0 gio-2.0 gio-unix-2.0) |
9 | +PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.28.0 gio-2.0 gio-unix-2.0) |
10 | |
11 | # |
12 | # dbus |
13 | |
14 | === modified file 'src/bamf-application.c' |
15 | --- src/bamf-application.c 2011-08-30 20:47:15 +0000 |
16 | +++ src/bamf-application.c 2011-12-15 11:35:15 +0000 |
17 | @@ -145,7 +145,9 @@ |
18 | gicon = g_app_info_get_icon (G_APP_INFO (desktop)); |
19 | |
20 | name = g_strdup (g_app_info_get_display_name (G_APP_INFO (desktop))); |
21 | - icon = g_icon_to_string (gicon); |
22 | + |
23 | + if (gicon) |
24 | + icon = g_icon_to_string (gicon); |
25 | |
26 | if (g_key_file_has_key(keyfile, G_KEY_FILE_DESKTOP_GROUP, STUB_KEY, NULL)) { |
27 | /* This will error to return false, which is okay as it seems |
28 | @@ -227,7 +229,12 @@ |
29 | } |
30 | |
31 | if (icon) |
32 | - self->priv->icon = icon; |
33 | + { |
34 | + if (self->priv->icon) |
35 | + g_free (self->priv->icon); |
36 | + |
37 | + self->priv->icon = icon; |
38 | + } |
39 | |
40 | if (name) |
41 | bamf_view_set_name (BAMF_VIEW (self), name); |
42 | @@ -611,6 +618,12 @@ |
43 | priv->app_type = NULL; |
44 | } |
45 | |
46 | + if (priv->icon) |
47 | + { |
48 | + g_free (priv->icon); |
49 | + priv->icon = NULL; |
50 | + } |
51 | + |
52 | if (priv->wmclass) |
53 | { |
54 | g_free (priv->wmclass); |
55 | |
56 | === modified file 'src/bamf-legacy-screen.c' |
57 | --- src/bamf-legacy-screen.c 2011-08-01 23:25:01 +0000 |
58 | +++ src/bamf-legacy-screen.c 2011-12-15 11:35:15 +0000 |
59 | @@ -160,6 +160,7 @@ |
60 | handle_window_opened (WnckScreen *screen, WnckWindow *window, BamfLegacyScreen *legacy) |
61 | { |
62 | BamfLegacyWindow *legacy_window; |
63 | + g_return_if_fail (WNCK_IS_WINDOW (window)); |
64 | |
65 | legacy_window = bamf_legacy_window_new (window); |
66 | |
67 | @@ -171,6 +172,36 @@ |
68 | g_signal_emit (legacy, legacy_screen_signals[WINDOW_OPENED], 0, legacy_window); |
69 | } |
70 | |
71 | +/* This function allows to push into the screen a window by its xid. |
72 | + * If the window is already known, it's just ignored, otherwise it gets added |
73 | + * to the windows list. The BamfLegacyScreen should automatically update its |
74 | + * windows list when they are added/removed from the screen, but if a child |
75 | + * BamfLegacyWindow is closed, then it could be possible to re-add it. */ |
76 | +void |
77 | +bamf_legacy_screen_inject_window (BamfLegacyScreen *self, guint xid) |
78 | +{ |
79 | + g_return_if_fail (BAMF_IS_LEGACY_SCREEN (self)); |
80 | + BamfLegacyWindow *window; |
81 | + GList *l; |
82 | + |
83 | + for (l = self->priv->windows; l; l = l->next) |
84 | + { |
85 | + window = l->data; |
86 | + |
87 | + if (bamf_legacy_window_get_xid (window) == xid) |
88 | + { |
89 | + return; |
90 | + } |
91 | + } |
92 | + |
93 | + WnckWindow *legacy_window = wnck_window_get(xid); |
94 | + |
95 | + if (WNCK_IS_WINDOW (legacy_window)) |
96 | + { |
97 | + handle_window_opened(NULL, legacy_window, self); |
98 | + } |
99 | +} |
100 | + |
101 | void |
102 | bamf_legacy_screen_set_state_file (BamfLegacyScreen *self, |
103 | const char *file) |
104 | |
105 | === modified file 'src/bamf-legacy-screen.h' |
106 | --- src/bamf-legacy-screen.h 2010-05-25 13:47:38 +0000 |
107 | +++ src/bamf-legacy-screen.h 2011-12-15 11:35:15 +0000 |
108 | @@ -67,4 +67,6 @@ |
109 | |
110 | BamfLegacyScreen * bamf_legacy_screen_get_default (void); |
111 | |
112 | +void bamf_legacy_screen_inject_window (BamfLegacyScreen *screen, guint xid); |
113 | + |
114 | #endif |
115 | |
116 | === modified file 'src/bamf-legacy-window.c' |
117 | --- src/bamf-legacy-window.c 2011-11-29 08:04:22 +0000 |
118 | +++ src/bamf-legacy-window.c 2011-12-15 11:35:15 +0000 |
119 | @@ -328,13 +328,41 @@ |
120 | { |
121 | g_return_if_fail (BAMF_IS_LEGACY_WINDOW (self)); |
122 | g_return_if_fail (WNCK_IS_WINDOW (window)); |
123 | - |
124 | + |
125 | + if (self->priv->legacy_window == window) |
126 | + { |
127 | + self->priv->is_closed = TRUE; |
128 | + g_signal_emit (self, legacy_window_signals[CLOSED], 0); |
129 | + } |
130 | +} |
131 | + |
132 | +static void |
133 | +handle_destroy_notify (gpointer *data, BamfLegacyWindow *self_was_here) |
134 | +{ |
135 | + BamfLegacyScreen *screen = bamf_legacy_screen_get_default (); |
136 | + bamf_legacy_screen_inject_window (screen, GPOINTER_TO_UINT (data)); |
137 | +} |
138 | + |
139 | +/* This utility function allows to set a BamfLegacyWindow as closed, notifying |
140 | + * all its owners, and to reopen it once the current window has been destroyed. |
141 | + * This allows to remap particular windows to different applications. */ |
142 | +void |
143 | +bamf_legacy_window_reopen (BamfLegacyWindow *self) |
144 | +{ |
145 | + g_return_if_fail (BAMF_IS_LEGACY_WINDOW (self)); |
146 | + g_return_if_fail (WNCK_IS_WINDOW (self->priv->legacy_window)); |
147 | + |
148 | + guint xid = bamf_legacy_window_get_xid (self); |
149 | + |
150 | + /* Adding a weak ref to this object, causes to get notified after the object |
151 | + * destruction, so once this BamfLegacyWindow has been closed and drestroyed |
152 | + * the handle_destroy_notify() function will be called, and that will |
153 | + * provide to iniject another window like this one to the BamfLegacyScreen */ |
154 | + g_object_weak_ref (G_OBJECT (self), (GWeakNotify) handle_destroy_notify, |
155 | + GUINT_TO_POINTER (xid)); |
156 | + |
157 | self->priv->is_closed = TRUE; |
158 | - |
159 | - if (window == self->priv->legacy_window) |
160 | - { |
161 | - g_signal_emit (self, legacy_window_signals[CLOSED], 0); |
162 | - } |
163 | + g_signal_emit (self, legacy_window_signals[CLOSED], 0); |
164 | } |
165 | |
166 | static void |
167 | @@ -381,7 +409,7 @@ |
168 | screen = wnck_screen_get_default (); |
169 | |
170 | priv->closed_id = g_signal_connect (G_OBJECT (screen), "window-closed", |
171 | - (GCallback) handle_window_closed, self); |
172 | + (GCallback) handle_window_closed, self); |
173 | } |
174 | |
175 | static void |
176 | |
177 | === modified file 'src/bamf-legacy-window.h' |
178 | --- src/bamf-legacy-window.h 2011-09-26 11:44:10 +0000 |
179 | +++ src/bamf-legacy-window.h 2011-12-15 11:35:15 +0000 |
180 | @@ -112,6 +112,8 @@ |
181 | |
182 | BamfLegacyWindow * bamf_legacy_window_get_transient (BamfLegacyWindow *self); |
183 | |
184 | +void bamf_legacy_window_reopen (BamfLegacyWindow *self); |
185 | + |
186 | BamfLegacyWindow * bamf_legacy_window_new (WnckWindow *legacy_window); |
187 | |
188 | #endif |
189 | |
190 | === modified file 'src/bamf-matcher.c' |
191 | --- src/bamf-matcher.c 2011-09-29 20:31:13 +0000 |
192 | +++ src/bamf-matcher.c 2011-12-15 11:35:15 +0000 |
193 | @@ -14,6 +14,7 @@ |
194 | * along with this program. If not, see <http://www.gnu.org/licenses/>. |
195 | * |
196 | * Authored by: Jason Smith <jason.smith@canonical.com> |
197 | + * Marco Trevisan (Treviño) <3v1n0@ubuntu.com> |
198 | * |
199 | */ |
200 | |
201 | @@ -65,7 +66,6 @@ |
202 | GHashTable * desktop_id_table; |
203 | GHashTable * desktop_file_table; |
204 | GHashTable * desktop_class_table; |
205 | - GHashTable * exec_list; |
206 | GHashTable * registered_pids; |
207 | GList * views; |
208 | GList * monitors; |
209 | @@ -135,12 +135,12 @@ |
210 | } |
211 | } |
212 | |
213 | -static void bamf_matcher_unregister_view (BamfMatcher *self, BamfView *view); |
214 | +static void bamf_matcher_unregister_view (BamfMatcher *self, BamfView *view, gboolean unref); |
215 | |
216 | static void |
217 | on_view_closed (BamfView *view, BamfMatcher *self) |
218 | { |
219 | - bamf_matcher_unregister_view (self, view); |
220 | + bamf_matcher_unregister_view (self, view, TRUE); |
221 | } |
222 | |
223 | static void |
224 | @@ -168,7 +168,7 @@ |
225 | } |
226 | |
227 | static void |
228 | -bamf_matcher_unregister_view (BamfMatcher *self, BamfView *view) |
229 | +bamf_matcher_unregister_view (BamfMatcher *self, BamfView *view, gboolean unref) |
230 | { |
231 | const char * path; |
232 | char * type; |
233 | @@ -181,8 +181,11 @@ |
234 | g_signal_handlers_disconnect_by_func (G_OBJECT (view), on_view_closed, self); |
235 | g_signal_handlers_disconnect_by_func (G_OBJECT (view), on_view_active_changed, self); |
236 | |
237 | - self->priv->views = g_list_remove (self->priv->views, view); |
238 | - g_object_unref (view); |
239 | + if (unref) |
240 | + { |
241 | + self->priv->views = g_list_remove (self->priv->views, view); |
242 | + g_object_unref (view); |
243 | + } |
244 | |
245 | g_free (type); |
246 | } |
247 | @@ -191,7 +194,7 @@ |
248 | get_open_office_window_hint (BamfMatcher * self, BamfLegacyWindow * window) |
249 | { |
250 | const gchar *name; |
251 | - char *exec; |
252 | + char *exec = NULL; |
253 | GHashTable *desktopFileTable; |
254 | GList *list; |
255 | |
256 | @@ -199,18 +202,44 @@ |
257 | g_return_val_if_fail (BAMF_IS_LEGACY_WINDOW (window), NULL); |
258 | |
259 | name = bamf_legacy_window_get_name (window); |
260 | + const gchar *class = bamf_legacy_window_get_class_name (window); |
261 | + BamfWindowType type = bamf_legacy_window_get_window_type (window); |
262 | |
263 | - if (name == NULL) |
264 | + if (name == NULL && class == NULL) |
265 | return NULL; |
266 | |
267 | - if (g_str_has_suffix (name, "OpenOffice.org Writer")) |
268 | + if (g_str_has_suffix (name, "LibreOffice Writer")) |
269 | + { |
270 | + exec = "libreoffice -writer %U"; |
271 | + } |
272 | + else if (g_str_has_suffix (name, "LibreOffice Calc")) |
273 | + { |
274 | + exec = "libreoffice -calc %U"; |
275 | + } |
276 | + else if (g_str_has_suffix (name, "LibreOffice Impress")) |
277 | + { |
278 | + exec = "libreoffice -impress %U"; |
279 | + } |
280 | + else if (g_str_has_suffix (name, "LibreOffice Math")) |
281 | + { |
282 | + exec = "libreoffice -math %U"; |
283 | + } |
284 | + else if (g_str_has_suffix (name, "LibreOffice Draw")) |
285 | + { |
286 | + exec = "libreoffice -draw %U"; |
287 | + } |
288 | + else if (g_strcmp0 (class, "libreoffice-startcenter") == 0) |
289 | + { |
290 | + exec = "libreoffice %U"; |
291 | + } |
292 | + else if (g_strcmp0 (name, "LibreOffice") == 0 && type == BAMF_WINDOW_NORMAL) |
293 | + { |
294 | + exec = "libreoffice %U"; |
295 | + } |
296 | + else if (g_str_has_suffix (name, "OpenOffice.org Writer")) |
297 | { |
298 | exec = "ooffice -writer %F"; |
299 | } |
300 | - else if (g_str_has_suffix (name, "OpenOffice.org Math")) |
301 | - { |
302 | - exec = "ooffice -math %F"; |
303 | - } |
304 | else if (g_str_has_suffix (name, "OpenOffice.org Calc")) |
305 | { |
306 | exec = "ooffice -calc %F"; |
307 | @@ -219,41 +248,63 @@ |
308 | { |
309 | exec = "ooffice -impress %F"; |
310 | } |
311 | + else if (g_str_has_suffix (name, "OpenOffice.org Math")) |
312 | + { |
313 | + exec = "ooffice -math %F"; |
314 | + } |
315 | else if (g_str_has_suffix (name, "OpenOffice.org Draw")) |
316 | { |
317 | exec = "ooffice -draw %F"; |
318 | } |
319 | - else if (g_str_has_suffix (name, "LibreOffice Writer")) |
320 | - { |
321 | - exec = "libreoffice -writer %U"; |
322 | - } |
323 | - else if (g_str_has_suffix (name, "LibreOffice Math")) |
324 | - { |
325 | - exec = "libreoffice -math %U"; |
326 | - } |
327 | - else if (g_str_has_suffix (name, "LibreOffice Calc")) |
328 | - { |
329 | - exec = "libreoffice -calc %U"; |
330 | - } |
331 | - else if (g_str_has_suffix (name, "LibreOffice Impress")) |
332 | - { |
333 | - exec = "libreoffice -impress %U"; |
334 | - } |
335 | - else if (g_str_has_suffix (name, "LibreOffice Draw")) |
336 | - { |
337 | - exec = "libreoffice -draw %U"; |
338 | + else if (g_strcmp0 (name, "OpenOffice.org") == 0 && type == BAMF_WINDOW_NORMAL) |
339 | + { |
340 | + exec = "ooffice %F"; |
341 | } |
342 | else |
343 | { |
344 | - return NULL; |
345 | + if (type != BAMF_WINDOW_NORMAL || bamf_legacy_window_get_transient (window)) |
346 | + { |
347 | + /* Child windows can generally easily be recognized by their class */ |
348 | + if (g_strcmp0 (class, "libreoffice-writer") == 0) |
349 | + { |
350 | + exec = "libreoffice -writer %U"; |
351 | + } |
352 | + else if (g_strcmp0 (class, "libreoffice-calc") == 0) |
353 | + { |
354 | + exec = "libreoffice -calc %U"; |
355 | + } |
356 | + else if (g_strcmp0 (class, "libreoffice-impress") == 0) |
357 | + { |
358 | + exec = "libreoffice -impress %U"; |
359 | + } |
360 | + else if (g_strcmp0 (class, "libreoffice-math") == 0) |
361 | + { |
362 | + exec = "libreoffice -math %U"; |
363 | + } |
364 | + else if (g_strcmp0 (class, "libreoffice-draw") == 0) |
365 | + { |
366 | + exec = "libreoffice -draw %U"; |
367 | + } |
368 | + } |
369 | + |
370 | + if (!exec) |
371 | + { |
372 | + /* By default fallback to the main launcher */ |
373 | + if (g_str_has_prefix (class, "OpenOffice")) |
374 | + { |
375 | + exec = "ooffice %F"; |
376 | + } |
377 | + else |
378 | + { |
379 | + exec = "libreoffice %U"; |
380 | + } |
381 | + } |
382 | } |
383 | |
384 | desktopFileTable = self->priv->desktop_file_table; |
385 | list = g_hash_table_lookup (desktopFileTable, exec); |
386 | |
387 | - g_return_val_if_fail (list, NULL); |
388 | - |
389 | - return (char *) list->data; |
390 | + return (list ? (char *) list->data : NULL); |
391 | } |
392 | |
393 | /* Attempts to return the binary name for a particular execution string */ |
394 | @@ -266,7 +317,7 @@ |
395 | gboolean regexFail; |
396 | GRegex *regex; |
397 | |
398 | - g_return_val_if_fail (execString && strlen (execString) > 0, g_strdup (execString)); |
399 | + g_return_val_if_fail ((execString && execString[0] != '\0'), g_strdup (execString)); |
400 | |
401 | exec = g_utf8_casefold (execString, -1); |
402 | parts = g_strsplit (exec, " ", 0); |
403 | @@ -996,7 +1047,7 @@ |
404 | self->priv->desktop_id_table, |
405 | self->priv->desktop_class_table); |
406 | |
407 | - g_list_free_full (dirs, (GDestroyNotify) g_free); |
408 | + g_list_free_full (dirs, g_free); |
409 | } |
410 | } |
411 | else if (filetype != G_FILE_TYPE_UNKNOWN) |
412 | @@ -1098,15 +1149,18 @@ |
413 | fill_desktop_file_table (self, directories, *desktop_file_table, |
414 | *desktop_id_table, *desktop_class_table); |
415 | |
416 | - g_list_free_full (directories, (GDestroyNotify) g_free); |
417 | + g_list_free_full (directories, g_free); |
418 | } |
419 | |
420 | static gboolean |
421 | is_open_office_window (BamfMatcher * self, BamfLegacyWindow * window) |
422 | { |
423 | - return g_str_has_prefix (bamf_legacy_window_get_class_name (window), "OpenOffice") || |
424 | - g_str_has_prefix (bamf_legacy_window_get_class_name (window), "LibreOffice") || |
425 | - g_str_has_prefix (bamf_legacy_window_get_class_name (window), "libreoffice"); |
426 | + const char *class_name = bamf_legacy_window_get_class_name (window); |
427 | + |
428 | + return (g_str_has_prefix (class_name, "LibreOffice") || |
429 | + g_str_has_prefix (class_name, "libreoffice") || |
430 | + g_str_has_prefix (class_name, "OpenOffice") || |
431 | + g_str_has_prefix (class_name, "openoffice")); |
432 | } |
433 | |
434 | static char * |
435 | @@ -1298,7 +1352,7 @@ |
436 | |
437 | if (trimmed) |
438 | { |
439 | - if (strlen (trimmed) > 0) |
440 | + if (trimmed[0] != '\0') |
441 | { |
442 | table_list = g_hash_table_lookup (priv->desktop_file_table, trimmed); |
443 | |
444 | @@ -1371,7 +1425,7 @@ |
445 | hint = get_window_hint (self, window, _NET_WM_DESKTOP_FILE); |
446 | const char *window_class = bamf_legacy_window_get_class_name (window); |
447 | |
448 | - if (hint && strlen (hint) > 0 && !is_web_app_window(self, window)) |
449 | + if (hint && hint[0] != '\0' && !is_web_app_window(self, window)) |
450 | { |
451 | desktop_files = g_list_prepend (desktop_files, hint); |
452 | /* whew, hard work, didn't even have to make a copy! */ |
453 | @@ -1569,13 +1623,7 @@ |
454 | g_object_unref (best); |
455 | } |
456 | |
457 | - for (l = possible_apps; l; l = l->next) |
458 | - { |
459 | - char *str = l->data; |
460 | - g_free (str); |
461 | - } |
462 | - |
463 | - g_list_free (possible_apps); |
464 | + g_list_free_full (possible_apps, g_free); |
465 | |
466 | bamf_view_add_child (BAMF_VIEW (best), BAMF_VIEW (bamf_window)); |
467 | } |
468 | @@ -1590,7 +1638,7 @@ |
469 | GHashTable *registered_pids; |
470 | char *window_hint = NULL; |
471 | gint i, pid; |
472 | - gint *key; |
473 | + gpointer key; |
474 | |
475 | g_return_if_fail (BAMF_IS_MATCHER (self)); |
476 | g_return_if_fail (BAMF_IS_LEGACY_WINDOW (window)); |
477 | @@ -1604,9 +1652,7 @@ |
478 | |
479 | if (pid > 0) |
480 | { |
481 | - key = g_new (gint, 1); |
482 | - *key = pid; |
483 | - |
484 | + gpointer key = GINT_TO_POINTER (pid); |
485 | const char* result = g_hash_table_lookup (registered_pids, key); |
486 | if (result && g_str_has_prefix (result, "/home/")) |
487 | { |
488 | @@ -1618,48 +1664,45 @@ |
489 | } |
490 | |
491 | window_hint = get_window_hint (self, window, _NET_WM_DESKTOP_FILE); |
492 | - if (window_hint && strlen (window_hint) > 0) |
493 | + if (window_hint) |
494 | { |
495 | - /* already set, make sure we know about this |
496 | - * fact for future windows of this applications */ |
497 | - pid = bamf_legacy_window_get_pid (window); |
498 | - |
499 | - if (pid > 0) |
500 | + if (window_hint[0] != '\0') |
501 | { |
502 | - key = g_new (gint, 1); |
503 | - *key = pid; |
504 | + /* already set, make sure we know about this |
505 | + * fact for future windows of this applications */ |
506 | + pid = bamf_legacy_window_get_pid (window); |
507 | |
508 | - if (!g_hash_table_lookup (registered_pids, key)) |
509 | + if (pid > 0) |
510 | { |
511 | - g_hash_table_insert (registered_pids, key, g_strdup (window_hint)); |
512 | + key = GINT_TO_POINTER (pid); |
513 | + |
514 | + if (!g_hash_table_lookup (registered_pids, key)) |
515 | + { |
516 | + g_hash_table_insert (registered_pids, key, g_strdup (window_hint)); |
517 | + } |
518 | } |
519 | + |
520 | + g_free (window_hint); |
521 | + return; |
522 | } |
523 | |
524 | g_free (window_hint); |
525 | - return; |
526 | - } |
527 | - |
528 | - if (is_open_office_window (self, window)) |
529 | - { |
530 | - window_hint = get_open_office_window_hint (self, window); |
531 | - } |
532 | - else |
533 | - { |
534 | - pids = pid_parent_tree (self, bamf_legacy_window_get_pid (window)); |
535 | - |
536 | - for (i = 0; i < pids->len; i++) |
537 | - { |
538 | - pid = g_array_index (pids, gint, i); |
539 | - |
540 | - key = g_new (gint, 1); |
541 | - *key = pid; |
542 | - |
543 | - window_hint = g_hash_table_lookup (registered_pids, key); |
544 | - if (window_hint != NULL && strlen (window_hint) > 0) |
545 | - break; |
546 | - } |
547 | - g_array_free (pids, TRUE); |
548 | - } |
549 | + window_hint = NULL; |
550 | + } |
551 | + |
552 | + pids = pid_parent_tree (self, bamf_legacy_window_get_pid (window)); |
553 | + |
554 | + for (i = 0; i < pids->len; i++) |
555 | + { |
556 | + pid = g_array_index (pids, gint, i); |
557 | + key = GINT_TO_POINTER (pid); |
558 | + |
559 | + window_hint = g_hash_table_lookup (registered_pids, key); |
560 | + if (window_hint != NULL && window_hint[0] != '\0') |
561 | + break; |
562 | + } |
563 | + |
564 | + g_array_free (pids, TRUE); |
565 | |
566 | if (window_hint) |
567 | set_window_hint (self, window, _NET_WM_DESKTOP_FILE, window_hint); |
568 | @@ -1690,24 +1733,31 @@ |
569 | bamf_matcher_setup_window_state (self, bamfwindow); |
570 | } |
571 | |
572 | -static gboolean |
573 | -open_office_window_setup_timer (OpenOfficeTimeoutArgs *args) |
574 | +static void |
575 | +on_open_office_window_name_changed (BamfLegacyWindow *window, BamfMatcher* self) |
576 | { |
577 | - if (bamf_legacy_window_is_closed (args->window)) |
578 | - { |
579 | - g_object_unref (args->window); |
580 | - return FALSE; |
581 | - } |
582 | - |
583 | - args->count++; |
584 | - if (args->count > 20 || get_open_office_window_hint (args->matcher, args->window)) |
585 | + g_return_if_fail (BAMF_IS_MATCHER (self)); |
586 | + g_return_if_fail (BAMF_IS_LEGACY_WINDOW (window)); |
587 | + |
588 | + char *old_hint; |
589 | + const char *new_hint; |
590 | + |
591 | + old_hint = get_window_hint (self, window, _NET_WM_DESKTOP_FILE); |
592 | + new_hint = get_open_office_window_hint (self, window); |
593 | + |
594 | + if (new_hint && g_strcmp0 (new_hint, old_hint) != 0) |
595 | { |
596 | - g_object_unref (args->window); |
597 | - handle_raw_window (args->matcher, args->window); |
598 | - return FALSE; |
599 | + bamf_legacy_window_reopen (window); |
600 | } |
601 | - |
602 | - return TRUE; |
603 | + |
604 | + g_free (old_hint); |
605 | +} |
606 | + |
607 | +static void |
608 | +on_open_office_window_closed (BamfLegacyWindow *window, BamfMatcher* self) |
609 | +{ |
610 | + g_signal_handlers_disconnect_by_func (window, on_open_office_window_name_changed, self); |
611 | + g_object_unref (window); |
612 | } |
613 | |
614 | static void |
615 | @@ -1724,24 +1774,35 @@ |
616 | bamf_matcher_register_view (self, BAMF_VIEW (bamfwindow)); |
617 | g_object_unref (bamfwindow); |
618 | |
619 | - return; |
620 | + return; |
621 | } |
622 | |
623 | - if (is_open_office_window (self, window) && get_open_office_window_hint (self, window) == NULL) |
624 | + if (is_open_office_window (self, window)) |
625 | { |
626 | - OpenOfficeTimeoutArgs* args = (OpenOfficeTimeoutArgs*) g_malloc0 (sizeof (OpenOfficeTimeoutArgs)); |
627 | - args->matcher = self; |
628 | - args->window = window; |
629 | - |
630 | + BamfWindowType win_type = bamf_legacy_window_get_window_type (window); |
631 | + |
632 | + if (win_type == BAMF_WINDOW_SPLASHSCREEN || win_type == BAMF_WINDOW_TOOLBAR) |
633 | + { |
634 | + return; |
635 | + } |
636 | + |
637 | + char *old_hint = get_window_hint (self, window, _NET_WM_DESKTOP_FILE); |
638 | + const char *new_hint = get_open_office_window_hint (self, window); |
639 | + |
640 | + if (new_hint && g_strcmp0 (old_hint, new_hint) != 0) |
641 | + { |
642 | + set_window_hint (self, window, _NET_WM_DESKTOP_FILE, new_hint); |
643 | + } |
644 | + |
645 | g_object_ref (window); |
646 | - /* we have an open office window who is not ready to match yet */ |
647 | - g_timeout_add (100, (GSourceFunc) open_office_window_setup_timer, args); |
648 | - } |
649 | - else |
650 | - { |
651 | - /* we have a window who is ready to be matched */ |
652 | - handle_raw_window (self, window); |
653 | - } |
654 | + g_signal_connect (window, "name-changed", (GCallback) on_open_office_window_name_changed, self); |
655 | + g_signal_connect (window, "closed", (GCallback) on_open_office_window_closed, self); |
656 | + |
657 | + g_free (old_hint); |
658 | + } |
659 | + |
660 | + /* we have a window who is ready to be matched */ |
661 | + handle_raw_window (self, window); |
662 | } |
663 | |
664 | static void |
665 | @@ -1806,14 +1867,7 @@ |
666 | } |
667 | } |
668 | |
669 | - for (l = possible_apps; l; l = l->next) |
670 | - { |
671 | - char *str = l->data; |
672 | - g_free (str); |
673 | - } |
674 | - |
675 | - |
676 | - g_list_free (possible_apps); |
677 | + g_list_free_full (possible_apps, g_free); |
678 | |
679 | if (best) |
680 | bamf_view_add_child (BAMF_VIEW (best), BAMF_VIEW (indicator)); |
681 | @@ -1847,20 +1901,16 @@ |
682 | const char * desktopFile, |
683 | gint pid) |
684 | { |
685 | - gint *key; |
686 | + gpointer key; |
687 | BamfLegacyScreen *screen; |
688 | GList *glist_item; |
689 | GList *windows; |
690 | - char *dup; |
691 | |
692 | g_return_if_fail (BAMF_IS_MATCHER (self)); |
693 | g_return_if_fail (desktopFile); |
694 | |
695 | - key = g_new (gint, 1); |
696 | - *key = pid; |
697 | - |
698 | - dup = g_strdup (desktopFile); |
699 | - g_hash_table_insert (self->priv->registered_pids, key, dup); |
700 | + key = GINT_TO_POINTER (pid); |
701 | + g_hash_table_insert (self->priv->registered_pids, key, g_strdup (desktopFile)); |
702 | |
703 | /* fixme, this is a bit heavy */ |
704 | |
705 | @@ -2217,8 +2267,8 @@ |
706 | |
707 | priv->known_pids = g_array_new (FALSE, TRUE, sizeof (gint)); |
708 | priv->bad_prefixes = g_array_new (FALSE, TRUE, sizeof (GRegex *)); |
709 | - priv->registered_pids = |
710 | - g_hash_table_new ((GHashFunc) g_int_hash, (GEqualFunc) g_int_equal); |
711 | + priv->registered_pids = g_hash_table_new_full (g_direct_hash, g_direct_equal, |
712 | + NULL, g_free); |
713 | |
714 | prefixstrings = prefix_strings (self); |
715 | for (i = 0; i < prefixstrings->len; i++) |
716 | @@ -2252,13 +2302,61 @@ |
717 | } |
718 | |
719 | static void |
720 | +bamf_matcher_finalize (GObject *object) |
721 | +{ |
722 | + BamfMatcher *self = (BamfMatcher *) object; |
723 | + BamfMatcherPrivate *priv = self->priv; |
724 | + GList *l; |
725 | + int i; |
726 | + |
727 | + for (i = 0; i < priv->bad_prefixes->len; i++) |
728 | + { |
729 | + GRegex *regex = g_array_index (priv->bad_prefixes, GRegex *, i); |
730 | + g_regex_unref (regex); |
731 | + } |
732 | + |
733 | + g_array_free (priv->bad_prefixes, TRUE); |
734 | + g_array_free (priv->known_pids, TRUE); |
735 | + g_hash_table_destroy (priv->desktop_id_table); |
736 | + g_hash_table_destroy (priv->desktop_file_table); |
737 | + g_hash_table_destroy (priv->desktop_class_table); |
738 | + g_hash_table_destroy (priv->registered_pids); |
739 | + |
740 | + for (l = priv->views; l; l = l->next) |
741 | + { |
742 | + bamf_matcher_unregister_view (self, (BamfView*)l->data, FALSE); |
743 | + } |
744 | + g_list_free_full (priv->views, g_object_unref); |
745 | + priv->views = NULL; |
746 | + |
747 | + for (l = priv->monitors; l; l = l->next) |
748 | + { |
749 | + g_signal_handlers_disconnect_by_func (G_OBJECT (l->data), |
750 | + (GCallback) on_monitor_changed, self); |
751 | + } |
752 | + g_list_free_full (priv->monitors, g_object_unref); |
753 | + priv->monitors = NULL; |
754 | + |
755 | + g_list_free_full (priv->favorites, g_free); |
756 | + priv->favorites = NULL; |
757 | + |
758 | + priv->active_app = NULL; |
759 | + priv->active_win = NULL; |
760 | + |
761 | + G_OBJECT_CLASS (bamf_matcher_parent_class)->finalize (object); |
762 | +} |
763 | + |
764 | +static void |
765 | bamf_matcher_class_init (BamfMatcherClass * klass) |
766 | { |
767 | + GObjectClass *object_class = G_OBJECT_CLASS (klass); |
768 | g_type_class_add_private (klass, sizeof (BamfMatcherPrivate)); |
769 | |
770 | dbus_g_object_type_install_info (BAMF_TYPE_MATCHER, |
771 | &dbus_glib_bamf_matcher_object_info); |
772 | |
773 | + object_class->finalize = bamf_matcher_finalize; |
774 | + |
775 | matcher_signals [VIEW_OPENED] = |
776 | g_signal_new ("view-opened", |
777 | G_OBJECT_CLASS_TYPE (klass), |
778 | @@ -2294,7 +2392,7 @@ |
779 | bamf_marshal_VOID__STRING_STRING, |
780 | G_TYPE_NONE, 2, |
781 | G_TYPE_STRING, G_TYPE_STRING); |
782 | - |
783 | + |
784 | matcher_signals [FAVORITES_CHANGED] = |
785 | g_signal_new ("favorites-changed", |
786 | G_OBJECT_CLASS_TYPE (klass), |
787 | |
788 | === modified file 'src/bamf-view.c' |
789 | --- src/bamf-view.c 2011-11-29 08:06:28 +0000 |
790 | +++ src/bamf-view.c 2011-12-15 11:35:15 +0000 |
791 | @@ -470,8 +470,14 @@ |
792 | |
793 | g_return_val_if_fail (bus, NULL); |
794 | |
795 | + GObject *old_object = dbus_g_connection_lookup_g_object (bus, path); |
796 | + if (G_IS_OBJECT (old_object)) |
797 | + { |
798 | + g_critical ("BAMF has already registered an object on path \"%s`, this should never happen!", path); |
799 | + dbus_g_connection_unregister_g_object (bus, old_object); |
800 | + } |
801 | + |
802 | dbus_g_connection_register_g_object (bus, path, G_OBJECT (view)); |
803 | - |
804 | g_signal_emit (view, view_signals[EXPORTED], 0); |
805 | } |
806 |
Firstly - I LOVE you for getting rid of all the 'key = g_new (gint, 1);' that just gross in so many ways :-) Now for some comments:
8 - icon = g_icon_to_string (gicon);
9 +
10 + if (gicon)
11 + icon = g_icon_to_string (gicon);
The function you're updating here, bamf_applicatio n_setup_ icon_and_ name(), seems to leak self->priv->icon right at the end if it's set before we assign it. Can you plug that while you're here?
30 void screen_ inject_ window (BamfLegacyScreen *self, guint xid)
31 +bamf_legacy_
It's not totally clear to me what this function is intended to do. Can you add a short docstring? (I assume most ppl wont bother to dig out of the revision history like me ;-))
94 +void window_ reopen (BamfLegacyWindow *self)
95 +bamf_legacy_
Same as the last new function :-)
101 + g_object_weak_ref (G_OBJECT (self), (GWeakNotify) handle_ destroy_ notify,
102 + GUINT_TO_POINTER (xid));
It's not very clear what the magic behind this weak ref is, could you add an explanatory comment?
373 + g_list_free_full (possible_apps, g_free);
The introduction of g_list_free_full() is nice, but requires glib >=2.28. Please add that check to configure.in.
423 + if (!g_hash_ table_lookup (registered_pids, key))
424 + {
425 + g_hash_table_insert (registered_pids, key, g_strdup (window_hint));
426 + }
Only updating if we don't already know the pid seems like it could cause things to go askew? If that's not the case I think this logic requires a comment in the code.
628 static void dispose (GObject *object)
629 +bamf_matcher_
This function does not look like it's safe to run N times on the same object. That is the contract of dispose(). Fx. looking at this block makes me suspicious:
642 + g_array_free (priv-> bad_prefixes, TRUE); table_destroy (priv-> desktop_ id_table) ; table_destroy (priv-> desktop_ file_table) ; table_destroy (priv-> desktop_ class_table) ; table_destroy (priv-> registered_ pids);
643 + g_array_free (priv->known_pids, TRUE);
644 + g_hash_
645 + g_hash_
646 + g_hash_
647 + g_hash_
You must add NULL-ification and NULL-checking on all members. If you don't want that override finalize() instead which is guaranteed to run only once.
695 + GObject *old_object = dbus_g_ connection_ lookup_ g_object (bus, path); connection_ unregister_ g_object (bus, old_object);
696 + if (G_IS_OBJECT (old_object))
697 + {
698 + dbus_g_
699 + }
The commit message for this chunk mentions that you log a g_critical()when you replace an object. I don't see that critical being logged - is that intentional?