Skip to content

Commit

Permalink
add docksStartupHook for handling docks when restarted
Browse files Browse the repository at this point in the history
  • Loading branch information
@f1u77y
f1u77y committed Jan 17, 2016
1 parent f73eb1c commit 28e9f8b
Showing 1 changed file with 13 additions and 2 deletions.
15 changes: 13 additions & 2 deletions XMonad/Hooks/ManageDocks.hs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module XMonad.Hooks.ManageDocks (
-- * Usage
-- $usage
manageDocks, checkDock, AvoidStruts, avoidStruts, avoidStrutsOn,
docksEventHook,
docksEventHook, docksStartupHook,
ToggleStruts(..),
SetStruts(..),
module XMonad.Util.Types,
Expand Down Expand Up @@ -45,7 +45,7 @@ import Data.Functor((<$>))
import qualified Data.Set as S
import qualified Data.Map as M
import Data.Maybe (fromMaybe, catMaybes)
import Control.Monad (when)
import Control.Monad (when, forM_, filterM)

-- $usage
-- To use this module, add the following import to @~\/.xmonad\/xmonad.hs@:
Expand Down Expand Up @@ -146,6 +146,17 @@ docksEventHook (UnmapEvent {ev_window = w}) = do
return (All True)
docksEventHook _ = return (All True)

docksStartupHook :: X ()
docksStartupHook = withDisplay $ \dpy -> do
rootw <- asks theRoot
(_,_,wins) <- io $ queryTree dpy rootw
docks <- filterM (runQuery checkDock) wins
forM_ docks $ \win -> do
rstrut <- getRawStrut win
broadcastMessage (UpdateDock rstrut)

refresh

getRawStrut :: Window -> X (Window, Maybe (Either [CLong] [CLong]))
getRawStrut w = do
msp <- fromMaybe [] <$> getProp32s "_NET_WM_STRUT_PARTIAL" w
Expand Down

27 comments on commit 28e9f8b

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to add this to the documentation. Also, this used to not be necessary. Do you happen to know since when is this necessary? Because now it's also necessary to add it here: https://github.com/xmonad/xmonad/blob/ec1a20c727b07dfc3b0eb11dd9aed00c7909f142/man/xmonad.hs#L78 otherwise when I reset the layout, all docks are ignored.

And it gets worse. I used to have multiple instances of AvoidStruts since I didn't want all layouts to avoid struts. But then just the first instance worked. Maybe I could run docksStartupHook whenever I switched a layout, but that's just plain silly, isn't it?

Shall I open a new issue?

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, it makes very little sense to store the cache and smap in the AvoidStruts instance. It's not a per-layout thing, it's a global cache. XMonad.Util.ExtensibleState should be used here, I think, and it should be a StateExtension, not PersistentExtension.

@f1u77y
Copy link
Contributor Author

@f1u77y f1u77y commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liskin

Do you happen to know since when is this necessary?

XMonad can be used not only as standalone WM but as WM for some DE. In that case DE's panel can start earlier than xmonad. Using xmonad --replace could be another case.

It's not a per-layout thing, it's a global cache.

Yes, it can be a global thing in many users' configs(I haven't seen anyone using avoidStrutsOn) but in the case when user has one layout with avoidStrutsOn [U] and other with avoidStrutsOn [L], the cache should be per-layout.

You forgot to add this to the documentation.

Oh, yes. Will add ASAP.

Because now it's also necessary to add it here: https://github.com/xmonad/xmonad/blob/ec1a20c727b07dfc3b0eb11dd9aed00c7909f142/man/xmonad.hs#L78

Yes, that's true. And I think it'll be better option to add function setLayoutAndUpdateDocks for that.

just the first instance worked

I think that's #76

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XMonad can be used not only as standalone WM but as WM for some DE. In that case DE's panel can start earlier than xmonad. Using xmonad --replace could be another case.

And in all these cases the original code worked properly, without requiring a startupHook. I'm asking when/why did it become necessary.

Yes, it can be a global thing in many users' configs(I haven't seen anyone using avoidStrutsOn) but in the case when user has one layout with avoidStrutsOn [U] and other with avoidStrutsOn [L], the cache should be per-layout.

Okay, the cache is a per-instance thing, but smap isn't. But it's only updated in the current layout instance by messages. So at least this should be a global thing.

Yes, that's true. And I think it'll be better option to add function setLayoutAndUpdateDocks for that.

I think that'd be a VERY VERY bad idea. Please don't do this. ManageDocks should just be made to work without bizarre stupid hacks in completely unrelated places. If you don't know how to do that, I will. Just PRETTY PLEASE don't make things worse by adding things like setLayoutAndUpdateDocks. Thanks.

#76 sounds like the problem I was having. I solved it by having just a single AvoidStruts instance and adding docksStartupHook all over the place, but I'd very much like to have ManageDocks fixed and revert to my original config.

@geekosaur
Copy link
Contributor

@geekosaur geekosaur commented on 28e9f8b Sep 1, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geekosaur By when/why, I was looking for a commit sha which would (had it had more than just a subject line) explain the why. But thanks for the answer anyway. :-)

@geekosaur
Copy link
Contributor

@geekosaur geekosaur commented on 28e9f8b Sep 1, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that issue where I asked for better commit log and got nothing instead. And now it's me again digging in those nondescriptive commits looking for what's wrong. As if I wasn't angry enough already. :-(

@geekosaur
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f1u77y your reasoning is incorrect. You need to cache the struts globally, possibly with an indication of which edge the struts is on to speed up the LRUD stuff, but apply the information per layout. Otherwise you're going to need to make sure every avoidStruts including the ones not active on the current workspace has an up to date cache (this is doable but annoying), or the inactive ones will fail to apply struts once they become active.

avoidStruts should cache whether struts are enabled or disabled. The struts themselves (window XID, position and size) should be global. Don't cache which struts are being obeyed; all the information needed for that should be in the cache (instead of having to query the X server), and activation/deactivation of struts along an edge changes the list significantly in ways that the ManageHook should not have to know or care about. It's also pointless duplication, and a hassle if the active lists get out of sync.

BTW, re the earlier comment, I have used avoidStrutsOn for a while now. Probably its most common use is to have struts supported on a workspace but defaulted to disabled (avoidStrutsOn []).

@f1u77y
Copy link
Contributor Author

@f1u77y f1u77y commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I've understood that strut cache should be global. But anyway rect cache should not be global, shouldn't it?

@geekosaur
Copy link
Contributor

@geekosaur geekosaur commented on 28e9f8b Sep 1, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f1u77y
Copy link
Contributor Author

@f1u77y f1u77y commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.
But i still have some related questions:

  1. We do refresh the cache on some events because we should avoid querying tree from X. But we have to do it at the startup. So, is docksStartupHook required or can we avoid it?
  2. (not so related) That discussion was caused by messages that were not sent to inactive layout. Wouldn't it be better to have some messages that are sent only to active sub-layout and others are sent to all sub-layouts? That would also make separate ReleaseResourses(and maybe some other messages) handling not required.

@geekosaur
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docksStartupHook will always be required at startup; some windows may already exist, especially if we are doing xmonad --replace.

broadcastMessage sends to all layouts, not just the active one (which is sendMessage). I thought I explained this the first time around.

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, docksStartupHook won't be required and shouldn't be. Just make the extensible state a Maybe. Then, in modifyLayoutWithUpdate, if it's Nothing, query X, if it's Just empty, don't.

Does broadcastMessage really send a message to both sides of |||?

@f1u77y
Copy link
Contributor Author

@f1u77y f1u77y commented on 28e9f8b Sep 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geekosaur I'm talking about inactive sub-layouts like arguments of (|||) or PerWorkspace. @liskin broadcastMessage does not send a message to both side of (|||) beacuse (|||) retranslates message only to active sub-layout.

@geekosaur
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liskin, yes, we understand that constantly calling XQueryTree suits your use case, and people who want more than about 12 windows should not be supported at all because that complicates your use case.

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not suggesting to constantly call XQueryTree, why would you think that? Calling it once per xmonad restart is fine I think and is exactly what I'm suggesting!

@f1u77y
Copy link
Contributor Author

@f1u77y f1u77y commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liskin but why shouldn't startup function be in a startup hook?

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's not necessary, is all. :-)

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a cache, for heaven's sake. It's easy to tell an uninitialized cache from a cached empty value. No need to bother the user calling a startup hook manually for that!

@geekosaur
Copy link
Contributor

@geekosaur geekosaur commented on 28e9f8b Sep 1, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geekosaur
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liskin, and the reason this gets forced into the manageHook is:

  • you start up or flush the cache, it is now Nothing
  • manageHook gets notified of a strut window being mapped

At that has no choice but to repopulate the cache immediately; if it just adds the new strut, the cache is no longer Nothing and won't get rebuilt at whatever point you consider more aesthetically pleasing.

I am ignoring everything from this point on as I cannot deal with it currently.

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for whatever is going on with your health. Please do take care of it first and don't worry about this issue. It can wait. I have a workaround. @f1u77y is probably working on a fix and I am in no position to merge or not merge it. Everything will be fine.

This whole me being someone who Knows Better and insists and whatever is a misunderstanding. I said that having to deal with ManageDocks implementation details in layout reset in user configs is a very bad idea and I stand behind that claim. I never said I wouldn't accept valid arguments for the startup hook issue. I do think it's not necessary, I still don't understand why something that happens exactly once during xmonad runtime can't happen in manageHook or runLayout, but I did start the IRC discussion in order to understand it and come to a mutual agreement. (But yeah, it's certainly possible I'm just too stupid and won't ever be able to understand it. If that's the case, sorry for wasting your time.)

@f1u77y
Copy link
Contributor Author

@f1u77y f1u77y commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if geekosaur can't answer I'll ask you, @liskin. Do you suggest rewriting docksEventHook without messages? If so, I don't realize how to handle newcoming docks well, in the other case I just don't understand what do you suggest.

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f1u77y What I had in mind is moving strutMap to extensible state, wrapped in Maybe to tell if the initial queryTree must be done. Then, UpdateDock/RemoveDock handling (which I think just updates the strutMap) could be done directly in docksEventHook, followed by a simple message that tells AvoidStruts that strutMap has changed (if it actually has). Do you think something like this could work?

For some reason the current code does call getRawStruts in modifyLayoutWithUpdate sometimes and that's probably expensive, too. I think that this might not be necessary if all PropertyEvents are handled by the event hook and stored in the global strutMap. Whether this is possible or if there's a possibility of not receiving some property changes due to selectInput being called in manageDocks is unfortunately beyond my X knowledge.

Finally, now that I've been forced to read most of ManageHooks.hs, I think I won't mind if the startup hook stays (as long as it's not required to run it anywhere else than in the actual xmonad startupHook). There's 3 things a user has to add already, so it's probably a good idea to do something like this:

xmonad-contrib/XMonad/Hooks/EwmhDesktops.hs

Line 55 in e41d713

ewmh c = c { startupHook = startupHook c +++ ewmhDesktopsStartup
, and then it becomes just an implementation detail, not something a user has to deal with (unless they are upgrading from an older version, which will happen sooner or later, sadly).

(And, unfortunately, I too should now switch to other things. I have a work deadline tomorrow and I'll be offline until Monday.)

@f1u77y
Copy link
Contributor Author

@f1u77y f1u77y commented on 28e9f8b Sep 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple message

But that message will be sent to AvoidStruts as inactive sub-layout. So, this won't solve the original issue(#76) but will make only make docksStartupHook usage cleaner(as a change notable for user)

@liskin
Copy link
Member

@liskin liskin commented on 28e9f8b Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the message isn't needed at all, refresh is enough. I tested your PR today and fixed a few issues. Let's continue the discussion there (#80).

Please sign in to comment.