Use libdbus directly instead of using QtDBus

[mddanish00]

Instead of unnecessarily depending on the Qt library, it is better to use it directly.

[gemini-code-assist]

Hello @mddanish00, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! gemini-code-assist here, providing a summary of this pull request to help everyone get up to speed quickly.

The primary goal of this PR, as indicated by the title and description, is to remove the dependency on the Qt library, specifically QtDBus, and instead use the native libdbus library directly for D-Bus communication. This change aims to reduce unnecessary dependencies.

Highlights

• Dependency Removal: Removes the dependency on QtDBus by switching to the native libdbus library.
• Build System Update: Updates the Makefile to link against dbus-1 instead of Qt5DBus.
• D-Bus API Migration: Replaces QtDBus API calls in kde-night-color.cpp with equivalent libdbus functions for connecting to the session bus, making method calls (inhibit, uninhibit), handling arguments, and managing messages.
• Error Handling: Adds explicit error handling for D-Bus connection and method calls using libdbus error mechanisms.

Changelog

• Makefile
  • Changed PKG_CONFIG flags from Qt5DBus to dbus-1 for both CFLAGS and LDFLAGS, switching the build dependency from QtDBus to libdbus.
• kde-night-color.cpp
  • Replaced #include <QtDBus> with #include <dbus/dbus.h>, #include <stdio.h>, and #include <string.h>.
  • Rewrote the inhibit_nc function to use libdbus API calls instead of QDBusInterface and QDBusConnection.
  • Implemented logic to connect to the D-Bus session bus once using dbus_bus_get.
  • Replaced QDBusReply logic for the inhibit call with libdbus message creation (dbus_message_new_method_call), sending (dbus_connection_send_with_reply_and_block), and argument parsing (dbus_message_get_args).
  • Replaced iface.call("uninhibit", ...) with libdbus message creation, argument appending (dbus_message_append_args), and sending (dbus_connection_send).
  • Added error checking and reporting for D-Bus connection and method call failures using DBusError.
  • Included a minor formatting adjustment in a printf statement (Diff 2).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

Qt dependency,
Removed with native D-Bus,
Code is now lighter.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request successfully replaces the QtDBus dependency with direct libdbus calls, which is a good move to reduce dependencies. The implementation for interacting with D-Bus is largely correct. I've identified a couple of areas for improvement, primarily concerning resource management and type consistency, which would enhance the robustness and correctness of the plugin. The Makefile changes are appropriate for the new dependency.

Summary of Findings

• Type consistency for D-Bus cookie: night_cookie should be uint32_t to match D-Bus return type. (Commented)
• D-Bus connection resource management: The static DBusConnection is not explicitly closed. It should be unreferenced on plugin shutdown. (Commented)
• Hardcoded D-Bus strings: D-Bus service name, path, interface, and method names are hardcoded strings in inhibit_nc. Using const char* constants could improve maintainability. (Not commented due to review settings: low severity)
• Error checking for dbus_connection_send: The return value of dbus_connection_send in the uninhibit branch (line 83) is not checked. While uninhibit might be fire-and-forget, checking if the message was successfully queued could aid debugging. (Not commented due to review settings: low severity)
• Redundant fflush(stderr) calls: Calls to fflush(stderr) (e.g., lines 25, 52, 65) are often unnecessary as stderr is typically unbuffered or line-buffered. (Not commented due to review settings: low severity)
• Thread safety of D-Bus connection initialization: The initialization of the static DBusConnection* conn (line 17-29) is not inherently thread-safe if inhibit_nc could be called concurrently from multiple threads. This is likely not an issue in the current mpv plugin architecture where events are handled serially, but it's a general consideration for this pattern. (Not commented due to review settings: low severity)

Merge Readiness

The pull request makes a valuable change by removing the Qt dependency. The core D-Bus logic is well-implemented. However, there are a couple of medium-severity issues related to D-Bus connection cleanup and type consistency for the inhibition cookie that I recommend addressing to ensure robustness and correctness.

As a reviewer, I am not authorized to approve pull requests. Please consider these suggestions and have the changes reviewed by others before merging.

[gemini-code-assist]

Code Review

This pull request successfully transitions the plugin from using QtDBus to libdbus directly, removing the Qt dependency as intended. The Makefile changes are correct, and the C++ code implements the D-Bus communication using libdbus, including resource management and basic error handling.

The introduction of constants for D-Bus service details and a dedicated cleanup_dbus_resources function are good improvements for maintainability and correctness. The core logic appears to be a faithful translation of the previous functionality.

I've identified a couple of areas where robustness could be improved, particularly around D-Bus call timeouts and argument appending. Since no specific style guide was provided, my review is based on common C/C++ best practices, focusing on correctness, resource management, and error handling.

Summary of Findings

• D-Bus Call Timeout: The inhibit D-Bus call uses an infinite timeout, which could lead to hangs if the service is unresponsive. A finite timeout with proper handling is recommended.
• Error Checking for dbus_message_append_args: The return value of dbus_message_append_args is not checked, which could lead to sending malformed messages if argument appending fails.
• Error Logging for D-Bus Message Creation (Low Severity - Not Commented): In inhibit_nc, failures from dbus_message_new_method_call (lines 46, 80) result in a silent return. While rare, logging these failures could aid debugging. This was not commented on due to review settings.
• Log Message Prefixing (Low Severity - Not Commented): Error messages printed via fprintf (e.g., line 28, 54, 66, 86) could be prefixed with the plugin name (e.g., [kde-night-color-playback]) for easier identification in system logs. This was not commented on due to review settings.

Merge Readiness

The pull request makes significant progress in removing the Qt dependency by using libdbus directly. The core functionality seems to be preserved. However, there are a couple of medium-severity issues related to D-Bus call robustness (infinite timeout and unchecked return value for argument appending) that should be addressed before merging to ensure stability. I am unable to approve pull requests, but I recommend making these changes for improved code quality. Once these are addressed, the PR should be in good shape.

[gemini-code-assist]

Code Review

This pull request successfully refactors the plugin to use libdbus directly, removing the dependency on QtDBus. This is a positive change for minimizing dependencies. The core D-Bus logic for inhibiting and uninhibiting KWin's Night Color seems mostly correct, with good use of constants and basic error handling.

I've identified a couple of areas, primarily around error handling in D-Bus communication, that could lead to inconsistencies between the plugin's internal state and the actual state of Night Color in KWin. These are detailed in the specific comments.

Additionally, please note that the README.md file will need to be updated to reflect that Qt is no longer a dependency for this plugin. Since the README was not part of this PR's changes, I cannot add a direct comment there.

Summary of Findings

• State Mismatch on Inhibit Failure: If parsing the D-Bus reply for an inhibit call fails, the plugin's internal state night_light_inhibited can become true even though a valid night_cookie was not obtained, leading to a mismatch with KWin's actual state.
• Incorrect Uninhibit State on D-Bus Error: If dbus_message_append_args fails during an uninhibit call, night_cookie is reset to 0, making the plugin believe uninhibition occurred. However, KWin was not notified, potentially leaving Night Color incorrectly inhibited. This is a high severity issue as it directly impacts user experience.
• README.md Update Needed: The README.md file still mentions Qt as a dependency, which is no longer true after these changes. It should be updated.

Merge Readiness

The pull request makes a good step by removing the Qt dependency. However, there's a high-severity issue related to error handling in the uninhibit D-Bus call that could lead to Night Color remaining active when it should be disabled. There's also a medium-severity issue concerning state consistency on inhibit call failures.

I recommend addressing these issues, particularly the high-severity one, before merging. I am not authorized to approve pull requests, so please ensure further review and approval as per your team's policy once the suggested changes are made.

[gemini-code-assist]

Following up on the previous comment regarding dbus_message_append_args failure: night_cookie should ideally be reset to 0 only if the uninhibit D-Bus call was successfully formulated and sent (or at least attempted to be sent).

If dbus_message_append_args fails, the uninhibit call doesn't proceed correctly. Resetting night_cookie here implies the uninhibit action was completed by the plugin, which isn't true, potentially leaving KWin in an inhibited state.

Consider moving night_cookie = 0; to only occur after a successful dbus_connection_send or at least within the block where dbus_message_append_args has succeeded.

Suggested change

	night_cookie = 0;
	// night_cookie = 0; // Moved to be conditional on successful send attempt

[gemini-code-assist]

Code Review

This pull request commendably refactors the D-Bus communication to use libdbus directly, removing the QtDBus dependency. The changes are largely well-implemented, including better D-Bus constant management and resource cleanup.

I've identified a couple of areas for improvement, primarily around error handling nuances and a significant behavioral change in the uninhibit logic that needs clarification. Given the README.md note about the code's origin, these points are particularly important to address for robustness.

Summary of Findings

• Behavioral Change in uninhibit Logic: The uninhibit function now only resets the night_cookie if the D-Bus message send is successful. If the send fails, the cookie remains, potentially causing the plugin to report an inhibited state despite an uninhibit attempt. This differs from the previous QtDBus version and needs clarification of intent. A related comment is also misleading.
• night_cookie State on inhibit Reply Parse Failure: If parsing the D-Bus reply for an inhibit call fails, night_cookie is not reset, potentially leading to an incorrect night_light_inhibited state if night_cookie held a stale value from a previous call.
• night_light_inhibited update after inhibit call: night_light_inhibited is updated based on the value of night_cookie after a successful inhibit call. If the call fails, the variable might retain its previous value, leading to an incorrect state.
• Global Constant Placement (Low Severity): The constant DBUS_TIMEOUT_MS is defined as a global const int. While functional, it could also be a static const int or a #define as it's used only within this file. (Not commented inline due to review settings).

Merge Readiness

The pull request makes good progress in removing the QtDBus dependency. However, due to the high-severity issue regarding the behavioral change in the uninhibit logic and the medium-severity issues concerning state handling on D-Bus reply parsing failure and night_light_inhibited update, I recommend addressing these points before merging. Clarifying the intended behavior for uninhibit is particularly important. I am not authorized to approve pull requests, so further review and approval by maintainers will be necessary after these changes are considered.

[gemini-code-assist]

There's a significant behavioral change here compared to the previous QtDBus implementation, and the comment on lines 102-103 is misleading.

Behavioral Change:
The old QtDBus code unconditionally set night_cookie = 0; after attempting to call uninhibit. The new code only resets night_cookie if dbus_connection_send() for the uninhibit message succeeds. If the send fails (e.g., D-Bus issue, KWin unresponsive), night_cookie remains non-zero. Consequently, night_light_inhibited (which depends on night_cookie != 0) will also remain true, meaning the plugin will report that night color is still inhibited even after a failed attempt to uninhibit.

Misleading Comment:
The comment // The message is still unref'd and cookie reset below... (lines 102-103) is incorrect in the context of this if block (send failure), as the cookie is not reset if the send fails.

Could you clarify if this change in behavior (not resetting night_cookie on uninhibit send failure) is intentional?

• If it is intentional (e.g., to more accurately reflect that the D-Bus command failed), the comment should be updated to reflect the actual logic.
• If it's unintentional, or if the previous behavior of always resetting the cookie is preferred, the logic for night_cookie = 0; would need to be adjusted (e.g., moved after dbus_message_unref(msg) to execute regardless of send success/failure for the uninhibit path).

This change could affect how the plugin's state is perceived, especially if D-Bus communication is temporarily flaky.

[gemini-code-assist]

When dbus_message_get_args fails to retrieve the cookie from KWin's reply, night_cookie currently retains its previous value. If this previous value was a valid cookie from an earlier successful inhibit call, night_light_inhibited (which is set to night_cookie != 0) could incorrectly reflect an inhibited state for the current failed operation.

To ensure the state accurately reflects the outcome of the current D-Bus call, would it be clearer to reset night_cookie to 0 if a new cookie cannot be obtained?

Suggested change

	fprintf(stderr, "[kde-night-color-playback] DBus Reply Error: %s\n", err.message);
	dbus_error_free(&err);
	}
	else
	{
	fprintf(stderr, "[kde-night-color-playback] DBus Reply Error: %s\n", err.message);
	dbus_error_free(&err);
	night_cookie = 0; // Explicitly indicate failure to obtain a new cookie
	}

[gemini-code-assist]

The night_light_inhibited variable is set based on whether night_cookie is non-zero. However, night_cookie is only updated after a successful inhibit call. If the inhibit call fails, night_cookie might retain its previous value, leading to an incorrect state for night_light_inhibited.

Consider checking the return value of inhibit_nc or setting night_light_inhibited based on the success of the D-Bus call.

Suggested change

	night_light_inhibited = (night_cookie != 0);
	inhibit_nc(should_inhibit);
	// night_light_inhibited = (night_cookie != 0);
	night_light_inhibited = should_inhibit; // Set based on intended state

[mddanish00]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively replaces the QtDBus dependency with direct libdbus calls, which is a commendable goal for reducing dependencies. The new implementation significantly improves error handling and resource management. The changes in the Makefile are correct for the new library dependencies. Overall, the C++ code demonstrates a good understanding of libdbus usage, with robust error checking and clear resource cleanup. The detailed comments explaining the logic, especially around state management, are also very helpful.

Summary of Findings

• Build System Update: Makefile updated correctly to link against dbus-1 instead of Qt5DBus. This change was not commented on as it's a correct and direct consequence of the PR's goal.
• D-Bus Constant Definitions and Type Safety: Introduced constants for D-Bus service, path, and interface, improving maintainability. night_cookie type changed to uint32_t for D-Bus compatibility. These are good practices but were not commented on due to review settings.
• Robust D-Bus Initialization: Implemented D-Bus connection initialization with proper error checking using dbus_bus_get and dbus_error_is_set. This was not commented on due to review settings.
• Detailed D-Bus Call Error Handling: Enhanced D-Bus method call error handling, for instance, by distinguishing between timeouts and other errors when calling inhibit. This was not commented on due to review settings.
• Improved mpv_get_property Error Handling: Added crucial error checking for mpv_get_property calls, preventing potential issues from failed property reads. This was commented on.
• Accurate Inhibition State Tracking: Refined the logic for night_light_inhibited to accurately reflect the D-Bus inhibition status based on night_cookie, improving correctness. This was commented on.
• D-Bus Resource Management: Introduced cleanup_dbus_resources function and ensured it's called on plugin shutdown by adding a call in mpv_open_cplugin. The call itself was commented on. The function definition is good practice (not commented).
• Logging Consistency: Standardized log messages with a [kde-night-color-playback] prefix, aiding in debugging. This was not commented on due to review settings.

Merge Readiness

The changes significantly improve the robustness and resource management of the plugin. The core objective of removing the Qt dependency while maintaining and even enhancing functionality appears to be well achieved. The code quality is good, and the improvements are substantial. Based on this review, the PR is in excellent shape. As always, thorough testing in various KDE Plasma environments is recommended to ensure full compatibility. I am not authorized to approve PRs, so please ensure it undergoes any additional required review processes before merging.