GNOME Bugzilla – Bug 723308
Track changes to windows being interesting
Last modified: 2014-02-15 11:08:39 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).
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.
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.
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.
Created attachment 267668 [details] [review] window-tracker: Remove is_window_interesting() It is now unused, so kill it.
Review of attachment 267668 [details] [review]: Reimplementing it and then dropping it is a bit silly, I'd say squash the two patches.
Review of attachment 267666 [details] [review]: Ok
(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 ...
Created attachment 267680 [details] [review] window-tracker: Use MetaWindow:skip-taskbar Updated for changes in bug 723307.
Created attachment 267681 [details] [review] shell-app: Track changes to MetaWindow:skip-taskbar Dto.
Created attachment 267682 [details] [review] Use meta_window_is_skip_taskbar() directly Dto.
Created attachment 267683 [details] [review] window-tracker: Remove is_window_interesting()
Review of attachment 267680 [details] [review]: OK.
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?
Review of attachment 267682 [details] [review]: OK.
Review of attachment 267683 [details] [review]: OK.
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.
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.
*** Bug 724406 has been marked as a duplicate of this bug. ***