GNOME Bugzilla – Bug 333548
Urgency hint and alt-tab
Last modified: 2008-04-26 19:05:08 UTC
A window with the urgency hint set shows up blinking in the window list. This also works if the window is on another workspace, which is very clever. However, these windows on another workspaces cannot be reached using alt-tab. My suggestion: alt-tab lists all windows on the current workspace PLUS all windows with the urgency hint set as the FIRST items in the list. Hitting alt-tab should switch to the workspace with the urgent window and focus that window.
This is happening to me too. A window on a different workspace sets the urgent hint and begins flashing on all workspaces. I cannot alt-tab to it. I actually find this feature really annoying though. If I know a window will starting flashing urgent, I will often send it to another workspace so I can safely ignore it. Now I can't. Is there some way I can disable this feature?
*** Bug 402710 has been marked as a duplicate of this bug. ***
This is really annoying, and it can be confusing sometimes. Metacity maintainers, this sounds easy to fix, can you leave a hint on how to solve to any willing contributor?
Sure, I'll dig around and write something up.
Okay, here's an attempt: http://marnanel.blogspot.com/2007/05/how-metacity-creates-tab-list.html
Alternatively, http://blogs.gnome.org/metacity/2007/05/29/tablist/ Marking gnome-love.
Created attachment 99267 [details] [review] This is a patch that fixed this bug 2007-11-18 Alex R.M. Turner <armtuk@gmail.com> * src/display.c (meta_get_tab_entry_list): Have the list also pull windows that are in other workspaces that have the wm_state_needs_attention flag set * src/window.c (meta_window_set_demands_attention): Make windows that are on other workspaces that demand attention that aren't obscured count as being obscured
Comment on attachment 99267 [details] [review] This is a patch that fixed this bug Hi, thanks for working on this. I quickly looked over part of the patch. I have a few comments on some of the things I read. >+ // Go through all screens >+ tmp1 = display->screens; >+ while (tmp1 != NULL) >+ { >+ // Doesn't this need a cast? >+ MetaScreen *l_screen = tmp1->data; >+ // Go through all workspaces >+ tmp2 = l_screen->workspaces; >+ while (tmp2 != NULL) >+ { >+ MetaWorkspace *l_workspace = tmp2->data; >+ if (l_workspace!=workspace) >+ { >+ // Go through all windows; It would seem a lot easier to just go through all the windows on each display (one big long list), than all windows on all workspaces on all screens on each display. There's a meta_display_list_windows() function for doing this since it's needed in a number of other places in the code. >- window->wm_state_demands_attention = TRUE; >+ { >+ window->wm_state_demands_attention = TRUE; >+ } I'd prefer to not make superfluous changes like this; it makes it harder to diff against different versions in the repository. >+ MetaWorkspace *workspace = meta_core_get_active_workspace(window->screen->xscreen); core.[hc] exists so that gtk+-using files in metacity can access window.[hc] and such; we don't allow direct access. See the HACKING file for details. Anyway, since this line of code is in window.c it makes no sense to call a meta_core_* function which simply wraps a function in window.c.
Created attachment 100087 [details] [review] A patch to fix this bug thats a bit neater than before This patch fixes the bug with fewer lines of code incorporating the suggestions that Elijah put forward.
(In reply to comment #9) > Created an attachment (id=100087) [edit] > A patch to fix this bug thats a bit neater than before This is looking really good. Some points and a few nitpicks: 1) In the new third pass in meta_display_get_tab_list, shouldn't we be checking that the windows AREN'T on the current workspace before we prepend them? Otherwise we will have urgent windows on the current workspace showing up twice. 2) I prefer if (foo) { bar (); } to if (foo) bar (); too, and I understand why you've made this change in a few places around the patch (so that the final code will be consistent) but it makes the changeset larger for no semantic benefit, and that is prima facie a bad thing. Which leads us to... 3) The HACKING file explicitly says you should feel free to clean up the code-- just not in the same patch where you're fixing bugs :) (Elijah, Havoc-- this policy still applies, right?) I doubt anyone would mind if you went through when everything was working and made the change in 2) above where relevant, and it would reduce the risk of people misreading the code. 4) C99 comments aren't used anywhere else in the code and it would be good if you used block comments. The comments beginning "Consider a window obscured..." actually run on from the end of a K&R-style comment block and should be merged with it. "// End while tmp3!=NULL" is actually wrong because the variable is tmp and not tmp3. "// Question is should we put these first or not?" is something we should hash out in bugzilla first (and I think the answer is "they should go first"). 5) A suggestion: you have + if (workspace!=window->workspace) + { + obscured = TRUE; + } + + if (!obscured) + { I think this would be clearer with an "else" + if (workspace!=window->workspace) + { + /* windows on other workspaces are necessarily obscured */ + obscured = TRUE; + } + else + { but that's just my opinion.
Just my $0.02... (In reply to comment #10) > 2) I prefer > > if (foo) > { > bar (); > } > to > if (foo) > bar (); Really? The only reason I thought people liked the former better would be the case of nested ifs and elses and trying to disambiguate the dangling else, but otherwise that the latter version was considered superior. That isn't the case? > too, and I understand why you've made this change in a few places around the > patch (so that the final code will be consistent) but it makes the changeset > larger for no semantic benefit, and that is prima facie a bad thing. Which > leads us to... It's a bad thing for two reasons: - It makes the patch take longer to review - It makes it harder to diff between versions in history. > 3) The HACKING file explicitly says you should feel free to clean up the > code-- just not in the same patch where you're fixing bugs :) (Elijah, Havoc-- > this policy still applies, right?) I doubt anyone would mind if you went > through when everything was working and made the change in 2) above where > relevant, and it would reduce the risk of people misreading the code. Yeah, it applies, but I always interpreted it as things like fixing FIXMEs, small restructuring to make things clearer, etc. I never thought of it in terms of coding style changes (not even to remove evil tabs). Personally, I'd rather not get a patch to fix all tabs to spaces or to 'fix' all if blocks. I just don't like breaking diff history for something so trivial. > 4) C99 comments aren't used anywhere else in the code and it would be good if > you used block comments. The comments beginning "Consider a window > obscured..." actually run on from the end of a K&R-style comment block and > should be merged with it. "// End while tmp3!=NULL" is actually wrong because > the variable is tmp and not tmp3. "// Question is should we put these first or > not?" is something we should hash out in bugzilla first (and I think the > answer is "they should go first"). Personally, I'd prefer sticking with just C-style comments since the code is C and it'd keep it more consistent. But even if I wanted to I wonder if we could change... We used to get a bug report from Jens Granseuer after virtually every release (micro ones too!) to 'fix' C99-isms that kept creeping in. There seems to have been an (unwritten) policy that GNOME work on C89 systems, despite the idiotic handling of variable declarations it enforced. I suspect it may still have some force today, though maybe it has finally gone the way of the dinosaurs? Again, that's just my $0.02.
2) Yes, the dangling else is a problem, but I was thinking particularly of cases where you might write: if (foo) bar (); something_else (); and then someone comes along a while later and there's not so much obvious to show that something_else () isn't part of the if statement, especially if indentation is screwed up (as in the next point), and people can get confused. (I've seen books on coding style recommend the same, so it's not just me!) 3) It is a shame that there doesn't seem to be a workaround for this (and that there hasn't been for years). 4) Maybe we should ask around on planet or something and find out whether there's still policy or consensus.
Any chance of this feature being fixed before 2.22? That would be great.
Created attachment 103600 [details] [review] A new patch that addresses the concerns of Elijah et al
>>Hi, thanks for working on this. I quickly looked over part of the patch. I >>have a few comments on some of the things I read. >> >>>+ // Go through all screens >>>+ tmp1 = display->screens; >>>+ while (tmp1 != NULL) >>>+ { >>>+ // Doesn't this need a cast? >>>+ MetaScreen *l_screen = tmp1->data; >>>+ // Go through all workspaces >>>+ tmp2 = l_screen->workspaces; >>>+ while (tmp2 != NULL) >>>+ { >>>+ MetaWorkspace *l_workspace = tmp2->data; >>>+ if (l_workspace!=workspace) >>>+ { >>>+ // Go through all windows; >> >>It would seem a lot easier to just go through all the windows on each display >>(one big long list), than all windows on all workspaces on all screens on each >>display. There's a meta_display_list_windows() function for doing this since >>it's needed in a number of other places in the code. This is helpful to know. >> >>>- window->wm_state_demands_attention = TRUE; >>>+ { >>>+ window->wm_state_demands_attention = TRUE; >>>+ } >> >>I'd prefer to not make superfluous changes like this; it makes it harder to >>diff against different versions in the repository. >> It's only superfluous if you don't care about code formatting. >>>+ MetaWorkspace *workspace = meta_core_get_active_workspace(window->screen->xscreen); >> >>core.[hc] exists so that gtk+-using files in metacity can access window.[hc] >>and such; we don't allow direct access. See the HACKING file for details. >>Anyway, since this line of code is in window.c it makes no sense to call a >>meta_core_* function which simply wraps a function in window.c. >> >>(In reply to comment #9) >>> Created an attachment (id=100087) [edit] >>> A patch to fix this bug thats a bit neater than before >> >>This is looking really good. Some points and a few nitpicks: >> >>1) In the new third pass in meta_display_get_tab_list, shouldn't we be checking >>that the windows AREN'T on the current workspace before we prepend them? >>Otherwise we will have urgent windows on the current workspace showing up >>twice. >> >>2) I prefer >> >>if (foo) >>{ >>bar (); >>} >> >>to >> >>if (foo) >>bar (); >> >>too, and I understand why you've made this change in a few places around the >>patch (so that the final code will be consistent) but it makes the changeset >>larger for no semantic benefit, and that is prima facie a bad thing. Which >>leads us to... No it isn't a prima facie bad thing, because semantics is not the be all and end all of code. If all you care about is semantics, then you end up with unreadable code that no-one will ever hack on. why not just condense it to: if (foo) bar(); it would improve the grepability of the code in that you would be able to see what condition yielded the given execution. but does anyone actually do that? >> >>3) The HACKING file explicitly says you should feel free to clean up the code-- >>just not in the same patch where you're fixing bugs :) (Elijah, Havoc-- this >>policy still applies, right?) I doubt anyone would mind if you went through >>when everything was working and made the change in 2) above where relevant, and >>it would reduce the risk of people misreading the code. >> >>4) C99 comments aren't used anywhere else in the code and it would be good if >>you used block comments. The comments beginning "Consider a window obscured..." >>actually run on from the end of a K&R-style comment block and should be merged >>with it. "// End while tmp3!=NULL" is actually wrong because the variable is >>tmp and not tmp3. "// Question is should we put these first or not?" is >>something we should hash out in bugzilla first (and I think the answer is "they >>should go first"). There is a good reason for using C99 comments: // Here is a comment if (something) { doThis(); } but we don't want that code for the moment for whatever reason, so it becomes: /* // Here is a comment if (something) { doThis(); } */ We can comment it out without causing problems to our comments. I suppose you could achieve the same with: #ifdef dontincludeme if (something) { doThis(); } #endif but there are some people who believe that macros etc in C are a very bad thing and should be avoided at all possible costs. >> >>5) A suggestion: you have >> >>+ if (workspace!=window->workspace) >>+ { >>+ obscured = TRUE; >>+ } >>+ >>+ if (!obscured) >>+ { >> >>I think this would be clearer with an "else" actually I disagree, because the if (!obscured) is not part of the group that is if (workspace!=window->workspace) which is a check that stands alone, and could be expanded to include other checks that cause a window to be considered obscured, therefore the two are separate pieces that should not be concatenated. >> >>+ if (workspace!=window->workspace) >>+ { >>+ /* windows on other workspaces are necessarily obscured */ >>+ obscured = TRUE; >>+ } >>+ else >>+ { >> >>but that's just my opinion. >> >> >>Just my $0.02... >> >>(In reply to comment #10) >>> 2) I prefer >>> >>> if (foo) >>> { >>> bar (); >>> } >>> to >>> if (foo) >>> bar (); >> >>Really? The only reason I thought people liked the former better would be the >>case of nested ifs and elses and trying to disambiguate the dangling else, but >>otherwise that the latter version was considered superior. That isn't the >>case? I have never worked anywhere where the latter was tolerated, nor have I known anyone who prefers it or who could argue for it with any good reason. As you can tell, I am biased against it significantly, though I'm interested to hear arguments to the contrary if you have them. I would use this as a call to action to develop a coding formatting document for metacity so that people like me who are coming in and hacking for the first time have a clear picture of what layout is expected/tolerated/required. (Yes I know that the GNU Coding standards are there, but they are, in my opinion, rather loose). >> >>> too, and I understand why you've made this change in a few places around the >>> patch (so that the final code will be consistent) but it makes the changeset >>> larger for no semantic benefit, and that is prima facie a bad thing. Which >>> leads us to... >> >>It's a bad thing for two reasons: >>- It makes the patch take longer to review >>- It makes it harder to diff between versions in history. >> Only if you consider formatting irrelevant does the first thing have any weight. The second is true, but is the price you pay if you want consistently formatted code (which apparently you don't consider important as evidenced by the fact that you have said patches that have just formatting changes are unwelcome, and that they mess up the diff history for no good reason, and that there isn't a published policy on code formatting for Metacity, and that the there have clearly been patches accepted that didn't meet what many would consider good code formatting practices (mixing tabs and spaces for starters)). >>> 3) The HACKING file explicitly says you should feel free to clean up the >>> code-- just not in the same patch where you're fixing bugs :) (Elijah, Havoc-- well, you say that (that we shouldn't put formatting in the same patch as bug fixes), but the HACKING file doesn't. You could amend it if that is your unswerving position. (Yes I disagree as I consider formatting fixes to be vital to the health of a project, and I also consider that if you don't fix it as you are going along, it's likely to never get fixed, and to me it's more important to have good layout than it is to have a perfectly clean diff history, a few extra braces and spaces will not really confuse things much anyway as far as I can tell). >>> this policy still applies, right?) I doubt anyone would mind if you went >>> through when everything was working and made the change in 2) above where >>> relevant, and it would reduce the risk of people misreading the code. >> >>Yeah, it applies, but I always interpreted it as things like fixing FIXMEs, >>small restructuring to make things clearer, etc. I never thought of it in >>terms of coding style changes (not even to remove evil tabs). Personally, I'd >>rather not get a patch to fix all tabs to spaces or to 'fix' all if blocks. I >>just don't like breaking diff history for something so trivial. Code formatting is trivial? do you really want code like this: MetaScreen* meta_screen_for_x_screen (Screen *xscreen) { MetaDisplay *display; display = meta_display_for_x_display (DisplayOfScreen (xscreen)); if (display == NULL) return NULL; return meta_display_screen_for_x_screen (display, xscreen); } Where do you draw the line as to what is acceptable and what isn't? >> >>> 4) C99 comments aren't used anywhere else in the code and it would be good if >>> you used block comments. The comments beginning "Consider a window >>> obscured..." actually run on from the end of a K&R-style comment block and >>> should be merged with it. "// End while tmp3!=NULL" is actually wrong because >>> the variable is tmp and not tmp3. "// Question is should we put these first or >>> not?" is something we should hash out in bugzilla first (and I think the >>> answer is "they should go first"). >> >>Personally, I'd prefer sticking with just C-style comments since the code is C >>and it'd keep it more consistent. But even if I wanted to I wonder if we could >>change... >> >>We used to get a bug report from Jens Granseuer after virtually every release >>(micro ones too!) to 'fix' C99-isms that kept creeping in. There seems to have >>been an (unwritten) policy that GNOME work on C89 systems, despite the idiotic >>handling of variable declarations it enforced. I suspect it may still have >>some force today, though maybe it has finally gone the way of the dinosaurs? >> It's 2008. These standards are nearly 10 years old. There are perfectly good C99 compliant compilers for just about every platform that anyone cares about, and I've given what I believe to be a compelling case that shows C99 comments are useful (unless you prefer preprocessor macros). >> >>Again, that's just my $0.02. I seek only to foster some discussion, not to criticise or malign anyone or their opinions, but to offer a new perspective. Sometimes I'm a bit blunt, but don't mind that. I'm also passionate about coding as it's what I do pretty much 16/7 (When I'm awake) for both work and fun.
Created attachment 103606 [details] [review] New Patch that has one small alteration - the placement of a comment I moved the comment into the block as suggested by Elijah.
Created attachment 103607 [details] [review] Fixed _ALL_ the C99 Style comments and some bad tabs :( Marn pointed out that I Didn't change all the C99 comments, and I noticed that I had been editing in a vim on this instance that didn't have my nice .vimrc and had introduced evil tabs, so I got rid of them.
I think this discussion has forked rather into both a discussion of coding standards and also the patch itself. While I'm working on reviewing the patch, here are my thoughts on the other side of the discussion. (In reply to comment #15) > >>too, and I understand why you've made this change in a few places around the > >>patch (so that the final code will be consistent) but it makes the changeset > >>larger for no semantic benefit, and that is prima facie a bad thing. Which > >>leads us to... > > No it isn't a prima facie bad thing, because semantics is not the be all and > end all of code. If all you care about is semantics, then you end up with > unreadable code that no-one will ever hack on. I didn't actually say that reformatting was prima facie bad; I said that making changesets larger for no semantic benefit was bad, i.e. that I didn't think patches should intermix reformatting and semantic changes. I said later that I thought reformatting-only patches might have their place (I think Elijah disagrees with me on this point). If all I cared about was semantics, I wouldn't have used the word "semantic benefit"! [ > if (foo) bar(); ] > but does anyone actually do that? Well, I've certainly seen it around the place, yes (I'm not sure how much it's used in Metacity offhand). I think in this case it would actually be less of a cause of subsequent messups than "if (foo)\n bar ();\n" (not that I'm recommending it). > There is a good reason for using C99 comments: [...] > but we don't want that code for the moment for whatever reason, so it becomes: *nods* I've worked several places with the same rule. Myself I'd rather get rid of unused code entirely, either to an offcuts directory, or just thrown out and trusting svn to keep it safe until we need it. > I suppose you could achieve the same with: [...] > but there are some people who believe that macros etc in C are a very bad > thing and should be avoided at all possible costs. We do tend to use #if 0...#endif to comment out blocks of code in Metacity for that purpose (hence no macros used, but perhaps these people don't like preproc use at all). The only serious use of #ifdef as far as I know is for the flags passed in by autotools. > >> > >>5) A suggestion[...] I think this would be clearer with an "else" > > actually I disagree, because the if (!obscured) is not part of the group that > is if (workspace!=window->workspace) which is a check that stands alone, and > could be expanded to include other checks that cause a window to be considered > obscured, therefore the two are separate pieces that should not be > concatenated. That's a fair argument. > I would use this as a call to action to develop a coding formatting document > for metacity so that people like me who are coming in and hacking for the first > time have a clear picture of what layout is expected/tolerated/required. (Yes > I know that the GNU Coding standards are there, but they are, in my opinion, > rather loose). I think that's a pretty good plan; it took me a while to figure out how to make my code fit in here (and I still miss up sometimes). The GNU guidelines have to be loose because they have to fit thousands of projects in dozens of languages, but the main point they make is to let your new code be consistent with your old code; that's fair enough, but until you've read over the old code enough it's hard to know what WILL be consistent, and a summary of this might well be helpful. > >>> 3) The HACKING file explicitly says you should feel free to clean up the > >>> code-- just not in the same patch where you're fixing bugs :) > > well, you say that (that we shouldn't put formatting in the same patch as bug > fixes), but the HACKING file doesn't. You could amend it if that is your > unswerving position. (Yes I disagree as I consider formatting fixes to be > vital to the health of a project, and I also consider that if you don't fix it > as you are going along, it's likely to never get fixed, and to me it's more > important to have good layout than it is to have a perfectly clean diff > history, a few extra braces and spaces will not really confuse things much > anyway as far as I can tell). I say that mixing reformatting and semantic fixes is a bad idea for the reasons given above, not just because HACKING says so; it doesn't. I referred to HACKING because it has some authority as a record of what other maintainers have thought in the past, not because it's the final authority on things. > >>> this policy still applies, right?) I doubt anyone would mind if you went > >>> through when everything was working and made the change in 2) above where > >>> relevant, and it would reduce the risk of people misreading the code. > >> > >>Yeah, it applies, but I always interpreted it as things like fixing FIXMEs, > >>small restructuring to make things clearer, etc. I never thought of it in > >>terms of coding style changes (not even to remove evil tabs). [to Elijah, not Alex:] I'm not really sure how restructuring differs from untabifying. (I do find that you write that line, r1837, so presumably you know what it meant.) > >>Personally, I'd > >>rather not get a patch to fix all tabs to spaces or to 'fix' all if blocks. > >>I just don't like breaking diff history for something so trivial. [to Elijah, not Alex:] I think the thing is that we have two bad things: bad formatting is bad and makes maintaining harder (code is harder to understand, easier to misinterpret, people are less likely to want to hack on it), and breaking diff history is bad and makes maintaining harder. I think we all agree they are both bad. Elijah, you seem to be saying that diff breaking is so bad that it's never worth whatever benefit the reformat may bring. > It's 2008. These standards are nearly 10 years old. There are perfectly good > C99 compliant compilers for just about every platform that anyone cares about, > and I've given what I believe to be a compelling case that shows C99 comments > are useful (unless you prefer preprocessor macros). I am amused in a way that you had just accidentally submitted a patch which didn't close a K&R comment, thus exactly making your point. :) [This is an agreement, not a point-and-laugh.] I think expectations like these tend to stick around for a very long time unless they are reconsidered. To be honest, I doubt there are any compilers which don't understand C99 that a modern release of Metacity is likely to see the inside of, but maybe we need to ask around and be sure. > I seek only to foster some discussion, not to criticise or malign anyone or > their opinions, but to offer a new perspective. Sometimes I'm a bit blunt, but > don't mind that. I'm also passionate about coding as it's what I do pretty > much 16/7 (When I'm awake) for both work and fun. You're very welcome here! Patches do get argued over a lot, because obviously it's very important what does or doesn't go in[1]; please don't get dissuaded, because you obviously know your stuff and there's a lot to do around here! :) [1] I don't know if you've read <http://ometer.com/features.html> but it covers a lot of the background of adding new features (as opposed to bug fixes like this one) and why and how they get argued about in Metacity.
This appears to meet all the points I raised, and it does work fine. When you view the alt-Tab list from another workspace, the included window appears at the end rather than at the beginning, but I think I said verbally a while ago that this was reasonable (is there anyone who wants to say otherwise and that it's best at the start? It would be a trivial change). For the record, for some reason, during testing the window wasn't always appearing in libwnck for me, but I think that's a separate matter and not your problem; it seems to happen with or without the patch and needs separate investigation. Therefore, I am marking the patch accepted-commit-now, but we will have to decide whether to carry on the coding standards discussion here or on the list or whether I should make a Metacity blog post to talk about it on. :)
I'd love to have those windows listed first. That way you'll be able to switch to attension seeking windows quickly.
(In reply to comment #0) > My suggestion: alt-tab lists all windows on the current workspace PLUS all > windows with the urgency hint set as the FIRST items in the list. Btw, this was what I originally suggested.
The only change necessary to do that is to move the line tab_list = g_list_reverse (tab_list); to before the line /* Question is should we put these first or not? */ (We could also do it by using g_list_append() instead of g_list_prepend() but that would rather destroy the point of using g_list_prepend() and then reversing, namely that g_list_append() is slow as crap.) --------- (Something I just noticed which I didn't previously remark on: Alex, you have g_list_prepend(tab_list, l_window); and meta_display_list_windows(display); where the rest of the code would write it as g_list_prepend (tab_list, l_window); and meta_display_list_windows (display); which again would be addressed in this proposed coding standards document. I can fix these on checkin if you like, or you can if you check it in; there's no need to submit a separate patch for just two spaces.)
Comment on attachment 103606 [details] [review] New Patch that has one small alteration - the placement of a comment Sorry, folks, I thought this was the sandbox.