Haiku port: more modules#2358
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 5 medium |
🟢 Metrics 9 complexity · 0 duplication
Metric Results Complexity 9 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR introduces several Haiku-specific detection modules but is currently not up to standards due to high-severity logic and architectural issues. Most critical is the multiple instantiation of BApplication across modules, which will lead to process crashes since Haiku strictly enforces a single application instance per process.
Additionally, the wmtheme implementation contains a guaranteed memory leak and relies on unstable private headers. The boot manager detection is also quite limited, lacking support for EFI and 64-bit loaders which are standard in modern Haiku installations. These architectural and stability issues must be addressed before merging.
About this PR
- The wmtheme implementation uses a private Haiku header (<private/interface/DecorInfo.h>). This is unstable and may break in future OS updates without notice.
Test suggestions
- Missing: Verify ffDetectBootmgr identifies the bios_ia32 loader path when it exists
- Missing: Verify ffDetectBrightness correctly iterates through multiple screens and reports current brightness
- Missing: Verify ffDetectWmTheme retrieves the active decorator name via BApplication and DecorInfoUtility
- Missing: Verify ffDetectWmTheme gracefully handles null returns from CurrentDecorator()
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing: Verify ffDetectBootmgr identifies the bios_ia32 loader path when it exists
2. Missing: Verify ffDetectBrightness correctly iterates through multiple screens and reports current brightness
3. Missing: Verify ffDetectWmTheme retrieves the active decorator name via BApplication and DecorInfoUtility
4. Missing: Verify ffDetectWmTheme gracefully handles null returns from CurrentDecorator()
Low confidence findings
- The brightness implementation relies on the experimental Haiku API 'GetMonitorInfo' for vendor and model details, which may exhibit inconsistent behavior across different hardware.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| DecorInfoUtility* util = new DecorInfoUtility(); | ||
| DecorInfo* decor = NULL; | ||
|
|
||
| if (util) { | ||
| decor = util->CurrentDecorator(); | ||
| } | ||
|
|
||
| if (util == NULL || decor == NULL) { |
There was a problem hiding this comment.
🔴 HIGH RISK
Memory leak: 'util' is allocated via 'new' but never deleted. Use stack allocation to ensure proper cleanup and remove the redundant null check.
Try running the following prompt in your IDE agent:
In
src/detection/wmtheme/wmtheme_haiku.cpp, refactor theDecorInfoUtilityusage to use stack allocation instead of heap allocation (e.g.,DecorInfoUtility util;) and update the pointer access (->) to member access (.) to prevent a memory leak. Remove the redundantif (util)checks as well.
|
|
||
| const char* ffDetectBrightness(FF_A_UNUSED FFBrightnessOptions* options, FFlist* result) { | ||
| // We need a valid be_app to query the app_server here. | ||
| BApplication app("application/x-vnd.fastfetch-cli-fastfetch"); |
There was a problem hiding this comment.
🔴 HIGH RISK
Haiku only allows one BApplication per process. Multiple modules are creating their own instances, which will lead to a crash. Check if 'be_app' is already initialized.
Try running the following prompt in your coding agent:
Refactor the BApplication initialization in Haiku modules to check if be_app is already non-null before creating a new BApplication instance.
| FFBrightnessResult* brightness = FF_LIST_ADD(FFBrightnessResult, *result); | ||
|
|
||
| if (err == B_OK) { | ||
| ffStrbufInitF(&brightness->name, "%s %s (%ld)", monitor.vendor, monitor.name, screen.ID().id); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Potential format string mismatch for int32 on 64-bit systems. screen.ID().id returns an int32, but %ld is used.
| ffStrbufInitF(&brightness->name, "%s %s (%ld)", monitor.vendor, monitor.name, screen.ID().id); | |
| ffStrbufInitF(&brightness->name, "%s %s (%ld)", monitor.vendor, monitor.name, (long) screen.ID().id); |
| #include "common/strutil.h" | ||
| } | ||
|
|
||
| #include <Application.h> |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The std::nanf function requires the <cmath> header.
| #include <Application.h> | |
| #include <cmath> | |
| #include <Application.h> |
Summary
Adds bootmgr, brightness and wmtheme module implementations for Haiku.
Checklist