Remove manual implementation of Serialize, Deserialize, and From<u32> for -common types

[TTWNO]

Role has a manual implementation of From<u32> and Into<u32> so that it can be converted to numbers easily. This should be some kind of derive macro. I noticed that we even test our implementation (because I messed it up at one point). The argument for this is that adding new ones will be easier, since the implementation's match block will require us to cover all cases.

Similar deal with State, Interface, they have manual Serialide and Deserialize implementations for the same reasons. Ideally, this could just be handled. IIRC, the reason for this was that I needed constant static strings of the interfaces and states, and I couldn't use a constant static string as a value for serde's de/serialize customizations.

I'll gladly close if that's still the case.

[luukvanderduim]

I am not sure I fully follow your train of thought, but I'll try. Please correct me where I go wrong.

Role has a manual implementation of From<u32> and Into<u32>
Implement From, get Into for free.

so that it can be converted to numbers easily. This should be some kind of derive macro.
I think we can safely cast.

Role is a simple type:

    <method name="GetRole">
      <arg direction="out" type="u"/>
    </method>

It is unambiguously defined as a u32. Should you want the u32 of a Role, casting Role is fine provided Role has #[repr(u32], which it has:

#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Type, Hash)]
/// Role docs
#[repr(u32)]
pub enum Role {

I may have a problem with this one:

TryFrom<u32> for Role

which I think is unneeded. If you want a Role in Rust, you will just create a Role::*.

I also think both attribute macros should not be separated by item docs.

Similar deal with State, Interface, they have manual Serialide and Deserialize implementations for the same reasons. Ideally, this could just be handled. IIRC, the reason for this was that I needed constant static strings of the interfaces and states, and I couldn't use a constant static string as a value for serde's de/serialize customizations.

I'll gladly close if that's still the case.

State has derived Serialize and Deserialize. Let's review:

State

State is a bit different, because we decided to keep it as BitFlags. A single state is a u32.

GetState returns an array of States which we keep as StateSet(Bitflags<State>)

    <method name="GetState">
      <arg direction="out" type="au"/>
      <annotation name="org.qtproject.QtDBus.QtTypeName.Out0" value="QSpiIntList"/>
    </method>

#[bitflags]
#[non_exhaustive]
#[repr(u64)]
#[derive(Clone, Copy, Debug, Serialize, Deserialize, PartialEq, Eq, Hash, Default)]
#[serde(rename_all = "kebab-case")]
pub enum State {

Note that State has Deserialize derived, which might work, if feature 'serde' is enabled on 'bitflags' - but I am less convinced of the derived Serialize because how does the 'bitflags' deserializer know we need the serialized type to be of size u32 if the State's representation is u64?
though? (what am I missing?)

StateSet

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Default)]
/// The bitflag representation of all states an object may have.
pub struct StateSet(BitFlags<State>);

The rust StateSet is a single u64. Its corresponding DBus representation is 'au', an [u32], so I think it is reasonable we have a custom Serializer and Deserializer in place. because each BitFlag<u64> needs to be translated into a u32 State value and placed in a Vec.

It seems warranted to have tests for this.

I am not sure why we'd need these though?:

impl From<State> for String 

impl From<String> for State 

DBus hands us u32's and if we want to create a State we can just instantiate a State::*, no?

Interface and InterfaceSet

In DBus, interfaces are stringly typed.

    <method name="GetInterfaces">
      <arg direction="out" type="as"/>
    </method>

Whereas rusty atspi wants to save space:

/// AT-SPI interfaces an accessible object can implement.
#[bitflags]
#[repr(u32)]
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum Interface 

This means we need custom Serialize / Deserialize for Interface and InterfaceSet.

Maybe TryFrom<&str> for Interface is not strictly needed?

[TTWNO]

Maybe TryFrom<&str> for Interface is not strictly needed?

Yes. This is sort of the idea I'm getting at:

pub enum Interface {
    #[serde(rename = ACCESSIBLE_INTERFACE_NAME)]
    Accessible,
    ...
}

Seems so much cleaner. Although, I'm not sure we can actually do this, since I think you need to type out a string literal 🤷 would be much better in either case because it removes our messing up the conversion in a match for us.

I am not sure why we'd need these though?:
impl From<State> for String
impl From<String> for State

That was my original thought as well. But I see your point: if you want a string representation (with Odilia does), then you'd have to write it yourself, which seems so silly! But do we want to provide "official" translations when somebody wants a French String? (Spoiler: No!) Why not send that responsibility down to the users? I'm not sure what the right option is here. Your advice would be appreciated.

I may have a problem with this one:
TryFrom<u32> for Role
which I think is unneeded. If you want a Role in Rust, you will just create a Role::*.

Right... That code was actually added by @DataTriny . They needed a way to convert from the number u32 to Role for accesskit. In addition, they needed to implement Role.name() which returns a reference to a static string slice. None of these are exactly desirable, but I do understand why they're needed upstream. This ties into earlier: "Should we provide the 'official' names of things which don't have 'official' string representations?" It seems to me that the answer is no, but I want input from you both on this.

P.S. Thanks Luuk, you took my thoughts (which were not even correct!) and converted them into useful, actionable items!

[DataTriny]

FYI in AccessKit we need textual representation of states to emit signals such as object:state-changed. The code could probably be refactored to take advantage of serde. However, we now have a new codepath for the upcoming Newton protocol in which we create an event struct meant to be consumed by ATs directly. We need these textual representations here as well, but D-Bus is not involved here so we don't serialize anything.

You could argue that Newton is a new thing so it should provide its own representation of such values and I would agree.

[luukvanderduim]

There are a number of subjects intersecting in this thread.

• From<Repr> for Enum
  The crate strum_macros provides derive macro FromRepr to replace tedious From<u32> for Role.
  We can safely cast to get the repr for the variant.

• 'static &str conversions
  AccessKit needs &'static strs for each variant in State which seems reasonable to offer.

The way we do it now, atleast for Role, implicitly relies on the ordering of const ROLE_NAMES: &[&str] and Role variants to coincide. I'd prefer a more direct relationship,
The strum_macros crate provides derive macros IntoStaticStr and AsRefStr which will do that for us.

• I propose to drop the conversions to owned strings.
  If we already offer From<Enum> for &'static str, users can always own it if they need it.

• Localization
  I agree to push that responsibility to application developers. I am currently not familiar with how that is done.
  Should users want us to adhere to some standard, I am sure we'll hear about it.

I would like to add these features to all or most public facing enums so that there is consistency across the board.

Also should I be missing the point(s), please help me see what is missing from above.

[DataTriny]

I will try inside accesskit_unix to make use of more types from atspi-common so that serde can take care of all these string representations.

Like I said in the past, I'd prefer not to add strum as yet another dependency, the tree is already big enough.

[DataTriny]

Since our remaining usecase cannot be solved by serde, I'd be OK having strum as an optional dependency.

[luukvanderduim]

Good, I'll do that.

Side note on the dependency count, due to zbus 4.x we lost a bit of weight.
atspi v0.19.0 has 241 unique packages found in Cargo.lock
atspi HEAD has 218 unique packages found in Cargo.lock

For reference accesskit v0_12_3 has a count of 296.

When in future time accesskit can justify to migrate to latest atspi, it will enjoy a lighter atspi.

[TTWNO]

Thank you Luuk, for clarifying the issue and adding important information!