X.L.ConditionalLayoutModifier: Init

[colonelpanic8]

Description

Include a description for your changes, including the motivation
behind them.

Checklist

• I've read CONTRIBUTING.md

• I've considered how to best test these changes (property, unit,
  manually, ...) and concluded: XXX

• I updated the CHANGES.md file

• I updated the XMonad.Doc.Extending file (if appropriate)

[liskin]

Include a description for your changes, including the motivation
behind them.

We'd really appreciate this. Sure, in your case we can go and look into your dotfiles for an example of usage and try to deduce the motivation, but in general this isn't feasible. We like and encourage contributions, but let's not turn this into “I'll throw undocumented code at you to save a couple minutes of my time at the expense of yours”, please. There must be a middle ground somewhere. We're happy to take blank PRs if agreed upon out-of-band (IRC, ZuriHac, …), but some communication ought to happen.

• I've considered how to best test these changes (property, unit,
  manually, ...) and concluded: XXX

“XXX” is not a very solid description of how this was (intended to be) tested. It's okay to say “I tried it for a day with the most basic of layouts” or “It seems to work even with MultiToggle” and we can take it from there. It's not meant to shame people into writing property tests. Maybe it fails at that, so let's improve the wording?

Unfortunately it seems this won't work well with any layout modifier that needs correct processing of ReleaseResources/Hide events, especially in multi-head configurations. See e.g. #75 and #354 (comment). Arguably IfMax and PerScreen still haven't been fixed so it's not entirely fair to hold you to higher standards. But now that we know about the bug and how to design it better, we might as well try to. :-)

Also, as the example in your dotfiles demonstrates, the condition itself won't work well in multi-head layouts, as currentWorkspace is the focused workspace, not the one being layouted. If you go with my suggestion and cache the condition state in the layout modifier record, you won't need to evaluate the condition outside of modifyLayout, and therefore you can pass WindowSpace to the condition.

So, to wrap it up, this is definitely a good idea. A generalization of IfMax/PerScreen is something we missed. It would be great, however, if we managed to implement it properly this time so that we can close a couple bugs and possibly deprecate IfMax/PerScreen in favor of this better one.

[colonelpanic8]

We'd really appreciate this. Sure, in your case we can go and look into your dotfiles for an example of usage and try to deduce the motivation, but in general this isn't feasible. We like and encourage contributions, but let's not turn this into “I'll throw undocumented code at you to save a couple minutes of my time at the expense of yours”, please. There must be a middle ground somewhere. We're happy to take blank PRs if agreed upon out-of-band (IRC, ZuriHac, …), but some communication ought to happen.

Sure, that is understandable. That said:

• The code is not undocumented. This description takes place twice in the PR:

	This module provides a LayoutModifier that modifies an existing
    LayoutModifier so that its modifications are only applied when a particular
    condition is met.

I'm not really sure what to write in the description other than that. That pretty much describes the entire intent of this change without omitting any details that arent related to the implementation.

“XXX” is not a very solid description of how this was (intended to be) tested. It's okay to say “I tried it for a day with the most basic of layouts” or “It seems to work even with MultiToggle” and we can take it from there. It's not meant to shame people into writing property tests. Maybe it fails at that, so let's improve the wording?

Sure.

Unfortunately it seems this won't work well with any layout modifier that needs correct processing of ReleaseResources/Hide events, especially in multi-head configurations. See e.g. #75 and #354 (comment). Arguably IfMax and PerScreen still haven't been fixed so it's not entirely fair to hold you to higher standards. But now that we know about the bug and how to design it better, we might as well try to. :-)`

Did you see this comment above handleMess

  -- This function is not allowed to have any downstream effect, so it seems
  -- more reasonable to simply allow the message to pass than to make it depend
  -- on the condition.

now, arguably, this doesn't quite solve the problem ALL the way, because I suppose that layouts implementing only handleMessOrMaybeModifyIt would still not receive these messages.

But this should get us at least 50% of the way there, right?

This is not quite a generalization of IfMax or PerScreen, since IfMax chooses between layouts, rather than layout modifiers, but some of the same issues may be exhibited.

Still this concept of using a type class to represent an arbitrary condition could and should be used to generalize those 2. I may add that generalization to this PR and change the module to offer utilities for conditionally choosing layouts.

Also, as the example in your dotfiles demonstrates, the condition itself won't work well in multi-head layouts, as currentWorkspace is the focused workspace, not the one being layouted. If you go with my suggestion and cache the condition state in the layout modifier record, you won't need to evaluate the condition outside of modifyLayout, and therefore you can pass WindowSpace to the condition.

Good point. I definitely did not think much about multi-head layouts. Still, this is actually not really a problem with the PR per se, as much as it is an issue with any condition that depends on the state of the workspace whose layout is being adjusted.

Caching as you suggest is one way to approach this, but I do worry that there could be some corner cases here. Another possibility is passing in the workspace whose layout is being performed to the condition. This would allow a condition where we look at the current layout of whichever workspace we are performing the update for.

and possibly deprecate IfMax/PerScreen in favor of this better one.

Instead of deprecation, why not just implement those in terms of the new approach.

[colonelpanic8]

I believe that I have come up with a solution that addresses the two issues that @liskin raised earlier:

• That certain messages should always go to the original modifier even if it is currently disabled

This is fixed by simply always forwarding messages to the underlying layout. Since the handle message call can not affect the current layout, this should be okay.

• That if a condition wants to act or not based on anything related to the current workspace it is operating on, relying on whatever the "current workspace" is will not work in the case of multi-head setups, since we may actually be doing the layout for a workspace that is only visible, but not necessarily "current"

This is fixed by passing the workspaceId as a parameter to the condition which can now be used to appropriately make decisions based on information about the workspace.

[colonelpanic8]

I worry that some may find this unpalatable for some reason, but I believe that the way I have implemented this leads to no change in the current behavior of anything.

[colonelpanic8]

This is crucial -- This function does not have any effect on layout, so we don't need to evaluate the condition to decide whether or not to pass the message along to the original modifier.

[colonelpanic8]

One small detail here is that we always modify messages even if the condition might be viewed as evaluating to false in the moment. I don't think this is a big problem because there is no way to affect the layout.

[liskin]

The code is not undocumented.

I should've said “underdocumented”, perhaps. Most modules in contrib come with example usage as many users don't know enough Haskell to understand it from the types themselves, and I would've appreciated an example as well, just to better understand the motivation for this.

(Obviously the documentation doesn't need to be perfect in a draft PR. It doesn't need to be there at all, in fact. As long as the motivation and the draft state of the PR is communicated somewhere.)

This description takes place twice in the PR:

	This module provides a LayoutModifier that modifies an existing
    LayoutModifier so that its modifications are only applied when a particular
    condition is met.

I'm not really sure what to write in the description other than that. That pretty much describes the entire intent of this change without omitting any details that arent related to the implementation.

As hinted above, an example would have been nice, but even just copypasting this bit would have been better than leaving the “Include a description for your changes, including the motivation behind them.” template intact.

Ideally and in general, the documentation of the module explains the what (what the module does, what use cases it's good for) and the how (examples of usage, etc.), possibly even the why (helps to understand why one might use it in their config). The pull request cover letter and/or the commit message¹ then include a brief summary of the what and a detailed why/how (motivation, an example, why something's being fixed in a certain way, notes from bug investigations, benchmarks, …).

¹) (in the case on single-commit PRs, the commit message becomes the cover letter automatically, or at least hub does it)

It's probably time we revised https://github.com/xmonad/xmonad/blob/master/CONTRIBUTING.md, made it shorter, spent less time explaining git rebase and generally bringed it up to our current preferences. But let me ask a honest question here (and please be honest both with us and with yourself):

Would you have read it or would you have just submitted a PR with false [X]s anyway?

(Not trying to be pedantic or something, I really want to know if it's worth the time.)

Did you see this comment above handleMess

  -- This function is not allowed to have any downstream effect, so it seems
  -- more reasonable to simply allow the message to pass than to make it depend
  -- on the condition.

now, arguably, this doesn't quite solve the problem ALL the way, because I suppose that layouts implementing only handleMessOrMaybeModifyIt would still not receive these messages.

I did indeed miss this one.

But this should get us at least 50% of the way there, right?

Yep, solves some of the problems that I had in mind, definitely not all of them.
One issue still there even with your latest implementation is that if a decorations modifier is made optional, those decorations don't disappear when the condition flips to false.

and possibly deprecate IfMax/PerScreen in favor of this better one.

Instead of deprecation, why not just implement those in terms of the new approach.

Yes, that'd be best.

---

I only had a quick look at the implementation. Looks a bit simpler, so that's good. The function added to LayoutModifier makes sense, and it's really stupid that it wasn't there in the first place—would make some things easier (even in other modules). Makes the API uglier and more complicated now.

I still believe it's worth trying to implement this as a layout instead of layout modifier, as I suspect the implementation would get even simpler and more robust. I'd be happy to give it a try, although I can't promise when.

[colonelpanic8]

Would you have read it or would you have just submitted a PR with false [X]s anyway?

Man you're really putting me through the ringer over this whole thing. I think that's actually kind of unfair, since I actually did:

• Update the changelog
• Provide a module level docstring

Also you said that I submitted a PR with False [X]'s which implies:

• That there were multiple checks that were false
• That it was False that I had "I've considered how to best test these changes (property, unit, manually)", which is simply not the case. I HAD actually tested things, with a few different layout modifiers (not only the one that actually made it into my dotfiles) and things did work. I do not have a multi-head setup, so that issue did slip by, but I did make an effort to test things.

Honestly I simply did not see the "and concluded XXX" part. I assumed that because there was a checkbox there it was a sort of "I have done some testing of these changes", because that sort of thing appears in many other repos, without the need to fill something out there. I think that having a checkbox there, sort of suggests to the user that they just need to check something off if they have done it. If you have to fill out text anyway, I don't think there's a point in also having the checkbox (but thats just my opinion).

I still believe it's worth trying to implement this as a layout instead of layout modifier, as I suspect the implementation would get even simpler and more robust. I'd be happy to give it a try, although I can't promise when.

Sure. Might also be able to make it more extensible and similar to something where you are choosing between multiple layouts.

[liskin]

Man you're really putting me through the ringer over this whole thing. I think that's actually kind of unfair

That's why I included the “(Not trying to be pedantic or something, I really want to know if it's worth the time.)” bit. What feels like me grilling you is just me trying to understand what makes our PR template fail in contact with real people. I mean, you're not the first who didn't fill it in completely, but since your contributions are of much higher quality than all the other people who failed the template, I thought I'd ask about it and learn something. Sorry about drilling so deep, and for being overly direct.

Also you said that I submitted a PR with False [X]'s which implies:

• That there were multiple checks that were false

I did indeed assume that someone who contributed in the past and who decided not to fill in the description might skip reading the CONTRIBUTING.md as well. And I did not mean to judge that, as I imagine I might skip it too, possibly. Just wanted a second opinion. (As you might have realized by now, I'm not the best communicator.)

• That it was False that I had "I've considered how to best test these changes (property, unit, manually)", which is simply not the case. I HAD actually tested things, with a few different layout modifiers (not only the one that actually made it into my dotfiles) and things did work. I do not have a multi-head setup, so that issue did slip by, but I did make an effort to test things.

Honestly I simply did not see the "and concluded XXX" part. I assumed that because there was a checkbox there it was a sort of "I have done some testing of these changes", because that sort of thing appears in many other repos, without the need to fill something out there. I think that having a checkbox there, sort of suggests to the user that they just need to check something off if they have done it. If you have to fill out text anyway, I don't think there's a point in also having the checkbox (but thats just my opinion).

This is valuable feedback, and thank you for spending the time to write it. So I guess replacing "XXX" with something like "<please write a short answer here>" might help? And a newline, perhaps.

Hope this clears up any misunderstandings. Sorry again.

[liskin]

So, I felt like trying to implement my idea today, and this is what I ended up with:

-- | 'ModifiedLayout' extended with a condition and its last evaluation result
-- (for methods that can't evaluate it).
data CondModifiedLayout c m l a = CondModifiedLayout Bool c (ModifiedLayout m l a) deriving (Read, Show)

instance (ModifierCondition c, LayoutModifier m a, LayoutClass l a, Typeable c, Typeable m)
    => LayoutClass (CondModifiedLayout c m l) a
  where
    runLayout (W.Workspace i cml@(CondModifiedLayout a c _) ms) r = do
        a' <- shouldApply c i
        cml' <- if a == a' then pure Nothing else Just . switch <$> hide cml
        fmap (<|> cml') <$> run (fromMaybe cml cml')
      where
        hide x = fmap (fromMaybe x) $ handleMessage x (SomeMessage Hide)
        switch (CondModifiedLayout a' c' ml') = CondModifiedLayout (not a') c' ml'

        run (CondModifiedLayout True c' ml) =
            fmap (fmap (CondModifiedLayout True c')) <$> runLayout (W.Workspace i ml ms) r
        run (CondModifiedLayout False c' (ModifiedLayout m l)) =
            fmap (fmap (CondModifiedLayout False c' . ModifiedLayout m)) <$> runLayout (W.Workspace i l ms) r

    handleMessage (CondModifiedLayout a c ml@(ModifiedLayout lm l)) m
        | Just ReleaseResources <- fromMessage m = fmap (CondModifiedLayout a c) <$> handleMessage ml m
        | a                                      = fmap (CondModifiedLayout a c) <$> handleMessage ml m
        | otherwise          = fmap (CondModifiedLayout a c . ModifiedLayout lm) <$> handleMessage l m

    description (CondModifiedLayout a _ ml@(ModifiedLayout _ l)) = if a then description ml else description l

conditional :: (ModifierCondition c) => c -> (l a -> ModifiedLayout m l a) -> (l a -> CondModifiedLayout c m l a)
conditional c ml = CondModifiedLayout True c . ml

Appears to work well with the following config:

import Data.List

import XMonad
import XMonad.Layout.ConditionalLayout
import XMonad.Layout.Tabbed
import qualified XMonad.StackSet as W

data MyCond = MyCond deriving (Read, Show)
instance ModifierCondition MyCond where
    shouldApply _ t = do
        w <- gets $ find ((t ==) . W.tag) . W.workspaces . windowset
        let ws = W.integrate' . W.stack <$> w
        pure $ Just 4 < (length <$> ws)

main :: IO ()
main = xmonad def{
    terminal = "urxvt",
    layoutHook = conditional MyCond (addTabsAlways shrinkText def) $ layoutHook def
}

Let me know what you think.

(I'll probably try to get rid of the gets $ find ((t ==) . W.tag) . W.workspaces . windowset later, and also take a stab at implementing CondChoose.)

[colonelpanic8]

Looks great!

[liskin]

(I'll be off for over a week so do feel free to explore this further, or just let it sit here until I'm ready to polish it and merge.)

[colonelpanic8]

@liskin What happened with this.

I'd really like to try to get some of my PRs merged, because its sort of annoying to continue maintaining my own fork of xmonad-contrib.

It seems like you have a strong preference for the approach you took, which is fine. I just think this is a very natural change and something that can accomplish this goal should be merged.

[slotThe]

@colonelpanic8 I think it is unlikely that liskin will pick this back up anytime soon. I also think merging this would be great, and don't have any strong preferences regarding what approach we need to take, as long as all mentioned issues are actually addressed. Do I read

It seems like you have a strong preference for the approach you took, which is fine.

correctly that you'd rather implement your approach?

[colonelpanic8]

Don't really have a strong preference either way except that I already have the modifiers written the way that I ser it up.

[liskin]

What happened with this.

Life got in the way, hard. I don't even remember what was wrong in Aug 2021, but I can confidently say everything is at least dozen times worse now. So I definitely agree with @slotThe's suggestion that waiting for me to polish it and merge by the end of August is probably a bad idea, primarily because 2 years have passed from the actual August.

But yeah, I do have a strong preference for an approach that

• works well with decorated layouts (Display bug with IfMax, PerScreen, PerWorkspace with decorated layouts  #75)
• can be trivially extended from just a conditional layout modifier into a conditional choose between two layouts
• and therefore lets us implement IfMax, PerScreen, PerWorkspace in terms of it, finally solving Display bug with IfMax, PerScreen, PerWorkspace with decorated layouts  #75
• happens to be simpler and doesn't require changes to the LayoutModifier class

I was hoping someone would just take my code, add a bit of docs, a changelog entry, and I'm sure we would've merged that ages ago. Instead people just keep asking "is it done yet?" and "what are we waiting for?" :-(

Anyway, I don't want to block progress either. If everyone else is happy with your approach (which I don't think handles decorated layouts well…), do whatever you all want. My capacity to care is limited.

(Arguably I should've just done what I promised to do back in 2021 instead of wasting time writing this comment. Oh well.)

[geekosaur]

@liskin, I'm looking your code over and don't see where ModifierCondition is defined. Is there more to it than shouldApply?

[slotThe]

@geekosaur it's just shouldApply. The reason is that CondModifiedLayout—being a layout—needs to implement Read and Show, and these conditions get passed as parameters to it

EDIT: FWIW I've implemented a version of CondChoose now (as a separate implementation, though) and am currently writing a few docs. Fingers crossed that this will get resolved soon :)

[geekosaur]

Oh, I know why it exists, I just want to make sure I wasn't missing something around e.g. messages or something.