GNOME Bugzilla – Bug 136666
Window switching popup should use same nomenclature as window list for minimized windows
Last modified: 2005-01-21 18:52:56 UTC
Minimized windows are shown in square brackets on the window list, but not in the Alt-Tab popup... should probably do the same for both. (Apologies if it does now, I'm not sitting in front of a very recent version of metacity right at the moment...)
it doesn't currently, and it should, I agree. Are we allowed to change stuff like that for Gnome 2.6 right now? I would classify it as "bug fix", and it doesn't involve a string change.
May also be nice to make the icon dimmer, as the tasklist appears to be doing.
*** Bug 164089 has been marked as a duplicate of this bug. ***
Created attachment 36024 [details] [review] dimm the window icon if the window is minimized Sorry for the duplicate :| Here is a patch that dimms the window icon when the window is minimized. Comments welcome :)
Created attachment 36025 [details] [review] better patch Here is a better patch (read : one that does not make your computer crash when you switch workspace).
Oh, sure...submit the fixed patch after I have a valgrind log showing the invalid reads and the crash. :-) But, yeah, the new patch works nicely and is valgrind-clean. (Unfortunately, several other things in Metacity aren't. If you could fix them, you'd be my hero. *hint* *hint*) The tasklist also brackets the description of minimized windows. Shouldn't we do the same here? There's also bug 91655 which is arguing about how and whether minimized windows should be marked, but I doubt it's going anywhere anytime soon and it'd be nice to have Metacity match libwnck in the Gnome 2.10 release. Havoc: I added the 2.10.x target milestone and threw this bug on it. Kick me if you don't like that and I wasn't supposed to.
Created attachment 36026 [details] [review] Even better patch Here is an even better patch, that puts the title of minimized windows in brackets []. Note also that the function meta_ui_tab_popup_new is kinda crowded... :P
Oh, and sorry about the crashing patch :D I could very well picture PCs crashing everywhere as people were testing this ;-)
No worries. :) A few nitpicks (note that Havoc has to do the real review, but he's likely to point out these same issues): You have a fair number of interspersed tabs and spaces on lines, whereas the surrounding lines don't seem to have tabs. (In general Metacity doesn't seem to have tabs, though they seem to have annoyingly crept in--I don't know what Havoc's exact opinion on this is, but consistency with surrounding code is probably best) You have some braces that don't align. It would help with review if you could submit patches made using the -p option.
Created attachment 36029 [details] [review] So much better patch This (I hope) last patch tries to follow the metacity indentation rules : 2 spaces/no tabs.
the general rule in the metacity codebase is spaces not tabs. In emacs M-x untabify is a convenient way to "fix" a section of code.
(setq-default indent-tabs-mode nil)
Comment on attachment 36029 [details] [review] So much better patch Does libwnck use the minimized flag only or does it also show "hidden" windows as minimized? I don't remember, just asking. Doesn't GTK have a pixbuf_saturate_and_pixelate or something that can be used to dim an icon? this comment may apply to libwnck also. Do we always create icon pixbufs with alpha? If not you may need to use pixbuf_add_alpha or whatever it is. I'm a little worried that dimming a ton of icons will make alt+tab come up too slowly, but I guess we can wait and see. guint minimized should have a :1 after it in the TabEntry struct. Looks good overall, nice change. Thanks!
Created attachment 36182 [details] [review] muchos muchos better patch Here is the same patch taking care of pixbuf without alphas, and adding a :1 in the structure definition. Havoc : what do you mean by "hidden" windows ? The function gdk_pixbuf_saturate_and_pixelate exists, but it can only modify the saturation, ie it darkens or lightens the icon. Dimming is different, I think.
Comment on attachment 36182 [details] [review] muchos muchos better patch Looks sane to me. Hidden is a thing in the EWMH, that covers reasons a window may not be visible other than minimization. If libwnck is using an "if (minimized)" check then it's OK for us to do it also.
Well, Havoc already responded, but I started typing this up before seeing his response... "Hidden" windows mean windows that shouldn't be shown in the pager (see the _NET_WM_STATE_HIDDEN property in the EWMH spec). This is supposed to be defined in Metacity to mean that the window is minimized (or its ancestor if the window is a transient) or shaded or that it is on a workspace which is in the "show desktop" state and it is neither a desktop or a dock window. I say supposed to, because I apparently broke it with my patch in bug 105665... Anyway, a lot of rules, but basically see window_showing_on_its_workspace() in window.c (minus the shaded bug). Anyway, I don't think it causes any problem here from testing your patch, but I need to file a separate bug about the shaded thing (and about place.c:751 using window->minimized when it should be doing something more thorough...)
Ok, thanks for the explanations. I committed the patch, thanks for all the help. I'm marking as fixed, please feel free to re-open if you run into any issue.
Thanks for the awesome patch, Vincent. This makes alt-tabbing much nicer for me. :-)
You're welcome ;-) I confess that in this case my motivation was very selfish - I wanted to improve my own task-switching experience (hence this patch and bug #163854) :P
Created attachment 36317 [details] [review] slight change... Vincent: I'm getting crashes with the patch you applied. I didn't look at the code very closely, but I've attached a one-line patch that seems to correct the issue. Is this what you had in mind, or should I be using something else here?
Elijah : yeah, that is definitely right. I don't understand how 1. I missed that and 2. I didn't get any crash yet. Please commit if you can - I won't be able to fix that until tomorrow morning ;)
Vincent: Okay, thanks. Havoc: Since it's causing some crashes, looked like an obvious fix, and it's just a one-liner, I went ahead and committed the last attached patch. :)
Comment on attachment 36317 [details] [review] slight change... All good, thanks.