fix(ui): distinguish missing vs pending features#1188
Conversation
WalkthroughThis change centralizes feature version validation and formatting in utility methods, updates error and UI messages to include required version information, and introduces a pure virtual Changes
Sequence Diagram(s)sequenceDiagram
participant Menu
participant Feature
participant FileSystem
Menu->>FileSystem: Check if feature INI file exists
alt INI file missing
Menu->>Feature: DrawUnloadedUI()
else INI file present
Menu->>Menu: Show "Pending restart" message
end
sequenceDiagram
participant Feature
participant FeatureVersions
Feature->>Feature: IsFeatureKnown(shortName)
alt Feature known
Feature->>Feature: GetFeatureRequiredVersion(shortName)
Feature->>Feature: Use required version in error/UI messages
else Feature unknown
Feature->>Feature: Use "unknown" in error/UI messages
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Feature.cpp(5 hunks)src/Feature.h(2 hunks)src/Features/TerrainHelper.cpp(1 hunks)src/Features/TerrainHelper.h(0 hunks)src/Features/TerrainVariation.cpp(1 hunks)src/Features/TerrainVariation.h(1 hunks)src/Features/WetnessEffects.cpp(0 hunks)src/Features/WetnessEffects.h(0 hunks)src/Menu.cpp(6 hunks)
💤 Files with no reviewable changes (3)
- src/Features/WetnessEffects.h
- src/Features/WetnessEffects.cpp
- src/Features/TerrainHelper.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (15)
src/Features/TerrainHelper.cpp (1)
169-169: LGTM: UI method removal aligns with centralization pattern.The removal of the
DrawUnloadedUI()method is consistent with the broader refactoring to centralize unloaded feature UI handling in the baseFeatureclass. This improves maintainability and reduces code duplication.src/Features/TerrainVariation.cpp (1)
28-29: Verify the stronger claim about pattern removal.The tooltip text was updated from "Reduces" to "Removes" the repeating pattern effect, which makes a stronger claim about the feature's effectiveness. Please ensure this accurately reflects the actual capability of the Terrain Variation feature.
src/Features/TerrainVariation.h (1)
26-32: Improved feature summary with clearer benefits.The updated feature summary provides more specific and compelling benefits:
- Changed from "reduces" to "removes" for stronger positioning
- Added specific bullet points about comprehensive fix and compatibility
- Better communicates the feature's value to users
The messaging is consistent with the tooltip changes in the implementation file.
src/Feature.h (2)
3-5: Good addition of utility includes for centralized functionality.Adding includes for
FeatureVersions.handUtils/Format.hsupports the centralized version formatting and error handling improvements.
78-119: Excellent improvement to error messaging and user guidance.The enhanced
DrawUnloadedUI()method provides significant improvements:
- Prioritized error handling: Detailed failure messages are shown first when available
- Informative missing file messages: Include specific INI filename and required version
- Centralized version formatting: Uses
Util::GetFormattedVersionfor consistency- Better user guidance: Clear indication of what's missing and what version is needed
- Maintained feature information: Still shows feature summary and key benefits
The logic flow is well-structured and the error messages are actionable for users.
src/Feature.cpp (5)
29-29: Good addition of centralized formatting utility.Including
Utils/Format.henables the use ofUtil::GetFormattedVersionfor consistent version string formatting throughout the codebase.
86-86: Excellent use of centralized version formatting.Replacing manual version string formatting with
Util::GetFormattedVersionimproves consistency and maintainability across the codebase.
108-118: Enhanced error messaging with version requirements.The improved error handling now includes the required version information in missing file messages, making it easier for users to understand what they need to install. The centralized version formatting ensures consistency.
135-135: Consistent version formatting for feature issues.Using
Util::GetFormattedVersionhere maintains consistency with other version formatting throughout the error handling system.
229-229: Verify removal of ExtendedTranslucency feature.The feature list appears to have been modified (removal of ExtendedTranslucency based on the AI summary). Please confirm this change is intentional and that all references to the removed feature have been properly cleaned up.
#!/bin/bash # Description: Verify that ExtendedTranslucency references have been cleaned up # Expected: No remaining references to ExtendedTranslucency in the codebase echo "Searching for ExtendedTranslucency references..." rg -i "extendedtranslucency" --type cpp --type h echo "Searching for include statements..." rg -i "ExtendedTranslucency\.h" --type cpp --type h echo "Searching for global variable references..." rg -i "extendedTranslucency" --type cpp --type hsrc/Menu.cpp (5)
2-2: LGTM: Filesystem include added for file existence checking.The
<filesystem>include is appropriately added to support the newIsFeatureInstalledhelper function.
17-17: LGTM: Additional includes for enhanced functionality.The new includes for
FeatureVersions.handUtils/Format.hsupport the improved version handling and formatting capabilities mentioned in the PR objectives.Also applies to: 24-24
759-767: LGTM: Correct implementation of feature status logic.This change properly addresses the PR objective by distinguishing between missing features (INI file doesn't exist) and features pending restart (INI file exists but not loaded). The color coding logic is appropriate:
- Grey for missing features
- Green for features pending restart
913-923: LGTM: Consistent status display logic in About tab.The About tab now correctly mirrors the feature list logic, showing appropriate status messages and colors based on actual installation state rather than just load state.
952-962: LGTM: Improved information messages for unloaded features.The conditional messaging properly directs users to the Settings tab for installation details when features are missing, while showing a simple "pending restart" message when features are installed but not loaded. This eliminates the confusion mentioned in the PR objectives.
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
@coderabbitai please suggest an appropriate conventional commit title within github length limits. |
|
Tip For best results, initiate chat on the files or code changes. @alandtse Here are a few conventional commit title options within GitHub's length limits: Recommended: (67 characters) Alternative options: (62 characters) (68 characters) The first option best captures the key user-facing improvements: properly distinguishing between features that are missing (no INI file) versus those pending restart (INI file present), and fixing the duplicate uninstalled text issues that were mentioned in the original PR description. |
alandtse
changed the title
2 participants
UI Fixes for the following:
Before:
After:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes