implement wayland fullscreen detector #261
Conversation
WalkthroughThe changes in this pull request primarily involve enhancements to the CMake configuration and various classes within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CWallpaperApplication
participant CRenderContext
participant CWallpaper
participant CVideo
User->>CWallpaperApplication: Start Application
CWallpaperApplication->>CRenderContext: Initialize
CRenderContext->>CWallpaper: Load Wallpapers
CWallpaper->>CVideo: Manage Video Playback
User->>CWallpaper: Pause Wallpapers
CWallpaper->>CRenderContext: Call setPause(true)
CRenderContext->>CWallpaper: Update Pause State
CWallpaper->>CVideo: Call setPause(true)
CVideo->>CVideo: Update m_paused state
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
src/WallpaperEngine/Render/CRenderContext.h (1)
38-38: Document the setPause method and consider additional improvements.The new
setPausemethod needs documentation to explain its purpose and behavior. Additionally, consider the following improvements:
- Add a corresponding
isPaused()getter for consistency- Document thread-safety guarantees if this can be called from multiple threads
- Consider adding a protected/private member variable to store the pause state
Here's a suggested implementation with documentation:
+ /** + * Sets the pause state of the render context. + * @param newState true to pause rendering, false to resume + */ void setPause(bool newState); + /** @return true if rendering is currently paused */ + [[nodiscard]] bool isPaused() const; private: + /** Current pause state */ + bool m_paused{false};src/WallpaperEngine/Render/CRenderContext.cpp (1)
45-48: Consider adding error handling for setPause operations.The implementation is clean and follows the single responsibility principle. However, consider adding error handling in case any wallpaper's setPause operation fails.
void CRenderContext::setPause (bool newState) { + std::vector<std::string> failedWallpapers; for (auto&& wallpaper : this->m_wallpapers) - wallpaper.second->setPause (newState); + try { + if (wallpaper.second != nullptr) + wallpaper.second->setPause (newState); + } catch (const std::exception& e) { + failedWallpapers.push_back(wallpaper.first); + } + if (!failedWallpapers.empty()) { + // Log or handle wallpapers that failed to pause + } }src/WallpaperEngine/Render/CWallpaper.h (1)
53-56: Add documentation for the setPause method.The new virtual method lacks documentation explaining its purpose, parameter meaning, and expected behavior in derived classes. Consider adding a documentation block similar to other methods in the class.
+ /** + * Controls the pause state of the wallpaper renderer + * + * @param newState true to pause rendering, false to resume + */ virtual void setPause (bool newState);CMakeLists.txt (1)
439-439: Consider a more robust solution for Vulkan linking.While removing the library works, a more robust solution would be to handle this at the linking level.
Consider these alternatives:
- Use
find_package(Vulkan)and properly link against it- Add explicit linking flags to exclude Vulkan symbols from MPV:
set_target_properties(${MPV_LIBRARY} PROPERTIES INTERFACE_LINK_OPTIONS "-Wl,--exclude-libs,libvulkan.so.1")src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.cpp (3)
27-35: Ensure safe iteration overwl_arrayintoplevelHandleState.In the function
toplevelHandleState, improve readability and safety by defining anendpointer for the loop. This clarifies the bounds and ensures correct iteration over the array.Apply this diff to enhance clarity:
void toplevelHandleState(void* data, struct zwlr_foreign_toplevel_handle_v1* handle, struct wl_array* state) { auto fullscreen = static_cast<FullscreenState*>(data); const auto begin = static_cast<uint32_t*>(state->data); + const auto end = begin + state->size / sizeof(uint32_t); fullscreen->pending = false; - for (auto it = begin; it < begin + state->size / sizeof(uint32_t); ++it) + for (auto it = begin; it < end; ++it) if (*it == ZWLR_FOREIGN_TOPLEVEL_HANDLE_V1_STATE_FULLSCREEN) fullscreen->pending = true; }
140-140: Consider the potential blocking behavior ofwl_display_roundtripinanythingFullscreen().Calling
wl_display_roundtrip(m_display);insideanythingFullscreen()can block the execution if the Wayland server is unresponsive, leading to potential performance issues. Consider handling this asynchronously or polling at appropriate intervals to avoid blocking the main thread.
91-94: DeclaretoplevelManagerListenerasconstexprfor consistency.The
toplevelHandleListeneris declared asconstexpr, buttoplevelManagerListeneris not. For consistency and potential compiler optimizations, consider declaringtoplevelManagerListenerasconstexpr.Apply this diff:
-zwlr_foreign_toplevel_manager_v1_listener toplevelManagerListener = { +constexpr zwlr_foreign_toplevel_manager_v1_listener toplevelManagerListener = { .toplevel = handleToplevel, .finished = handleFinished, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocols/wlr-foreign-toplevel-management-unstable-v1.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
- CMakeLists.txt (6 hunks)
- src/WallpaperEngine/Application/CWallpaperApplication.cpp (9 hunks)
- src/WallpaperEngine/Render/CRenderContext.cpp (2 hunks)
- src/WallpaperEngine/Render/CRenderContext.h (2 hunks)
- src/WallpaperEngine/Render/CWallpaper.cpp (2 hunks)
- src/WallpaperEngine/Render/CWallpaper.h (2 hunks)
- src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.cpp (1 hunks)
- src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.h (1 hunks)
- src/WallpaperEngine/Render/Wallpapers/CVideo.cpp (3 hunks)
- src/WallpaperEngine/Render/Wallpapers/CVideo.h (2 hunks)
🔇 Additional comments (20)
src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.h (3)
9-11: LGTM! Forward declarations are properly used.Good use of forward declarations for Wayland types to minimize header dependencies.
13-15: LGTM! Class structure follows best practices.Good use of:
finalspecifier to prevent further inheritance- Clear namespace organization
- Proper inheritance hierarchy
16-20: LGTM! Public interface is well-designed.Good use of:
explicitconstructor[[nodiscard]]attributeoverridespecifier- const-correctness
src/WallpaperEngine/Render/Wallpapers/CVideo.h (2)
21-21: LGTM: setPause method addition aligns with fullscreen detection requirements.The override of setPause provides video-specific pause control, which is essential for the Wayland fullscreen detector implementation.
35-37: Verify the impact of int64_t type change for dimensions.While using int64_t for dimensions provides better support for large resolutions, please ensure:
- Memory alignment is maintained
- No implicit conversion issues with OpenGL/MPV APIs that might expect 32-bit integers
✅ Verification successful
Let me analyze the code for potential type mismatches. I need to check how these dimensions are used in the getter methods and MPV API calls.
Based on the code analysis, I can now generate the final response:
No issues with int64_t dimensions in OpenGL/MPV interactions
The codebase safely handles the dimension types:
- MPV API calls explicitly cast dimensions to int:
static_cast<int>(this->m_width)- OpenGL texture/viewport operations receive the dimensions through getWidth()/getHeight() methods which handle the conversion
- Memory alignment is maintained as int64_t is properly aligned
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential integer type mismatches in MPV API calls rg -A 2 "mpv_render" # Check for GL viewport/texture operations that might need type matching rg -A 2 "glViewport|glTexImage"Length of output: 4364
Script:
#!/bin/bash # Check the getter implementations and their return types ast-grep --pattern 'getWidth() { $$$ }' ast-grep --pattern 'getHeight() { $$$ }' # Check MPV struct initialization with dimensions rg -A 5 "mpv_opengl_fbo" # Check the base class or interface definitions fd "CWallpaper" --exec cat {}Length of output: 50988
src/WallpaperEngine/Render/CRenderContext.cpp (1)
45-48: Verify thread safety with render method.The setPause method modifies wallpaper states that might be accessed during rendering. Let's verify if there are any synchronization mechanisms in place.
✅ Verification successful
Let me gather more specific information about the render method and its interaction with wallpapers.
Let me check one more aspect - the rendering loop and how these methods are invoked.
Based on the gathered information, I can now provide the final response:
Thread safety concerns appear to be minimal in this context
The code analysis reveals that:
- The render and setPause operations occur in a single-threaded manner through
CWallpaperApplication::updatesetPauseis a simple state change operation, with the base implementation being empty- No concurrent access patterns or thread-related operations were found in the codebase
- The wallpaper container (
m_wallpapers) is only accessed from the main rendering context🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mutex/lock usage in CRenderContext and related classes echo "Checking for synchronization mechanisms..." rg -l "mutex|lock" --type cpp # Check if render or setPause are called from multiple threads echo "Checking for thread-related calls..." ast-grep --pattern 'std::thread|async|std::future' # Look for any existing thread safety documentation echo "Checking for thread safety documentation..." rg -i "thread.*safe|concurrent|synchronized" --type cppLength of output: 1329
Script:
#!/bin/bash # Check the render method implementation and wallpaper access echo "Checking render method implementation..." ast-grep --pattern 'render($$$) { $$$ }' # Check wallpaper container access patterns echo "Checking wallpaper container access..." rg -A 5 "m_wallpapers" --type cpp # Check if there are any other methods accessing wallpapers echo "Checking other wallpaper access patterns..." ast-grep --pattern 'wallpaper.second->'Length of output: 2366
Script:
#!/bin/bash # Check for main render loop or where render/setPause are called echo "Checking render loop and method invocations..." rg -B 3 -A 3 "render\s*\([^)]*\)" --type cpp # Check CWallpaper class implementation for any synchronization echo "Checking CWallpaper implementation..." rg -l "class CWallpaper" --type cpp rg -B 2 -A 5 "void.*render|void.*setPause" src/WallpaperEngine/Render/CWallpaper.cpp || trueLength of output: 9882
src/WallpaperEngine/Render/Wallpapers/CVideo.cpp (1)
18-18: LGTM!The member variable is properly initialized in the constructor's initialization list.
src/WallpaperEngine/Render/CWallpaper.h (1)
149-150: LGTM!The parameter reordering in
fromWallpaperis a non-functional change that aligns with the implementation updates.src/WallpaperEngine/Render/CWallpaper.cpp (1)
Line range hint
292-296: Verify browserContext parameter usage in derived classes.The
browserContextparameter is only used in theCWebconstructor. Let's verify that all derived classes handle this parameter correctly.✅ Verification successful
Let me gather more information about how browserContext is used in these files.
Based on the gathered information, I can now provide the final response:
The browserContext parameter is correctly handled in the codebase
The verification shows that:
browserContextis only used inCWebclass as intendedCWebproperly stores and initializes the browser context in its constructor- The parameter is correctly passed through the factory method
CWallpaper::fromWallpaper- Other wallpaper types (CScene, CVideo) don't receive this parameter, which is the expected behavior
- The application has safeguards to stop the browser context when it's not being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CWeb constructor implementation to verify browserContext usage ast-grep --pattern 'CWeb::CWeb($$$browserContext$$$)' # Search for other potential uses of browserContext rg -l "browserContext" --type cppLength of output: 377
Script:
#!/bin/bash # Check CWeb constructor implementation ast-grep --pattern 'CWeb::CWeb($$$) { $$$ }' # Check how browserContext is used in CWeb.h cat src/WallpaperEngine/Render/Wallpapers/CWeb.h # Check how browserContext is used in CWallpaper.h cat src/WallpaperEngine/Render/CWallpaper.h # Look for other classes that might use browserContext rg "browserContext" -A 5 -B 5Length of output: 20740
🧰 Tools
🪛 cppcheck
[error] 295-295: Found an exit path from function with non-void return type that has missing return statement
(missingReturn)
src/WallpaperEngine/Application/CWallpaperApplication.cpp (7)
59-59: LGTM! Improved error message formatting.The addition of space after the comma enhances readability of the error message.
176-178: LGTM! Enhanced conditional readability.The conditions are now properly formatted with line breaks, making the code more readable while maintaining the same logic.
248-250: LGTM! Correct pixel copying implementation.The pixel copying logic correctly handles RGB components while maintaining proper array indexing.
280-287: LGTM! Helpful debug logging for window server detection.The added debug logging improves visibility into available window server support during initialization.
353-355: LGTM! Improved input context initialization formatting.The initialization is now properly formatted with line breaks for better readability.
452-452: LGTM! Proper namespace closing format.The added space before the namespace closing comment improves readability.
407-413: 🛠️ Refactor suggestionConsider improving the fullscreen detection implementation.
While the implementation is functional, there are several potential improvements:
- The sleep duration should be configurable to allow users to adjust responsiveness.
- The busy-wait loop could be replaced with a condition variable for better CPU efficiency.
- Consider adding a timeout mechanism to prevent indefinite pausing.
Here's a suggested improvement using a condition variable:
+ static constexpr auto FULLSCREEN_TIMEOUT = std::chrono::seconds(30); + std::condition_variable fullscreen_cv; + std::mutex fullscreen_mutex; + // check for fullscreen windows and wait until there's none fullscreen if (this->m_fullScreenDetector->anythingFullscreen() && this->m_context.state.general.keepRunning) { m_renderContext->setPause(true); - while (this->m_fullScreenDetector->anythingFullscreen() && this->m_context.state.general.keepRunning) - usleep(FULLSCREEN_CHECK_WAIT_TIME); + std::unique_lock<std::mutex> lock(fullscreen_mutex); + fullscreen_cv.wait_for(lock, FULLSCREEN_TIMEOUT, [this] { + return !this->m_fullScreenDetector->anythingFullscreen() || + !this->m_context.state.general.keepRunning; + }); m_renderContext->setPause(false); }This implementation:
- Uses a condition variable instead of busy-waiting
- Adds a timeout to prevent indefinite pausing
- Reduces CPU usage while waiting
Let's verify if the fullscreen detection is properly integrated with the Wayland implementation:
CMakeLists.txt (3)
43-71: LGTM! Well-structured platform detection logic.The platform detection logic is comprehensive, covering all major architectures (arm, arm64, x86_64) across different operating systems.
162-163: LGTM! Well-organized Wayland source files.The Wayland source files are logically organized and include all necessary components for implementing the fullscreen detector.
Line range hint
1-1: Verify integration of Wayland fullscreen detector components.The implementation includes all necessary components for the Wayland fullscreen detector. Please ensure:
- The foreign-toplevel-management protocol is supported by your Wayland compositor
- The detector is properly initialized in the Wayland driver
✅ Verification successful
All Wayland fullscreen detector components are properly integrated
The verification confirms proper integration of all components:
- Protocol support:
wlr-foreign-toplevel-managementprotocol is present inprotocols/- Implementation: Complete detector implementation in
CWaylandFullScreenDetector.{h,cpp}with proper protocol handling- Integration: Detector is correctly initialized in
CWallpaperApplicationwhen Wayland is enabled- Protocol binding: Proper runtime binding and event handling for
zwlr_foreign_toplevel_manager_v1🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the Wayland compositor supports the required protocol echo "Checking for foreign-toplevel-management protocol support..." rg -l "zwlr_foreign_toplevel_manager" protocols/Length of output: 237
Script:
#!/bin/bash # Check if the protocol is properly integrated in the build system echo "Checking protocol integration in build system..." rg -l "wlr-foreign-toplevel-management" CMakeLists.txt # Check if the protocol is used in source files echo "Checking protocol usage in source files..." rg -l "foreign_toplevel_manager" src/ # Check for Wayland detector implementation echo "Checking Wayland detector implementation..." fd -e cpp -e hpp . src/ -x grep -l "wayland.*fullscreen"Length of output: 615
Script:
#!/bin/bash # Check the actual implementation details of the Wayland fullscreen detector echo "Checking Wayland fullscreen detector implementation details..." rg "class.*WaylandFullScreenDetector" -A 5 src/ rg "foreign_toplevel_manager" -A 5 -B 5 src/ # Check if the detector is properly initialized in the driver echo "Checking detector initialization in Wayland driver..." rg "WaylandFullScreenDetector" -A 5 -B 5 src/ --type cppLength of output: 18297
src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.cpp (1)
54-68: Verify thatfullscreenis not used after deletion intoplevelHandleClosed.After deleting
fullscreen, ensure there are no further references to it to prevent dangling pointers, which can lead to undefined behavior.Run the following script to check for any references to
fullscreenafter it's deleted:Expect no references to
fullscreenin the output after deletion.✅ Verification successful
Based on the results, I can see that there are no references to
fullscreenafter thedelete fullscreenstatement. However, let me verify the complete function implementation and surrounding context to ensure there are no other potential issues.
Let me try a different approach using ripgrep to get the complete function context.
Let me try one more approach to get the function context using a simpler ripgrep pattern.
No issues found with
fullscreenusage after deletionBased on the complete function context, I can confirm that
fullscreenis not used after thedelete fullscreenstatement. The function properly:
- Uses the
fullscreenpointer for state management- Cleans up the Wayland handle with
zwlr_foreign_toplevel_handle_v1_destroy- Safely deletes the
fullscreenpointer as the last operation- Has no further references to
fullscreenafter deletionThe implementation is safe and free from any potential use-after-free issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'fullscreen' is not used after deletion. # Search for 'fullscreen' references after the 'delete fullscreen' line. rg 'delete fullscreen' -A 10 | rg 'fullscreen'Length of output: 145
Script:
#!/bin/bash # Get the complete function implementation and surrounding context ast-grep --pattern 'void toplevelHandleClosed($$$)' -A 15Length of output: 59
Script:
#!/bin/bash # Get the complete function implementation and surrounding context rg -U "void toplevelHandleClosed.*?\{.*?\}" -A 15 --multilineLength of output: 63
Script:
#!/bin/bash # Get the function implementation with more context rg "void toplevelHandleClosed" -B 5 -A 20Length of output: 2752
| private: | ||
| wl_display* m_display = nullptr; | ||
| zwlr_foreign_toplevel_manager_v1* m_toplevelManager = nullptr; | ||
|
|
||
| uint32_t m_fullscreenCount = 0; | ||
|
|
||
| friend void handleGlobal (void* data, struct wl_registry* registry, uint32_t name, const char* interface, | ||
| uint32_t version); |
There was a problem hiding this comment.
💡 Codebase verification
Resource management appears to be incomplete
The code review reveals actual issues with resource management:
-
The class lacks a destructor to clean up Wayland resources:
m_displayneeds to be freed withwl_display_disconnectm_toplevelManageris only destroyed in thehandleFinishedcallback, but not when the detector is destroyed
-
Missing copy/move semantics could lead to issues since the class manages raw pointers. Default copy operations would cause double-free or use-after-free scenarios.
Recommendations:
- Add a destructor to properly clean up
m_display - Explicitly delete copy operations or implement proper deep copying
- Consider implementing move operations if the detector needs to be relocatable
🔗 Analysis chain
Verify resource management for Wayland objects.
The class manages raw Wayland pointers (m_display and m_toplevelManager) but:
- There's no visible destructor to clean up these resources
- Copy/move semantics aren't explicitly defined, which could lead to double-free or resource leaks
Let's check the implementation file for proper resource management:
Consider:
- Adding a destructor declaration if not already present
- Explicitly declaring copy/move semantics (Rule of Five) or deleting them if not needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for destructor and copy/move semantics implementation
# Look for destructor implementation
echo "Checking for destructor:"
rg "CWaylandFullScreenDetector::~CWaylandFullScreenDetector" -A 5
# Look for copy/move operations
echo "Checking for copy/move operations:"
rg "CWaylandFullScreenDetector::CWaylandFullScreenDetector\(const CWaylandFullScreenDetector&\)|CWaylandFullScreenDetector::operator=" -A 5
Length of output: 347
Script:
#!/bin/bash
# Let's try a broader search to find the class implementation and header
# First, find all files that might contain the class
echo "Files containing CWaylandFullScreenDetector:"
fd -t f . | xargs rg "class.*CWaylandFullScreenDetector" -l
# Look for destructor-like cleanup code
echo -e "\nChecking for cleanup/destruction related code:"
rg "wl_display_disconnect|zwlr_foreign_toplevel_manager_v1_destroy" -A 2
# Look for the implementation file
echo -e "\nChecking implementation file content:"
fd -t f . | xargs rg "CWaylandFullScreenDetector::" -A 2
Length of output: 2202
Script:
#!/bin/bash
# Let's check the complete implementation file to see all the methods and resource management
echo "Complete implementation file content:"
cat src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.cpp
# Also check the complete header file
echo -e "\nComplete header file content:"
cat src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.h
Length of output: 6495
| void CWallpaper::setPause (bool newState) {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making setPause a virtual method.
The empty implementation suggests this is meant to be overridden by derived classes. Consider:
- Making it virtual to clearly communicate the interface contract
- Adding documentation to describe the expected behavior
-void CWallpaper::setPause(bool newState) {}
+/// @brief Controls the pause state of the wallpaper
+/// @param newState true to pause, false to resume
+virtual void CWallpaper::setPause(bool newState) {}Committable suggestion was skipped due to low confidence.
| auto extension = filename.extension (); | ||
|
|
||
| if (extension == ".bmp") { | ||
| stbi_write_bmp (filename.c_str(), width, height, 3, bitmap); | ||
| stbi_write_bmp (filename.c_str (), width, height, 3, bitmap); | ||
| } else if (extension == ".png") { | ||
| stbi_write_png (filename.c_str(), width, height, 3, bitmap, width * 3); | ||
| stbi_write_png (filename.c_str (), width, height, 3, bitmap, width * 3); | ||
| } else if (extension == ".jpg" || extension == ".jpeg") { | ||
| stbi_write_jpg (filename.c_str(), width, height, 3, bitmap, 100); | ||
| stbi_write_jpg (filename.c_str (), width, height, 3, bitmap, 100); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using string_view for extension comparison.
The extension handling is correct, but could be optimized using std::string_view for more efficient string comparisons.
- auto extension = filename.extension();
+ std::string_view extension = filename.extension().string();
- if (extension == ".bmp") {
+ if (extension == std::string_view{".bmp"}) {Committable suggestion was skipped due to low confidence.
| execute_process( | ||
| COMMAND ${WaylandScanner} client-header ${CMAKE_SOURCE_DIR}/protocols/wlr-foreign-toplevel-management-unstable-v1.xml wlr-foreign-toplevel-management-unstable-v1-protocol.h | ||
| WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) | ||
| execute_process( | ||
| COMMAND ${WaylandScanner} private-code ${CMAKE_SOURCE_DIR}/protocols/wlr-foreign-toplevel-management-unstable-v1.xml wlr-foreign-toplevel-management-unstable-v1-protocol.c | ||
| WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for protocol generation.
While the protocol generation commands are correct, they could be more robust by checking the command results and handling potential failures.
Consider wrapping the protocol generation in a function that checks for errors:
function(generate_wayland_protocol SCANNER PROTOCOL_FILE HEADER_OUTPUT CODE_OUTPUT)
execute_process(
COMMAND ${SCANNER} client-header ${PROTOCOL_FILE} ${HEADER_OUTPUT}
RESULT_VARIABLE HEADER_RESULT
ERROR_VARIABLE HEADER_ERROR
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
if(NOT "${HEADER_RESULT}" STREQUAL "0")
message(FATAL_ERROR "Failed to generate protocol header: ${HEADER_ERROR}")
endif()
execute_process(
COMMAND ${SCANNER} private-code ${PROTOCOL_FILE} ${CODE_OUTPUT}
RESULT_VARIABLE CODE_RESULT
ERROR_VARIABLE CODE_ERROR
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
if(NOT "${CODE_RESULT}" STREQUAL "0")
message(FATAL_ERROR "Failed to generate protocol code: ${CODE_ERROR}")
endif()
endfunction()
# Usage:
generate_wayland_protocol(
${WaylandScanner}
${CMAKE_SOURCE_DIR}/protocols/wlr-foreign-toplevel-management-unstable-v1.xml
wlr-foreign-toplevel-management-unstable-v1-protocol.h
wlr-foreign-toplevel-management-unstable-v1-protocol.c)| if (!m_display) | ||
| sLog.exception ("Failed to query wayland display"); | ||
|
|
There was a problem hiding this comment.
Handle failure to connect to the Wayland display appropriately.
If wl_display_connect fails and returns nullptr, the code logs an exception but continues execution. Ensure that the application handles this failure properly, possibly by throwing an exception or exiting to prevent undefined behavior.
Apply this diff to handle the error correctly:
m_display = wl_display_connect(nullptr);
if (!m_display)
- sLog.exception("Failed to query wayland display");
+ {
+ sLog.error("Failed to connect to Wayland display");
+ throw std::runtime_error("Failed to connect to Wayland display");
+ }Ensure that the exception is caught at a higher level, and the application exits or attempts recovery as appropriate.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!m_display) | |
| sLog.exception ("Failed to query wayland display"); | |
| if (!m_display) | |
| { | |
| sLog.error("Failed to connect to Wayland display"); | |
| throw std::runtime_error("Failed to connect to Wayland display"); | |
| } |
| const auto detector = static_cast<CWaylandFullScreenDetector*> (data); | ||
| if (strcmp (interface, zwlr_foreign_toplevel_manager_v1_interface.name) == 0) { | ||
| detector->m_toplevelManager = static_cast<zwlr_foreign_toplevel_manager_v1*> ( | ||
| wl_registry_bind (registry, name, &zwlr_foreign_toplevel_manager_v1_interface, 3)); |
There was a problem hiding this comment.
Avoid hardcoding the version number in wl_registry_bind.
In handleGlobal, the version number is hardcoded to 3 when binding zwlr_foreign_toplevel_manager_v1. This may cause compatibility issues if the compositor supports a different version. Consider using the provided version parameter to ensure compatibility.
Apply this diff to use the appropriate interface version:
if (strcmp(interface, zwlr_foreign_toplevel_manager_v1_interface.name) == 0) {
- detector->m_toplevelManager = static_cast<zwlr_foreign_toplevel_manager_v1*>(
- wl_registry_bind(registry, name, &zwlr_foreign_toplevel_manager_v1_interface, 3));
+ uint32_t bind_version = std::min(version, static_cast<uint32_t>(ZWLR_FOREIGN_TOPLEVEL_MANAGER_V1_VERSION));
+ detector->m_toplevelManager = static_cast<zwlr_foreign_toplevel_manager_v1*>(
+ wl_registry_bind(registry, name, &zwlr_foreign_toplevel_manager_v1_interface, bind_version));
if (detector->m_toplevelManager) {Remember to include <algorithm> for std::min and define ZWLR_FOREIGN_TOPLEVEL_MANAGER_V1_VERSION as the maximum version your code supports.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/WallpaperEngine/Render/CWallpaper.h (1)
53-56: Document the setPause method and clarify thread safety.The new virtual method needs documentation to explain:
- The purpose and expected behavior when paused
- Whether it's safe to call from multiple threads
- Requirements for derived class implementations
Add documentation like this:
/** + * Sets the pause state of the wallpaper renderer. + * @param newState true to pause rendering, false to resume + * @thread_safety This method is/isn't thread-safe + */ virtual void setPause(bool newState);src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.cpp (1)
148-148: Document the purpose of the empty reset method.The
reset()method is empty but appears to be part of an interface. Consider adding a comment explaining why no implementation is needed in the Wayland case.-void CWaylandFullScreenDetector::reset() {} +// No reset implementation needed for Wayland as the state is automatically +// managed through the compositor's foreign-toplevel protocol +void CWaylandFullScreenDetector::reset() {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocols/wlr-foreign-toplevel-management-unstable-v1.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
- CMakeLists.txt (6 hunks)
- src/WallpaperEngine/Application/CWallpaperApplication.cpp (9 hunks)
- src/WallpaperEngine/Render/CRenderContext.cpp (2 hunks)
- src/WallpaperEngine/Render/CRenderContext.h (2 hunks)
- src/WallpaperEngine/Render/CWallpaper.cpp (2 hunks)
- src/WallpaperEngine/Render/CWallpaper.h (2 hunks)
- src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.cpp (1 hunks)
- src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.h (1 hunks)
- src/WallpaperEngine/Render/Wallpapers/CVideo.cpp (3 hunks)
- src/WallpaperEngine/Render/Wallpapers/CVideo.h (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- CMakeLists.txt
- src/WallpaperEngine/Application/CWallpaperApplication.cpp
- src/WallpaperEngine/Render/CRenderContext.cpp
- src/WallpaperEngine/Render/CRenderContext.h
- src/WallpaperEngine/Render/CWallpaper.cpp
- src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.h
- src/WallpaperEngine/Render/Wallpapers/CVideo.h
🔇 Additional comments (3)
src/WallpaperEngine/Render/Wallpapers/CVideo.cpp (1)
18-18: LGTM: Member initialization is correct.The
m_pausedmember is properly initialized tofalsein the constructor's initialization list.src/WallpaperEngine/Render/CWallpaper.h (1)
149-150: LGTM: Parameter reordering in fromWallpaper.The reordering of parameters is a safe change that improves consistency.
Let's verify all call sites have been updated:
✅ Verification successful
Parameter order in fromWallpaper calls matches the declaration
Based on the search results, I found all occurrences of
fromWallpaperin the codebase. The call site inCWallpaperApplication.cppmatches the parameter order in the declaration and implementation inCWallpaper.handCWallpaper.cpp:
- wallpaper
- context
- audioContext
- browserContext
- scalingMode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to fromWallpaper to ensure they match the new parameter order ast-grep --pattern 'CWallpaper::fromWallpaper($$$)'Length of output: 53
Script:
#!/bin/bash # Search for fromWallpaper method calls with ripgrep for better coverage rg "fromWallpaper" -A 3 # Also search for the method definition to understand the old parameter order rg "static std::shared_ptr<CWallpaper> fromWallpaper" -A 5Length of output: 1597
src/WallpaperEngine/Render/Drivers/Detectors/CWaylandFullScreenDetector.cpp (1)
11-26: LGTM! Clean implementation of state management.The
FullscreenStatestruct and basic event handlers are well-designed. Empty handlers for unused events are appropriate as they're required by the protocol but not needed for fullscreen detection.
| void CVideo::setPause (bool newState) { | ||
| if (m_paused == newState) | ||
| return; | ||
| m_paused = newState; | ||
| int pause = newState; | ||
| mpv_set_property (m_mpv, "pause", MPV_FORMAT_FLAG, &pause); | ||
| } |
There was a problem hiding this comment.
Add null pointer check for m_mpv.
The method should verify that m_mpv is not null before attempting to set the property, as it could be called during video teardown.
Here's the suggested fix:
void CVideo::setPause (bool newState) {
if (m_paused == newState)
return;
m_paused = newState;
+ if (!m_mpv)
+ return;
int pause = newState;
mpv_set_property (m_mpv, "pause", MPV_FORMAT_FLAG, &pause);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void CVideo::setPause (bool newState) { | |
| if (m_paused == newState) | |
| return; | |
| m_paused = newState; | |
| int pause = newState; | |
| mpv_set_property (m_mpv, "pause", MPV_FORMAT_FLAG, &pause); | |
| } | |
| void CVideo::setPause (bool newState) { | |
| if (m_paused == newState) | |
| return; | |
| m_paused = newState; | |
| if (!m_mpv) | |
| return; | |
| int pause = newState; | |
| mpv_set_property (m_mpv, "pause", MPV_FORMAT_FLAG, &pause); | |
| } |
| if (!m_toplevelManager) { | ||
| return false; | ||
| } | ||
| wl_display_roundtrip (m_display); | ||
| return m_fullscreenCount > 0; |
There was a problem hiding this comment.
Add error handling for display roundtrip.
The wl_display_roundtrip call can fail and return -1. Consider adding error handling to detect connection issues.
if (!m_toplevelManager) {
return false;
}
-wl_display_roundtrip(m_display);
+if (wl_display_roundtrip(m_display) < 0) {
+ sLog.error("Wayland connection error during roundtrip");
+ return false;
+}
return m_fullscreenCount > 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!m_toplevelManager) { | |
| return false; | |
| } | |
| wl_display_roundtrip (m_display); | |
| return m_fullscreenCount > 0; | |
| if (!m_toplevelManager) { | |
| return false; | |
| } | |
| if (wl_display_roundtrip(m_display) < 0) { | |
| sLog.error("Wayland connection error during roundtrip"); | |
| return false; | |
| } | |
| return m_fullscreenCount > 0; |
| void toplevelHandleState (void* data, struct zwlr_foreign_toplevel_handle_v1* handle, struct wl_array* state) { | ||
| auto fullscreen = static_cast<FullscreenState*> (data); | ||
| const auto begin = static_cast<uint32_t*> (state->data); | ||
|
|
||
| fullscreen->pending = false; | ||
| for (auto it = begin; it < begin + state->size / sizeof (uint32_t); ++it) | ||
| if (*it == ZWLR_FOREIGN_TOPLEVEL_HANDLE_V1_STATE_FULLSCREEN) | ||
| fullscreen->pending = true; | ||
| } | ||
|
|
||
| void toplevelHandleDone (void* data, struct zwlr_foreign_toplevel_handle_v1* handle) { | ||
| auto fullscreen = static_cast<FullscreenState*> (data); | ||
| if (fullscreen->current != fullscreen->pending) { | ||
| fullscreen->current = fullscreen->pending; | ||
| if (fullscreen->current) { | ||
| ++(*fullscreen->count); | ||
| } else { | ||
| // sanity check | ||
| if (*fullscreen->count == 0) { | ||
| sLog.error ("Fullscreen count underflow!!!"); | ||
| } else { | ||
| --(*fullscreen->count); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void toplevelHandleClosed (void* data, struct zwlr_foreign_toplevel_handle_v1* handle) { | ||
| auto fullscreen = static_cast<FullscreenState*> (data); | ||
|
|
||
| if (fullscreen->current) { | ||
| // sanity check | ||
| if (*fullscreen->count == 0) { | ||
| sLog.error ("Fullscreen count underflow!!!"); | ||
| } else { | ||
| --(*fullscreen->count); | ||
| } | ||
| } | ||
|
|
||
| zwlr_foreign_toplevel_handle_v1_destroy (handle); | ||
| delete fullscreen; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicate underflow checks.
The underflow check logic is duplicated in both toplevelHandleDone and toplevelHandleClosed. Consider extracting this into a helper function for better maintainability.
+namespace {
+void decrementWithUnderflowCheck(uint32_t* count, const char* context) {
+ if (*count == 0) {
+ sLog.error("Fullscreen count underflow in {}!!!", context);
+ } else {
+ --(*count);
+ }
+}
+}
void toplevelHandleDone(void* data, struct zwlr_foreign_toplevel_handle_v1* handle) {
auto fullscreen = static_cast<FullscreenState*>(data);
if (fullscreen->current != fullscreen->pending) {
fullscreen->current = fullscreen->pending;
if (fullscreen->current) {
++(*fullscreen->count);
} else {
- if (*fullscreen->count == 0) {
- sLog.error("Fullscreen count underflow!!!");
- } else {
- --(*fullscreen->count);
- }
+ decrementWithUnderflowCheck(fullscreen->count, "done handler");
}
}
}
void toplevelHandleClosed(void* data, struct zwlr_foreign_toplevel_handle_v1* handle) {
auto fullscreen = static_cast<FullscreenState*>(data);
if (fullscreen->current) {
- if (*fullscreen->count == 0) {
- sLog.error("Fullscreen count underflow!!!");
- } else {
- --(*fullscreen->count);
- }
+ decrementWithUnderflowCheck(fullscreen->count, "close handler");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void toplevelHandleState (void* data, struct zwlr_foreign_toplevel_handle_v1* handle, struct wl_array* state) { | |
| auto fullscreen = static_cast<FullscreenState*> (data); | |
| const auto begin = static_cast<uint32_t*> (state->data); | |
| fullscreen->pending = false; | |
| for (auto it = begin; it < begin + state->size / sizeof (uint32_t); ++it) | |
| if (*it == ZWLR_FOREIGN_TOPLEVEL_HANDLE_V1_STATE_FULLSCREEN) | |
| fullscreen->pending = true; | |
| } | |
| void toplevelHandleDone (void* data, struct zwlr_foreign_toplevel_handle_v1* handle) { | |
| auto fullscreen = static_cast<FullscreenState*> (data); | |
| if (fullscreen->current != fullscreen->pending) { | |
| fullscreen->current = fullscreen->pending; | |
| if (fullscreen->current) { | |
| ++(*fullscreen->count); | |
| } else { | |
| // sanity check | |
| if (*fullscreen->count == 0) { | |
| sLog.error ("Fullscreen count underflow!!!"); | |
| } else { | |
| --(*fullscreen->count); | |
| } | |
| } | |
| } | |
| } | |
| void toplevelHandleClosed (void* data, struct zwlr_foreign_toplevel_handle_v1* handle) { | |
| auto fullscreen = static_cast<FullscreenState*> (data); | |
| if (fullscreen->current) { | |
| // sanity check | |
| if (*fullscreen->count == 0) { | |
| sLog.error ("Fullscreen count underflow!!!"); | |
| } else { | |
| --(*fullscreen->count); | |
| } | |
| } | |
| zwlr_foreign_toplevel_handle_v1_destroy (handle); | |
| delete fullscreen; | |
| } | |
| namespace { | |
| void decrementWithUnderflowCheck(uint32_t* count, const char* context) { | |
| if (*count == 0) { | |
| sLog.error("Fullscreen count underflow in {}!!!", context); | |
| } else { | |
| --(*count); | |
| } | |
| } | |
| } | |
| void toplevelHandleState (void* data, struct zwlr_foreign_toplevel_handle_v1* handle, struct wl_array* state) { | |
| auto fullscreen = static_cast<FullscreenState*> (data); | |
| const auto begin = static_cast<uint32_t*> (state->data); | |
| fullscreen->pending = false; | |
| for (auto it = begin; it < begin + state->size / sizeof (uint32_t); ++it) | |
| if (*it == ZWLR_FOREIGN_TOPLEVEL_HANDLE_V1_STATE_FULLSCREEN) | |
| fullscreen->pending = true; | |
| } | |
| void toplevelHandleDone (void* data, struct zwlr_foreign_toplevel_handle_v1* handle) { | |
| auto fullscreen = static_cast<FullscreenState*> (data); | |
| if (fullscreen->current != fullscreen->pending) { | |
| fullscreen->current = fullscreen->pending; | |
| if (fullscreen->current) { | |
| ++(*fullscreen->count); | |
| } else { | |
| decrementWithUnderflowCheck(fullscreen->count, "done handler"); | |
| } | |
| } | |
| } | |
| void toplevelHandleClosed (void* data, struct zwlr_foreign_toplevel_handle_v1* handle) { | |
| auto fullscreen = static_cast<FullscreenState*> (data); | |
| if (fullscreen->current) { | |
| decrementWithUnderflowCheck(fullscreen->count, "close handler"); | |
| } | |
| zwlr_foreign_toplevel_handle_v1_destroy (handle); | |
| delete fullscreen; | |
| } |
|
The PR looks good, gave it a test on my multi-screen machine and everything seems to work as it should. I'll take care of some of the things coderabbit highlighted myself as they're pretty minor things. Thanks for the PR and sorry for taking so much time in looking into it! |
No description provided.