X.H.ManageDocks: Init strut cache on demand if necessary #406
Conversation
|
@geekosaur We had a heated argument about this in 2016 so I thought I'd just show you the code instead. I still believe this is a good solution and I still don't understand what your objections were. Users are still getting bitten by this, because they're still using configs with just a Would you mind taking a look one last time? |
This makes docksStartupHook unnecessary. That is desirable because we didn't add a changelog entry about it being necessary, and 4 years after its introduction we still have users grabbing old configs and reporting (xmonad/xmonad#21 (comment)) that ManageDocks doesn't work properly. I would love to finally settle this. Full story follows: xmonad-contrib 0.12 introduced (00be056, merged in April 2013) caching to ManageDocks to avoid queryTree calls on every runLayout, which brought in several bugs. Attempts to fix these bugs in xmonad-contrib 0.13 introduced (28e9f8b, merged in February 2016) a breaking change in ManageDocks that required users to add docksStartupHook (or docks, after e38fb3b got merged in November 2016) to their configs for avoidStruts to still work after xmonad restart. Unfortunately this was never mentioned in the changelog nor in any compilation warning (which get discarded by xmonad --recompile anyway !!!), so as of March 2020 we still have users being bitten by this. Back in September 2016 in a discussion about other bugs introduced in 28e9f8b I suggested that we use Maybe to indicate whether the cache had been initialized and initialize it on demand when it had not. Unfortunately I wasn't sufficiently clear about what I meant and Brandon was going through some health issues, so we just got into a heated argument and ended up merging a suboptimal solution. :-( And since we're _still_ getting complaints from users every now and then, I believe it's worth dealing with even after all these years. If nothing else, let this serve as a reminder that breaking users' configs without any warning is wrong. (Oh and we should probably stop hiding xmonad.hs compilation warnings, otherwise we can't ever hope to provide smooth deprecation and upgrade paths.) Fixes: 00be056 ("Cache results from calcGap in ManageDocks") Fixes: 28e9f8b ("add docksStartupHook for handling docks when restarted") Fixes: e38fb3b ("Make usage of ManageDocks simpler and more robust") Related: xmonad#118 Related: xmonad#30 Related: xmonad#80 Related: xmonad/xmonad#21
liskin
force-pushed
the
managedocks-cache-on-demand
branch
from
4c59cbf
to
14dc3a7
Compare
|
There were objections to this in the past, but I believe those were due to misunderstanding what I was about to implement, so I don't think they're relevant. Still, I'm setting the merge timer on this one to two weeks, just to be sure. @geekosaur, please take a look, thanks! |
|
Can you point me at the argument?
I'm not seeing anything in particular wrong with this implementation aside
from some misgivings about doing a potentially expensive XQueryTree in
quasi-random places.
…On Sat, Jan 23, 2021 at 5:29 AM Tomáš Janoušek ***@***.***> wrote:
There were objections to this in the past, but I believe those were due to
misunderstanding what I was about to implement, so I don't think they're
relevant. Still, I'm setting the merge timer on this one to *two weeks*,
just to be sure. @geekosaur <https://github.com/geekosaur>, please take a
look, thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#406 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPRFIDUJOL7SZRRU4UCDETS3KQG7ANCNFSM4TX4G5PA>
.
--
brandon s allbery kf8nh
allbery.b@gmail.com
|
#30 (comment) and #30 (comment), but these links don't really work any more (which is especially ironic, as this exact thread is where I mentioned that only using github numbers in commits messages is bad for future references), so here's a screenshot to help you find it:
I think we also continued the discussion on IRC and somehow still couldn't agree on how many times I'm suggesting to call queryTree. (The answer has always been: once.) |
Yeah, obviously we need to be careful. But let's not forget the fact that the current implementation is in reality more "quasi-random", as the layout runs at least once before the startupHook: https://github.com/xmonad/xmonad/blob/master/src/XMonad/Main.hs#L252-L263 |
This is primarily a cleanup to make it easier to use `setDocksMask` from the on-demand cache init (see further commits), but it makes the code nicer: - the logic to refresh and cache a strut is now concentrated in `updateStrut` instead of being spread over `updateStrutCache` and `docksEventHook` - the logic to initialize the cache if not yet initialized is now concentrated in `maybeInitStrutCache` instead of being spread over `initStrutCache` and `getStrutCache`, so the dual-purpose return type of `getStrutCache` is no more - the logic to detect changes and refresh is now always handled by `XS.modifiedM` instead of an additional `||` Related: xmonad#406
This is primarily a cleanup to make it easier to use `setDocksMask` from the on-demand cache init (see further commits), but it makes the code nicer: - the logic to refresh and cache a strut is now concentrated in `updateStrut` instead of being spread over `updateStrutCache` and `docksEventHook` - the logic to initialize the cache if not yet initialized is now concentrated in `maybeInitStrutCache` instead of being spread over `initStrutCache` and `getStrutCache`, so the dual-purpose return type of `getStrutCache` is no more - the logic to detect changes and refresh is now always handled by `XS.modifiedM` instead of an additional `||` Related: xmonad#406
Description
This makes docksStartupHook unnecessary. That is desirable because we
didn't add a changelog entry about it being necessary, and 4 years after
its introduction we still have users grabbing old configs and reporting
(xmonad/xmonad#21 (comment)) that
ManageDocks doesn't work properly.
I would love to finally settle this.
More context
Full story follows:
xmonad-contrib 0.12 introduced (00be056, merged in April 2013)
caching to ManageDocks to avoid queryTree calls on every runLayout,
which brought in several bugs. Attempts to fix these bugs in
xmonad-contrib 0.13 introduced (28e9f8b, merged in February 2016) a
breaking change in ManageDocks that required users to add
docksStartupHook (or docks, after e38fb3b got merged in November
2016) to their configs for avoidStruts to still work after xmonad
restart. Unfortunately this was never mentioned in the changelog nor in
any compilation warning (which get discarded by xmonad --recompile
anyway !!!), so as of March 2020 we still have users being bitten by
this.
Back in September 2016 in a discussion about other bugs introduced in
28e9f8b I suggested that we use Maybe to indicate whether the cache
had been initialized and initialize it on demand when it had not.
Unfortunately I wasn't sufficiently clear about what I meant and Brandon
was going through some health issues, so we just got into a heated
argument and ended up merging a suboptimal solution. :-(
And since we're still getting complaints from users every now and
then, I believe it's worth dealing with even after all these years.
If nothing else, let this serve as a reminder that breaking users'
configs without any warning is wrong.
(Oh and we should probably stop hiding xmonad.hs compilation warnings,
otherwise we can't ever hope to provide smooth deprecation and upgrade
paths.)
Fixes: 00be056 ("Cache results from calcGap in ManageDocks")
Fixes: 28e9f8b ("add docksStartupHook for handling docks when restarted")
Fixes: e38fb3b ("Make usage of ManageDocks simpler and more robust")
Related: #118
Related: #30
Related: #80
Related: xmonad/xmonad#21
Checklist
I've read CONTRIBUTING.md
I tested my changes with xmonad-testing
I updated the
CHANGES.mdfile(n/a) I updated the
XMonad.Doc.Extendingfile (if appropriate)