After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723308 - Track changes to windows being interesting
Track changes to windows being interesting
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 724406 (view as bug list)
Depends on: 723307
Blocks:
 
 
Reported: 2014-01-30 18:19 UTC by Florian Müllner
Modified: 2014-02-15 11:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
window-tracker: Use MetaWindow:is-interesting (1.89 KB, patch)
2014-01-30 18:19 UTC, Florian Müllner
none Details | Review
shell-app: Track changes to MetaWindow:is-interesting (3.87 KB, patch)
2014-01-30 18:19 UTC, Florian Müllner
accepted-commit_now Details | Review
Use meta_window_is_interesting() consistently (4.02 KB, patch)
2014-01-30 18:19 UTC, Florian Müllner
none Details | Review
window-tracker: Remove is_window_interesting() (2.12 KB, patch)
2014-01-30 18:19 UTC, Florian Müllner
reviewed Details | Review
window-tracker: Use MetaWindow:skip-taskbar (1.92 KB, patch)
2014-01-30 23:06 UTC, Florian Müllner
committed Details | Review
shell-app: Track changes to MetaWindow:skip-taskbar (3.86 KB, patch)
2014-01-30 23:06 UTC, Florian Müllner
committed Details | Review
Use meta_window_is_skip_taskbar() directly (3.36 KB, patch)
2014-01-30 23:06 UTC, Florian Müllner
committed Details | Review
window-tracker: Remove is_window_interesting() (2.12 KB, patch)
2014-01-30 23:07 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2014-01-30 18:19:41 UTC
We have always missed when a window changed its "interesting" property after being mapped, but since we started to use it for an app's running state, this has changed from a being a minor annoyance to a potential crasher (when ending up with a "running" application without any windows).
Comment 1 Florian Müllner 2014-01-30 18:19:44 UTC
Created attachment 267665 [details] [review]
window-tracker: Use MetaWindow:is-interesting

The code from shell_window_tracker_is_window_interesting() is now
in mutter, so use it to avoid code duplication.
Comment 2 Florian Müllner 2014-01-30 18:19:48 UTC
Created attachment 267666 [details] [review]
shell-app: Track changes to MetaWindow:is-interesting

So far we have assumed that whether or not a window is interesting
is static. In general this is the case, but as it is legal for the
underlying properties to change at any time, there are of course
offenders that actually do this (flash I'm looking at ya).
While we used the property to determine whether a window should be
tracked or not, the worst case was showing windows that should be
hidden or missing windows that should be shown.
However as we nowadays base an app's running state on the number of
interesting windows, we need to be more careful in order to avoid
ending up with running apps with no windows.
Comment 3 Florian Müllner 2014-01-30 18:19:51 UTC
Created attachment 267667 [details] [review]
Use meta_window_is_interesting() consistently

Rather than going through ShellWindowTracker, we can just clean up
the code to use the underlying MetaWindow property directly.
Comment 4 Florian Müllner 2014-01-30 18:19:55 UTC
Created attachment 267668 [details] [review]
window-tracker: Remove is_window_interesting()

It is now unused, so kill it.
Comment 5 Giovanni Campagna 2014-01-30 18:42:09 UTC
Review of attachment 267668 [details] [review]:

Reimplementing it and then dropping it is a bit silly, I'd say squash the two patches.
Comment 6 Giovanni Campagna 2014-01-30 18:46:22 UTC
Review of attachment 267666 [details] [review]:

Ok
Comment 7 Florian Müllner 2014-01-30 21:14:23 UTC
(In reply to comment #5)
> Reimplementing it and then dropping it is a bit silly, I'd say squash the two
> patches.

We can't drop it on the stable branch, so the idea was to push (1) and (2) to both stable and master and only do the cleanup of (3) and (4) on master. But I don't really mind just going with (2) for the stable branch and leave the code duplication there - it's hardly likely to diverge now after having been untouched for years ...
Comment 8 Florian Müllner 2014-01-30 23:06:15 UTC
Created attachment 267680 [details] [review]
window-tracker: Use MetaWindow:skip-taskbar

Updated for changes in bug 723307.
Comment 9 Florian Müllner 2014-01-30 23:06:31 UTC
Created attachment 267681 [details] [review]
shell-app: Track changes to MetaWindow:skip-taskbar

Dto.
Comment 10 Florian Müllner 2014-01-30 23:06:44 UTC
Created attachment 267682 [details] [review]
Use meta_window_is_skip_taskbar() directly

Dto.
Comment 11 Florian Müllner 2014-01-30 23:07:02 UTC
Created attachment 267683 [details] [review]
window-tracker: Remove is_window_interesting()
Comment 12 Jasper St. Pierre (not reading bugmail) 2014-01-30 23:19:40 UTC
Review of attachment 267680 [details] [review]:

OK.
Comment 13 Jasper St. Pierre (not reading bugmail) 2014-01-30 23:22:33 UTC
Review of attachment 267681 [details] [review]:

::: src/shell-app.c
@@ +923,2 @@
 static void
+shell_app_maybe_start_stop (ShellApp *app)

I'd call this "shell_app_sync_state", since I do love my "sync" naming.

@@ +941,3 @@
+
+  if (meta_window_is_skip_taskbar (window))
+    app->running_state->interesting_windows--;

The one thing that does concern me here is that "notify" events, technically, don't need to force updates. They simply mean that the property has the possibility of being updated.

If the code that does g_object_notify on skip-taskbar makes the guarantee that the value certainly has changed, then I'd leave a comment in here about it.

@@ +1044,3 @@
   g_signal_connect (window, "unmanaged", G_CALLBACK(shell_app_on_unmanaged), app);
   g_signal_connect (window, "notify::user-time", G_CALLBACK(shell_app_on_user_time_changed), app);
+  g_signal_connect (window, "notify::skip-taskbar", G_CALLBACK(shell_app_on_skip_taskbar_changed), app);

Hm, has the whitespace in here been messed up all along?
Comment 14 Jasper St. Pierre (not reading bugmail) 2014-01-30 23:23:11 UTC
Review of attachment 267682 [details] [review]:

OK.
Comment 15 Jasper St. Pierre (not reading bugmail) 2014-01-30 23:23:22 UTC
Review of attachment 267683 [details] [review]:

OK.
Comment 16 Jasper St. Pierre (not reading bugmail) 2014-01-30 23:24:06 UTC
Review of attachment 267682 [details] [review]:

::: js/ui/workspaceThumbnail.js
@@ +511,1 @@
                win.get_meta_window().showing_on_its_workspace();

Actually, you should probably fix this indentation while you're at it.
Comment 17 Florian Müllner 2014-01-31 12:55:16 UTC
Attachment 267680 [details] pushed as 8d5771e - window-tracker: Use MetaWindow:skip-taskbar
Attachment 267681 [details] pushed as d44f40d - shell-app: Track changes to MetaWindow:skip-taskbar
Attachment 267682 [details] pushed as b4680a5 - Use meta_window_is_skip_taskbar() directly
Attachment 267683 [details] pushed as de1bb4e - window-tracker: Remove is_window_interesting()

(In reply to comment #13)
> +shell_app_maybe_start_stop (ShellApp *app)
> 
> I'd call this "shell_app_sync_state", since I do love my "sync" naming.

OK, renamed.


> The one thing that does concern me here is that "notify" events, technically,
> don't need to force updates. They simply mean that the property has the
> possibility of being updated.

Yeah. I consider it good style to only notify on actual changes, but there's no enforcement of any kind. MetaWindow:skip-taskbar does check the previous value though, so we should be fine here (otherwise we'd have to change tracking the actual "interesting" windows or something) ... added a comment as requested.
Comment 18 Florian Müllner 2014-02-15 11:08:39 UTC
*** Bug 724406 has been marked as a duplicate of this bug. ***