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

Add D-Bus signals to notify session lock and unlock states #609

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Iamanaws
Copy link

@Iamanaws Iamanaws commented Dec 25, 2024

Send an "Unlock" D-Bus signal to notify programs listening for Unlock events.

This enables tools like hypridle to detect the Unlock event and execute the unlock_cmd.

Issues:
hyprwm/hypridle#79
hyprwm/hypridle#112

@PaideiaDilemma
Copy link
Collaborator

While i understand what you want to achieve, i think that is not the way this dbus interface is supposed to work.

From org.freedesktop.login1:

Lock()/Unlock() is sent when the session is asked to be screen-locked/unlocked. A session manager of the session should listen to this signal and act accordingly. This signal is sent out as a result of the Lock() and Unlock() methods, respectively.

So from what i understand Unlock is not supposed to signal that we just unlocked, but to request an unlock?? Not entirely clear to me how it is supposed to be used though.

@Iamanaws
Copy link
Author

Yes, I agree it’s a bit unclear, but from what I understand, Unlock() is intended to inform the Session Manager and other programs listening to D-Bus that the lock screen should be removed. It’s up to each listening process to handle this notification as they see fit.

In this case, Hyprlock directly manages the unlock process via Wayland using the ext_session_lock_v1 interface. In other scenarios, a lock screen might call Unlock() to signal the Session Manager that the unlock should proceed after successful authentication.

The key point is that Unlock() does not handle the unlocking itself—it simply notifies that it should occur or has occurred. By sending the Unlock() method after handling the unlock, Hyprlock ensures compatibility with tools and listeners on D-Bus, even if it doesn’t rely on them.

Since Hyprlock already performs the actual unlock via Wayland, calling Unlock() is purely informational and has no functional impact on the unlocking process.

@PaideiaDilemma
Copy link
Collaborator

Can't you just use smth like hyprlock && loginctl unlock-session??

@PaideiaDilemma
Copy link
Collaborator

But i agree that it would be better if hypridle's unlock_cmd just worked...
Do you know any lock screen app that sends Unlock?

@Iamanaws
Copy link
Author

Can't you just use smth like hyprlock && loginctl unlock-session??

Yes, but we should map every hyprlock call or make an alias.

But i agree that it would be better if hypridle's unlock_cmd just worked... Do you know any lock screen app that sends Unlock?

I'm looking into it, there's not much documentation or code about it out there.

I see GDM calls login1.Manager UnlockSession() method that uses Session Unlock. GDM

LockSession() asks the session with the specified ID to activate the screen lock. UnlockSession() asks the session with the specified ID to remove an active screen lock, if there is any. This is implemented by sending out the Lock() and Unlock() signals from the respective session object which session managers are supposed to listen on. org.freedesktop.login1.Manager#Methods

@Iamanaws
Copy link
Author

Iamanaws commented Dec 25, 2024

i3-lock and swaylock lack the functionality.

But I found there's a LockedHint property that is part of the Session too and could be an option.
I like it because it removes the ambiguity of Unlock() signal, but in that case, I will have to make a change to Hypridle to be listening to the LockedHint property, and will have to test how well it works.


Also, I found a comment with an interesting interpretation of the org.freedesktop.login1 documentation and how it could be properly implemented.

compliant implementation

With Gnome-Shell and GDM it would be something like:

  1. Session.Lock() -> Gnome-Shell -> GDM
  2. Session.SetLockedHint(true)
  3. .
    3.1 Session.Unlock -> Gnome-Shell
    3.2 GDM -> Manager.UnlockSession() -> Session.Unlock() -> Gnome-Shell
  4. Session.SetLockedHint(false)

Translated will be similar to this path:

  1. Session.Lock() (loginctl) -> Hypridle -> Hyprlock -> Wayland (ext_session_lock_manager_v1_lock)
  2. hyprlock -> Session.SetLockedHint(true)
  3. hyprlock -> Wayland (ext_session_lock_v1_unlock_and_destroy)
  4. hyprlock -> Session.Unlock()
  5. hyprlock -> Session.SetLockedHint(false)

Another solution would be to have a configuration option for a unlock_cmd in Hyprlock, so we can set
unlock_cmd "loginctl unlock-session" from hyprlock. as discussed on sway issue

@Iamanaws Iamanaws changed the title Add D-Bus unlock functionality Add D-Bus signals to notify session lock and unlock states Dec 26, 2024
@PaideiaDilemma
Copy link
Collaborator

LockedHint sounds like a better solution thanks!

The only thing is that we now would have 2 dbus connection objects for org.freedesktop.login1, when a user enables fingerprint authentication as well.

We should centralize that by implementing a dbus manager class. Idk if you would want to try to do that. If you don't , I think we can do it afterwards.

Also it would be good to add an option to enable/disable this. Enable per default.

@Iamanaws
Copy link
Author

I centralized the dbus connection, could you give it a look please?

src/core/DBusManager.cpp Outdated Show resolved Hide resolved
src/core/DBusManager.hpp Outdated Show resolved Hide resolved
}

std::shared_ptr<sdbus::IConnection> DBusManager::getConnection() {
std::lock_guard<std::mutex> lock(m_mutex);
Copy link
Collaborator

@PaideiaDilemma PaideiaDilemma Jan 4, 2025

Choose a reason for hiding this comment

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

this lock does nothing cause it passes its raw resource out of the function, same below. outer std::shared_pointer is thread safe. about inner check https://github.com/Kistler-Group/sdbus-cpp/blob/master/docs/using-sdbus-c++.md#thread-safety-in-sdbus-c
I should be fine but it says it is generally better to create exclusive proxies for different threads.

Copy link
Author

@Iamanaws Iamanaws Jan 4, 2025

Choose a reason for hiding this comment

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

Got it, thanks for the feedback!
I removed the locks (b22009f), and since our usage is pretty low-volume I don't think we really need multiple threads at the moment.

If you have any other comments, let me know

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right we don't even use the proxies in different threads.
In that case can you use SP<> (shared pointer impl from hyprutils) instead of std::shared_ptr?

@PaideiaDilemma
Copy link
Collaborator

I am still a bit unsure if this is the right solution.

For me it is unclear if it is appropriate to call this method as a session-lock client.
I think it would be more appropriate for the compositor that implements the server side of session-lock to set the locked hint. But hyprland currently doesn't do dbus stuff and i think that is fine. Now if some compositor would implement it, I guess we would not break it, we would just set the hint twice right?.

What i also did not consider initially is that unlock_cmd actually has a proper functionality already, but is easy to be misunderstood. You could for example do

unlock_cmd = pkill -USR1 hyprlock

to have loginctl unlock-session unlock the session properly.
this would not make any sense any more, if unlock_cmd would be triggered by hyprlock!

However signaling lock/unlock is also related to the start before suspend problem that we also have. If we can properly signal lock and unlock, we can solve that by waiting for locked in before_sleep_cmd.

What do you think about just implementing an internal way of signaling lock/unlock to hypridle and use that for two additional triggers in hypridle and to wait for locked when launching hyprlock via before_sleep_cmd.

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.

2 participants