fix: Windows cross-platform compatibility and bug fixes#91
fix: Windows cross-platform compatibility and bug fixes#91seventhback777 wants to merge 3 commits into
Conversation
- Database.h: add stub implementations so the project compiles on Windows (SplashKit database API is missing on Windows; TODO: replace with raw SQLite3) - Rating.h: fix rating lower boundary to start from 1 instead of 0 - Helper.h: prevent double-slash in path concatenation; add early return when games directory does not exist - Configuration.h: add ROWS/COLS grid layout macros - ConfigData.h/cpp: change setFolder() from inline to proper declaration/definition; add missing <cstring> include for strcpy - resources/bundles/resources.txt: remove extra spaces in MUSIC resource entries - .gitignore: fix games/ directory pattern (was games/*)
kottochii
left a comment
kottochii
left a comment
There was a problem hiding this comment.
Recommended changes in multiple positions.
There was a problem hiding this comment.
As changed in #87, dependency on this function has been removed, so there is no need to add this.
There was a problem hiding this comment.
There is no particular need to override 0 stars to max rating.
| system(("git clone " + url + " " + dir).c_str()); | ||
| } | ||
| else if (info.st_mode &S_IFDIR) | ||
| bool isPull = (stat(dir, &info) == 0 && (info.st_mode & S_IFDIR)); |
There was a problem hiding this comment.
This will still allow the program to run git commands, even if the contents of dir are not a git repository, or dir is a file or unwritable.
There was a problem hiding this comment.
You can also use fs from the previous function here, rather than using C API.
| return true; | ||
| // Verify the directory actually exists after the git operation | ||
| struct stat checkInfo; | ||
| bool success = (stat(dir, &checkInfo) == 0 && (checkInfo.st_mode & S_IFDIR)); |
There was a problem hiding this comment.
This will show success regardless of the success of the git operation, because it just checks that dir is a directory after the operation.
There was a problem hiding this comment.
Exact same macros are defined in ArcadeMachine.h, and these don't seem to be used. In case of including both this file and the ArcadeMachine.h, you will have a conflict. These have to be either wrapped in #ifndef or completely removed from the file.
- Replace C stat() calls with fs:: in getFromGit() for consistency - Check for .git directory to confirm dir is a valid git repo before pull - Capture system() return code to properly detect git operation success - Remove duplicate ROWS/COLS macros from Configuration.h (already in ArcadeMachine.h)
RealH4D35
left a comment
RealH4D35
left a comment
There was a problem hiding this comment.
Review
Tested on: Arch Linux / MinGW
Critical issues
Button.cppdoes not compile
center_point(this->m_btn)is invalid (sprite ≠ circle).
Fix: replace withcenter_point(sprite_circle(this->m_btn)).
- Database stubs break all platforms
#if 1ininclude/Database.hactivates empty stubs everywhere.
Fix: change to#ifdef _WIN32OR merge PR #87 (rewrites DB with raw SQLite) and then drop the stubs entirely.
Minor
GridLayout.cpp:96– unused variableindex(warning).
Works well
- Path handling, game config scanning, and logging improvements.
Recommendation: Fix the Button error, and either fix the stub guard or merge PR #87 first (preferred).
kottochii
left a comment
kottochii
left a comment
There was a problem hiding this comment.
This would fail to compile because ROWS and COLS (defined in ArcadeMachine.h) are not included. You can either include that file or include COLS and ROWS from some other common file.
Also, as outlined in the comments, exit codes have to be handled by special macros, if running under non-Windows (even though that perfectly works for Windows).
The other comments from my former review still stand.
| // dir exists -- pull instead | ||
| system(("git -C " + std::string(dir) + " pull " + url).c_str()); | ||
| std::cerr << "[Git] pulling: " << url << " → " << dir << std::endl; | ||
| exitCode = system(("git -C " + std::string(dir) + " pull " + url).c_str()); |
There was a problem hiding this comment.
This will not work on linux (even though it perfectly does on Windows). You will have to wrap it in the special macros to get the actual exit code. Please see system manual entry on linux:
the return value is a "wait status" that can be examined using the macros described in waitpid(2). (i.e., WIFEXITED(), WEXITSTATUS(), and so on)
| // dir does not exist -- clone from scratch | ||
| system(("git clone " + url + " " + dir).c_str()); | ||
| std::cerr << "[Git] cloning: " << url << " → " << dir << std::endl; | ||
| exitCode = system(("git clone " + url + " " + dir).c_str()); |
There was a problem hiding this comment.
Similarly to my comment on 197, pointing out that this is not going to work on linux.
- Configuration.h: re-add ROWS/COLS with #ifndef guards to prevent redefinition conflict when ArcadeMachine.h is also included - ConfigData.cpp: fix system() exit code extraction on Linux by using WIFEXITED()/WEXITSTATUS() macros from sys/wait.h; Windows path uses exitCode == 0 directly (no wait status wrapping)
3 participants
Summary
Database.h: add stub implementations so the project compiles on Windows (SplashKit database API unavailable on Windows; TODO: replace with raw SQLite3)Rating.h: fix rating lower boundary to start from 1 instead of 0Helper.h: prevent double-slash in path concatenation; add early return when games directory does not existConfiguration.h: addROWS/COLSgrid layout macrosConfigData.h/cpp: changesetFolder()from inline to proper declaration/definition; add missing<cstring>include forstrcpyresources/bundles/resources.txt: remove extra spaces in MUSIC resource entries.gitignore: fixgames/directory patternTest plan