chore(cpn): application settings fixes housekeeping and refactoring#7362
Open
elecpower wants to merge 17 commits into
Open
chore(cpn): application settings fixes housekeeping and refactoring#7362elecpower wants to merge 17 commits into
elecpower wants to merge 17 commits into
Conversation
elecpower
added
companion
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoves component release metadata into per-profile ComponentReleaseData objects, adds Profile::compRelease with accessors, wires AppData to initialize/reset/store per-profile releases, updates CompStoreObj grouping/registrations, and refactors UpdateInterface to use the current profile's release entries. ChangesRelease Metadata Per-Profile Storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@companion/src/storage/appdata.cpp`:
- Around line 302-305: The constructor
ComponentReleaseData::ComponentReleaseData() calls
CompStoreObj::addObjectMapping(propertyGroup(), this) too early (propertyGroup()
reads profileIndex/index before it's initialized) and registers a stale mapping;
remove that call from the ctor and instead invoke
addObjectMapping(propertyGroup(), this) from setIndexes(...) immediately after
assigning the real profile id (or otherwise after profileIndex/index is
initialized) so the mapping is registered with the correct id; alternatively, if
you prefer to keep registration in ctor, ensure profileIndex/index is fully
initialized prior to calling propertyGroup() but the preferred change is to move
the addObjectMapping invocation into setIndexes.
- Around line 962-967: The reset path no longer clears moved release state
because resetUpdatesSettings() (the code block that calls
component[i].resetAll()) only clears component-level data; you must also clear
the per-profile last-known release stored in profile[].compRelease[] to avoid
stale update status. Update resetUpdatesSettings() to iterate over all profiles
and for each profile set compRelease[k] to a neutral/empty value (or call an
existing profile method that resets component release state) for k in
0..MAX_COMPONENTS-1 after/alongside component[i].resetAll(), and ensure any
related moved-release flags in profile or component (e.g., compRelease,
movedRelease flags) are cleared as well.
In `@companion/src/storage/appdata.h`:
- Around line 498-501: Profile's custom copy/assignment currently omits the new
compRelease[MAX_COMPONENTS] member so copied/assigned Profile objects lose
per-component release state; update Profile's copy constructor and operator=
(and any clone/assign helpers) to copy the entire compRelease array (e.g., copy
each ComponentReleaseData in compRelease or use std::copy) and ensure
getCompRelease semantics remain consistent so reordering and other copy paths
preserve per-component release state.
- Around line 462-488: The refactor moved persisted fields (release, releaseId,
version, prerelease, date) from the old "Components/component%1/*" path into
ComponentReleaseData's new propertyGroup()/settingsPath() under
"Profiles/profile%1/component%2/"; add a one-time migration that runs when the
stored settings version is bumped: locate where settings are loaded (e.g.
AppData initialization or Profile load) and for each profile/component attempt
to read legacy keys from
"Components/component%1/<release|releaseId|version|prerelease|date>" and, if
present, write them into the new ComponentReleaseData setters (or new settings
path) and remove/mark the old keys migrated; also increment the settings-version
constant so the migration executes exactly once.
- Around line 474-475: ComponentReleaseData now stores keys under
"Profiles/profile%1/component%2/" but CompStoreObj's validation only checks the
first path segment so keys like release/version/releaseId are not recognized;
update the CompStoreObj validation method (the is-known-key / validation
routine) to resolve nested settings paths by either stripping leading
"Profiles/profile<index>/" or by splitting the settings path and checking the
final segment(s) against known keys, so keys stored by
propertyGroup()/settingsPath() (the inline methods propertyGroup() and
settingsPath() in appdata.h) are correctly recognized during validation and
export/import cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4aec7d0-e793-4b57-96fc-3df176592ed4
📒 Files selected for processing (3)
companion/src/storage/appdata.cppcompanion/src/storage/appdata.hcompanion/src/updates/updateinterface.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
companion/src/storage/appdata.cpp (1)
944-961:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
resetUpdatesSettings()does not reset the per-profile release state.After refactoring, the last-known release/version data now lives in
profile[*].compRelease[*], but this function only resetscomponent[i]and its assets. Users who reset update settings will retain stale release tracking data.🔧 Suggested fix
void AppData::resetUpdatesSettings() { updateCheckFreqReset(); downloadDirReset(); decompressDirReset(); decompressDirUseDwnldReset(); updateDirReset(); updateDirUseSDReset(); updDelDownloadsReset(); updLogLevelReset(); for (int i = 0; i < MAX_COMPONENTS; i++) { component[i].resetAll(); for (int j = 0; j < MAX_COMPONENT_ASSETS; j++) component[i].asset[j].resetAll(); } + + for (int i = 0; i < MAX_PROFILES; i++) { + for (int j = 0; j < MAX_COMPONENTS; j++) + profile[i].compRelease[j].resetAll(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/storage/appdata.cpp` around lines 944 - 961, resetUpdatesSettings() currently resets component[] and asset[] state but misses clearing per-profile release/version tracking stored in profile[].compRelease[], leaving stale release data after a reset; update resetUpdatesSettings() to iterate over all profiles and all components and call/reset each profile[p].compRelease[c] (or invoke a provided reset method on compRelease entries) so every profile's per-component release state is cleared alongside component[i].resetAll(), referencing profile and compRelease to locate the fields to clear.companion/src/storage/appdata.h (1)
503-506:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Profilecopy semantics do not includecompReleasearray.
Profile::operator=(inappdata.cpp) only copiesQ_PROPERTYmembers via Qt's meta-object system. ThecompRelease[MAX_COMPONENTS]array is a regular member and won't be copied when profiles are assigned or reordered (e.g., inmoveCurrentProfileToTop()).🔧 Suggested fix in Profile::operator= (appdata.cpp)
Profile & Profile::operator= (const Profile & rhs) { for (int i = metaObject()->propertyOffset(), e = metaObject()->propertyCount(); i < e; ++i) { const QMetaProperty & prop = metaObject()->property(i); if (!prop.isValid() || !prop.isWritable()) { qWarning() << "Could not copy property" << QString(prop.name()) << "isValid:" << prop.isValid() << "isWritable:" << prop.isWritable(); continue; } prop.write(this, prop.read(&rhs)); } + for (int i = 0; i < MAX_COMPONENTS; ++i) { + compRelease[i] = rhs.compRelease[i]; + } return *this; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/storage/appdata.h` around lines 503 - 506, Profile::operator= currently only copies Q_PROPERTY members via Qt meta-object reflection and therefore omits the regular member array compRelease[MAX_COMPONENTS], causing compRelease to be lost on assignment or when reordering profiles (e.g., moveCurrentProfileToTop()). Fix operator= in appdata.cpp (the Profile::operator= implementation) to explicitly copy the compRelease array into the destination Profile — e.g., use std::copy or a loop to copy MAX_COMPONENTS entries of ComponentReleaseData from the source to the target — so that compRelease and any data returned by getCompRelease(int) are preserved during assignment and moves.
🧹 Nitpick comments (1)
companion/src/storage/appdata.cpp (1)
296-300: 💤 Low value
profileIndexis not initialized in the constructor.While
indexis initialized to-1,profileIndexis left uninitialized. This is safe becausesetIndexes()is called before use, but explicit initialization improves defensive coding and prevents potential issues if usage patterns change.♻️ Suggested improvement
-ComponentReleaseData::ComponentReleaseData() : CompStoreObj(), index(-1) +ComponentReleaseData::ComponentReleaseData() : CompStoreObj(), profileIndex(-1), index(-1) { }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/storage/appdata.cpp` around lines 296 - 300, The ComponentReleaseData constructor leaves profileIndex uninitialized; update ComponentReleaseData::ComponentReleaseData() to initialize profileIndex (e.g., to -1) alongside index to make the object defensively initialized; you can verify usage via setIndexes() but ensure the constructor sets profileIndex to a safe default to prevent potential future UB.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@companion/src/storage/appdata.cpp`:
- Around line 205-211: splitGroupedPath is using list.size() - 2 to derive the
group which breaks 5+ segment paths (e.g.,
NamedJSData/0/JsCalibration/1/stick_axe); change the logic so that for paths
with 5 or more segments you return the first two segments as the group (join
list.mid(0,2)), otherwise keep the existing behavior (for size>1 return
list.mid(0, list.size()-2).join("/"), fallback "General"); update
CompStoreObj::splitGroupedPath so
propertyPathIsValidNonDefault()/clearUnusedSettings()/exportSettings see the
correct registered group for nested NamedJSData paths.
In `@companion/src/storage/appdata.h`:
- Line 467: Implement the missing copy assignment for ComponentReleaseData in
appdata.cpp by defining ComponentReleaseData::operator=(const
ComponentReleaseData &rhs) to copy all Qt properties using
metaObject()->propertyOffset()/propertyCount() and QMetaProperty::read/write,
logging a warning if a property is invalid or not writable; return *this at the
end. Place this implementation alongside the ComponentReleaseData constructor in
appdata.cpp so copying compRelease data links correctly.
---
Duplicate comments:
In `@companion/src/storage/appdata.cpp`:
- Around line 944-961: resetUpdatesSettings() currently resets component[] and
asset[] state but misses clearing per-profile release/version tracking stored in
profile[].compRelease[], leaving stale release data after a reset; update
resetUpdatesSettings() to iterate over all profiles and all components and
call/reset each profile[p].compRelease[c] (or invoke a provided reset method on
compRelease entries) so every profile's per-component release state is cleared
alongside component[i].resetAll(), referencing profile and compRelease to locate
the fields to clear.
In `@companion/src/storage/appdata.h`:
- Around line 503-506: Profile::operator= currently only copies Q_PROPERTY
members via Qt meta-object reflection and therefore omits the regular member
array compRelease[MAX_COMPONENTS], causing compRelease to be lost on assignment
or when reordering profiles (e.g., moveCurrentProfileToTop()). Fix operator= in
appdata.cpp (the Profile::operator= implementation) to explicitly copy the
compRelease array into the destination Profile — e.g., use std::copy or a loop
to copy MAX_COMPONENTS entries of ComponentReleaseData from the source to the
target — so that compRelease and any data returned by getCompRelease(int) are
preserved during assignment and moves.
---
Nitpick comments:
In `@companion/src/storage/appdata.cpp`:
- Around line 296-300: The ComponentReleaseData constructor leaves profileIndex
uninitialized; update ComponentReleaseData::ComponentReleaseData() to initialize
profileIndex (e.g., to -1) alongside index to make the object defensively
initialized; you can verify usage via setIndexes() but ensure the constructor
sets profileIndex to a safe default to prevent potential future UB.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 764b5e3d-b336-41a0-8ea4-22fd2aaa6933
📒 Files selected for processing (2)
companion/src/storage/appdata.cppcompanion/src/storage/appdata.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@companion/src/storage/appdata.cpp`:
- Around line 981-984: The loop is using MAX_COMPONENT_ASSETS to clear
profile[i].compRelease but compRelease is a component-sized array elsewhere;
change the inner loop to iterate MAX_COMPONENTS instead of MAX_COMPONENT_ASSETS
so that profile[i].compRelease[j].resetAll() covers the actual compRelease
bounds (update the loop that references MAX_COMPONENT_ASSETS, keeping the call
to resetAll() on profile[i].compRelease[j]).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1225e6e-82e3-4bf3-8e74-d018490bd5d9
📒 Files selected for processing (1)
companion/src/storage/appdata.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
companion/src/storage/appdata.cpp (2)
242-242: 💤 Low valueRemove debug logging before merge.
This
qDebug()statement will emit logging output during normal operation whenkeyToNameMap()is first accessed.🧹 Suggested fix
map.insert(it.key(), grpMap); - qDebug() << it.key() << grpMap; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/storage/appdata.cpp` at line 242, Remove the stray debug output by deleting or guarding the qDebug() << it.key() << grpMap; call inside keyToNameMap() (or the function that iterates with variable it and grpMap) so normal operation does not emit logs; either remove the statement entirely or wrap it in a debug-only guard (e.g., `#ifdef` QT_DEBUG) to ensure it only runs in debug builds.
925-932: 💤 Low valueRemove debug logging from
exportSettings.These
qDebug()statements will produce verbose output during normal export operations.🧹 Suggested fix
foreach (const QString & key, m_settings.allKeys()) { const QVariant newVal = m_settings.value(key); - qDebug() << key << newVal; // Skip export if property does not exist or is the default value. if (newVal.isValid() && CompStoreObj::propertyPathIsValidNonDefault(key, newVal)) toSettings->setValue(key, newVal); - else qDebug() << "SKIPPING:" << key << newVal; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@companion/src/storage/appdata.cpp` around lines 925 - 932, Remove the temporary debug logging inside the settings export loop: delete the two qDebug() calls so exportSettings no longer prints every key/value or "SKIPPING" lines; keep the logic that checks m_settings.allKeys(), uses m_settings.value(key), calls CompStoreObj::propertyPathIsValidNonDefault(key, newVal), and writes to toSettings->setValue(key, newVal) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@companion/src/storage/appdata.cpp`:
- Around line 498-503: The code is mistakenly writing stick_min twice and never
saving the inversion flag; in the block that assigns joystick fields (the
snippet updating namedJS[i].joystick[j]) replace the duplicated
namedJS[i].joystick[j].stick_min(joystick[j].stick_min()) with
namedJS[i].joystick[j].stick_inv(joystick[j].stick_inv()); do the same
corrective change in loadNamedJS where stick_min is duplicated (replace the
second stick_min assignment with stick_inv) so stick_inv is persisted and
restored correctly.
---
Nitpick comments:
In `@companion/src/storage/appdata.cpp`:
- Line 242: Remove the stray debug output by deleting or guarding the qDebug()
<< it.key() << grpMap; call inside keyToNameMap() (or the function that iterates
with variable it and grpMap) so normal operation does not emit logs; either
remove the statement entirely or wrap it in a debug-only guard (e.g., `#ifdef`
QT_DEBUG) to ensure it only runs in debug builds.
- Around line 925-932: Remove the temporary debug logging inside the settings
export loop: delete the two qDebug() calls so exportSettings no longer prints
every key/value or "SKIPPING" lines; keep the logic that checks
m_settings.allKeys(), uses m_settings.value(key), calls
CompStoreObj::propertyPathIsValidNonDefault(key, newVal), and writes to
toSettings->setValue(key, newVal) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1752a57d-cf0c-46b2-b7b8-6e4eec80552d
📒 Files selected for processing (2)
companion/src/storage/appdata.cppcompanion/src/storage/appdata.h
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Unit tests committed locally. Commit: |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #7367 |
elecpower
changed the title
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
1 participant
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.
Fixes #7360
Summary of changes:
Summary by CodeRabbit