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

Add atk-bridge patches and make dbus dependency optional #101

Merged
merged 3 commits into from
Jul 23, 2023

Conversation

stefan11111
Copy link
Contributor

I don't know if these patches are right for this project, but here are the patches for making the dependency on dbus optional by disabling atk-bridge.
I tried my luck with upstream, but they said no and stopped responding.

@lah7 lah7 added the patch new Adds a new enhancement or feature label Jul 23, 2023
Copy link
Owner

@lah7 lah7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the upstream report:
https://gitlab.gnome.org/GNOME/gtk/-/issues/5964

I appreciate the art of a lightweight, as few dependencies as possible OS, so I am happy to accept the patch. #40 confirmed that it was once possible to use --without-atk-bridge at build time, which was removed over a decade ago.

However, because GTK 3 apps these days might expect "ATK Bridge" to be present, I think our "classic" patchset should still ensure it is built by default, but allow users to explicitly opt out of this and keep all the pieces if it breaks.

We don't know what in the wild might mysteriously break/crash because it used an accessibility feature or indirectly needed D-Bus, which seems risky for this project to have disabled by default.

The changes proposed here look good, so I think we just need to change:

-option('atk_bridge', type: 'boolean', value: false,
+option('atk_bridge', type: 'boolean', value: true,

so that on the packaging side (Arch, Ubuntu, etc), this doesn't accidentally build without atk_bridge and potentially break things.

@lah7 lah7 linked an issue Jul 23, 2023 that may be closed by this pull request
@stefan11111
Copy link
Contributor Author

stefan11111 commented Jul 23, 2023

Setting it to be enabled by default, like you proposed, is the better choice.
I copied the patches from my gentoo patches directory, where I had to set it to default-disabled because there is no way to toggle it without a patched ebuild.

I suspect something similar might need to be done for the gentoo ebuild for these patches, given that it's supposed to work alongside gtk+ from ::gentoo. Maybe a use flag to switch between adding these patches or not.

Also, I am really curious how you manage to dig out there old upstream commits. Is there a tool to search through commits?

@lah7
Copy link
Owner

lah7 commented Jul 23, 2023

Not familiar with Gentoo myself, but as this default arrangement works then it's all good. I'll get this merged.

Also, I am really curious how you manage to dig out there old upstream commits. Is there a tool to search through commits?

I didn't find that one - it was from issue #40.

Though, Git's "blame" feature is useful for tracing back how a line of code had changed. There's a "blame" button when looking at a file in GitHub or GitLab. There is also a way to see the file prior to that commit:

Screenshot_20230723_153142

Otherwise, searching within GitHub/GitLab is powerful too - code, issue or commit. From there you could explore the repository to piece together how things came to be.

@lah7 lah7 merged commit d2de7bd into lah7:master Jul 23, 2023
@lah7 lah7 mentioned this pull request Jul 23, 2023
@lah7 lah7 added the build time Patch only affects build dependencies or configuration label Oct 25, 2023
stefan11111 added a commit to stefan11111/minimal-gtk3 that referenced this pull request Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build time Patch only affects build dependencies or configuration patch new Adds a new enhancement or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: No D-Bus?
2 participants