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

cmake: Add WITH_DBUS configuration option #176

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Apr 28, 2024

This PR adds a new WITH_DBUS configuration option, which is an analogue of Autotools' --with-qtdbus.

@hebasto hebasto added the enhancement New feature or request label Apr 28, 2024
@hebasto hebasto added this to the UI, 23th May milestone Apr 28, 2024
@fanquake
Copy link

Is this actually something we need (to port) an option for.

@hebasto
Copy link
Owner Author

hebasto commented Apr 28, 2024

Is this actually something we need (to port) an option for.

cc @laanwj

@laanwj
Copy link

laanwj commented Apr 28, 2024

Is this actually something we need (to port) an option for.

i think so. Not all UNIX platforms use DBUS. On Linux it's safe to assume it's there. On MacOS and Windows it's not a thing.

FWIW i tried to get rid of the custom notification stuff a while ago, but Qt's notificator still falls back to ancient X11 notifications which are ignored (or look very shitty) on modern Linux. Maybe qt6 changes this? i dunno. But for now t's really necessary.

@theuni
Copy link

theuni commented May 3, 2024

Should probably get rid of the HAVE_CONFIG_H (in qt/notificator.cpp) ?

@hebasto
Copy link
Owner Author

hebasto commented May 3, 2024

Should probably get rid of the HAVE_CONFIG_H (in qt/notificator.cpp) ?

Sure. Removed.

@hebasto
Copy link
Owner Author

hebasto commented May 4, 2024

@theuni

Should probably get rid of the HAVE_CONFIG_H (in qt/notificator.cpp) ?

FWIW, #177 removes unsed HAVE_CONFIG_H in the entire codebase.

@hebasto
Copy link
Owner Author

hebasto commented May 4, 2024

Rebased.

@hebasto
Copy link
Owner Author

hebasto commented May 8, 2024

Rebased on top of the merged #173.

Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

lgtm

@hebasto hebasto merged commit a38691b into cmake-staging May 8, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants