Implement the session lock protocol #2162

lcolitti · 2024-02-25T09:03:24Z

Implement the session lock protocol

Currently implemented:

Locking and unlocking.
Display above all desktop layers, including OVERLAY (but below
DWIDGET).
Waiting for the client to display locked surfaces before
declaring the screen to be locked. If the client does not
display all surfaces, locks anyway after a timeout.
Multiple outputs, including outputs being added/removed while
locking or locked.
Leaving the screen locked if the client crashes (but allows a
new client to come in and lock the screen.

TODO:

Deal with outputs being resized.
Properly display text if the screenlocker crashes. Currently it
just displays two a big "explosion" emoji.
Use a view instead of a plain wlr_surface_node?

Tested with swaylock.

Fixes #1494
Fixes #1358

plugins/protocols/session-lock.cpp

ammen99

I have a few requests/suggestions on the implementation, but the general approach seems good!

src/api/wayfire/core.hpp

ammen99 · 2024-02-25T09:20:18Z

plugins/protocols/session-lock.cpp

+
+            void handle_keyboard_enter(wf::seat_t *seat)
+            {
+                // TODO: use a keyboard grab instead?


I think this is fine - wlroots grabs receive events if Wayfire decides to forward requests to wlroots - with HIGH focus importance and putting the lockscreen above everything else, Wayfire should always feed the focus to the lockscreen first.

BTW, any reason not to use wf::input_manager_t::set_exclusive_focus here? Seems like it would be simpler. If the client crashes I assume that focus will go nowhere, but it sounds like that's what we want.

set_exclusive_focus was meant for the older implementation of screenlockers, I think we can even remove this function nowadays. It is better to use the scenegraph API for this ;)

plugins/protocols/session-lock.cpp

ammen99 · 2024-02-25T09:27:49Z

plugins/protocols/session-lock.cpp

+
+                    lock_surface_destroy.disconnect();
+                });
+                lock_surface_destroy.connect(&lock_surface->events.destroy);


You will need one listener per created surface (because listeners cannot connect to multiple object's signals unlike Wayfire's signals). So lock_surface_destroy() probably will have to be moved to the node class.

Thanks for the suggestion. Done, I think.

plugins/protocols/session-lock.cpp

ammen99 · 2024-02-25T09:36:01Z

The swaylock indicator isn't displayed. Perhaps this is because swaylock uses a subsurface to draw it, and wlr-surface-node doesn't support child surfaces?

Yeah, you'd need to create a subsurface controller: https://github.com/WayfireWM/wayfire/blob/master/src/view/surface-impl.hpp#L15

Probably should be moved to the unstable APIs.

ammen99 · 2024-02-25T09:37:48Z

Deal with outputs being resized

I think it might actually make sense to actually create two nodes: one node is compositor-generated, saying 'Waiting for screenlocker ...' and above it, we should add the screenlock.

This covers basically all cases: before the screenlock surface is there, we hide the contents of the desktop. If the screenlock crashes, the second node below will still cover / lock the screen. And, if we have to resize the output, the second node will cover the screen while the screenlocker is resizing.

lcolitti · 2024-02-26T15:03:14Z

The swaylock indicator isn't displayed. Perhaps this is because swaylock uses a subsurface to draw it, and wlr-surface-node doesn't support child surfaces?

Yeah, you'd need to create a subsurface controller: https://github.com/WayfireWM/wayfire/blob/master/src/view/surface-impl.hpp#L15

Probably should be moved to the unstable APIs.

Done in this commit.. I also renamed them, but I can keep the existing names if you prefer.

lcolitti · 2024-02-28T12:51:23Z

I think it might actually make sense to actually create two nodes: one node is compositor-generated, saying 'Waiting for screenlocker ...' and above it, we should add the screenlock.

This covers basically all cases: before the screenlock surface is there, we hide the contents of the desktop. If the screenlock crashes, the second node below will still cover / lock the screen. And, if we have to resize the output, the second node will cover the screen while the screenlocker is resizing.

I'm not sure if this is the best approach. For one thing, if we create the surface immediately, it will be visible/jarring. Ideally we just display the lockscreen surface like we do today without this plugin. Personally my lockscreen is the same as my background so when the lockscreen activates it just looks as if the windows smoothly fade out. I'd hate to make that less slick 😄 I guess we could add this extra surface after a timeout? Not sure how to do that yet but there's probably a way.

Also, what would happen when an output is added? With the code as is, the plugin doesn't do anything. The screenlocker will probably react to this by creating a new surface, and when it does, the new output will be locked. But while the client creates the surface, I think there is a race where it's possible to display or interact with the new output, even if the others are locked. But can we close that race completely even if we create the surface in the compositor? There could still be a window between when the output is created and the surface is added.

ammen99 · 2024-02-28T13:12:42Z

But can we close that race completely even if we create the surface in the compositor? There could still be a window between when the output is created and the surface is added.

We most certainly can. The compositor is in full control of drawing every frame on every output. If you create a window/node on a new output in the added handler for that event, it will be present on the very first output frame :)

I'd hate to make that less slick 😄 I guess we could add this extra surface after a timeout? Not sure how to do that yet but there's probably a way.

Ok, I guess this is a good point. Maybe the screenlocker wants to implement fade-in or a slide effect, so we shouldn't create the surface immediately. Maybe say 5 seconds after the lockscreen has kicked in? I'm not sure whether we want this to be configurable, so maybe just hardcode it and we can always add an option later.

ammen99 · 2024-02-28T13:15:06Z

Another thing I just realized: your implementation directly creates a scenegraph node for the lockscreen. But this prevents the lockscreen from interacting with stuff like blur and animations (they require views). If you feel up for the task, you can also implement a simple view as a wrapper for the node class, see for example the color rect views implemented in compositor-view.hpp/cpp in core.

EDIT: if it wasn't clear, I consider this an optional feature, just saying if you care about smooth animations/transitions :)

lcolitti · 2024-03-03T08:14:42Z

Ok, this looks closer to done now.

I think it deals correctly with outputs being added and removed, and (as required by the protocol) it now waits for the screenlocker to display a frame before it sends back the lock event. It uses @ammen99 's suggestion to use two nodes to ensure that if the screenlocker crashes, the screen remains locked. The UX of that is a bit primitive (a big, pixelated explosion emoji), but that shouldn't be too hard to fix by modifying simple-text-node.

It still also uses a surface instead of a view, but maybe that's OK for now.

plugins/common/wayfire/plugins/common/simple-text-node.hpp

ammen99 · 2024-03-11T08:27:28Z

plugins/protocols/session-lock.cpp

+{
+    class wayfire_session_lock;
+
+    class lock_surface_node_t : public wf::scene::wlr_surface_node_t


Note that all nodes are shared_ptrs - this means that in general, it is possible that a node outlives the session-lock plugin to begin with, it can also outlive the output it is on, etc.

So, you should try to add code so that the object can 'work' (at least not crash) even as the underlying surface, lock, output, etc. are destroyed. I have tried to add some suggestions on how to accomplish this.

ammen99 · 2024-03-11T08:29:07Z

plugins/protocols/session-lock.cpp

+                wf::wlr_surface_controller_t::try_free_controller(this->lock_surface->surface);
+                wf::scene::remove_child(shared_from_this());
+                lock_surface_destroy.disconnect();
+                this->lock->surface_destroyed(this->output);


Re lifetimes: I would personally set the destroy callback in the wayfire_session_lock class (this way, you don't have to pass the lock parameter.

ammen99 · 2024-03-11T08:31:43Z

plugins/protocols/session-lock.cpp

+                lock_surface_destroy.disconnect();
+                this->lock->surface_destroyed(this->output);
+                const char *name = this->output->handle ? this->output->handle->name : "(deleted)";
+                LOGD("lock_surface on ", name, " destroyed");


You can/should reset the keyboard interaction to something else (something like this: https://github.com/WayfireWM/wayfire/blob/master/src/view/wlr-surface-node.cpp#L102) so that we don't get a crash if someone holds on to the node after the surface/output are destroyed.

And also re lifetimes, you should set lock_surface to NULL here, and make sure it is not used in the other functions if it is NULL already (where it makes sense)

Attempted to reset the keyboard interaction.

I don't think lock_surface can be used after it's destroyed. The only other method that uses it is attach_to_layer. But attach_to_layer will only be called if the output_state->surface is non-null. But when the lock_surface is destroyed, the callback for the destroy event calls this->lock->surface_destroyed, which nulls out the output_state->surface, so attach_to_layer will not be called.

Ok, it might still be useful to set it to NULL so that if in the future we change anything, we directly get a null pointer crash instead of a dangling pointer.

ammen99 · 2024-03-11T08:38:08Z

plugins/protocols/session-lock.cpp

+            plugin(plugin), lock(lock)
+        {
+            auto& ol = wf::get_core().output_layout;
+            output_added.set_callback([this] (wf::output_added_signal *ev)


Instead of doing this manually, check out https://github.com/WayfireWM/wayfire/blob/master/src/api/wayfire/per-output-plugin.hpp#L30

You can add it as a base class and then it will connect to the signals, etc. for you.

Looking at the code below, I assume you did this per hand simply because the mixin does not support non-plugin template types (yet). I think it should be very easy (and a good idea) to fix the mixin class - with an if constexpr (std::is_base_v<per_output_plugin_instance_t, ConcretePluginType>) {} around code blocks which are specific for per-output-plugin instances, so that the helper can be used generically.

I did use the mixin for a while, to create the per-output objects. The reason I stopped using it is that the mixin expects the per-output instances to have zero-arg constructors. But here, the instances need to have a backpointer to a plugin-global object, for example to call notify_lock_state. Not sure if this is easy to do in the mixin - my command of C++ syntax isn't great.

plugins/protocols/session-lock.cpp

ammen99 · 2024-03-11T08:45:52Z

plugins/protocols/session-lock.cpp

+
+    void fini() override
+    {
+        // TODO: unlock everything?


I don't think we actually need to do anything. Plugins that cannot be unloaded like this one will be destroyed only at compositor shutdown. At that point the wayland display is being destroyed, so wlroots will auto-destroy the protocol and it will emit destruction signals for the various lock surfaces, etc. (I hope it does so at least).

Actually right now I'm looking at the shutdown crashes problem, and will have to make it so that plugins are destroyed before the display is destroyed. This means that here we should destroy the wlr session lock object.

plugins/protocols/session-lock.cpp

ammen99 · 2024-03-11T08:53:33Z

I went through your code, the overall design & implementation looks good. Lifetimes are hard to get right, especially when you have a shared_ptr, that needs some care.

These classes are useful and needed by any code that deals with surfaces, because such code usually needs to deal with subsurfaces as well. Move the include files into api/wayfire/unstable so they can be used by code all over the tree, and rename them to reflect the utility classes that they contain.

This will allow lockscreens to be implemented in plugins.

This adds a simple node that can display text.

This will be used by the upcoming implementation of the ext-session-lock protocol.

Currently implemented: - Locking and unlocking. - Display above all desktop layers, including OVERLAY (but below DWIDGET). - Waiting for the client to display locked surfaces before declaring the screen to be locked. If the client does not display all surfaces, locks anyway after a timeout. - Multiple outputs, including outputs being added/removed while locking or locked. - Leaving the screen locked if the client crashes (but allows a new client to come in and lock the screen). TODO: - Deal with outputs being resized. - Properly display text if the screenlocker crashes. Currently it just displays two a big "explosion" emoji. - Use a view instead of a plain wlr_surface_node? Tested with swaylock. Fixes WayfireWM#1494 Fixes WayfireWM#1358

This reverts commit ec91bfb.

ammen99 · 2024-03-21T15:49:13Z

@lcolitti Sorry I mistakenly merged this PR without even reviewing your latest changes .. Would you mind opening a new pull request or reopening this one? I'm not sure how to make GitHub do that ...

This reverts commit ec91bfb.

lcolitti · 2024-03-22T00:36:34Z

Sorry I didn't update this PR for a while. I've been travelling and can't test with my multi-monitor setup.

I don't think there's a way to reopen this after the merge. Opened #2237 instead. @ammen99 can you take a look? The code there addresses most of your review comments, including hopefully not crashing on shutdown. I've been running this code on my machine for a while and it seems to work well.

soreau reviewed Feb 25, 2024

View reviewed changes

plugins/protocols/session-lock.cpp Outdated Show resolved Hide resolved

ammen99 reviewed Feb 25, 2024

View reviewed changes

lcolitti mentioned this pull request Feb 25, 2024

Support ext-session-lock #1494

Closed

lcolitti force-pushed the support-session-lock branch from cf7fcac to b02854d Compare February 26, 2024 14:50

lcolitti closed this Feb 26, 2024

lcolitti force-pushed the support-session-lock branch from b02854d to 982605d Compare February 26, 2024 15:00

lcolitti reopened this Feb 26, 2024

lcolitti force-pushed the support-session-lock branch 2 times, most recently from cfe5264 to f3fce82 Compare February 28, 2024 12:35

lcolitti closed this Mar 3, 2024

lcolitti force-pushed the support-session-lock branch from f3fce82 to 27ddca3 Compare March 3, 2024 07:58

lcolitti reopened this Mar 3, 2024

lcolitti marked this pull request as ready for review March 3, 2024 08:14

lcolitti changed the title ~~WIP: Implement the session lock protocol~~ Implement the session lock protocol Mar 3, 2024

lcolitti force-pushed the support-session-lock branch 2 times, most recently from c0ad25c to efda3fe Compare March 4, 2024 13:55

ammen99 requested changes Mar 11, 2024

View reviewed changes

lcolitti force-pushed the support-session-lock branch from efda3fe to b1ff262 Compare March 13, 2024 14:55

lcolitti added 4 commits March 14, 2024 17:00

Add a public API to inhibit/uninhibit outputs.

2861cfa

This will allow lockscreens to be implemented in plugins.

Factor the text label rendering out of wset into a utility class.

34fb51d

This adds a simple node that can display text.

Add a LOCK scenegraph layer.

91088a2

This will be used by the upcoming implementation of the ext-session-lock protocol.

lcolitti force-pushed the support-session-lock branch from fb627ba to 4174425 Compare March 14, 2024 08:00

ammen99 merged commit ec91bfb into WayfireWM:master Mar 21, 2024
3 of 4 checks passed

ammen99 added a commit that referenced this pull request Mar 21, 2024

Revert "Implement the session lock protocol (#2162)"

f5147dd

This reverts commit ec91bfb.

ammen99 added a commit that referenced this pull request Mar 21, 2024

Revert "Implement the session lock protocol (#2162)" (#2233)

bea2d5e

This reverts commit ec91bfb.

ammen99 added a commit that referenced this pull request Mar 21, 2024

Revert "Implement the session lock protocol (#2162)"

227b3c6

This reverts commit ec91bfb.

lcolitti added a commit to lcolitti/wayfire that referenced this pull request Mar 22, 2024

Revert "Implement the session lock protocol (WayfireWM#2162)"

f0f732b

This reverts commit ec91bfb.

lcolitti mentioned this pull request Mar 22, 2024

Implement the session lock protocol, take 2 #2237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the session lock protocol #2162

Implement the session lock protocol #2162

lcolitti commented Feb 25, 2024 •

edited by ammen99

Loading

ammen99 left a comment

ammen99 Feb 25, 2024

lcolitti Feb 28, 2024

ammen99 Feb 28, 2024

ammen99 Feb 25, 2024

lcolitti Feb 28, 2024

ammen99 commented Feb 25, 2024

ammen99 commented Feb 25, 2024

lcolitti commented Feb 26, 2024

lcolitti commented Feb 28, 2024

ammen99 commented Feb 28, 2024

ammen99 commented Feb 28, 2024 •

edited

Loading

lcolitti commented Mar 3, 2024

ammen99 Mar 11, 2024

ammen99 Mar 11, 2024

ammen99 Mar 11, 2024

ammen99 Mar 11, 2024

lcolitti Mar 13, 2024

ammen99 Mar 13, 2024

ammen99 Mar 11, 2024

ammen99 Mar 11, 2024

lcolitti Mar 13, 2024

ammen99 Mar 11, 2024 •

edited

Loading

ammen99 commented Mar 11, 2024

ammen99 commented Mar 21, 2024

lcolitti commented Mar 22, 2024

Implement the session lock protocol #2162

Implement the session lock protocol #2162

Conversation

lcolitti commented Feb 25, 2024 • edited by ammen99 Loading

ammen99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammen99 commented Feb 25, 2024

ammen99 commented Feb 25, 2024

lcolitti commented Feb 26, 2024

lcolitti commented Feb 28, 2024

ammen99 commented Feb 28, 2024

ammen99 commented Feb 28, 2024 • edited Loading

lcolitti commented Mar 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammen99 Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

ammen99 commented Mar 11, 2024

ammen99 commented Mar 21, 2024

lcolitti commented Mar 22, 2024

lcolitti commented Feb 25, 2024 •

edited by ammen99

Loading

ammen99 commented Feb 28, 2024 •

edited

Loading

ammen99 Mar 11, 2024 •

edited

Loading