Skip to content
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

Actual PerScreen implementation #813

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geekosaur
Copy link
Contributor

@geekosaur geekosaur commented May 3, 2023

Description

I have implemented an actual PerScreen layout (onScreen and onScreens). The old ifWidth is moved to a new module XMonad.Layout.ByWidth, and re-exported with a deprecation warning pointing to the new module.

I'd like someone who can to test a corner case with respect to rescreen. I think it will usually work, but may glitch if you rapidly plug and unplug an external monitor to which this layout would apply. This can't do more than a brief glitch though, as anything that would trigger it will have another rescreen pending and xmonad will fix itself.

Checklist

  • I've read CONTRIBUTING.md

  • I've considered how to best test these changes (property, unit,
    manually, ...) and concluded: manually tested except for the corner case mentioned above

  • I updated the CHANGES.md file

@geekosaur geekosaur marked this pull request as ready for review May 4, 2023 22:16
@geekosaur geekosaur changed the title Actual PerScreen implementation (WIP) Actual PerScreen implementation May 4, 2023
@geekosaur
Copy link
Contributor Author

I squashed it down a bit.

Copy link
Member

@TheMC47 TheMC47 left a comment

Choose a reason for hiding this comment

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

LGTM, one minor nitpick.

-- to use the 'S' constructor.
onScreens :: (LayoutClass l1 a, LayoutClass l2 a)
=> [ScreenId] -> l1 a -> l2 a -> OnScreen l1 l2 a
onScreens ss l1 l2 = OnScreen ss l1 l2 False -- @@@ is this right?
Copy link
Member

Choose a reason for hiding this comment

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

@@@ is this right?

Are you asking about the flag? I think so since as soon as runLayout is executed the flag will be set correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was what I figured but I can't recall if there are special cases where the default might be used inappropriately. But even if there were it would be on an inactive screen by definition, which is why I used False.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there are cases where it might be used inappropriately. But we're also not using the flag enough. We need to Hide whenever the flag flips, and dispatch ReleaseResources to both layouts.

(Might be better to finish #582 and get #75 fixed for free, like I mentioned in another comment.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be done, although I cheated a bit on ReleaseResources: should I check for a new constructor being produced by the forwarded message?

Also, if we receive Hide, should it be relayed to both sublayouts or only the active one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now have both fully implemented ReleaseResources and a direct implementation of Hide which sends to both sublayouts because it's a PITA to send only to the active one. Also I'm relying in a few places on Hide being idempotent, but we may have other issues if it's not anyway.

XMonad/Layout/PerScreen.hs Outdated Show resolved Hide resolved
@geekosaur
Copy link
Contributor Author

geekosaur commented May 7, 2023

In case anyone's wondering, @liskin expressed a concern and plans to review this within the next few days, so don't rush into committing it just yet.

runLayout (W.Workspace i p@(PerScreen w _ lt lf) ms) r
| rect_width r > w = do (wrs, mlt') <- runLayout (W.Workspace i lt ms) r
return (wrs, Just $ mkNewPerScreenT p mlt')
| otherwise = do (wrs, mlt') <- runLayout (W.Workspace i lf ms) r
Copy link
Member

Choose a reason for hiding this comment

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

Just doing runLayout with the other layout whenever the condition changes will leave decorations (and other stuff like this) around. In other words, this suffers from #75. You need to send Hide to the last layout first before runLayouting the new one, like this: #582 (comment)

Also, you need to handle ReleaseResources and pass it on to both layouts.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, now I see you're just moving stuff around and this is still the same code that was there. So I guess that's an argument for not fixing it in the scope of this PR.

That then leads me to a thought whether it wouldn't be better to not hijack the PerScreen name and just add an X.L.OnScreen module… Deprecating PerScreen in favor of ByWidth or IfWidth can stay, of course.

Oh, and related to my previous comment about #75 - the plan was to implement #582 generic enough so that IfWidth can be implemented using it and thus get #75 fixed for free. So we may want to limit the exports a bit… although we can always use pattern for backwards compat.

Copy link
Contributor Author

@geekosaur geekosaur May 9, 2023

Choose a reason for hiding this comment

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

The main reason I reused PerScreen is consistency with PerWorkspace and friends. Also, PerScreen was never the right name for a module whose only combinator was ifWidth.

And yes, the only changes I made to ByWidth were renames; everything else is the same, including the weirdnesses @TheMC47 noticed. I was actually somewhat afraid to touch them, because @ezyang is more of a Haskeller than I am and I tend to assume "fancy" code has some reason not visible to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re Hide and ReleaseResources: I did think about that, but thought we had a more global plan to deal with those somewhere. I can add that. What worries me is I don't think it scales; are there, or will there at some point be, more "global" messages that need to be dealt with by individual layouts?

runLayout (W.Workspace i p@(OnScreen ss l1 l2 _) ms) r = do
-- this sucks.
-- You might think we could use the existing screen detail, but we may be
-- invoked from 'rescreen' in which case only 'rescreen' and 'windows'
Copy link
Member

Choose a reason for hiding this comment

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

But windows does modify (\s -> s { windowset = ws }) pretty early (definitely before running any layouts) so why can't you just withWindowSet like X.L.NoBorders does (it also has logic to detect which screen is which based on the rect passed)?

Copy link
Member

Choose a reason for hiding this comment

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

By not using the windowset this won't work with XMonad.Layout.LayoutScreens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think LayoutScreens should be deprecated in favor of using xrandr: applications can't ever work with LayoutScreens.

-- to use the 'S' constructor.
onScreens :: (LayoutClass l1 a, LayoutClass l2 a)
=> [ScreenId] -> l1 a -> l2 a -> OnScreen l1 l2 a
onScreens ss l1 l2 = OnScreen ss l1 l2 False -- @@@ is this right?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there are cases where it might be used inappropriately. But we're also not using the flag enough. We need to Hide whenever the flag flips, and dispatch ReleaseResources to both layouts.

(Might be better to finish #582 and get #75 fixed for free, like I mentioned in another comment.)

@geekosaur
Copy link
Contributor Author

I renamed ByWidth.hs to IfWidth.hs for consistency with IfMax.hs and with its ifWidth function, and squashed everything in the process. AFAIK this is just waiting on someone to confirm that I'm handling the Hide and ReleaseResources messages properly.

The old PerScreen module has been renamed to X.L.IfWidth, and is
re-exported for backward compatibility with a deprecation notice.
The new PerScreen supports specifying screens by ScreenId (literal
numbers will work because it has a Num instance).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants