-
-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Breaking change in EWMH window activation #396
Comments
Well.. it invokes Extensible state, well.. in previous version (#128) it still was used to tell
Isn't this the same as was in version #128 ? If so, then read #162 for the
Well, i don't think, that having several What other fields of 'EwmhConfig' would be?
'NetActivated' extensible state can be easily amended with this information.
Not exactly. I hardly imaging someone who really cares about that confusing Also, your patch in #110 allows far lesser ways to configure focus behavior, |
Oh, this is an unfortunatel confusion. I'm not at all suggesting to go with #110! I like what you did, and I'm definitely in favor of keeping Sorry if my comments weren't clear about this. I should have stated upfront that most of your changes are wonderful and only then proceed to the critique. (This is also why I'm putting this at the top of this reply.) I'll keep it in mind next time.
Yeah, you were told by @geekosaur to use
Invoking the
Again, I'm not saying we should use the
Nope, see above.
The only point of having several
Right now I'm thinking about configrable fullscreen request handling and activation request handling (the issue at hand). In the past there was some trouble with _NET_WORKAREA (0a8e68b) which could also be made configurable instead of just removing it, as people with just one output could possibly use this feature. (Not suggesting to reintroduce this, just an example of what could possibly be made configurable in EwmhDesktops.)
Yeah, that's one option, but it'd really be better if we could avoid passing information out-of-band via extensible state. Having two
Anyone that uses a pager like rofi or alttab to switch windows will have to care as your changes break this (in the default configuration). |
Separate hook is certainly far better option, but i understand that as a
Ok, so are you suggesting to add 'focusHook' to 'XConfig'?
Well, surprisingly, that annoying default comes in handy for someone :) I've tried these programs and as you said they don't work. But it's not that So, to properly handle pagers we need to run 'FocusHook' also at I think, this's more serious problem and i don't yet know how to solve it. So, |
A few comments:
* my `FadeWindows` example was intended to be of how to use a separate
`ManageHook`, not the `logHook`
* The proposal is to add a new config record for `ewmh`, not add a new
field to `XConfig`
…On Fri, Oct 30, 2020 at 1:08 PM sgf-dma ***@***.***> wrote:
#192 <#192> changed
ewmhDesktopsEventHook to invoke logHook instead of focusing the
window that requested activation through _NET_ACTIVE_WINDOW, and now
logHook
is supposed to invoke a ManageHook through activateLogHook which consults a
global NetActivated in extensible state to tell if it's being invoked from
ewmhDesktopsEventHook. This seems convoluted to me.
Well.. it invokes logHook because i was told to rewrite it that way in
#162 <#162>.
Yeah, you were told by @geekosaur <https://github.com/geekosaur> to use
logHook but @pjones <https://github.com/pjones> suggested a
separate hook in #162
(comment)
<#162 (comment)>
and I believe that is a far better option.
Separate hook is certainly far better option, but i understand that as a
suggestion to add one more hook into 'XConfig', which (i suppose) will be
far
more difficult and require far more discussions. So, i've choosed to
modifying
existing ones.
I still think, that my original design of invoking 'manageHook' is the
better
one, but i certainly doesn't know enough to argue that this is correct from
the xmonad point of view. And i was told, that indeed it is wrong, and
changed
as was suggested (after all, it does not make any big difference for the
module).
Invoking *the* manageHook is indeed a terrible design choice and I agree
with @geekosaur <https://github.com/geekosaur> on this. Invoking *a*
ManageHook (uppercase M) is okay,
I think. Invoking *the* logHook may be less terrible, but it still
feels somewhat convoluted as it causes additional logging, refreshes,
requires out-of-band communication via extensible state, etc. And
fundamentally, both logHook and manageHook are wrong as they're built
for a different purpose:
manageHook for decisions about new windows, logHook for telling external
entities (panels, pagers, compositors, etc.) about xmonad state. For
reacting to events, there's handleEventHook, and it's no accident that the
activation code used to be there. @geekosaur
<https://github.com/geekosaur>'s example of
XMonad.Hooks.FadeWindows usage of logHook is unfortunate, as it's more
about telling the compositor something than it is about reacting to a focus
change.
Extensible state, well.. in previous version (#128
<#128>) it still was used to
tell
whether window was activated or not. There're should be a way to
distinguish
in 'ManageHook' between new and activated windows, otherwise 'FocusHook'
has
no sense.
Again, I'm not saying we should use *the* manageHook, I'm saying we
should use *a separate* ManageHook (capital M).
Ok, so are you suggesting to add 'focusHook' to 'XConfig'?
A much better and simpler design, in my opinion, is to invoke the
ManageHook
directly from ewmhDesktopsEventHook,
Isn't this the same as was in version #128
<#128> ? If so, then read
#162 <#162> for the
reasons it was changed.
Nope, see above.
Well, i don't think, that having several ewmh is better, but it does not
make any difference for me. If you think, it's better, let's do it.
The only point of having several ewmh is backwards compatibility, of
course. Ideally we'd have just one parametric ewmh, but that ship has
sailed a long time ago.
What other fields of 'EwmhConfig' would be?
Right now I'm thinking about configrable fullscreen request handling and
activation request handling (the issue at hand). In the past there was some
trouble with _NET_WORKAREA
(0a8e68b
<0a8e68b>
)
which could also be made configurable instead of just removing it, as
people
with just one output could possibly use this feature. (Not suggesting to
reintroduce this, just an example of what could possibly be made
configurable in EwmhDesktops.)
We should also add a pagerFocusHook because while one might want to ignore
browsers activating themselves, one usually doesn't want to ignore pagers
(rofi, alttab, etc.) asking to activate said browser windows.
'NetActivated' extensible state can be easily amended with this
information.
Then, it may be used in 'FocusHook' to decide how to activate (by e.g.
adding
predicate 'activatedFromPager' etc).
Yeah, that's one option, but it'd really be better if we could avoid
passing
information out-of-band via extensible state. Having two ManageHooks isn't
all that bad, is it? Especially given that you can define them as one
myFocusHook :: Bool → ManageHook and then set focusHook = myFocusHook
False and pagerFocusHook = myFocusHook True.
I'm marking this as blocker for release because #192
<#192> had been reverted
before for similar reasons (introducing a major behaviour change, #161
<#161>), and
I believe it's worthwhile to try to avoid breaking changes if possible.
Especially if said breaking change comes with a convoluted design. :-)
Not exactly. I hardly imaging someone who really cares about that confusing
focus stealing window activation behavior, which was and is default for
years.
It was reverted, as i said above, because it breaks something by using
'manageHook' not the way it was intended.
Anyone that uses a pager like rofi or alttab to switch windows will have to
care as your changes break this (in the default configuration).
Well, surprisingly, that annoying default comes in handy for someone :)
I couldn't even imagine.
I've tried these programs and as you said they don't work. But it's not
that
easy to fix, as i suppose: when the window is on the hidden workspace,
rofi/alttab first send _NET_CURRENT_DESKTOP which switches workspace
(avoiding
'FocusHook' altogether) and then send window activate request which is run
through 'FocusHook'.
So, to properly handle pagers we need to run 'FocusHook' also at
_NET_CURRENT_DESKTOP event, but 'FocusHook' just doesn't designed for that
(well, like 'ManageHook').
I think, this's more serious problem and i don't yet know how to solve it.
So,
feel free to revert and make a release.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#396 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPRFIETTJ7EP2O2IO7Z63TSNLXIRANCNFSM4TE25KNQ>
.
--
brandon s allbery kf8nh
[email protected]
|
No, I'm suggesting to add a
No, to solve this we only need a different focusHook for pagers (which we detect via https://github.com/xmonad/xmonad-contrib/pull/110/files#diff-cf9308345d1d5de4c1e403c5874d91d723ffc5d4ff79b21c4dbfe6390a592598R236). Or, alternatively, we can simply hardcode into the event handler that if a pager is asking, no hook is consulted at all and the window is just activated. It's only if we wanted to actually tweak the pager activation behaviour that we'd need to introduce the second hook and maybe also care about _NET_CURRENT_DESKTOP. Anyway, as I said, I'll be happy to make all these changes and I believe they're not difficult at all, I just wanted to hear what others think. |
Ok.
Ok. Here is proof of concept fix for pagers based on your code. https://github.com/sgf-dma/xmonad-contrib/tree/x.h.focus-2 with following
we don't need
In fact, because with this _NET_CURRENT_DESKTOP event sent by pagers
Ok, i didn't write any Haskell for a long time now, so it would be better, if |
Oh, I just realized that we already discussed a possible extensible API of EwmhDesktops with @geekosaur in #109. How could I forget that? I guess I better read up! :-)) Anyway, I'm working on this now, I'll have some code to show today or tomorrow I hope. |
Especially insteresting is this comment of mine: #109 (comment). Apparently in 2016 I decided not to add a config parameter to |
Oh, right, back then I was talking about configurability of Also I'll try to summarize what @geekosaur and me were talking about: it seems we wanted to turn I think that maybe adding an So, yeah, |
…gnored This makes window switching apps like rofi and alttab able to activate windows even if the logHook doesn't activate them (which it doesn't by default, and that's a regression). Related: xmonad#396
…gnored This makes window switching apps like rofi and alttab able to activate windows even if the logHook doesn't activate them (which it doesn't by default, and that's a regression). Related: xmonad#396 Related: xmonad#110
…th it We need to make EwmhDesktops configurable: not just workspaceListTransform, but users also need a way to customize the handling of _NET_ACTIVE_WINDOW, enable/disable fullscreen handling, etc. Instead of having them manually piece together chains of hooks in their XConfigs, let's introduce a EwmhConfig record and a `ewmh'` function that takes care of everything. Related: xmonad#396 Related: xmonad#109
TODO: documentation in X.H.EwmhDesktops TODO: adapt X.H.Focus TODO: alternatively just set urgency? Related: xmonad#396 Related: xmonad#110
Related: xmonad#396 Related: xmonad#109
So far I got this: #399 Usage in my xmonad.hs looks like this:
Tomorrow I'll try to adapt X.H.Focus and address all the other TODOs (documentation, etc.). |
…gnored This makes window switching apps like rofi and alttab able to activate windows even if the logHook doesn't activate them (which it doesn't by default, and that's a regression). Related: xmonad#396 Related: xmonad#110
…th it We need to make EwmhDesktops configurable: not just workspaceListTransform, but users also need a way to customize the handling of _NET_ACTIVE_WINDOW, enable/disable fullscreen handling, etc. Instead of having them manually piece together chains of hooks in their XConfigs, let's introduce a EwmhConfig record and a `ewmh'` function that takes care of everything. Related: xmonad#396 Related: xmonad#109
TODO: documentation in X.H.EwmhDesktops TODO: adapt X.H.Focus TODO: alternatively just set urgency? Related: xmonad#396 Related: xmonad#110
Related: xmonad#396 Related: xmonad#109
…gnored This makes window switching apps like rofi and alttab able to activate windows even if the logHook doesn't activate them (which it doesn't by default, and that's a regression). Related: xmonad#396 Related: xmonad#110
…th it We need to make EwmhDesktops configurable: not just workspaceListTransform, but users also need a way to customize the handling of _NET_ACTIVE_WINDOW, enable/disable fullscreen handling, etc. Instead of having them manually piece together chains of hooks in their XConfigs, let's introduce a EwmhConfig record and a `ewmh'` function that takes care of everything. Related: xmonad#396 Related: xmonad#109
TODO: documentation in X.H.EwmhDesktops TODO: changelog TODO: adapt X.H.Focus Related: xmonad#396 Related: xmonad#110
Don't assume ewmh/docks are the only xmonad config combinator out there. Related: xmonad#396 Related: xmonad#399
Related: xmonad#396 Related: xmonad#399
Related: xmonad#396 Related: xmonad#399 Related: xmonad#192
…gnored This makes window switching apps like rofi and alttab able to activate windows even if the logHook doesn't activate them (which it doesn't by default, and that's a regression). Fixes: 45052b9 ("X.H.EwmhDesktops. run 'logHook' for activated window.") Related: xmonad#396 Related: xmonad#110 Related: xmonad#192
…tom with it We need to make EwmhDesktops configurable: not just workspaceListTransform, but users also need a way to customize the handling of _NET_ACTIVE_WINDOW, enable/disable fullscreen handling, etc. Instead of having them manually piece together chains of hooks in their XConfigs, let's introduce a EwmhConfig record and a `ewmh'` function that takes care of everything. Related: xmonad#396 Related: xmonad#109
TODO: documentation in X.H.EwmhDesktops TODO: changelog TODO: adapt X.H.Focus Related: xmonad#396 Related: xmonad#110
Ugh I really don't care to read about everything that is going on here. All I know is that _NET_ACTIVE_WINDOW activation has stopped working from taffybar. @liskin do you have any idea why this might be? EDIT: okay, now I see the change is described in EwmhDesktops. Not really sure why this had to be done this way. Pretty confusing for users... and seems like the interface is worse since now more configuration is required to get normal behavior. |
It didn't have to be done this way, and it will be done differently, once I get my brain back into a usable state and turn #399 into something mergeable. |
(Definitely do feel free to merge #399 into your local fork and use it. I've been running that code for months without any problems, it's just missing docs and stuff, and I might take a slightly different approach to API/configuration now that we have https://github.com/xmonad/xmonad-contrib/blob/master/XMonad/Util/ExtensibleConf.hs, but if you just want to bring back sane behaviour, #399 is easiest.) |
@IvanMalison Oh, and I just realized that you commented that "_NET_ACTIVE_WINDOW activation has stopped working from taffybar" several months after I pushed 41ba7fd, which suggests that taffybar doesn't fill in source indication. If it did, window activation from taffybar would still work. Indeed, https://github.com/taffybar/taffybar/blob/4d3227e9cd8308c1e43846496a8a7ab417ef8e5b/src/System/Taffybar/Information/X11DesktopInfo.hs#L191 should probably be
instead. (This |
|
TODO Fixes: xmonad#396 Related: xmonad#110 Related: xmonad#192 Related: xmonad#128
xmonad#192 introduced a breaking change: * `XMonad.Hooks.EwmhDesktops`   `ewmh` function will use `logHook` for handling activated window. And now  by default window activation will do nothing. This breaking change can be avoided if we designed that a bit differently. xmonad#192 changed `ewmhDesktopsEventHook` to invoke `logHook` instead of focusing the window that requested activation and now `logHook` is supposed to invoke a `ManageHook` through `activateLogHook` which consults a global `NetActivated` extensible state to tell if it's being invoked from `ewmhDesktopsEventHook`. This seems convoluted to me. A better design, in my opinion, is to invoke the `ManageHook` directly from `ewmhDesktopsEventHook`, and we just need a way to configure the hook. Luckily, we now have `X.U.ExtensibleConf` which makes this straightforward. So we now have a `setEwmhActivateHook`, and the activation hook defaults to focusing the window, undoing the breaking change. Fixes: xmonad#396 Related: xmonad#110 Related: xmonad#192 Related: xmonad#128
xmonad#192 introduced a breaking change: * `XMonad.Hooks.EwmhDesktops`   `ewmh` function will use `logHook` for handling activated window. And now  by default window activation will do nothing. This breaking change can be avoided if we designed that a bit differently. xmonad#192 changed `ewmhDesktopsEventHook` to invoke `logHook` instead of focusing the window that requested activation and now `logHook` is supposed to invoke a `ManageHook` through `activateLogHook` which consults a global `NetActivated` extensible state to tell if it's being invoked from `ewmhDesktopsEventHook`. This seems convoluted to me. A better design, in my opinion, is to invoke the `ManageHook` directly from `ewmhDesktopsEventHook`, and we just need a way to configure the hook. Luckily, we now have `X.U.ExtensibleConf` which makes this straightforward. So we now have a `setEwmhActivateHook`, and the activation hook defaults to focusing the window, undoing the breaking change. Fixes: xmonad#396 Related: xmonad#110 Related: xmonad#192 Related: xmonad#128
Fixes: xmonad#396 Related: xmonad#192 Related: xmonad#128
xmonad#192 introduced a breaking change: * `XMonad.Hooks.EwmhDesktops`   `ewmh` function will use `logHook` for handling activated window. And now  by default window activation will do nothing. This breaking change can be avoided if we designed that a bit differently. xmonad#192 changed `ewmhDesktopsEventHook` to invoke `logHook` instead of focusing the window that requested activation and now `logHook` is supposed to invoke a `ManageHook` through `activateLogHook` which consults a global `NetActivated` extensible state to tell if it's being invoked from `ewmhDesktopsEventHook`. This seems convoluted to me. A better design, in my opinion, is to invoke the `ManageHook` directly from `ewmhDesktopsEventHook`, and we just need a way to configure the hook. Luckily, we now have `X.U.ExtensibleConf` which makes this straightforward. So we now have a `setEwmhActivateHook`, and the activation hook defaults to focusing the window, undoing the breaking change. Fixes: xmonad#396 Related: xmonad#110 Related: xmonad#192 Related: xmonad#128
Fixes: xmonad#396 Related: xmonad#192 Related: xmonad#128
xmonad#192 introduced a breaking change: * `XMonad.Hooks.EwmhDesktops`   `ewmh` function will use `logHook` for handling activated window. And now  by default window activation will do nothing. This breaking change can be avoided if we designed that a bit differently. xmonad#192 changed `ewmhDesktopsEventHook` to invoke `logHook` instead of focusing the window that requested activation and now `logHook` is supposed to invoke a `ManageHook` through `activateLogHook` which consults a global `NetActivated` extensible state to tell if it's being invoked from `ewmhDesktopsEventHook`. This seems convoluted to me. A better design, in my opinion, is to invoke the `ManageHook` directly from `ewmhDesktopsEventHook`, and we just need a way to configure the hook. Luckily, we now have `X.U.ExtensibleConf` which makes this straightforward. So we now have a `setEwmhActivateHook`, and the activation hook defaults to focusing the window, undoing the breaking change. Fixes: xmonad#396 Related: xmonad#110 Related: xmonad#192 Related: xmonad#128
Fixes: xmonad#396 Related: xmonad#192 Related: xmonad#128
#192 introduced a breaking change:
I believe this breaking change can be avoided if we designed that a bit differently.
#192 changed
ewmhDesktopsEventHook
to invokelogHook
instead of focusing the window that requested activation through_NET_ACTIVE_WINDOW
, and nowlogHook
is supposed to invoke aManageHook
throughactivateLogHook
which consults a globalNetActivated
in extensible state to tell if it's being invoked fromewmhDesktopsEventHook
. This seems convoluted to me.A much better and simpler design, in my opinion, is to invoke the
ManageHook
directly fromewmhDesktopsEventHook
, and we just need to figure out how to configure it. I propose addingewmh'
which would take an additionalEwmhConfig
record with afocusHook
field. Additionally, this record can be extended with ahandleFullscreen :: Boolean
to makeewmhFullscreen
unnecessary (and thus impossible to get the order betweenewmh
andewmhFullscreen
wrong, improving the user experience). We should also add apagerFocusHook
because while one might want to ignore browsers activating themselves, one usually doesn't want to ignore pagers (rofi, alttab, etc.) asking to activate said browser windows.I'm marking this as blocker for release because #192 had been reverted before for similar reasons (introducing a major behaviour change, #161), and I believe it's worthwhile to try to avoid breaking changes if possible. Especially if said breaking change comes with a convoluted design. :-)
The text was updated successfully, but these errors were encountered: