fix xmonad/xmonad#21

[f1u77y]

this seems to fix the regression(i've tested with dzen2 and plasmashell) but i'm not sure that this wouldn't cause the issue (couldn't reproduce it), fix of which caused the regression

[geekosaur]

The right answer here is that docksEventHook should watch for the strut properties changing and flush the cache. I've had a note about that in my stuff for a while (because we don't currently see it at all, so even without caching we don't react to struts changing until something forces the layout to run).

[geekosaur]

I should probably describe the full implementation.

1. docksEventHook should be mandatory.
2. manageDocks should select for property events on every dock window it (un)manages. It should also retrieve any strut information on the window and broadcast it with refresh to the layout, along with the window's XID.
3. docksEventHook should check for PropertyNotify events on struts and broadcast with refresh to the layout.
4. The strut cache (probably a Map Window ...) can now be stored in the avoidStruts constructor, and consulted when laying out the workspace. XQueryTree() is not needed, so it should not be slow on a workspace with many windows (remember that Decoration adds a bunch of windows! Easy way to get quadratic behavior from XQueryTree, and for that matter anything that iterates the StackSet).
5. docksEventHook should also watch for DestroyWindow events and broadcast-with-refresh to the layout, so avoidStruts can remove the strut for that window from the cache.

You need to broadcast-with-refresh so that xmonad will re-layout the workspace after a strut change; otherwise the strut will only take effect when something else forces a re-layout. (Docks get doIgnore-d, so will not trigger a re-layout.)

This requires at least two new "internal" struts-related messages: one to indicate the current strut for a window, and the other to delete the window from the cache.

[geekosaur]

Oh, another note re slowness of the current implementation: caching won't help much, because the real problem is that XQueryTree on the root window will return every window the X server knows about. This includes but is not limited to:

• top level windows
• unmanaged windows
• unmapped windows (i.e. on workspaces that aren't visible)
• internal windows: many toolkits will use a separate window for (almost) any UI element that can be clicked. Some will use one for every single UI element, no matter how simple.

You can easily be iterating through thousands of windows looking for the one or two that have struts. I don't recall the code offhand, but if it does not already restrict itself to direct children of the root window, it's also issuing two getProperty calls for each of those windows.

[f1u77y]

@geekosaur done

[f1u77y]

can this be merged?

[liskin]

I am in no position to merge this or even decide whether it should be, but as someone who has had contributions accepted in the past I'd like to comment on this a bit anyway:

I really don't like your commit messages, they make it very difficult to review the changes. "fix xmonad/xmonad#21" is very much github-specific and is of no value to anyone who (a) for some reason doesn't have access to github/the internet at the moment, or (b) doesn't already know that it's exactly github they need. And then it doesn't explain what was wrong and what was done to make it better. The other commits look like a series of "change this" and "change that" and "code review amendments" etc. and in my opinion the entire branch should be appropriately rearranged and perhaps some of the commits squashed to tell a clear story of what was wrong and how it was fixed. @geekosaur's comments here could be a good material to put in the commit messages.

Here are some good tips on creating nice commits that are a pleasure to read, review and merge:
http://who-t.blogspot.cz/2009/12/on-commit-messages.html
https://github.com/erlang/otp/wiki/Writing-good-commit-messages

[geekosaur]

Aside from agreeing with liskin above, I don't see anything wrong here offhand. I'd prefer having more people test it before merging, but as about th only way we'd get any testing is to merge it....