Add enumflags2 for DeviceCapability enum

[axelkar]

This removes the need for slices and Vecs, and makes client and server code a lot simpler to implement with BitFlags::contains and friends.

[ids1024]

wayland-rs uses bitflags. Not sure what reasons there might be to prefer one over the other between that and enumflags2.

[axelkar]

I originally based this PR on bitflags, but changed to enumflags2 because you can get enums in the iterator. It means less expect and unreachable!. I'd strongly prefer that, especially for an API.

I think wayland-rs might use bitflags because it automatically supports storing unknown bits. With enumflags2, it definitely is possible by parsing FromBitsError and storing unknown bits separately, but it's not the main use case. If there's a const _ = !0 in bitflags! in wayland-rs, it means that the generated struct will store unknown bits.

[axelkar]

As the server, we don't need to store unknown bits, because we control the meaning of the bits. As the client, we won't do anything with bits we don't know the meaning of; that's why the error for an unknown interface.

As a maintainer, that means that when you advertise support for a device capability interface in the handshake, you need to add it to the enum. Nothing else really.

[ids1024]

I think wayland-rs might use bitflags because it automatically supports storing unknown bits. With enumflags2, it definitely is possible by parsing FromBitsError and storing unknown bits separately, but it's not the main use case. If there's a const _ = !0 in bitflags! in wayland-rs, it means that the generated struct will store unknown bits.

Wayland-rs doesn't currently use bitflags this way for storing unknown bits (at least for things it receives), but it probably should: Smithay/wayland-rs#793

It seems with wayland, it's valid with some enums but not others to advertise a value that is not advertised in the version of the protocol in use: https://gitlab.freedesktop.org/wayland/wayland/-/issues/497, though that's has never been clearly specified.

With libei, it looks like https://gitlab.freedesktop.org/libinput/libei/-/blob/main/proto/protocol.dtd and https://gitlab.freedesktop.org/libinput/libei/-/blob/main/proto/ei-scanner do define a bitfield property, but it's not currently used anywhere in https://gitlab.freedesktop.org/libinput/libei/-/blob/main/proto/protocol.xml. So we may need to generate bitflags of some sort for the protocol in the future, but not currently. (And if we do, would we also want to allow unrecognized bits, as with Wayland?)

Though reis doesn't promise anything about avoiding API breaks (and is unlikely to ever be as much of a pain to deal with as wayland-rs API breaks), so it's not a huge problem if we have to change this in the future.

I originally based this PR on bitflags, but changed to enumflags2 because you can get enums in the iterator.

What exactly is different here? https://docs.rs/bitflags/latest/bitflags/example_generated/struct.Flags.html shows iter() and iter_names() methods.

[axelkar]

And if we do, would we also want to allow unrecognized bits, as with Wayland?

For the capability stuff, as the client, the opaque/dynamic masks are stored in capability_map. The server decides the meaning of each bit in the capability masks.

For bitmasks in the protocol, it really depends on the definition. Probably yes.

With enumflags2, you can simply store unknowns next to the known flags, like so:

#[enumflags2::bitflags]
#[repr(u64)]
enum MyFlag {}

struct ProtoMyFlags {
  known: enumflags2::BitFlags<MyFlag>,
  unknown: u64
}

With a PR, enumflags2::BitFlags could probably be made store unknowns within the same integer.

You can always do this too:

#[enumflags2::bitflags]
#[repr(u64)]
enum MyFlag {}

struct ProtoMyFlags(u64);

impl ProtoMyFlags {
  fn iter(&self) -> enumflags2::Iter {
    BitFlags::<MyFlag>::from_bits_truncate(self.0).iter()
  }
  fn has_flag(&self, flag: MyFlag) {
    BitFlags::<MyFlag>::from_bits_truncate(self.0).contains(flag)
  }
  fn get_unknown(&self) -> u64 {
    self.0 & !BitFlags::<MyFlag>::ALL.bits()
  }
}

I originally based this PR on bitflags, but changed to enumflags2 because you can get enums in the iterator.

What exactly is different here? https://docs.rs/bitflags/latest/bitflags/example_generated/struct.Flags.html shows iter() and iter_names() methods.

With bitflags, you don't get a static guarantee (like a Rust enum) that iter_names gives you valid flags. I.e.

flags.iter_names(|flag| match flag.bits() {
  Self::A => do_thing(),
  _ => unreachable!() // the compiler forces this on you!
})

fn interface_name(&self) -> Option<&'static str> {
  match self.bits() {
    Self::A => Some("a"),
    _ => None // same thing, and here the library isn't even promising that `self` is always a single valid flag
  }
}

Compare to the equivalent with enumflags2:

flags.iter(|flag| match flag {
  Self::A => do_thing(),
  // It's a Rust enum so we know this is exhaustive
})

fn interface_name(&self) -> &'static str {
  match self {
    Self::A => "a",
    // same thing
  }
}

---

While writing this, I got a reply to meithecatte/enumflags2#66. enumset looks way better for the use case of contiguous reprs.

[ids1024]

Ah right. So basically enumflags2 makes the individual flag an enum (as is in the name) and district from the Bitflags<T> containing it. Fair enough.

I think enumflags2 should be fine. If needed we can change this later anyway. This at least needs a cargo fmt to pass CI.

[axelkar]

Ugh.. Is there an MSRV of 1.70.0? Shouldn't it be specified in the Cargo.toml?

[ids1024]

Shouldn't it be specified in the Cargo.toml?

reis/Cargo.toml

Line 9 in a531810

rust-version = "1.70.0"

[axelkar]

Sorry for the delay! I wondered why Rust or Clippy didn't check the MSRV: rust-lang/compiler-team#772

Can you help fix the libei test? I don't see how I broke it

[ids1024]

Running the tests locally, it seems test/test_protocol.py::TestEiProtocol::test_connect_receive_seat is failing because that tests expects the server to advertise the EI_SCROLL and EI_BUTTON capabilities, but is missing those from the list of interfaces the client negotiates. Adding those to the setup.InterfaceVersion loop fixes it: https://gitlab.freedesktop.org/libinput/libei/-/blob/main/test/test_protocol.py?ref_type=heads#L554-630

So that breaks with the self.has_interface() test added in add_seat.

Then there's also test/test_protocol.py::TestEiProtocol::test_seat_bind_invalid_caps_expect_disconnection failing. That is from removing the capabilities & 0x7e != capabilities test from reis-demo-server. So we want some version of that check to pass tests.

[ids1024]

https://gitlab.freedesktop.org/libinput/libei/-/merge_requests/352 has the fix for that test.

[axelkar]

Seems that CI doesn't run cargo test. It was failing from a doc-test, so I fixed it.

[axelkar]

Still fails with the below??

=========================== short test summary info ============================
FAILED test/test_protocol.py::TestEiProtocol::test_seat_bind_invalid_caps_expect_disconnection - AssertionError: Expected seat to get destroyed but didn't
assert False
======================== 1 failed, 38 passed in 22.96s =========================

[ids1024]

The test passes for me with these two changes:

diff --git a/src/event.rs b/src/event.rs
index e042fc0..4728acf 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -680,17 +680,17 @@ pub struct Keymap {
 #[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
 pub enum DeviceCapability {
     /// Capability for relative pointer motion.
-    Pointer = 1 << 0,
+    Pointer = 1 << 1,
     /// Capability for absolute pointer motion.
-    PointerAbsolute = 1 << 1,
+    PointerAbsolute = 1 << 2,
     /// Capability for keyboard input events.
-    Keyboard = 1 << 2,
+    Keyboard = 1 << 3,
     /// Capability for touch input events.
-    Touch = 1 << 3,
+    Touch = 1 << 4,
     /// Capability for scroll input events.
-    Scroll = 1 << 4,
+    Scroll = 1 << 5,
     /// Capability for mouse button input events.
-    Button = 1 << 5,
+    Button = 1 << 6,
 }

 impl DeviceCapability {
diff --git a/src/request.rs b/src/request.rs
index 4b08a3f..fb0e90c 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -27,7 +27,7 @@ pub enum RequestError {
 impl fmt::Display for RequestError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            Self::InvalidCapabilities => write!(f, "invalid capabilities"),
+            Self::InvalidCapabilities => write!(f, "Invalid capabilities"),
         }
     }
 }

The use of ei.send(seat.Bind(0x1)) in the test should probably be change to find the first bit that is not used the compositor for any capability, instead of assuming the first bit is unused by capabilities. But I don't think the specific bitflag values would be considered a stable API of reis here, so we can just skip using that bit for now.

[axelkar]

@ids1024 What do you think of this? Is this still required with the changes you mention?

[axelkar]

The test passes for me with these two changes:

Modifying DeviceCapability shouldn't do anything because we have the capability map. That's really weird.

[axelkar]

What do you think of RUSTDOCFLAGS="--deny warnings" cargo doc --no-deps --all-features and --all-targets for Clippy so it lints the examples in CI? CI had missed the broken intra-doc link.

[ids1024]

Modifying DeviceCapability shouldn't do anything because we have the capability map. That's really weird.

The libei tests are just testing the server-side implementation (reis-demo-server; which is designed to match libei's eis-demo-server and pass the test suite). While CapabilityMap is client-side.

What do you think of RUSTDOCFLAGS="--deny warnings" cargo doc --no-deps --all-features and --all-targets for Clippy so it lints the examples in CI? CI had missed the broken intra-doc link.

Yeah, sounds good to have CI test that.

[axelkar]

Modifying DeviceCapability shouldn't do anything because we have the capability map. That's really weird.

The libei tests are just testing the server-side implementation (reis-demo-server; which is designed to match libei's eis-demo-server and pass the test suite). While CapabilityMap is client-side.

I forgot I used event::DeviceCapability in the request module as well. Oops.

The use of ei.send(seat.Bind(0x1)) in the test should probably be change to find the first bit that is not used the compositor for any capability, instead of assuming the first bit is unused by capabilities. But I don't think the specific bitflag values would be considered a stable API of reis here, so we can just skip using that bit for now.

Ohhh now I get it... libei/test/test_protocol.py hardcodes that the bit value 0x1 must be invalid, instead of listening to ei_seat.capability events. I don't see it as an error on reis's side as the protocol states that the server controls the meaning of every bit.

[axelkar]

https://gitlab.freedesktop.org/libinput/libei/-/merge_requests/354

[ids1024]

Re-running the CI job now that libei PR is merged, it passes.

[axelkar]

Great! Is this ready for merge?

[ids1024]

Looking over everything here again, it seems good.