Detect lid state with dbus-monitor instead of libinput

chmduquesne commented 4 years ago

In #169, I contributed a way to detect if the lid is open/closed based on libinput.

Today I discovered that dbus-monitor was also getting that information. You may test this by running the following command and opening/closing the lid:

dbus-monitor --system "type='signal',path=/org/freedesktop/UPower"

What I don't know yet (because I don't have external monitors at hand) is whether this can be used to detect external monitors being plugged. If somebody can check that by running dbus-monitor --system with no filter expression and plugging/unplugging a monitor, it would be very helpful because we could replace the systemd triggers neatly with a single autostart script.

Let me know what you think!

chmduquesne commented 4 years ago

Note for whoever wants to turn this suggestion into an implementation:

https://wiki.python.org/moin/DbusExamples recommends pydbus as a modern dbus library.

chmduquesne commented 4 years ago

I had access to an external monitor today, so I could experiment a bit. The following script triggers autorandr when a monitor is plugged/unplugged or when the lid is open/closed.

#!/usr/bin/python3
from pydbus import SystemBus
from gi.repository import GLib
import subprocess

def run_autorandr():
    subprocess.call(['autorandr', '--change', '--default', 'default'])

def on_upower_properties_changed(*args):
    properties = args[1]
    if 'LidIsClosed' in properties:
        run_autorandr()

def on_colord_device_updated(*args):
    run_autorandr()

bus = SystemBus()

upower = bus.get('org.freedesktop.UPower', '/org/freedesktop/UPower')
upower.PropertiesChanged.connect(on_upower_properties_changed)

colord = bus.get('org.freedesktop.ColorManager',
        '/org/freedesktop/ColorManager')
colord.DeviceAdded.connect(on_colord_device_updated)
colord.DeviceRemoved.connect(on_colord_device_updated)

loop = GLib.MainLoop()
loop.run()

It works like a charm for me. It could be shipped with autorandr and started by an autostart script. I believe this would be a nice replacement for all the udev and systemd hooks.

What do you think?

chmduquesne commented 4 years ago

Also, I have seen some tickets related to virtual monitors. I am unfamiliar with those, but in order to detect when monitors are plugged/unplugged, this script relies on the dbus interface of colord, which lists "virtual devices" as supported subsystems. It could be worth having a look: https://www.freedesktop.org/software/colord/intro.html

chmduquesne commented 4 years ago

Hi! Has anybody any interest in this?

I could do a PR and do a big cleanup of all the other autorandr launchers, but if I am the only one who wants this, I will not bother...

fidergo-stephane-gourichon commented 4 years ago

Hi. Naive question here: as a user that just wants things to work, what's the point of detecting closure of the lid? In other words, to understand the benefit of the work you offer, what happens to that user without the effort done, what is the concrete benefit for that user, with it?

chmduquesne commented 4 years ago

This is just code refactoring with no direct benefit for the user, except that there will be only a dependency on dbus, and no longer a need for udev rules/pm-utils scripts/systemd scripts.

The lid state monitoring is already a feature, actually. It is automatically started by /etc/xdg/autostart/autorandr-lid-listener.desktop.

The benefit for the user is to be able to treat a closed lid and an open lid as 2 different configurations, something most people expect to be able to do. There is usually no way to physically unplug the lid, and therefore opening/closing it is the next best thing.

fidergo-stephane-gourichon commented 4 years ago

Thanks @chmduquesne for your explanation. I'm just a user of autorandr on XFCE (and noticed that recent XFCE ship similar features, yet I still run autorandr I guess there's still a reason).

The benefit for the user is to be able to treat a closed lid and an open lid as 2 different configurations, something most people expect to be able to do. There is usually no way to physically unplug the lid, and therefore opening/closing it is the next best thing.

Thanks! I understand now how detecting lid closure if something that just-want-it-to-work-users can appreciate.

To go or not to go?

Here are some questions that come to mind:

"Turn worse into better"? If the code refactoring actually simplifies the code, or even only gets rid of "worse" interfaces in favor of "better" interfaces, then I guess it's probably a good thing.
"Less bad"? Is libinput kind of deprecated in favor of DBUS-based solutions?
"Lost value"? Will there be anything lost after this effort is done? For example I'm wondering if some users/distros/embedded system would miss the libinput code path?

If every light is green on the path, then feel free to go for it!

Perhaps this very comment can act as the "yes, at least one man will notice". Also, I have a laptop and external monitor(s) and may probably do some quick test when you ask.

Cheers!

phillipberndt commented 4 years ago

Hey, sorry for taking so long to respond.. first of all, thanks for taking the initiative! :+1:

A Dbus integration sounds like a good way to go. I myself use the launcher and am much happier with that than I ever was with the udev solution. But check out #195 before you start working on that! Also, it'd be great to keep dependencies of the overall autorandr package small. (And those of the main binary to zero.) So e.g. glib shouldn't be required. Maybe for pydbus there's a more widespread alternative, or maybe having a C helper as with the launcher is more compatible. (I don't know and don't even have the suspicion. That's just a question.)

chmduquesne commented 4 years ago

👍

Also, it'd be great to keep dependencies of the overall autorandr package small. (And those of the main binary to zero.) So e.g. glib shouldn't be required.

Ok, this one is hard... The problem is that pydbus is the best library to query dbus, so you are basically asking to avoid using python for this solution, because I don't think there is a more widespread dependency.

maybe having a C helper as with the launcher is more compatible.

That would be moving the problem from python to C. Then you need to link to the dbus system libraries, which is incredibly painful.

Generally I think using dbus is a good solution in terms of dependencies for the user, because nowadays if you are running systemd, you have it.

The most minimalistic solution that I can think of, in terms of dependencies, is to continuously filter the output of dbus-monitor. Then there will be no compilation problem, and since dbus-monitor ships with dbus, no install dependencies either. However it will be more wasteful of resources, because instead of properly mapping the right events to actions, there will be a lot of grep-ing on a quite verbose output. I don't mind writing that code, but I want to warn you in advance.

I find this solution still preferable to what already exists, because I really do not like the multiple hooks approach: if I want to debug who launched autorandr, I have to look all over the place. Having one unique launcher would be much cleaner from a sysadmin perspective.

phillipberndt commented 4 years ago

Another option is to take the dependency, but don't make the new launcher the default configuration / automatically skip it if dependencies are unfulfilled.. but we'd have to sync on that with the package maintainers.

chmduquesne commented 4 years ago

This would be elegant.

So basically if pydbus is missing:

the launcher exits silently

autorandr issues a warning message:

For autorandr to run automatically whenever a new screen is plugged or your lid state changes, pydbus needs to be on your system. If you want to stop seeing this warning, please install python3-pydbus or create the file ~/.config/autorandr/pydbus-nowarn.

Would that work for you?

chmduquesne commented 4 years ago

I just thought of a potentially even better solution, which is to rewrite the code of the launcher to require pydbus, but if it is not here, fall back to using dbus-monitor. Then there is no warning message, but we can tell the package maintainers to put python-pydbus as an optional dependency in their package, and the user is free to install it if they want more the launcher to consume less resources.

mrngm commented 4 years ago

Just a thought. The wiki page referenced here was last updated in 2016. The last release of pydbus (v0.6.0) is equally old, and as far as I can tell, there is little to no maintenance on the project itself.

I would think twice before depending on a library that's not actively maintained.

chmduquesne commented 4 years ago

as far as I can tell, there is little to no maintenance on the project itself.

Meh, you are right.

https://stackoverflow.com/questions/58639602/what-is-recommended-to-use-pydbus-or-dbus-python-and-what-are-the-differences

Ok then I am lost about what is the clean way to query dbus from python. I am going to write the "dirty" fallback solution based on dbus-monitor, because it is a no-dependency thing. In the meantime if anybody has a clean dbus api to recommend, I am all ears.

phillipberndt commented 4 years ago

That was my thinking when I mentioned the C wrapper.

dbus-monitor has a "watch expressions" parameter. Maybe that allows to do the filtering,too?

chmduquesne commented 4 years ago

It does some of it.

Getting the screen events: dbus-monitor --system "type='signal',path=/org/freedesktop/UPower"

Getting the monitor events: dbus-monitor --system "type='signal',path=/org/freedesktop/ColorManager"

The output is multi-line and not all events are relevant, which is the reason why you need additional parsing. I don't know if we need additional events (e.g. suspend) but we can probably get them in a similar fashion as well.

mrngm commented 4 years ago

It does some of it.

Getting the screen events: dbus-monitor --system "type='signal',path=/org/freedesktop/UPower"

Getting the monitor events: dbus-monitor --system "type='signal',path=/org/freedesktop/ColorManager"

The output is multi-line and not all events are relevant, which is the reason why you need additional parsing. I don't know if we need additional events (e.g. suspend) but we can probably get them in a similar fashion as well.

If you're confident in binary parsing, you could try this type of output:

$ sudo dbus-monitor --system --binary "type='signal',path=/org/freedesktop/UPower" | xxd
00000000: 6c04 0101 0d00 0000 0200 0000 9500 0000  l...............
00000010: 0101 6f00 1500 0000 2f6f 7267 2f66 7265  ..o...../org/fre
00000020: 6564 6573 6b74 6f70 2f44 4275 7300 0000  edesktop/DBus...
00000030: 0201 7300 1400 0000 6f72 672e 6672 6565  ..s.....org.free
00000040: 6465 736b 746f 702e 4442 7573 0000 0000  desktop.DBus....
00000050: 0301 7300 0c00 0000 4e61 6d65 4163 7175  ..s.....NameAcqu
00000060: 6972 6564 0000 0000 0601 7300 0800 0000  ired......s.....
00000070: 3a31 2e31 3037 3931 0000 0000 0000 0000  :1.10791........
00000080: 0801 6700 0173 0000 0701 7300 1400 0000  ..g..s....s.....
00000090: 6f72 672e 6672 6565 6465 736b 746f 702e  org.freedesktop.
000000a0: 4442 7573 0000 0000 0800 0000 3a31 2e31  DBus........:1.1
000000b0: 3037 3931 006c 0401 010d 0000 0004 0000  0791.l..........
000000c0: 0095 0000 0001 016f 0015 0000 002f 6f72  .......o...../or
000000d0: 672f 6672 6565 6465 736b 746f 702f 4442  g/freedesktop/DB
000000e0: 7573 0000 0002 0173 0014 0000 006f 7267  us.....s.....org
000000f0: 2e66 7265 6564 6573 6b74 6f70 2e44 4275  .freedesktop.DBu
00000100: 7300 0000 0003 0173 0008 0000 004e 616d  s......s.....Nam
00000110: 654c 6f73 7400 0000 0000 0000 0006 0173  eLost..........s
00000120: 0008 0000 003a 312e 3130 3739 3100 0000  .....:1.10791...
00000130: 0000 0000 0008 0167 0001 7300 0007 0173  .......g..s....s
00000140: 0014 0000 006f 7267 2e66 7265 6564 6573  .....org.freedes
00000150: 6b74 6f70 2e44 4275 7300 0000 0008 0000  ktop.DBus.......
00000160: 003a 312e 3130 3739 3100 6c04 0101 3c00  .:1.10791.l...<.
00000170: 0000 9405 0000 8700 0000 0101 6f00 1700  ............o...
00000180: 0000 2f6f 7267 2f66 7265 6564 6573 6b74  ../org/freedeskt
00000190: 6f70 2f55 506f 7765 7200 0201 7300 1f00  op/UPower...s...
000001a0: 0000 6f72 672e 6672 6565 6465 736b 746f  ..org.freedeskto
000001b0: 702e 4442 7573 2e50 726f 7065 7274 6965  p.DBus.Propertie
000001c0: 7300 0801 6700 0873 617b 7376 7d61 7300  s...g..sa{sv}as.
000001d0: 0000 0301 7300 1100 0000 5072 6f70 6572  ....s.....Proper
000001e0: 7469 6573 4368 616e 6765 6400 0000 0000  tiesChanged.....
000001f0: 0000 0701 7300 0600 0000 3a31 2e35 3737  ....s.....:1.577
00000200: 0000 1600 0000 6f72 672e 6672 6565 6465  ......org.freede
00000210: 736b 746f 702e 5550 6f77 6572 0000 1800  sktop.UPower....
00000220: 0000 0b00 0000 4c69 6449 7343 6c6f 7365  ......LidIsClose
00000230: 6400 0162 0000 0000 0000 0000 0000 6c04  d..b..........l.
00000240: 0101 3c00 0000 9505 0000 8700 0000 0101  ..<.............
00000250: 6f00 1700 0000 2f6f 7267 2f66 7265 6564  o...../org/freed
00000260: 6573 6b74 6f70 2f55 506f 7765 7200 0201  esktop/UPower...
00000270: 7300 1f00 0000 6f72 672e 6672 6565 6465  s.....org.freede
00000280: 736b 746f 702e 4442 7573 2e50 726f 7065  sktop.DBus.Prope
00000290: 7274 6965 7300 0801 6700 0873 617b 7376  rties...g..sa{sv
000002a0: 7d61 7300 0000 0301 7300 1100 0000 5072  }as.....s.....Pr
000002b0: 6f70 6572 7469 6573 4368 616e 6765 6400  opertiesChanged.
000002c0: 0000 0000 0000 0701 7300 0600 0000 3a31  ........s.....:1
000002d0: 2e35 3737 0000 1600 0000 6f72 672e 6672  .577......org.fr
000002e0: 6565 6465 736b 746f 702e 5550 6f77 6572  eedesktop.UPower
000002f0: 0000 1800 0000 0b00 0000 4c69 6449 7343  ..........LidIsC
00000300: 6c6f 7365 6400 0162 0000 0100 0000 0000  losed..b........

I suppose there is a format description, but it's not in the man-page of dbus-monitor.

doctorcolossus commented 2 months ago

The benefit for the user is to be able to treat a closed lid and an open lid as 2 different configurations, something most people expect to be able to do. There is usually no way to physically unplug the lid, and therefore opening/closing it is the next best thing.

Actually I had the opposite expectation, that my configuration should depend simply on whether my external monitor was plugged in or not. It was not until I found and read through this issue that I understood why my configured configuration (switching to the external monitor) worked when plugging in my monitor with the lid open, but not with the lid closed. This was very confusing and time-consuming for me to figure out. I think that most users would set up their configuration with the lid initially open, since they would initially need to use the built-in display to do so.

So I think it should be made clear in the documentation that lid status is an integral part of a configuration.

phillipberndt / autorandr

Detect lid state with dbus-monitor instead of libinput #213

To go or not to go?