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

X.H.RescreenHook: New module (custom rescreen and randr event hooks) #460

Merged
merged 7 commits into from
Jun 3, 2021

Conversation

liskin
Copy link
Member

@liskin liskin commented Feb 1, 2021

Description

Inspired by
https://github.com/fis/dot-xmonad/blob/a8c8e804873f8657fc1f1a0d93ee3f58f7a56e22/zem/src/Zem/AutoRandr.hs
and my own
https://github.com/liskin/dotfiles/blob/43999573d006311292a9fadde02f3b234140d999/.xmonad/xmonad.hs#L308-L339,
this adds a generic hook module that allows attaching a custom hook to
rescreen and randr screen change notify event, hiding the complexity of
suppressing duplicate events (debouncing) and distinguishing actual
xrandr configuration changes from output connects/disconnects.

Commits

Add X.H.RescreenHook module (custom rescreen hooks)

X.H.{EwmhDesktops,ManageDocks}: Improve the usage doc a bit

Don't assume ewmh/docks are the only xmonad config combinator out there.

X.H.RescreenHook: Add randrHook

X.H.RescreenHook: Merge the two hooks together and improve their behaviour

Now that randrChangeHook is only invoked for changes that don't result
in rescreen, it can actually be used for autorandr.

Update CHANGES, X.D.Extending: add X.H.Rescreen

Checklist

  • I've read CONTRIBUTING.md

  • I updated the CHANGES.md file

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

@liskin
Copy link
Member Author

liskin commented Feb 1, 2021

Ping @fis. Could you perhaps test your Autorandr with these hooks? I'm especially interested if my "debouncing" logic works, as it's possibly less robust (but also less hacky) than yours and doesn't use sleep. :-)

@liskin
Copy link
Member Author

liskin commented Feb 1, 2021

Somewhat related: liskin/xmonad@839d0d9
I have no idea if that is actually necessary, though. Probably not, but it could be if someone uses https://hackage.haskell.org/package/X11/docs/Graphics-X11-Xrandr.html to get more information about the situation, like https://github.com/fis/dot-xmonad/blob/a8c8e804873f8657fc1f1a0d93ee3f58f7a56e22/zem/src/Zem/AutoRandr.hs does.

That xmonad commit also needs xmonad/X11#72, which is otherwise not required by this PR, but should be merged sooner or later anyway.

@fis
Copy link

fis commented Feb 1, 2021

Heh. It may take me a day or three to actually get around to it, but I can definitely try to test my AutoRandr code on top of these hooks.

Regarding the debouncing logic, some context: my setup involves two monitors connected over a single (USB-C) cable with DisplayPort MST. If memory serves, to X plugging in the cable looks like two consecutive (but distinct) configuration changes: first a single monitor appears at the "DP-0" output, and then that changes into a DP "hub", with two monitors connected as "DP-0-1" and "DP-0-8". I think I added the sleep-based debouncing as an initial attempt to work around that. That said, I don't think it really was stable enough (the timing is unpredictable), and I ended up instead just not configuring any action for the "single external monitor" configuration. So it's quite possible just clearing the actually immediate X events would work well enough (for my purposes).

@liskin
Copy link
Member Author

liskin commented Feb 1, 2021

Absolutely no need to rush it! :-)

I also have a DP MST hub here (Lenovo ThinkPad Ultra Dock) and I had all sorts of problems with it in the past, but right now it seems to work rather well. Your setup is probably different, though, as I guess one of your monitors acts as the MST hub, but it really could look like a single monitor for a brief moment. Not sure if this is something we can reliably handle in xmonad. We could probably add some configurable sleep to the event clearing or something to make sure the hook really fires only once it's settled. Or we can leave it up to users to add sleep and some actual debouncing to the hook if they need that.

In my own config, I added flock to my bash layout-auto: liskin/dotfiles@706db34#diff-c57c136cf207a3cd44f4fac23bde2146a5f8259c4391333981aec1a8aba66898 and now I can just freely add sleeps if they ever become needed. And my configuration also won't reconfigure xrandr for unknown monitor configuration, I still need to manually press a key for that.

Comment on lines +122 to +137
-- | Remove all X events of a given window and type from the event queue,
-- return whether there were any.
Copy link
Member

Choose a reason for hiding this comment

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

This looks really cool; not only in the context of this module but in general even. Do we have a place where these kinds of "XUtils" (sadly XMonad.Util.XUtils does not seem to be this place) can go?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not currently. Sadly evicting and reclaiming XMonad.Util.XUtils would be a breaking change; but it's frankly misnamed.

That said, maybe it deserves to be in XMonad.Util.EvUtils (this doesn't exist yet).

Copy link
Member Author

Choose a reason for hiding this comment

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

What's wrong with XMonad.Util.XUtils?

On the other hand: should I really try to move a function that nobody needed for a decade somewhere where it's only slightly easier to find than where it is? After all, if anyone opens a PR that adds a similar function for their module, surely I'll notice and suggest factoring it out then. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

(Anyway, if we ever make the function available, it should come with some warnings as it may change the order of events. It does put them back in the correct order relative to one another, but the order is changed relative to events that weren't picked up by XCheckTypedWindowEvent. It is extremely unlikely to matter in this usecase, but with something else than ConfigureEvent, it may matter.)

Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with XMonad.Util.XUtils?

The module description says "A module for painting on the screen" and that's pretty much what all of the functions do in some capacity; not really a place for general X utility functions. Then again this is only there as a doc, so we may well repurpose it and give all the drawing stuff a heading of its own.

On the other hand: should I really try to move a function that nobody needed for a decade somewhere where it's only slightly easier to find than where it is? After all, if anyone opens a PR that adds a similar function for their module, surely I'll notice and suggest factoring it out then. :-)

I'll give you that one; probably a bit of a premature optimization, especially since no good module already exists where we could put it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The module description says "A module for painting on the screen" and that's pretty much what all of the functions do in some capacity; not really a place for general X utility functions.

There are also a couple of X wrappers for IO functions with Display parameter. We can move those under a separate haddock heading and then add another heading for utilities (which I define as more than a wrapper) like my clearTypedWindowEvents. None of that needs to be done now, but I think this is the most sensible thing to do if we want a module for X utils.

liskin added a commit to liskin/xmonad-contrib that referenced this pull request May 17, 2021
It's often difficult to make contrib modules work together. When one
depends on a functionality of another, it is often necessary to expose
lots of low-level functions and hooks and have the user combine these
into a complex configuration that works. This is error-prone, and
arguably a bad UX in general.

This commit presents a simple solution to that problem inspired by
"extensible state": extensible config. It allows contrib modules to
store custom configuration values inside XConfig. This lets them create
custom hooks, ensure they hook into xmonad core only once, and possibly
other use cases I haven't thought of yet.

This requires changes to xmonad core: xmonad/xmonad#294

A couple examples of what this gives us:

* [X.H.RescreenHook](xmonad#460)
  can be made safe to apply multiple times, making it composable and
  usable in other contrib modules like X.H.StatusBar

* `withSB` from X.H.StatusBar can also be made safe to apply multiple
  times, and we can even provide an API [similar to what we had
  before](https://hackage.haskell.org/package/xmonad-contrib-0.16/docs/XMonad-Hooks-DynamicLog.html#v:statusBar)
  if we want (probably not, consistency with the new dynamic status bars
  of xmonad#463 is more important)

* The [X.H.EwmhDesktops refactor](xmonad#399)
  can possibly be made without breaking the `ewmh`/`ewmhFullscreen` API.
  And we will finally be able to have composable EWMH hooks.

Related: xmonad/xmonad#294
@liskin liskin marked this pull request as draft May 17, 2021 17:56
liskin added a commit to liskin/xmonad-contrib that referenced this pull request May 17, 2021
It's often difficult to make contrib modules work together. When one
depends on a functionality of another, it is often necessary to expose
lots of low-level functions and hooks and have the user combine these
into a complex configuration that works. This is error-prone, and
arguably a bad UX in general.

This commit presents a simple solution to that problem inspired by
"extensible state": extensible config. It allows contrib modules to
store custom configuration values inside XConfig. This lets them create
custom hooks, ensure they hook into xmonad core only once, and possibly
other use cases I haven't thought of yet.

This requires changes to xmonad core: xmonad/xmonad#294

A couple examples of what this gives us:

* [X.H.RescreenHook](xmonad#460)
  can be made safe to apply multiple times, making it composable and
  usable in other contrib modules like X.H.StatusBar

* `withSB` from X.H.StatusBar can also be made safe to apply multiple
  times, and we can even provide an API [similar to what we had
  before](https://hackage.haskell.org/package/xmonad-contrib-0.16/docs/XMonad-Hooks-DynamicLog.html#v:statusBar)
  if we want (probably not, consistency with the new dynamic status bars
  of xmonad#463 is more important)

* The [X.H.EwmhDesktops refactor](xmonad#399)
  can possibly be made without breaking the `ewmh`/`ewmhFullscreen` API.
  And we will finally be able to have composable EWMH hooks.

Related: xmonad/xmonad#294
XMonad/Hooks/Rescreen.hs Outdated Show resolved Hide resolved
liskin added a commit to liskin/xmonad-contrib that referenced this pull request May 21, 2021
It's often difficult to make contrib modules work together. When one
depends on a functionality of another, it is often necessary to expose
lots of low-level functions and hooks and have the user combine these
into a complex configuration that works. This is error-prone, and
arguably a bad UX in general.

This commit presents a simple solution to that problem inspired by
"extensible state": extensible config. It allows contrib modules to
store custom configuration values inside XConfig. This lets them create
custom hooks, ensure they hook into xmonad core only once, and possibly
other use cases I haven't thought of yet.

This requires changes to xmonad core: xmonad/xmonad#294

A couple examples of what this gives us:

* [X.H.RescreenHook](xmonad#460)
  can be made safe to apply multiple times, making it composable and
  usable in other contrib modules like X.H.StatusBar

* `withSB` from X.H.StatusBar can also be made safe to apply multiple
  times, and we can even provide an API [similar to what we had
  before](https://hackage.haskell.org/package/xmonad-contrib-0.16/docs/XMonad-Hooks-DynamicLog.html#v:statusBar)
  if we want (probably not, consistency with the new dynamic status bars
  of xmonad#463 is more important)

* The [X.H.EwmhDesktops refactor](xmonad#399)
  can possibly be made without breaking the `ewmh`/`ewmhFullscreen` API.
  And we will finally be able to have composable EWMH hooks.

Related: xmonad/xmonad#294
liskin added a commit to liskin/xmonad-contrib that referenced this pull request Jun 1, 2021
It's often difficult to make contrib modules work together. When one
depends on a functionality of another, it is often necessary to expose
lots of low-level functions and hooks and have the user combine these
into a complex configuration that works. This is error-prone, and
arguably a bad UX in general.

This commit presents a simple solution to that problem inspired by
"extensible state": extensible config. It allows contrib modules to
store custom configuration values inside XConfig. This lets them create
custom hooks, ensure they hook into xmonad core only once, and possibly
other use cases I haven't thought of yet.

This requires changes to xmonad core: xmonad/xmonad#294

A couple examples of what this gives us:

* [X.H.RescreenHook](xmonad#460)
  can be made safe to apply multiple times, making it composable and
  usable in other contrib modules like X.H.StatusBar

* `withSB` from X.H.StatusBar can also be made safe to apply multiple
  times, and we can even provide an API [similar to what we had
  before](https://hackage.haskell.org/package/xmonad-contrib-0.16/docs/XMonad-Hooks-DynamicLog.html#v:statusBar)
  if we want (probably not, consistency with the new dynamic status bars
  of xmonad#463 is more important)

* The [X.H.EwmhDesktops refactor](xmonad#399)
  can possibly be made without breaking the `ewmh`/`ewmhFullscreen` API.
  And we will finally be able to have composable EWMH hooks.

Related: xmonad/xmonad#294
@liskin liskin marked this pull request as ready for review June 3, 2021 00:03
@liskin liskin requested a review from TheMC47 June 3, 2021 00:03
@liskin liskin added this to the v0.17 milestone Jun 3, 2021
@liskin liskin added this to In progress in xmonad 0.17 via automation Jun 3, 2021
@liskin
Copy link
Member Author

liskin commented Jun 3, 2021

Two things are still missing here:

Even without these, it should be useful for many people and should be good enough to be used by default in X.H.StatusBar, so I'd like to get it merged soon.

xmonad 0.17 automation moved this from In progress to To merge Jun 3, 2021
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.

This is working beautifully with X.H.StatusBar! Good job

I really like the safety that the ExtensibleConfig gives us. LGTM

This appears to be more natural. The function will most often be fixed
by the module using `XC.once` and the configuration will often be
supplied by users of those modules, so it's better to partially apply
the function first.
…reenChangeNotifyEvent

The new X.H.Rescreen provides a simpler composable API for hooking into
rescreen. Unfortunately it also breaks other code that listens for
RRScreenChangeNotifyEvent, so this conversion isn't really optional.

Not that there's any dispute this is nicer, is there? :-)
@liskin liskin merged commit 5230f03 into xmonad:master Jun 3, 2021
xmonad 0.17 automation moved this from To merge to Done Jun 3, 2021
@liskin liskin deleted the pr/rescreen branch June 3, 2021 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants