feat(ui): cleaned uninstalled feature missing ini message, added "required-version" based on cs version#1153
Closed
davo0411 wants to merge 25 commits into
Closed
feat(ui): cleaned uninstalled feature missing ini message, added "required-version" based on cs version#1153davo0411 wants to merge 25 commits into
davo0411 wants to merge 25 commits into
Conversation
Contributor
## Walkthrough
This change refines feature version error handling by normalizing minimal required version strings for consistent messaging and adding detailed version info when feature INI files are missing. It alters UI behavior by changing the default of `DrawFailLoadMessage()` to false, updating `DrawUnloadedUI()` to show dynamic messages, and removing several feature-specific UI methods and failure message renderings.
## Changes
| File(s) | Change Summary |
|-------------------------------------------|----------------------------------------------------------------------------------------------------|
| src/Feature.cpp, src/Feature.h | Normalized minimal required version strings; changed default `DrawFailLoadMessage()` to false; updated error message construction and UI logic for unloaded features. |
| src/Features/TerrainHelper.cpp, .h | Removed `DrawUnloadedUI()` and `DrawFailLoadMessage()` methods and their declarations; updated feature summary with usage note. |
| src/Features/TerrainVariation.cpp, .h | Removed `DrawFailLoadMessage()` method and declaration; changed feature summary wording from "Reduces" to "Removes" terrain texture tiling. |
| src/Menu.cpp | Removed conditional UI rendering of feature failure messages in the feature configuration interface. |
| src/FeatureIssues.cpp | Applied version string cleaning utility to display version info in feature issue UI elements. |
| src/Utils/Format.cpp, .h | Added `Util::CleanVersionString()` function to remove trailing "-0" suffix from version strings. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Menu
participant Feature
User->>Menu: Open feature configuration
Menu->>Feature: Check if feature is loaded
alt Feature not loaded
Feature->>Feature: Get minimal required version (normalize string)
Feature-->>Menu: Return required version or "Unknown"
Menu->>User: Display detailed error message with required version info
else Feature loaded
Menu->>Feature: Render feature settings UI
Feature-->>Menu: UI elements
Menu->>User: Show settings
endPossibly related PRs
Poem
|
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Features/TerrainVariation.h (1)
26-32: Bullet wording contradicts the preceding descriptionLine 26 says the feature “reduces the repeating pattern”, while the new bullet (line 28) claims it “Removes terrain texture tiling”. “Removes” is a stronger statement than “reduces” and may set unrealistic expectations.
Consider making the wording consistent (e.g., “Reduces terrain texture tiling”) or adjusting the description above the bullet to match the stronger claim.
src/Feature.cpp (1)
110-114: Minor UX: “Version required: Unknown” may confuse usersIf the feature’s short-name is missing from
FEATURE_MINIMAL_VERSIONS,GetRequiredVersion()returns"Unknown", resulting in a message like
“… not installed! Version required: Unknown”.
That does not help users understand what they need to install.
- Detect the
"Unknown"sentinel and tailor the message, e.g.failedLoadedMessage = std::format( "The {} file is missing. This feature is not installed!", ini_filename);
- Alternatively, enrich
FEATURE_MINIMAL_VERSIONSso “unknown” never happens.src/Feature.h (1)
72-88: Guard UI calls when message/summary are empty
DrawUnloadedUI()now prints the description and key-feature list unconditionally when non-empty – good.
However,ImGui::Spacing()is always called after each block, which can leave trailing blank space if, say, there is no description but there are key features.Wrap each
ImGui::Spacing()inside the sameifthat prints the preceding block to avoid extra padding.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Feature.cpp(2 hunks)src/Feature.h(1 hunks)src/Features/TerrainHelper.cpp(0 hunks)src/Features/TerrainHelper.h(0 hunks)src/Features/TerrainVariation.cpp(0 hunks)src/Features/TerrainVariation.h(1 hunks)src/Menu.cpp(0 hunks)
💤 Files with no reviewable changes (4)
- src/Features/TerrainVariation.cpp
- src/Features/TerrainHelper.cpp
- src/Features/TerrainHelper.h
- src/Menu.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
|
✅ A pre-release build is available for this PR: |
davo0411
changed the title
davo0411
changed the title
alandtse
requested changes
Jun 16, 2025
Collaborator
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Removing a virtual function from feature.h needs to be justified. It's used by every feature.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
alandtse
requested changes
Jun 17, 2025
alandtse
requested changes
Jun 18, 2025
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Utils/Format.h (1)
9-10: Preferstd::string_view(and add brief doc comment) for zero-copy inputThe new helper doesn’t need ownership of the input; taking a
std::string_viewavoids unnecessary allocations when the caller already has a view or a temporary.
While touching the declaration, add a one-liner comment (as done forFixFilePath) to keep the public API self-documenting.- std::string CleanVersionString(const std::string& versionString); + /** + * @brief Strips trailing “-0” from semantic version strings (e.g. “1-2-3-0” → “1-2-3”). + */ + std::string CleanVersionString(std::string_view versionString);(Implementation change is trivial: accept
std::string_viewand construct the returnstd::stringat the end.)Please confirm no call-sites rely on mutating the argument; a
ripgrepforCleanVersionString(will show all usages.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Feature.cpp(7 hunks)src/Feature.h(3 hunks)src/FeatureIssues.cpp(4 hunks)src/Utils/Format.cpp(1 hunks)src/Utils/Format.h(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Utils/Format.cpp
- src/FeatureIssues.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Feature.h
- src/Feature.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
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.
Collaborator
Author
|
Pushed the function to Format.cpp/h. I think its been shrunk as much as it can. |
alandtse
requested changes
Jun 18, 2025
alandtse
requested changes
Jun 19, 2025
Collaborator
There was a problem hiding this comment.
Please walk through all edge cases in the Advanced Ini test section. Your edits will break proper detection of bad inis and resurface cleaned up items. Until you understand what this code is doing for each edge case, this PR won't work.
Perhaps you should consider scoping down to a refactor of the messages so you focus on the stuff that won't break the main logic,
|
|
||
| if (rc < 0) { | ||
| if (!FeatureIssues::IsObsoleteFeature(GetShortName())) | ||
| if (FeatureIssues::IsObsoleteFeature(GetShortName())) { |
Collaborator
There was a problem hiding this comment.
No. This doesn't make sense. If the file is deleted we don't have to talk about it.
| FeatureIssues::FeatureIssueInfo::IssueType::OBSOLETE, fileInfo, ""); | ||
| } else { | ||
| logger::info("{} failed to load, feature disabled", ini_filename); | ||
|
|
Collaborator
There was a problem hiding this comment.
Same here. I don't think you understand the code. The ini is missing because they didn't install or deleted it. Why is that an issue to raise?
Collaborator
Author
|
to simplify: Just use minimumVersionRequired in FeatureIssue.cpp and print that in the uninstall error. |
Collaborator
|
Perhaps. Honestly this must be redundant with feature issues. You can probably sync some language or something. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Cleans up the duplicate error messages when features are uninstalled or not loaded.
Adds a statement "Version required 1-0-0" (or whatever version).
This indicates to users who might not have the latest base cs the specific version they should download to avoid conflicts.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style