odilia-app / atspi

A fast, zbus-based, permissively licensed AT-SPI library written in pure Rust!
Apache License 2.0
30 stars 6 forks source link

Handle New Event Types #25

Closed TTWNO closed 1 year ago

TTWNO commented 1 year ago

As of right now, the Connection::event_stream() function will provide a stream of events for use by client programs (or other libraries). Right now, it can only serialize two types of events: EventBody (sent by all applications except QT), and EventBodyQT (the type sent by QT applications). QT should be updating their implementation to be compliant with all the others if what I read in the at-spi2-core issues is correct.

Now, there are other kinds of events that may come through that same connection, for example Cache::AddAccessible and Cache::RemoveAccessible return data with types ((so)(so)(so)iiassusau) and (so) respectively.

Other signals on interfaces whoes signature is currently not supported by atspi: DeviceEventListener::KeystrokeListener(De)registered, Socket::Available, Registry:EventListener(De)registered.

Ok, so here's the issue: do we use an enum to transport all of these events. Something like this:

enum A11yEvent {
    RegistryEvent(RegEventBody),
    UserEvent(EventBody),
    DeviceListenerEvent(DeviceListenerBody),
    CacheAddAccessible(AddAccessibleBody),
    CacheRemoveAccessible(RemoveAccessibleBody),
}

Or is there some other way to handle this that makes more sense?

Paging @albertotirla , @mcb2003 , @DataTriny for options on this since it will directly effect projects they work on. (Thanks again for all y'alls effort and contributions you've brought so far!)

@luukvanderduim has been able to produce a working example by slimming down our problem into a smaller binary instead of a huge code-base like a screenreader. With this, we've found that lack of more diverse serialization types has caused these other types of events to be unable to be processed by assistive technologies at the higher level.

Returning an enum like this would be a breaking change since the event type returned by event_stream() will be different. Any other suggestions or is everyone ok with the breakage?

I'm personally fine with it, since that part of Odilia's code can be easily modified, but I just want to be sure that others haven't done too much with it, especially DataTriny.

TTWNO commented 1 year ago

See example with some small modifications here: https://github.com/TTWNO/a11y-bus-cache-events

luukvanderduim commented 1 year ago

Offering one stream that serves up contextually different types means burdening our end user with sorting what they really need or want. Maybe having functionally grouped streams that describe their origins is the clearest or 'least surprising' way to offer the variety. It also maps to what we get from the bus.

How about something like this?

Most events (57) are user events, which are all the same Bodies in stream. I think that is very convenient, because this would likely be also the most used stream.

Cache events would come in two flavors but that is doable for whoever wants these I think.

TTWNO commented 1 year ago

I'm starting to wonder why we don't have struct types for this? I understand it's raw data, but I think we could strongly type it al, which would have its advantages. That'd introduce a big enum anyway, and that wohld be the "Rust way". Anyone opposed to strongly typing incoming data? So instead of event.detail1(), it'd be event.start_index() (only for the TextChanged event), or event.cursor_pos() (for TextMoved event).

Personally, I like this idea better.

DataTriny commented 1 year ago

I'd be more in favor of multiple streams as described by @luukvanderduim . It is hard to say whether this approach would be practical inside Odilia at this time though. Which types or code could be shared between client and provider side? In general I am for more serializable types as it makes the API safer to use, and put name on otherwise mysterious values.

TTWNO commented 1 year ago

If you want multiple streams, create multiple connections and have those streams handle specific types in your code. I don't see a reason that separate streams should be enforced at the library level.

Looks like we're all for deserializing the types, which is not surprising, but I can't quite figure out what the reasoning for a separate stream would be.

If we type the data properly, EventBody will no longer by the type being returned to us anyway. We'll need to split it into an enum anyway. The only difference is I'm suggesting we put all events in it like so:

enum ObjectEvent {
    TextCaretMoved(TextCaetMovedEvent),
    FocusChanged(FocusChangedEvent),
    ...
}
enum CacheEvent { ...}
enum AtspiEvent {
    Object(ObjectEvent),
    Cache(CacheEvnet),
    ...
}

Again, if you'd like to separate the streams, it makes sense to do that with separate connections to dbus. I can absolutely see the utility in having multiple streams for different types, I just think it would make more sense if it was not enforced by atspi itself.

Is there something I'm missing as to why multiple streams would be better? Easier to use? More intuitive? Faster?

luukvanderduim commented 1 year ago

What I like about the approach you suggest here is that the semantics, what do the event details signify in the context of this event, are encoded in the types and our users no longer have to look it up. Your approach reduces chances for mistakes and misunderstandings.

I think I liked the aesthetics of separating by context and letting the crate do the separation seemed ergonomic as compared to having a handful odd events (eg. Cache, EventListener, ... ) and the generic event. But.. I can absolutely see how users end up just select!-ing on those streams in one place anyway so I am pretty much convinced your idea is better for the reasons outlined above.

luukvanderduim commented 1 year ago

Been sleeping this over.

Both ideas, distinct streams with unambiguous 'Item' types and having fully typed events which encode Atspi UI semantics are not mutually exclusive. Maybe we should not strive for either, but look for a construction elegant enough to serve both goals equally and maximize both clarity and convenience.

Joining both ideas

We could leave it up to the end user to try_from()? on stream items instead of doing this beforehand. We could have distinct streams for each signature and one Item type has a TryFrom<Event> for Enum(PreciseType)) on it and the others do not because their Item is the struct we need at the end.

It could look something like this:

#[non_exhaustive]
pub enum TypedAtspiEvent {
        TextCaretMove(CaretEvent),
        Focus(FocusEvent),
       WindowRaise(RaiseEvent),
       ...
};

impl TryFrom<Event> for TypedAtspiEvent {
    type Error = &'static str;

    fn try_from(value: Event) -> Result<Self, Self::Error> {
        let body = ev.body().detai
        match ev.type() {
            "TextCaretMoved" => { TypedAtspiEvent::CaretMove(CaretEvent { pos: ev.body().detail1() } ) }
            "Focus" => { TypedAtspiEvent::Focus( FocusEvent { accessible: (UniqueName, ObjectPath ) } )},
            _ => Err("Neatly typed error error!")
        }
    }
}
} 

Such that you could do on a the generic event:

    tokio::pin!(event_stream);
    while let Some(ev) = event_stream.next().await? {
         lf let TypedAtspiEvent::Focus( fe ) = ev.try_into()? {
             let (name, path) = fe.accessible();
        }
    }

and on one of the other streams:

    tokio::pin!(cache_add_stream);
    while let Some(cae) = event_stream.next().await? {
         let added_object = cae.object();

         // insert cache object in store
    }

I think this is flexible, not particularly ugly and it keeps the UI semantics in types as well.

Not everyone likes the philosophy of common grounds, marriage of ideas but I had to try because this kept me awake.

There I said it. ;)

TTWNO commented 1 year ago

After seeing your implementation, this looks cool! The try from makes sense since I think we're already doing thst anyway.

luukvanderduim commented 1 year ago

Cool! Ill make it happen. The difference between either over the other is not that big indeed. If it proves to be impractical, converting would be easy, like pulling a sock inside out.

luukvanderduim commented 1 year ago

As far a I know now, there are six type signatures on the bus. Signature "(so)" is shared by two events of totally different character. One notifies us the registry daemon just started and is ready for service and the other use is for Cache removal notifications. Not something we'd want in one stream

I split those up to make the resulting signatures:

const ATSPI_EVENT: Signature = Signature::from_static_str("siiva{sv}").unwrap();
const QSPI_EVENT: Signature = Signature::from_static_str("siiv(so)").unwrap();
const AVAILABLE: Signature = Signature::from_static_str("(so)").unwrap();
const EVENT_LISTENER: Signature = Signature::from_static_str("(ss)").unwrap();
const CACHE_ADD: Signature = Signature::from_static_str("((so)(so)(so)iiassusau)").unwrap();
const CACHE_REM: Signature = Signature::from_static_str("(so)").unwrap();
const DEVICE_EVENT: Signature = Signature::from_static_str("(souua(iisi)u(bbb))").unwrap();

The corresponding streams are 'done'. Currently working on plumbing and types and making sure the event types deserialize.