Apply best practices across the project #84
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Apply comprehensive makefile improvements based on expert review: - Add shared warning flags via workspace/all/common/cflags.mk - Extract tool pak construction to scripts/construct-tool-paks.sh - Fix portable date command for timestamp generation (macOS compatibility) - Add missing .PHONY declarations to prevent conflicts - Fix ARCH variable misuse in desktop platform - Add hash.txt dependency to player build - Silence build output (75% reduction: 1889 → 475 lines) Build system improvements: - Change all platform makefiles to use $(MAKE) instead of bare make - Add @ prefix to silence command echo across 35+ makefiles - Fix jobserver warnings by removing -j from recursive make calls - Add MAKEFLAGS += --no-print-directory to workspace root - Silence third-party builds (SDL, cmake, DinguxCommander) - Consolidate warning flags to eliminate duplication Developer experience: - Add help target to root Makefile with categorized commands - Improve .DEFAULT error message with actionable guidance - Maintain full output in DEBUG builds for troubleshooting - Preserve compiler warnings/errors visibility
- Fix NULL dereference in getEmuName() after strchr - Fix buffer overflow in putInt() (8 -> 12 bytes) - Fix memory leak: free disc_paths in Menu_quit_ctx() - Fix backwards fread comparison in PlayerState_read() - Add strdup NULL check in player_menu.c - Improve allocFile() error handling for fseek/fread - Convert sprintf to snprintf throughout codebase - Add VALID_YEAR_MIN/MAX constants for date validation - Document GFX_scaleToFit() ownership semantics - Document INT_ARRAY_MAX constant (26 letters + #)
- Add test_helpers.h/c with centralized setUp/tearDown utilities - Add test_temp.h/c for automatic temp file management and cleanup - Add `make test-asan` for AddressSanitizer + UBSan testing - Update test_recent_writer, test_binary_file_utils, test_player_memory to use new helpers - Document new patterns in tests/README.md
Fixed coverage script paths (Unity framework location, test file paths, stb_ds includes). Added 25 tests for launcher_map.c functions that were previously untested (Map_load, Map_free, Map_loadForDirectory), improving coverage from 62% to 87%. Removed test_ui_layout.c fake tests - file contained a copied implementation of UI_initLayout that had drifted from real code in api.c (wrong PPI divisor, missing SCALE_MODIFIER/EDGE_PADDING, different algorithm). Replaced with placeholder documenting needed refactoring to extract UI_initLayout from api.c.
Extracted UI_initLayout() and UI_Layout struct from api.c into new
ui_layout.{c,h} module and added 23 comprehensive tests covering the
190-line DP scaling and row fitting algorithm.
Changes:
- Created ui_layout.h with UI_Layout struct and function declarations
- Created ui_layout.c with UI_initLayout implementation
- Updated api.{c,h} to use extracted module
- Updated 8 makefiles (launcher, player, dev, common build, test)
- Added 23 tests for PPI calculation, row fitting, derived sizes, edge
cases
- Updated CLAUDE.md documentation (total: 1470 tests across 46 suites)
- Added clarifying comments for DP macro sync and SCALE_MODIFIER testing
Test coverage:
- PPI and DP scale calculation for multiple devices
- Row fitting algorithm (even-pixel preference, no overlap validation)
- Derived size calculations (buttons, options, settings)
- Edge cases (small/large/portrait screens)
- Full integration tests (Miyoo Mini, Trimui Smart, RG35XX Plus)
Fixed my355 KEYMON configuration placed outside header guard (introduced in b91d342). Moved KEYMON section before #endif and added missing KEYMON_INPUT_COUNT. Standardized documentation across all 12 platform files: - Added comments documenting defaults vs overrides - Explained rationale for custom values (A7 timing, CPU scaling, screen size) - Consistent section ordering and structure - Set minimum EDGE_PADDING to 5 (zero28, magicmini) All platforms now explicitly document whether they use defaults or override them, making the configuration intent clear to future maintainers.
Converted all unsafe string functions to safe alternatives: - sprintf → snprintf (40 calls across 8 files) - strcat → snprintf (2 calls in ui_keyboard.c) - All conversions use sizeof() for correct buffer bounds
There was a problem hiding this comment.
Pull request overview
This PR applies comprehensive best practices across the LessUI project, focusing on build system standardization, code safety improvements, and test infrastructure enhancements. The changes affect 100+ files and include:
- Standardizing Makefile naming (lowercase
makefile→ capitalizedMakefile) - Replacing unsafe string functions (
sprintf→snprintf,strcat→snprintf) - Adding
@prefix to Makefile commands for cleaner build output - Refactoring UI layout code into separate module for better testability
- Improving test infrastructure with temp file management and helper utilities
Reviewed changes
Copilot reviewed 99 out of 152 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| workspace/*/Makefile | Standardized naming and added @ prefix for silent builds |
| workspace/*/platform/platform.h | Reorganized configuration sections, moved SCALE_MODIFIER/EDGE_PADDING |
| workspace/all/common/ui_layout.* | Extracted UI layout logic from api.c for testability |
| workspace/all//.c | Replaced sprintf with snprintf throughout |
| tests/support/test_temp.* | New temporary file/directory management for tests |
| tests/support/test_helpers.* | New centralized test setup/teardown utilities |
| scripts/*.sh | Updated to reference Makefile (capitalized) |
| docs/*.md | Updated documentation to reference Makefile |
The changes are well-structured and improve code safety, maintainability, and developer experience without altering functionality.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Eliminates hash.txt file-based approach in favor of exporting BUILD_HASH from host Makefile and passing it to Docker containers via -e flag. This fixes the issue where hash.txt would become stale after commits, requiring manual 'make setup' to regenerate. Now BUILD_HASH is always current, and 'make build PLATFORM=X' works standalone without setup.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR applies comprehensive best practices across the LessUI project, focusing on build system standardization, code safety improvements, and test infrastructure enhancements. The changes affect 100+ files and include: