Skip to content
This repository has been archived by the owner on Aug 3, 2024. It is now read-only.

[WIP] Idle Support #265

Merged
merged 3 commits into from
Feb 13, 2019
Merged

[WIP] Idle Support #265

merged 3 commits into from
Feb 13, 2019

Conversation

SethBarberee
Copy link
Contributor

Closes #100

WIP as I can't get it to see the seat pointer. I was trying to do what server decoration does but 🤷‍♂️

Everything else is supposedly working according to cargo... 🤞

IF working, we can support swayidle now.

@Timidger
Copy link
Member

I can't get it to see the seat pointer

Can you elaborate? If you have a Seat you should be able to call as_ptr on it to get the *mut wlr_seat.

@SethBarberee
Copy link
Contributor Author

When you mentioned wlr_seat, you gave me the solution. Compilation works on my machine.. now we wait for CI.

}

/// Restart the timers for the seat
pub fn notify_activity(&mut self, seat: *mut wlr_seat) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not safe, you should be passing a &Seat here and internally using as_ptr to convert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


/// Restart the timers for the seat
pub fn notify_activity(&mut self, seat: *mut wlr_seat) {
unsafe { wlr_idle_notify_activity(self.manager, seat as *mut wlr_seat) }
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

/// If we are passed a null pointer,
/// update timers for all seats.
Copy link
Member

Choose a reason for hiding this comment

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

Put this on one doc line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/// If we are passed a null pointer,
/// update timers for all seats.
pub fn set_enabled(&mut self, seat: *mut wlr_seat, enabled: bool) {
wlr_log!(WLR_INFO, "Idle timer status: {:?}", enabled);
Copy link
Member

Choose a reason for hiding this comment

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

Remove this log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


/// If we are passed a null pointer,
/// update timers for all seats.
pub fn set_enabled(&mut self, seat: *mut wlr_seat, enabled: bool) {
Copy link
Member

Choose a reason for hiding this comment

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

This should also be a &Seat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/// update timers for all seats.
pub fn set_enabled(&mut self, seat: *mut wlr_seat, enabled: bool) {
wlr_log!(WLR_INFO, "Idle timer status: {:?}", enabled);
unsafe { wlr_idle_set_enabled(self.manager, seat as *mut wlr_seat, enabled) }
Copy link
Member

Choose a reason for hiding this comment

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

Also an unnecessary cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -35,6 +35,26 @@ mod generated {
include!(concat!(env!("OUT_DIR"), "/server_decoration_server_api.rs"));
}
}
pub mod idle {
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 mainly copied-pasted this part and subbed wl_surface with wl_seat. What's the point of this section?

Copy link
Member

Choose a reason for hiding this comment

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

The code generated in the build script is placed into files, specifically <interface_name>_api.rs and <interface_name>_interfaces.rs. We need these definitions because they implement glue code for taking our Rust types and marshaling them into a form to be sent over the Wayland socket. Since we marshal to this type, this means the response can be consumed by any language (because they can convert from the Wayland types to their lang types).

The use statements are here because we need literally copy and paste the code from the files (kind of like how #include works in C actually) using the include! macro.

It needs to be in a module because this generates a lot of code we don't care about. The only code we about is in the generated server module, which we re-export at the end of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So considering that I only did it for idle, should I do a PR for the rest of the extensions that I've done so far? Since from what I understand, the other extensions aren't still linked up properly then..

Copy link
Member

Choose a reason for hiding this comment

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

Hm, actually looking at this again I don't think this is necessary. I haven't looked at the extensions in a while, so it might have changed.

This should not be here actually, since wlroots already links to it. I believe this to be the case, so this generation code can actually be removed since it's dead code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this code is dead. The only one that is needed is the server decoration, and that's for an enum defined in the protocol. So sorry about that, your other protocols are fine. So is this one, it's just that this code is unnecessary. I'll make an issue for that now: #266

@SethBarberee
Copy link
Contributor Author

I dug a little more and found out my main mistake of using the unsafe wlr_seat versus the wrapped Seat that you already made safe.. My knowledge of Rust and this codebase is slowly growing. Fixing all of the commented stuff right now and will get it updated soon.

@Timidger
Copy link
Member

Good! In order to make sure the knowledge is correct, let me explain further.

At first glance the two seem isomorphic, since you can get the pointer from the Seat (ignoring for the moment that as_ptr is pub(crate), which doesn't export it to the library user, since it will eventually be exposed), however it changes the safety constraints of the function.

A function is safe if there's no way to produce memory safety in any of its inputs. For a Seat, the only way to safely construct one is using Seat::create, which ensures the pointer is valid (we are trusting wlroots to give us a valid, non-null pointer here). Since a *mut T can be null (which is invalid in this context) or be constructed from any arbitrary number we can't have this in our interface without marking the function as unsafe (as in the function is labeled unsafe fn ...). Nothing force us to do this, we can say it's safe but it's not safe.

@Timidger
Copy link
Member

Also also the Seat keeps track that the Seat is still "alive" because you get a &mut Seat only from handle upgrades. That technically isn't 100% necessary for Seat since it's owned by the compositor, but the other objects need to be that because it's not owned and so I made the interfaces the same in order to make it easier.

Though that's getting into the weeds of the rest of the library, which has a very complicated memory model and not very relevant to this immediate work.

@Timidger
Copy link
Member

Thanks!

@Timidger Timidger merged commit e6ffafc into swaywm:master Feb 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants