Enum enrichment #172

luukvanderduim · 2024-04-07T15:10:15Z

This commit hopes to address (in part) #157

Adds strum and derives Display, FromRepr and IntoStaticStr on public enums (where this makes sense).

FromRepr adds T::from_repr(U) where U is the underlying type of the enum.
FromStaticStr implements From<Enum> for &static str (implicitly Into too).
Display implements Display (implicitly does ToString)

nit: I do not like how adding the derives causes the #[derive( .. )] span several lines.
With this PR, the Interface's Deserialize implementation avoids copying the string, thus may have become a little bit faster for what that is worth.

Various other changes and added tests. Please review and scrutinize before merging.

DataTriny · 2024-04-07T15:40:49Z

@luukvanderduim I think we should first use serde in places where it is currently missing, and then use strum for what is left. Right now you have added strum as a required dependency, which is not good.

TTWNO · 2024-04-12T18:09:14Z

Right now you have added strum as a required dependency, which is not good.

Although I do sympathize with you wanting to keep accesskit's dependency tree small, I think this is a very small ask here. strum is not a particularly heavy crate, and it only depends on 3 sub crates which are not already transitive dependencies of atspi (one of which is simply the strum_macros crate itself).

Again, I'm fine restricting the use of large libraries going forward, since Odilia has a similar problem with a massive number of dependencies. But I think this is an incredibly small addition which decreases manual maintenance by a large amount: serialization is the vast majority of what atspi-common does.

Additionally, the new zbus 4.0 release has dropped a few dependencies, moving us down from 225 to 218, so we will still create a new loss across the zbus 4.0 transition.

luukvanderduim · 2024-04-14T15:10:34Z

@luukvanderduim I think we should first use serde in places where it is currently missing, and then use strum for what is left. Right now you have added strum as a required dependency, which is not good.

@DataTriny

In the issue discussion we had earlier:

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

So I am a bit confused to see that you do in the end wish to use serde.

DataTriny · 2024-04-14T16:32:26Z

@luukvanderduim sorry if I wasn't clear: I was refering to an upcoming use case for which the needs are not yet fully defined, but could require the use of strum. However I don't think it is a good idea to address this now.

With #174 I started removing some manual implementations of the serde traits, and I think we should try to do it wherever we can.
The problematic instances are the event body structs. But these structs have another issue: you must know the AT-SPI internals to use them properly. We should find a way to better use the type system to represent them, which will certainly allow us to directly serialize/deserialize the types that they carry such as Role and State.

luukvanderduim · 2024-04-14T18:30:16Z

@luukvanderduim sorry if I wasn't clear: I was refering to an upcoming use case for which the needs are not yet fully defined, but could require the use of strum. However I don't think it is a good idea to address this now.

With #174 I started removing some manual implementations of the serde traits, and I think we should try to do it wherever we can. The problematic instances are the event body structs. But these structs have another issue: you must know the AT-SPI internals to use them properly. We should find a way to better use the type system to represent them, which will certainly allow us to directly serialize/deserialize the types that they carry such as Role and State.

But these structs have another issue: you must know the AT-SPI internals to use them properly.

Users do not need to know AT-SPI2 to use our event stream. They receive converted user facing types.
That being said.. You are right in that, for the event types, we now always have the intermediary EventBody which may not be necessary.

I have dabbled with the idea to:

Keeping the EventBody<'_> type
Implement Deserialize for all user facing event types in zero-copy fashion.

This way we could elide copying body data, creating, possibly allocating on each event. (but it may be the compiler is smart enough to see we aren't using the map or the string..)

We should find a way to better use the type system to represent them, which will certainly allow us to directly serialize/deserialize the types that they carry such as Role and State.

We find Role and State most often if not always in Method return, Method argument or as part of a Property, as far as I remember these never part of EventBody which is used to encode events / signals.

`State`

The StateChanged signal (see: Event.xml) uses 'string' to encode the state,
(NOT by a u32 representing the enum variant! as one might expect)
In atspi State's Rust representation is a u64 bitfield, which is then serialized /
deserialized to / from a string.

The bitfield representation comes in handy when in use with the StateSet struct.

The GetState method (see: Accessible.xml) encodes any combination of states using a bitfield.
This is currently achieved by sending an array of two u32s, which is then interpreted as a u64.
(NOT by sending a u64 directly ! )
so we need to serialize / deserialize StateSet to and from a bitfield.
StateSet is counterpart to the AtspiStateSet struct in the C AT-SPI2 library.

*It thook me a while to get my head around why and how the puzzle was solved the way it is..)

Role

Role is simply represented with a u on the bus and represented with a u32 in atspi. So no bitmaps, no RoleSet's with different D-Bus encoding.

The only thing I found is that AccesKit has a specific display requirement that needed attention.

I would have appreciated if you had offered commits on top of this tree. That would have made your intentions immediately apparent.

DataTriny · 2024-04-14T19:25:43Z

@luukvanderduim

The StateChanged signal (see: Event.xml) uses 'string' to encode the state,
(NOT by a u32 representing the enum variant! as one might expect)
In atspi State's Rust representation is a u64 bitfield, which is then serialized /
deserialized to / from a string.

I know all of this, in fact I brought State and StateSet here from AccessKit where I had implemented them. I was precisely refering to this signal. Since StateSet manually implements the serde traits, and doesn't rely on the serialization/deserialization implementations of the State type to accomplish this, and since to my knowledge there is nowhere in AT-SPI where a single state is represented by its numerical representation, I think the way forward is to modify our event body types so that they can directly contain a State where needed. This is why I added #[serde(rename_all = "kebab-case")] to the enum in the first place.

The only thing I found is that AccesKit has a specific display requirement that needed attention.

I don't know what you are refering to. Can you please point out what you've found on AccessKit? If there is something valuable to add here, I'll gladly do it.

luukvanderduim · 2024-04-14T20:32:01Z

@luukvanderduim

The StateChanged signal (see: Event.xml) uses 'string' to encode the state,
(NOT by a u32 representing the enum variant! as one might expect)
In atspi State's Rust representation is a u64 bitfield, which is then serialized /
deserialized to / from a string.

I know all of this,

Good. I hope you found the summary palatable.

As for as StateChanged signal, which indeed is found in the EventBody, would you like a custom Deserialize as I descripbed above.

The only thing I found is that AccesKit has a specific display requirement that needed attention.

I meant the scheme with spaces.
Maybe serde rename_all has a scheme to support lowercase-with-spaces, strum, afaik does not.

        /// Object is a label indicating the keyboard accelerators for the parent.
	#[strum(serialize = "accelerator label")]
	AcceleratorLabel,

[edit: The signal is an EventBody, my bad]

DataTriny · 2024-04-14T20:55:03Z

You mentioned we get State and Role were obtained from EventBody , I presume you meant the Message's body then

For the StateChanged signal a State has to fit into the kind field, and in the case of the PropertyChange signal a Role must go into any_data. This might get abstracted away on the client side by the event stream, but on the server side I will have to construct these EventBody manually once I update AccessKit.

I meant the scheme with spaces.

So you are talking about the value we are supposed to return from the GetRoleName method of the org.a11y.atspi.Accessible interface. We don't implement it on AccessKit and I don't plan to do so as I think it is pointless. I might convince the GNOME team to deprecate it at some point.

TTWNO · 2024-04-15T02:18:07Z

GetRoleName method of the org.a11y.atspi.Accessible interface. We don't implement it on AccessKit and I don't plan to do so as I think it is pointless. I might convince the GNOME team to deprecate it at some point.

Completely agree. Localization shouldn't be done by sending messages over DBus. GetRole is sufficient.

luukvanderduim · 2024-04-15T11:45:58Z

You mentioned we get State and Role were obtained from EventBody , I presume you meant the Message's body then

For the StateChanged signal a State has to fit into the kind field, and in the case of the PropertyChange signal a Role must go into any_data.

Yes, of course.
I realized I was mistaken there some minutes after posting..

This might get abstracted away on the client side by the event stream, but on the server side I will have to construct these EventBody manually once I update AccessKit.

I think I understand.
Ideally you would want a Serialize implementation for, let's take Role, that could serialize to EventBody, EventBodyQt, Role?

We cannot have that in one Serialize impl.
We can have the type itself, and two newtypes and on the newtypes you could each implement separate Serialize impls.

I meant the scheme with spaces.

> So you are talking about the value we are supposed to return from the `GetRoleName` method of the `org.a11y.atspi.Accessible` interface. We don't implement it on AccessKit and I don't plan to do so as I think it is pointless. I might convince the GNOME team to deprecate it at some point.

I did not know you had the strings there for that purpose.

I think GNOME people need little convincing as it is currently only said to be used for roles outside of the enumeration and itś removal is eluded to in the specs:

    <!--
        GetRoleName:

        Gets a UTF-8 string corresponding to the name of the role played by an object.
        This method will return useful values for roles that fall outside the
        enumeration used in the GetRole method. Implementing this method is
        optional, and it may be removed in a future version of the API.
        Libatspi will only call id in the event of an unknown role.
    -->

TTWNO · 2024-04-17T17:41:45Z

Nice find, Luuk! Good to know.

There is no need to have the same dependency twice as `[dev-dependencies]` is the sum of `[dependencies]` and what it adds.

We are exploring deriving simple functionality on public enums using strum.

- Remove consts as we cannot use these inside the attribute macros anyway. (likely because these are expanded after proc macros get run.) - Make up for loss of consts by adding tests for each. Note that this does not include strum::FromRepr because the representation differs from what is used on the bus, this makes the internal representatation meaningless to users. Instead I implemented FromStr, which makes more sense. Because of internal representation and bus representations iffer, we need custom Serialize and Deserialize implementations. Note that the Deserializer received a small optimization. No longer is the interface string required to be copied to stack before converting it to an `Interface`, instead the `Deserialize` implementation now takes a reference to the string and converts it. This also expands tests to cover the changes.

RelationType is now also checked with zbus_lockstep. As a consequence the type validation document is updated too.

`FromRepr`, `Display`, `IntoStaticStr` and `EnumIter` I changed two variants that stood out odd: `Role::Editbar` -> `Role::EditBar` ``Role::CHART` -> `Role::Chart` In the case of `EditBar`, this means it is now displayed as "edit bar" instead of "editbar". If this raises concerns, please inform me why. the `EnumIter` may be useful in tests. That being said, this enum has a simple `u32` representation. fSo checking whether each gets serialized and deserialized feels overly cautious because `zvariant` is surely able to (de)serialize these. In other words, it is not our concern to test that. We added no logic to get this done. Same goes for `&'static str` matching with the `Display` impl. It does not hurt to test, but it is not our responsibility.

Clipyp suggests not to index in the match argument. Using `strip_prefix` exits early or hands the match string to `match`. The indexing pattern ``` &match_me[prefix.len()..] {} ``` May be more expensive becaus it requires a bounds check and requires panic code to go alongside. (TIL)

`FromRepr`, `Display`, `IntoStaticStr` now implemented for - `Live` - `ScrollType` - `Layer` - `Granularity` - `ClipType` - `CoordType` - `MatchType` - `TreeTraversalType` - `SortOrder` These _currently use 'snake_case' character translations, but we can pick some other scheme. I would really like some feedback and thoughts here. The derive macros are now found just above the type instead of above the comments. This makes it easier to see which implementations a type has derived, especially if the docs on the type are more than just a few lines.

Use `from_repr() -> Role` instead of `From<i32> for Role`. Remove import for a single use of `Hash`.

- Adds notes tp clarify peculiarities found when revisiting this module. - Adds strum derives `IntoStaticStr`, `Display` and `FromRepr`

DataTriny · 2024-04-17T18:25:06Z

Sorry @luukvanderduim I missed your last comment. I think we are both on the same track now! It's not clear what's the solution for me either, maybe we should open a new issue to explore how to improve the event types. But I really think we can go quite far by making full use of serde, without introducing strum.

TTWNO · 2024-04-17T18:37:22Z

You may be right, @DataTriny , but I think having an optional dependency makes sense here. FromRepr, and IntoStaticStr seem to be really useful abstractions. Display is whatever, since that can be done with a macro_rules!.

TTWNO · 2024-04-17T18:59:36Z

After making strum optional, and gating all uses of it behind the feature "strum", it seems that Strum has become a bit more fundamental on this branch than before. It causes a cascading set of conversion errors all the way up, and so this branch does indeed make strum required.

DataTriny · 2024-04-17T19:23:14Z

@TTWNO I'm definitely not advocating for ditching all the work already done by @luukvanderduim with this PR. Maybe strum will be necessary to fully support features such as fine-grained event filtering. But I think we need to first modify our types to make full use of serde, then we can revisit this PR and possibly add strum as an optional dependency if it is only useful to AT-SPI clients for instance. strum might be just one dependency, but it adds more procedural macros, which hurt compile times. Since we already have quite a few in the Rust accessibility stack, I think we should try to avoid it where we can.

clippy

…o-impl for BusProperties

Cargo format Clippy

1. BusProperties. This contains static information about the DBus properties of a particular event type. This is implemented for indivudal event types only and is NOT object safe; it is similar to GenericEvent, but without path() and sender() functions. - DBUS_MEMBER - DBUS_INTERFACE - MATCH_RULE_STRING - REGISTRY_EVENT_STRING - type Body - build() function 2. EventTypeProperties. This is an object-safe trait that specifies similar data as BusProperties, but as runtime functions instead of constants. This will be implemented on individual event types, and wrapper types like `Event`. - member() -> &'static str - interface() -> &'static str - match_rule() -> &'static str - registry_string() -> &'static str - EventProperties is blanket implemented for any type that implenents BusPropeties. 3. EventProperties. This is an object-safe trait specifying qualities of a particular event, not its type. - name() -> BusName<'_> - path() -> ObjectPath<'_> - object_ref() -> ObjectRef (auto-impl) This fixes #176, addresses, at least in part #148.

Cargo fmt

Whoopsie

fmt Fix tests

All objects in any server's tree implements the `Accessible` interface. Bringing `ObjectRefExt` into scope allows users to call `as_accessible_proxy` on any `ObjectRef` to obtain an `AccessibleProxy` from the `ObjectRef`.

Makes the trait public In the trait definition, adds the send bound in the impl, resuggars the impl

TTWNO · 2024-05-21T19:32:57Z

Yeah, I shouldn't merge with main but GH interface was complaining.

…ev-dep fmt

…specified d

codecov · 2024-05-21T20:08:45Z

Codecov Report

Attention: Patch coverage is 96.64083% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 87.27%. Comparing base (85d7518) to head (682afee).
Report is 13 commits behind head on main.

Files	Patch %	Lines
atspi-common/src/lib.rs	65.21%	8 Missing ⚠️
atspi-common/src/interface.rs	99.22%	2 Missing ⚠️
atspi-common/src/events/object.rs	66.66%	1 Missing ⚠️
atspi-common/src/relation_type.rs	80.00%	1 Missing ⚠️
atspi-common/src/state.rs	98.18%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   83.74%   87.27%   +3.53%     
==========================================
  Files          39       39              
  Lines        3112     3451     +339     
==========================================
+ Hits         2606     3012     +406     
+ Misses        506      439      -67

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TTWNO · 2024-05-21T20:13:58Z

@DataTriny

Curious if this would work for you? We've fully feature-gated strum, and still provided the vast majority of the implementations despite that. The only implementations that are not available are EventTypeProperties for some specific events. Assuming you don't use this (except for in tests, where I assume you are not as worried about compile times and deps) this should work out well.

Let me know.

DataTriny · 2024-05-22T16:46:43Z

@TTWNO Thanks for all the efforts you put into this! Unfortunately this PR seem to contain way too many unrelated changes for me to review right now, but I checked that indeed the common crate can compile without the strum feature. However you have tied it to the features for async runtime selection, which mean that AccessKit would still pull in strum as we use a few proxies from atspi-proxies. Is this a requirement, or could an async executor be enabled without strum?

TTWNO · 2024-05-22T17:19:32Z

@DataTriny right now, any use of zbus brings in the TryFrom/TryInto<Message> impls for each event. These require strum.

Does accesskit use these implementations? If not, we can add a separate feature flag for the message conversions; separate from zbus support in proxies.

DataTriny · 2024-05-22T17:56:32Z

No we currently don't use these, although I was eventually planning on doing so. If feature-gating these convertions is the only way forward, I'm fine with what you propose.

TTWNO · 2024-05-22T19:38:29Z

If you intend on using the TryFrom/TryInto implementations eventually, and they require strum does that mean that acceskit is ok to depend on strum?

If not, does that mean that you will write your own Message (de)serializers?

DataTriny · 2024-05-22T19:43:47Z

For now we create messages to emit signals by hand. Since the code is already there I will probably leave it as is if relying on atspi would bring strum.

luukvanderduim marked this pull request as ready for review April 14, 2024 15:37

luukvanderduim added 13 commits April 17, 2024 11:55

remove static-assertions from dev-dependencies.

2d29a88

There is no need to have the same dependency twice as `[dev-dependencies]` is the sum of `[dependencies]` and what it adds.

Add strum as dependency

701f880

We are exploring deriving simple functionality on public enums using strum.

atspi-proxies remove serde_plain as it is not in use in this crate.

3bcb82c

trailing space policing

33d5a26

Apply strum traits that make sense to RelationType

d13c4b3

RelationType is now also checked with zbus_lockstep. As a consequence the type validation document is updated too.

Adjust to Role changes

ce4b10a

Use `from_repr() -> Role` instead of `From<i32> for Role`. Remove import for a single use of `Hash`.

Adds strum derivation on State enum

68196c1

- Adds notes tp clarify peculiarities found when revisiting this module. - Adds strum derives `IntoStaticStr`, `Display` and `FromRepr`

Fixup Clippy CI errors

1a452b4

Fixup occurence of Accessible where ObjectRef is more appropriate

cbb3ca8

TTWNO force-pushed the enum-enrichment branch from 02f732c to cbb3ca8 Compare April 17, 2024 17:58

TTWNO and others added 17 commits May 21, 2024 13:21

Explicitly pass codecov token as required by v4

58eae45

Make ObjectRef more explicit in type

37e6a3d

Use from_static_str for Default implementaiton

5d96bd3

clippy

GenericEvent -> BusProperties, GenericEvent is now an object-safe aut…

6253a90

…o-impl for BusProperties

Update atspi-connection to include changes to trait names

97a9706

Update docs

d686c3a

Cargo format Clippy

Implement EventTypeProperties and EventProperties for wrapper types

54e95df

Add tests to check for inner transparency on the Event type

b2145b3

Cargo fmt

Fix, add to docs

4d5ba89

Whoopsie

Add additional required traits for each individual event type

284652d

Remove stray new_events.diff file.

a9e3007

Add object-safety comments

2c7728a

BusProperties::build -> from_message_parts

70af88a

fmt Fix tests

Obtain an AccessibleProxy from any ObjectRef with ObjectRefExt

a648d15

All objects in any server's tree implements the `Accessible` interface. Bringing `ObjectRefExt` into scope allows users to call `as_accessible_proxy` on any `ObjectRef` to obtain an `AccessibleProxy` from the `ObjectRef`.

Changes implementation to an async trait

d8a2698

Making the trait public

4b83940

Makes the trait public In the trait definition, adds the send bound in the impl, resuggars the impl

TTWNO force-pushed the enum-enrichment branch from 084d9dd to 4b83940 Compare May 21, 2024 19:24

Merge branch 'main' into enum-enrichment

9f8fa2b

TTWNO added 3 commits May 21, 2024 13:52

default_features -> default-features

ac8ece0

Feature gate additional impls behind the 'strum' flag; make strum a d…

6c5ace0

…ev-dep fmt

Remove zbus flag in favour of re-exporting zbus if either runtime is …

682afee

…specified d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum enrichment #172

Enum enrichment #172

luukvanderduim commented Apr 7, 2024

DataTriny commented Apr 7, 2024

TTWNO commented Apr 12, 2024

luukvanderduim commented Apr 14, 2024

DataTriny commented Apr 14, 2024

luukvanderduim commented Apr 14, 2024

DataTriny commented Apr 14, 2024

luukvanderduim commented Apr 14, 2024 •

edited

Loading

DataTriny commented Apr 14, 2024 •

edited

Loading

TTWNO commented Apr 15, 2024

luukvanderduim commented Apr 15, 2024

TTWNO commented Apr 17, 2024

DataTriny commented Apr 17, 2024

TTWNO commented Apr 17, 2024

TTWNO commented Apr 17, 2024

DataTriny commented Apr 17, 2024

TTWNO commented May 21, 2024

codecov bot commented May 21, 2024

TTWNO commented May 21, 2024

DataTriny commented May 22, 2024

TTWNO commented May 22, 2024 •

edited

Loading

DataTriny commented May 22, 2024

TTWNO commented May 22, 2024

DataTriny commented May 22, 2024

Enum enrichment #172

Are you sure you want to change the base?

Enum enrichment #172

Conversation

luukvanderduim commented Apr 7, 2024

DataTriny commented Apr 7, 2024

TTWNO commented Apr 12, 2024

luukvanderduim commented Apr 14, 2024

DataTriny commented Apr 14, 2024

luukvanderduim commented Apr 14, 2024

State

Role

DataTriny commented Apr 14, 2024

luukvanderduim commented Apr 14, 2024 • edited Loading

DataTriny commented Apr 14, 2024 • edited Loading

TTWNO commented Apr 15, 2024

luukvanderduim commented Apr 15, 2024

TTWNO commented Apr 17, 2024

DataTriny commented Apr 17, 2024

TTWNO commented Apr 17, 2024

TTWNO commented Apr 17, 2024

DataTriny commented Apr 17, 2024

TTWNO commented May 21, 2024

codecov bot commented May 21, 2024

Codecov Report

TTWNO commented May 21, 2024

DataTriny commented May 22, 2024

TTWNO commented May 22, 2024 • edited Loading

DataTriny commented May 22, 2024

TTWNO commented May 22, 2024

DataTriny commented May 22, 2024

`State`

luukvanderduim commented Apr 14, 2024 •

edited

Loading

DataTriny commented Apr 14, 2024 •

edited

Loading

TTWNO commented May 22, 2024 •

edited

Loading