GNOME Bugzilla – Bug 168455
Dimmed icons are not shown in tabpopup when "Show desktop" is used.
Last modified: 2005-11-12 00:34:36 UTC
Please describe the problem: The TabEntry only creates a dimm_icon if the window is minimized. The "Show Desktop" functionality is implemented, not by minimizing windows (which would be incorrect), but via an XChangeProperty with a metacity defined Xatom. tabentry.c should be checking if we are in "Show Desktop" mode and dimming icons in the TabPopup accordingly. Steps to reproduce: 1. Open a window 2. Hit "Show Desktop" 3. Notice that your window in the window list has brackets "[]" around it and a dimmed icon. 4. Alt+tab cycle through windows and notice that no dimm icon is presented here. Actual results: Expected results: I expect the icon in the TabPopup to be dimm Does this happen every time? Yes Other information: I've looked at trying to call XGetWindowProperty on the root window to see if the atom_net_showing_desktop is set however this strikes me as being nasty code. I also can't seem to access the current MetaWorkspace from tabpopup.c, nor do I want to break the API for such a stupid reason. I need access to some variable/method that tells me if the current workspace is in "Show Desktop" mode. Is there an easy way to do this? Am I missing something stupid?
Take a look at meta_window_showing_on_its_workspace(). :)
To be a little more clear, I don't think the code in tabpopup.c should be trying to figure out whether the workspace is in Show Desktop mode. The code in screen.c that sets up the entries is currently passing whether the window is minimized, but that looks like an error to me--it should instead be passing whether the window is showing or not.
Created attachment 37983 [details] [review] Modifies screen.c to make icons dimmed in TabPopup This patch fixes this bug. However it overloads the semantics of MetaWindow.minimized to mean either the window is minimized or the window should _look_ like it's minimized. I think the latter meaning is already used by Metacity and causes problems. For instance 1) Open a terminal on a blank workspace 2) launch "wm-tester --icon-windows" 3) click "show desktop" 4) alt-tab to show the terminal 5) alt-tab again through all the windows. Notice that the wm-tester windows have ugly black marks to the tight of their name (assuming R-to-L locale) in the window list. I think this is because the windows are marked as being minimized but are not actually minimized, they're just left over from the XChangeProperty call to display workspace. In fact, if you minimize some of the wm-tester windows before you do "show desktop" you notice that the truly minimized windows don't show black marks whereas the windows that appear minimized do. I'll have a poke at the code later to see if this is a bug in the applet itself. So there's an existing semantic overload of MetaWindow.minimized. It breaks things, but changing the API to have a MetaWindow.minimized and a MetaWindow.look_minimized is probably crack. So Elijahs' suggestion of an MetaWindow.is_window_showing seems the best option, but some programs may have to be told about the change in semantics.
You're changing the semantics of the minimized field of MetaTabEntry, not of MetaWindow. ;-) The patch misses some cases. For example, if you use Ctrl-Alt-Tab, it treats the panels and desktop as not showing, even though they clearly are. If you had a transient window without the skip_taskbar hint set and the parent was minimized, then you'd probably not display the transient correctly either. Basically, your patch attempts to figure out "whether a window should be showing", but you don't include all the cases; it'd be easier to just use the function that already handles them all. Those are interesting bugs you point out. I couldn't quite duplicate one thing--I get the ugly black lines no matter what. However, I think it's a separate bug. Anyway, changing the MetaTabEntry's field from minimized to hidden does sound fine to me (MetaWindow's minimized field should not be changed and should stay as it is.) Also, we shouldn't have to update apps because Metacity already provides information about which windows are hidden via the _NET_WM_STATE_HIDDEN property.
Created attachment 38010 [details] [review] Patch uses meta_window_showing_on_its_workspace() Doh! I should've put two and two together.
Looks good, but note that !meta_window_showing_on_its_workspace(window) already includes window->minimized, so you can shorten that line. Also you need a space between a function name and the open parenthesis to match the code style elsewhere.
Created attachment 38012 [details] [review] Shortened line and added space I think my new .sig will be "Elijah is correct" :)
If you did that, I'd have to make my new .sig be "Aidan needs to give me $1000". ;-) Patch looks great to me. For post-2.10, we'd probably want to s/minimized/hidden/ in the MetaTabEntry struct, but considering how close we are to hard code freeze (it's tomorrow), I'd guess that we'd just want to apply that patch as it is. But, of course, Havoc will have to weigh in. Havoc: look okay to you?
Havoc: I think the patch is fairly obvious and safe, so I'm committing in order to make it in the tarball today. Let me know if that's a mistake and we need to revert it... 2005-02-28 Elijah Newren <newren@gmail.com> Patch from Aidan Delaney to make sure that icons in the alt-tab popup are dimmed for all hidden windows, not just minimized ones. Fixes #168455. * src/screen.c: (meta_screen_ensure_tab_popup): make use meta_window_showing_on_its_workspace() instead of just checking if the window is minimized.
Good call, thanks.
Rob: But now we're going to forget to s/minimized/hidden/ in the MetaTabEntry struct... ;-)
You really should do s/minimized/hidden/ ...
Reopening so we don't forget to do the s/minimized/hidden/ ...
Created attachment 54540 [details] [review] s/minimized/hidden/ patch to screen.c One of three patches
Created attachment 54541 [details] [review] s/minimized/hidden/ patch Second of three patches
Created attachment 54542 [details] [review] s/minimized/hidden/ MetaTabPopup
It'd be easier if you just put it all in one patch...
Created attachment 54571 [details] [review] Patch to s/minimized/hidden/ in MetaTabPopup struct
Created attachment 54572 [details] [review] s/hidden/minimized/ in MetaTabPopup D'oh. There was some junk at the start of the previous patch.
The extra junk at the start of the patch can be ignored just fine by the patch command, so the previous patch would have been fine too. Anyway, looks good. Go ahead and commit (if you don't have a cvs account, just post a ChangeLog entry here in a comment and I'll do it for you).
ChangeLog entry (edit it as you see fit, if you think it's too long). I don't have CVS commit access. 2005-11-11 Aidan Delaney <a.j.delaney@brighton.ac.uk> Changed the minimized field of the MetaTabEntry struct to hidden. Fixes reopened bug #168455. * src/tabpopup.h: (struct _MetaTabEntry): renamed the minimized field to hidden * src/tabpopup.c: (meta_ui_tab_popup_new): changed references from entries[i].minimized to entries[i].hidden * src/screen.c: (meta_screen_ensure_tab_popup): changed references from entries[i].minimized to entries[i].hidden
I modified the changelog slightly for brevity, but it looks great. Thanks for the patch! 2005-11-11 Aidan Delaney <a.j.delaney@brighton.ac.uk> * src/tabpopup.h: (struct _MetaTabEntry): * src/tabpopup.c: (meta_ui_tab_popup_new): * src/screen.c: (meta_screen_ensure_tab_popup): Changed the 'minimized' field of the MetaTabEntry struct to 'hidden'. Fixes reopened bug #168455.