xmonad / xmonad-contrib

Contributed modules for xmonad
https://xmonad.org
BSD 3-Clause "New" or "Revised" License
586 stars 274 forks source link

The `X.H.Focus` module (and necessary changes) need to be resubmitted #162

Closed pjones closed 3 years ago

pjones commented 7 years ago

The default focusing behavior was changed too much in #128 so I reverted it with #161.

@sgf-dma Please refactor and resubmit.

sgf-dma commented 7 years ago

Hi.

I've tried with activated firefox window on Debian 8 and with two configs: minimal

import XMonad

import XMonad.Hooks.EwmhDesktops

main :: IO ()
main = do
        let xcf = ewmh $ def
                    { modMask = mod4Mask
                    , manageHook = manageHook def
                    }
        xmonad xcf

and from XMonad/Config/Example.hs. In both cases, when ewmh is enabled and no X.H.Focus combinators are used (with Example.hs it is enabled in desktopConfig) focus does not change upon firefox window activation (neither when firefox window is on another workspace, nor on current).

Though, i've observed strange behavior, that when firefox window is on current workspace and ewmh is not used, focus actually changes without moving red border. This is reproducible before and after my PR. And seems not related to it (because ewmh is not used). Though, enabling ewmh (after my PR) fixes accidently this bug (?).

Thus, i can't reproduce. Please, provide your config.

pjones commented 7 years ago

My config is here but my manage hook is probably what you are after.

This issue doesn't occur in Example.hs because it uses the default manage hook.

Running someone's manage hook for every focus change seems like it would lead to a lot of bug reports especially if the mange hook is using insertPosition like I am.

geekosaur commented 7 years ago

On Tue, Apr 11, 2017 at 2:12 PM, Peter J. Jones notifications@github.com wrote:

Running someone's manage hook for every focus change seems like it would lead to a lot of bug reports especially if the mange hook is using insertPosition like I am.

Uhhhh. No, don't do that. I'm supposed to know to conditionalize everything in a ManageHook on whether it's being run as a ManageHook or as a focus change masquerading as a ManageHook? Bad API, bad behavior change.

-- brandon s allbery kf8nh sine nomine associates allbery.b@gmail.com ballbery@sinenomine.net unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

sgf-dma commented 7 years ago

I can't compile your config, though i've tried several times in different ways: copy files to my build system (it uses stack), compile yours using stack with lts-8.5 on Debian 8, - all fails by some reason or another.

For now i've removed some code until it compiles and tried with chromium triggering activation with

$ chromium google.ru

in terminal. But, though, it had worked fine for firefox, something very strange goes with chromium: nothing happens for some time, then new window opens (i.e. entirely new, without tabs opened before). And, of course, new window appears on current workspace. So, can you provide some shorter config with a reliable way (and commands) to reproduce the issue?

And about when manageHook runs: no, it does not run at every focus change. It runs at window activation apart from what was before. And to prevent this and return to old behavior completely it should be enough to add not <$> activated before your MH:

manageHook = not <$> activated --> manageHook desktopConfig

This may be changed:

  1. If we decide to not run ManageHook for activated windows at all, like it was before, the H.Focus has no sense - it won't work.
  2. If we decide to just not touch ewmh function, we may revert to special handler function - handleFocusQuery - similar to ewmh, which was in earlier versions of a module. It will conflict with ewmh (i.e. ewmh will override its behavior and, when both are used, H.Focus won't work). This almost makes the module useless, because all EWMH stuff needs to be enabled manually then or copied to handleFocusQuery.
  3. We may decide to provide two ewmh functions: old one and new one - ewmhNew, - which will run ManageHook for activated windows. This will differ from the previous in that ewmhNew will be defined in H.EwmhDesktops, will share all EWMH stuff with ewmh and enable it. But ewmh and ewmhNew will still conflict with each other.
  4. Or leave as is.

And we can't just keep focus in ewmh, because this is what all H.Focus's code does and this requires H.EwmhDesktops to depend on it.

But before taking any decision, i want to try to understand what goes wrong with @pjones config.

pjones commented 7 years ago

@sgf-dma Are you saying we can't have a separate focusHook that uses the logHook to do what you need?

sgf-dma commented 7 years ago

Hm.. i don't understand your question. Can you explain?

geekosaur commented 7 years ago

And about when manageHook runs: no, it does not run at every focus change. It runs at window activation apart from what was before. And to prevent this and return to old behavior completely it should be enough to add not <$> activated before your MH:

This is not acceptable.

The ManageHook is run to manage windows. It is not an ActivationHook, and it is not supposed to be an ActivationHook. The LogHook exists for that purpose; use it.

pjones commented 7 years ago

@sgf-dma What I'm saying is, instead of running the user's manage hook to control which window gets the focus, we should have a totally separate hook system to control focus. It's not acceptable to run the mange hook more than once per window mapping.

It seems like it should be possible to have a FocusHook type similar to ManageHook that only runs when an existing window is asking for focus. The FocusHook would be run to decide if that window should be allowed to "steal" the focus or not.

For new windows being mapped, you can already control their focus behavior with XMonad.Hooks.InsertPosition.

geekosaur commented 7 years ago

XMonad.Hooks.FadeWindows is an example of adding your own hook via the LogHook.

sgf-dma commented 7 years ago

@pjones The separate hook for focus will certainly work, but requires far more changes. But if using logHook.. @geekosaur @pjones Ok, i'll look at how it works.

sgf-dma commented 7 years ago

Hi.

Sorry for a long delay, i haven't time to look at this earlier. I've changed what you had asked, see x.h.focus branch in my repo. I will create a PR in a few days, when i update documentation. But you may test it now (well, may be it won't be necessary to create PR and update documentation, if it won't work again). In short, now it should be used like:

import XMonad

import XMonad.Hooks.EwmhDesktops
import XMonad.Hooks.Focus

main :: IO ()
main = do
        let fa1 :: ManageHook
            fa1 = activateSwitchWs
            fa2 :: ManageHook
            fa2 = not <$> (className =? "Firefox" <||> className =? "Iceweasel") --> activateSwitchWs
            fa3 :: ManageHook
            fa3 = manageFocus (not <$> focusedCur (className =? "Gnome-terminal") --> liftQuery activateSwitchWs)
            fa4 :: ManageHook
            fa4 = manageFocus (not <$> focused (className =? "Gnome-terminal") --> liftQuery activateSwitchWs)
            fc1 :: ManageHook
            fc1 = activateOnCurrentWs
            fc2 :: ManageHook
            fc2 = not <$> (className =? "Firefox" <||> className =? "Iceweasel") --> activateOnCurrentWs
            fc3 :: ManageHook
            fc3 = manageFocus (not <$> focusedCur (className =? "Gnome-terminal") --> liftQuery activateOnCurrentWs)
            fc4 :: ManageHook
            fc4 = manageFocus (not <$> focused (className =? "Gnome-terminal") --> liftQuery activateOnCurrentWs)
            fk1 :: ManageHook
            fk1 = activateOnCurrentKeepFocus
            fk2 :: ManageHook
            fk2 = not <$> (className =? "Firefox" <||> className =? "Iceweasel") --> activateOnCurrentKeepFocus
            fk3 :: ManageHook
            fk3 = manageFocus (not <$> focusedCur (className =? "Gnome-terminal") --> liftQuery activateOnCurrentKeepFocus)
            fk4 :: ManageHook
            fk4 = manageFocus (not <$> focused (className =? "Gnome-terminal") --> liftQuery activateOnCurrentKeepFocus)
            xcf = ewmh $ def {modMask = mod4Mask, logHook = focusLogHook fk4 <+> logHook def}
        xmonad xcf

So, pass your FocusHook (converted to ManageHook by manageFocus) to focusLogHook, if you want it to apply only to activated window. You may still append it to manageHook, if you want it to run on new windows too.

liskin commented 3 years ago

With #192 merged and #396 and #399 in progress, we can close this one.