feat: Basic Unix platform adapter

[DataTriny]

Closes #56

Remaining work:

• Raise more events (children changed, bounds changed...),
• Get rid of the strum crate and push more event related types to the atspi crate,
• Implement keyboard event handling.

[mwcampbell]

Please remember to add an entry for platforms/unix in release-please-config.json. I forgot to do this for the macOS adapter and ended up making a mess.

[mwcampbell]

It doesn't look like we actually need ObjectId::from_str_unchecked. By dropping that, we can eliminate the only use of unsafe in the whole crate.

[mwcampbell]

You forgot the copyright notice at the top of platforms/unix/src/atspi/interfaces/action.rs.

Also, the accesskit_unix crate metadata says that the crate is only under the Apache license, not the MIT/Apache dual license. Is this intentional?

[mwcampbell]

Why do you manually drop the window bounds in ComponentInterface::get_accessible_at_point?

[mwcampbell]

Are you sure the Sensitive state should be set on all non-read-only nodes?

Also, shouldn't the Enabled state be clear on nodes that are disabled but not read-only?

[mwcampbell]

I notice that the Active state is always set for the top-level window node, regardless of whether anything in the tree is focused. But then, it's currently not possible to determine the former focus state of the tree as a whole from a detached node, so I'm not sure what to do.

[mwcampbell]

In PlatformNode::get_accessible_at_point, you currently have to apply the inverse of the node's transform to the point before calling node_at_point. I don't know why I did it that way, but that's how it currently is, and I already decided not to make breaking changes in the consumer crate for now. Look, for example, at PlatformNode::ElementProviderFromPoint in the Windows adapter.

[mwcampbell]

The methods on PlatformNode to get the node's parent or children should use the filter, as the corresponding methods in the other adapters do. This will be particularly important when working with text, since the inline text boxes are filtered out. But it's also important for filtering out hidden nodes.

The tricky part, of course, is that when comparing the parent and children with those of the old version of a node, we can't currently use the full filter. One solution would be to look at the raw parent and children when doing comparisons to determine which events to fire, but then use the filtered parent and children for navigation. In that case, we'd also need to use the filtered parent and children when calculating the values to include in the events themselves. If we do this, once the children-changed event is implemented, we'd end up firing extraneous events when inline text boxes are added or removed, but I don't know of a better solution at this time.

Also, to implement PlatformNode::index_in_parent in a way that takes filtering into account, I think we'll have to do it somewhat inefficiently, by counting the number of results from Node::preceding_filtered_siblings.

[mwcampbell]

When you implement keyboard event handling, are you planning to block the main thread while waiting on the AT-SPI registry to return the result for a given keyboard event? The current design of on_event in the winit adapter seems to require this. This seems likely to be rejected by GUI toolkits and (especially) game engines. But I guess the only other option is a second type of winit user event.

[mwcampbell]

Speaking of blocking the main thread, this might also be a problem with the way AT-SPI events are currently emitted. I wonder if this should be done on a background thread.

[mwcampbell]

I now understand that AT-SPI's "sensitive" state means something different than I thought. Still, I think the node's disabled flag needs to be checked separately from its read-only flag.

[mwcampbell]

The recently pushed fixes all look good.

[mwcampbell]

Please either go back to a published version of the atspi crate, or ask the crate maintainers to publish a new version on crates.io before we merge this PR. I want to publish new AccessKit crates as soon as this PR is merged, and published crates can only depend on other published crates.

[mwcampbell]

The node_added, node_updated, and node_removed methods also need to take filtering into account. In particular, if a node is filtered out of the tree, then it never needs to be registered with zbus, and no events need to be fired for it. If we can avoid doing all of the comparisons necessary to determine which events to fire for a large set of nodes, such as all of the inline text boxes in an edit control, that could significantly improve performance. The slightly tricky part is that if the filter result changes between the old version of a node and the new version, that is equivalent to adding or removing the node.

[mwcampbell]

To be consistent, PlatformNode::child_count and PlatformNode::child_at_index must also use filtered children. This will be inefficient for now. If it becomes an actual problem, we can add caching later.

[mwcampbell]

I wonder if we need to call SocketProxyBlocking::unembed to remove our root object from the registry when the adapter is dropped. It seems like that might be necessary to cleanly unregister the AT-SPI tree.

[DataTriny]

Please either go back to a published version of the atspi crate, or ask the crate maintainers to publish a new version on crates.io before we merge this PR.

I have to push other changes related to device controller so I plan to ask for a new version once everything is merged. I'll do this right now.

PlatformNode::child_count and PlatformNode::child_at_index must also use filtered children.

How did I miss that? I will handle this as well as fixing the TreeChangeHandler just after, so probably today.

[DataTriny]

I wonder if we need to call SocketProxyBlocking::unembed to remove our root object from the registry when the adapter is dropped.

From at-spi2-core's xml/Socket.xml:

Unregisters an application from the accesibility registry.  It is not necessary to
call this method; the accessibility registry detects when an application
disconnects from the bus.

[mwcampbell]

When looking at filter results for the TreeChangeHandler methods, I don't think it's a good idea to compare the result directly to ExcludeNode, because this overlooks the fact that one of the possible filter results is ExcludeSubtree.

The node_added method already does this the right way. I would change the conditional in node_removed to:

if filter_detached(node) === FilterResult::Include

And finally, in node_updated, I'd use the following series of conditionals:

if filter_new != filter_old {
    if filter_new == FilterResult::Include {
        self.add_node(new_node);
    } else if filter_old == FilterResult::Include {
        self.remove_node(old_node);
    }
} else if filter_new == FilterResult::Include {
    // actual update handler
}

This eliminates the initial empty if branch currently in the method, it treats ExcludeNode and ExcludeSubtree as equivalent, and it correctly does nothing in the hypothetical case where the filter result changed from ExcludeNode to ExcludeSubtree or vice versa.

[DataTriny]

For some reason I thought that FilterResult::ExcludeSubtree meant to only exclude children when I wrote this...

We're back on a published atspi crate!

[DataTriny]

@mwcampbell I just pushed a commit containing everything on the accesskit_unix crate to send the keyboard events. The accesskit_winit crate API have been modified but the event conversion is not done yet.

I am obviously not a big fan of placing a type parameter on all platform adapters, feel free to suggest a better option. I also looked at the egui integration and I think the current API should work.

[mwcampbell]

Just one small nit with the Unix adapter: I don't think we need to re-export EventType, since we ended up defining our own KeyState enum. I also reviewed the new asynchronous event notification; sorry I didn't get to that yesterday. That all looks good.

[DataTriny]

The exported types for keyboard events might change, I'm still figuring out the cleanest way for users to not deal with AT-SPI internal stuff. I might export a whole Key event as well, we'll see.

[mwcampbell]

In the winit adapter, I have an idea to avoid the type parameter. Give me some time to implement this on my own branch so I can show it to you.

[mwcampbell]

Check out the winit-keyboard-handler branch in the main AccessKit repo to see what I came up with. Note that I haven't done anything on the actual keyboard event conversion, and the winit example still needs to be updated.

[DataTriny]

Yes, dynamic dispatch can help here.

I've merged your branch and started translating the keyboard events. We'll use x11-dl (re-exported by winit) to get the keysym.

Running the example I can already see a problem: we'll want to give back the events to the app if the registry takes too long to reply. We'd need a way to sleep for 100ms or so and cancel the sending future if it isn't already done. But it's the job of an executor to sleep, and zbus Executor abstraction seems to not offer that. I found in one of their tests that they use async_io::Timer if the tokio feature is disabled, tokio::sleep otherwise.

Should we try to do the same and introduce a tokio feature? Or should I try bringing our own executor again?

Maybe a tokio feature is not so bad afterall, I assume that people already running an executor would like zbus to play along nicely.

[mwcampbell]

I don't think accesskit_unix needs to offer the choice between async-io and tokio. As far as any users of that crate are concerned, accesskit_unix does most of its work on one or more other threads, and the user doesn't need to know how. So just go ahead and use async-io directly to implement the timeout.

[mwcampbell]

I'd like to see both accesskit_unix and the atspi crate use the futures-lite crate instead of the futures crate. accesskit_unix would also need to use the async-channel crate instead of the channel implementation in the futures crate. async-io and related crates use futures-lite rather than futures, and zbus already depends on async-channel. So eliminating a dependency would be good. Also, the example of how to implement a timeout in the async-io documentation uses futures-lite.

[DataTriny]

We absolutely need to reduce the number of dependencies, so I'll take care of that before we merge.

There is no way we can have this land today anyway. We can't just send back KeyboardInput events or otherwise the app will receive events like ReceivedChar without prior key down event for instance.

Therefore I am trying to collect every input related event and process them all once we receive a MainEventsCleared event.

[mwcampbell]

This is now blocked on odilia-app/atspi#40. The impact of that dependency on our dependency graph is unacceptable in my opinion.

[mwcampbell]

To be safe, KeyboardEvent::character should be Option<String>, not Option<char>. A Rust char is a single Unicode scalar value (code point), but sometimes characters are made of multiple code points. This is the case for some emoji, but IIRC it also sometimes happens with accented letters.

[mwcampbell]

I don't think it's accurate to refer to what we're doing with the winit window events as "reentrant". I suggest calling them deferred events instead.

I understand why you're doing what you did with the static lifetime and the way that Adapter::on_event consumes the event and then optionally returns it.

[mwcampbell]

The current way of converting keycodes to keysyms using Xlib doesn't work if the app is natively using Wayland, does it?

[DataTriny]

The current way of converting keycodes to keysyms using Xlib doesn't work if the app is natively using Wayland, does it?

No it doesn't since it requires a Display, events will immediately be sent back to the app. Doing this mapping seems quite involving, so it would be out of scope for us in my opinion. I don't really know how Wayland deals with this, but it doesn't matter since AT-SPI expects X11 values.

[ids1024]

I don't really know how Wayland deals with this, but it doesn't matter since AT-SPI expects X11 values.

Wayland clients use libxkbcommon to map keycodes to keysyms based on the current keymap, provided with wl_keyboard::keymap, and the modifier state, which the compositor generates from key events with libxkbcommon, and passes to the client using wl_keyboard::modifiers.

This is mostly the same as how it works on X, though on X the modifier state is passed as part of the KeyPress/KeyRelease event instead of being a separate one. And for some arcane reason the X keycodes are offset +8 relative to the values used by libinput/Wayland.

[DataTriny]

Thanks very much @ids1024 for this summary. We'll revert the work on keyboard handling for now though, and try to get it fixed upstream thanks to @mwcampbell 's generosity.

[ids1024]

Keycode/keysym handling seems to have inadvertently ended up my area of expertise based on a couple things I've worked on, so if you have any questions about that, or generally about how Wayland protocols work, I may be able to help.

[DataTriny]

The time has come. Waiting for your final review @mwcampbell !

[Be-ing]

Thank you for your work on this! And I'm glad you did it without bringing in C dependencies beyond the required X & Wayland libraries that GUI applications are linking already anyway.