Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: Lazily activate Unix adapters #324

Merged
merged 9 commits into from
Jan 3, 2024
Merged

fix!: Lazily activate Unix adapters #324

merged 9 commits into from
Jan 3, 2024

Conversation

DataTriny
Copy link
Member

@DataTriny DataTriny commented Dec 26, 2023

Fixes #314

When a Unix adapter is created, we now start monitoring the D-Bus session bus for property change signals, which indicate if assistive technologies are currently running. If this becomes true, all currently registered adapters will be initialized. If for some reason the AT-SPI bus disappear, tree updates will still happen but nothing will be sent through D-Bus. If the AT-SPI bus is launched again, the connection will be re-established, the D-Bus object server will be populated and signals will be emitted like before.

@DataTriny DataTriny changed the title fix: Lazily activate Unix adapters fix!: Lazily activate Unix adapters Dec 26, 2023
@mwcampbell
Copy link
Contributor

I just did a first pass through the changes, and the biggest problem that I see is that the implementation of update in the winit adapter is incorrect. The update method (as opposed to update_if_active) is supposed to force the adapter to fully initialize, calling the original tree source function to return the initial tree state, then update the newly created tree with the tree update passed to the update method. But the new Unix backend in the winit adapter does nothing. This is incorrect, because when the adapter is eventually lazily initialized, it'll be initialized with the original tree source function; the new update will be lost.

I know that doing a correct implementation of update would be complicated in the new implementation of lazy initialization. So I propose that, since we're making a semver-breaking change anyway, we drop the update method from the winit adapter entirely. I used to use that method as a shortcut in our sample code, but I don't anymore, and no production-quality GUI toolkit should use it; they should always use update_if_active. To make sure that API-breaking change is prominent in the changelog, I suggest doing that change in its own PR, then rebasing this one.

@DataTriny
Copy link
Member Author

I wanted to get rid of it as well. I'll make the changes.

@mwcampbell
Copy link
Contributor

Likewise in the C SDL example, we should drop accesskit_sdl_adapter_update, which the example doesn't actually use.

@mwcampbell
Copy link
Contributor

What happens if there's no D-Bus session bus running? It looks to me like the program would crash, though I haven't yet verified this. While a full-featured desktop environment will always have a D-Bus session bus, some developers will want their GUI applications to run in a stripped-down environment without D-Bus, while also supporting accessibility for those who need it. So can we handle the lack of a session bus more gracefully?

@DataTriny
Copy link
Member Author

My assumption was that the session bus would indeed always be there. I'm not even sure if it exists a desktop environment that doesn't require D-Bus. I'll see what I can do though.

@mwcampbell
Copy link
Contributor

I wonder if @federicomenaquintero and @zeenix would have time to review this PR, to ensure it's implementing AT-SPI correctly and making the best possible use of zbus and the async Rust ecosystem. They might notice things that aren't obvious to me.

@mwcampbell
Copy link
Contributor

@DataTriny Based on the results of your testing, I'm willing to go ahead and merge this without further review from Federico or Zeeshan. If any bugs or suboptimal implementations are found, those can be fixed later. I'm satisfied that this PR doesn't introduce any regression from what we already have.

@mwcampbell mwcampbell merged commit 54ed036 into main Jan 3, 2024
5 checks passed
@mwcampbell mwcampbell deleted the unix-lazy-init branch January 3, 2024 18:50
@mwcampbell mwcampbell mentioned this pull request Jan 1, 2024
@zeenix
Copy link

zeenix commented Jan 3, 2024

I'm willing to go ahead and merge this without further review from Federico or Zeeshan.

I'm terribly sorry but I just didn't have time. :( I did have a very quick look and nothing looked bad.

@mwcampbell
Copy link
Contributor

@zeenix Thanks for giving it a quick look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AT-SPI2: Activate tree updates only if org.a11y.Status's IsEnabled is true
3 participants