GNOME Bugzilla – Bug 336184
More inclusive window groupings
Last modified: 2006-03-30 19:27:03 UTC
Below is a patch which is supposed to make MetaGroup's more inclusive. It is supposed to help the patch on Bug #94682. Without this patch, Alt+F6 doesn't include all windows in an application. See comment #25 for examples where that is problematic. The patch fixes that without any negative side-effects that I have seen so far. The grouping behaviour is still not optimal. For example, two separate Firefox windows (or two Nautilus windows) are thrown in the same group because they both share the xgroup_leader window. It seems like it might be very hard to fix that problem.
Created attachment 62117 [details] [review] fixes better groupings
I believe group.c was supposed to already do that; I just didn't catch the bug in Thomas' patch in bug 328211. I'll attach a patch which makes transiency correctly override grouping and which fixes the behavior you saw in bug 94682.
Created attachment 62250 [details] [review] Alternative fix, by allowing the inclusive grouping that used to exist but was broken by patch in bug 328211
Sorry about that. What was the bug?
Thomas: No worries, I missed it too when I was reviewing it. The goal of your patch was to have transiency override the normal grouping; but instead of using the ancestor's group in such cases, your patch used the ancestor _as_ the group. This fails because the group_leader is a special window that isn't an existing top_level window in most cases while the transient_for hint always is (for non-buggy apps, anyway).
When walking up xtransient_for, I think in some places we were using tortoise-and-hare to avoid getting in infinite loops. Would it be appropriate to use that bit of paranoia here also? I don't see anything obviously broken about the patch in general.
Yeah, that paranoia probably makes sense. I'll add an updated patch that does that. It's probably worth noting that the tortoise-and-hare thing in meta_window_foreach_ancestor() only prevents certain kinds of infinite loops (you could have a longer cycle that goes undetected). It also doesn't handle group-modal dialogs (not that my patch did either). But all those extra complications are all part of bug 126489, so using the tortoise thing until it's fixed sounds like a good idea. :)
Created attachment 62412 [details] [review] Fixed fix to 328211 ;-)
I just looked at Björn's patch again, and now understand the flow to it. I think the other patch is a little more clear, and it also handles some nastier cases (e.g. multiple top-level self-transient window part of the same app) -- though cases that we'll probably never actually see. Björn: Thanks for the work; I think you still did the majority of the work by discovering the issue, listing a concrete testcase, and coming up with an initial patch. :-) 2006-03-29 Elijah Newren <newren gmail com> Fix grouping in the presence of ancestors; caught by Björn. #336184