Move Windows-only resource files to Platform/Windows/ (#343)#44
Move Windows-only resource files to Platform/Windows/ (#343)#44
Conversation
Group Windows-specific resource files into src/source/Platform/Windows/ so the source root no longer mixes platform-specific files with cross-platform code. Sets up the conventional Platform/<os>/ layout used by Godot, Qt, SDL. Moved: - resource.h, resource.rc, resource.aps - icon1.ico, icon2.ico - Global Release/resource.res (also drops the stray-space folder) Updated: - Winmain.cpp: #include "Platform/Windows/resource.h" - src/CMakeLists.txt: target_sources path for the RC file
Summary of ChangesHello, I'm Gemini Code Assist1! 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! This pull request refactors the project structure by centralizing Windows-only resource files into a 'Platform/Windows/' directory. This change aligns the repository with conventional layouts used by major projects like Godot and Qt, improving maintainability and removing unnecessary directory nesting. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request relocates Windows-specific resource files to a dedicated platform directory and updates the CMake configuration and source code to reflect these changes. The reviewer suggests using target_include_directories in the CMake configuration to avoid hardcoding platform-specific paths in the source code, which would allow the resource header to be included without its full relative path.
| target_sources(Main PRIVATE | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/source/resource.rc" | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/source/Platform/Windows/resource.rc" | ||
| ) |
There was a problem hiding this comment.
Instead of hardcoding the platform-specific path in the source code, it is better to add the platform directory to the target's include paths. This keeps the source code cleaner and more consistent with the conventions of the projects mentioned in the PR description (like Godot or SDL), where platform-specific headers are typically resolved via include paths rather than hardcoded relative paths in the source.
target_include_directories(Main PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/source/Platform/Windows")
target_sources(Main PRIVATE
"${CMAKE_CURRENT_SOURCE_DIR}/source/Platform/Windows/resource.rc"
)
| #include "DSPlaySound.h" | ||
|
|
||
| #include "resource.h" | ||
| #include "Platform/Windows/resource.h" |
There was a problem hiding this comment.
If the platform-specific directory is added to the include path in CMakeLists.txt (as suggested in the corresponding review comment), you can continue using a simple include here. This avoids leaking the internal directory structure into the source code and makes future refactoring (like moving Winmain.cpp itself into the Platform/Windows directory) easier.
#include "resource.h"
Summary
src/source/Platform/Windows/(resource.h, resource.rc, resource.aps, icon1.ico, icon2.ico, and Global Release/resource.res)Winmain.cppinclude andsrc/CMakeLists.txtRC path to matchGlobal Release/folderSets up the conventional
Platform/<os>/layout used by Godot, Qt, SDL.Self-contained: 6 files renamed (history preserved via
git mv), 2 files updated.Refs sven-n#343.
Test plan
Unix Makefiles+mingw-w64-i686.cmaketoolchain) compiles RC from new path:Building RC object src/CMakeFiles/Main.dir/source/Platform/Windows/resource.rc.resMain.exelinks successfully