Variable framework

[magnesj]

PR Type

Enhancement

---

Description

• Implement value multiplexer framework for field synchronization

• Add variable mapping system for path management

• Create UI commands for multiplexer creation

• Integrate field change notification system

---

Diagram Walkthrough

flowchart LR
  A["Field Change"] --> B["PdmUiCommandSystemProxy"]
  B --> C["ValueMultiplexerCollection"]
  C --> D["Connected Fields"]
  D --> E["UI Update"]
  F["RimProject"] --> G["VariableMapper"]
  G --> H["Path Variables"]

Loading

File Walkthrough

	Relevant files
Enhancement	11 files RicCreateValueMultiplexerFeature.cpp Add command to create value multiplexers                                  +61/-0    RimContextCommandBuilder.cpp Register multiplexer creation command in context menu        +2/-0      RimProject.cpp Add multiplexer collection and path mapping functionality +158/-0  RimValueMultiplexer.cpp Implement value multiplexer for field synchronization        +180/-0  RimValueMultiplexerCollection.cpp Implement collection to manage value multiplexers                +173/-0  cafPdmUiCommandSystemProxy.cpp Add field change multiplexer interface support                      +32/-2    RicCreateValueMultiplexerFeature.h Header for value multiplexer creation command                        +33/-0    RimProject.h Add multiplexer collection and path mapping declarations  +4/-0      RimValueMultiplexer.h Header for value multiplexer class                                              +55/-0    RimValueMultiplexerCollection.h Header for multiplexer collection class                                    +54/-0    cafPdmUiCommandSystemProxy.h Add field change multiplexer interface declaration              +11/-1
Dependencies	1 files RimDepthSurface.cpp Add includes for multiplexer integration                                  +3/-0
Tests	4 files MainWindow.cpp Add variable server for testing framework                                +32/-1    VariableServer.cpp Implement test variable server and client classes                +526/-0  MainWindow.h Add variable server member declaration                                      +6/-0      VariableServer.h Header for test variable server classes                                    +131/-0
Configuration changes	3 files CMakeLists_files.cmake Add multiplexer command to build system                                    +10/-0    CMakeLists_files.cmake Add multiplexer classes to build system                                    +4/-0      CMakeLists.txt Add variable server to test build                                                +2/-0

---

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Behavioral Change Risk The change to suppress command-interface updates when connected fields exist alters undo/redo and notification flow. Verify that connected-field updates still trigger expected UI refreshes and history entries where required, and that mixed scenarios (both selection-related fields and connected fields) behave correctly. { if ( otherField != editorField ) { fieldsToUpdate.push_back( otherField ); } } } std::vector<caf::PdmFieldHandle*> connectedFields; if ( m_fieldChangedMultiplexer ) { connectedFields = m_fieldChangedMultiplexer->connectedFields( editorField ); for ( auto connectedField : connectedFields ) { if ( std::find( fieldsToUpdate.begin(), fieldsToUpdate.end(), connectedField ) == fieldsToUpdate.end() ) { fieldsToUpdate.push_back( connectedField ); } } } if ( m_commandInterface && !fieldsToUpdate.empty() && connectedFields.empty() ) { Performance Concern Options building scans all descendants and all fields on every options request. In large projects this can be expensive. Consider caching or filtering (e.g., by class/type) and deferring heavy scans. QList<caf::PdmOptionItemInfo> RimValueMultiplexer::calculateValueOptions( const caf::PdmFieldHandle* fieldNeedingOptions ) { QList<caf::PdmOptionItemInfo> options; if ( fieldNeedingOptions == &m_source || fieldNeedingOptions == &m_destination ) { auto root = RimProject::current(); if ( root ) { auto allObjects = root->descendantsOfType<caf::PdmObjectHandle>(); for ( auto objHandle : allObjects ) { auto obj = dynamic_cast<caf::PdmObject*>( objHandle ); if ( obj ) { QString text = obj->uiName() + " - " + obj->classKeyword(); options.push_back( caf::PdmOptionItemInfo( text, obj ) ); } } } } else if ( fieldNeedingOptions == &m_sourceFieldName ) { if ( m_source() ) { auto allFields = m_source()->fields(); for ( auto field : allFields ) { if ( auto valueField = dynamic_cast<caf::PdmValueField*>( field ) ) { options.push_back( caf::PdmOptionItemInfo( valueField->keyword(), valueField->keyword() ) ); } } } } else if ( fieldNeedingOptions == &m_destinationFieldName ) { if ( m_destination() ) { auto allFields = m_destination()->fields(); for ( auto field : allFields ) { if ( auto valueField = dynamic_cast<caf::PdmValueField*>( field ) ) { options.push_back( caf::PdmOptionItemInfo( valueField->keyword(), valueField->keyword() ) ); } } } } return options; } Robustness Path variable transfer/distribution performs string token parsing and replacements; edge cases with malformed tokens or unexpected formats may lead to incorrect names or resets. Add safeguards and logging when variables are not found or tokens are partial. void RimProject::transferPathsToGlobalPathList() { RiaVariableMapper variableMapper( m_globalPathList() ); for ( caf::FilePath* filePath : allFilePaths() ) { QString path = filePath->path(); if ( !path.isEmpty() ) { QString pathId = variableMapper.addPathAndGetId( path ); filePath->setPath( pathId ); } } for ( auto summaryCase : allSummaryCases() ) { if ( summaryCase->displayNameType() == RimCaseDisplayNameTools::DisplayName::CUSTOM ) { // At this point, after the replace of variables into caf::FilePath objects, the variable name is // stored in the summary case object. Read out the variable name and append "_name" for custom // summary variables. QString variableName = summaryCase->summaryHeaderFilename(); variableName = variableName.remove( RiaVariableMapper::variableToken() ); variableName = RiaVariableMapper::variableToken() + variableName + RiaVariableMapper::postfixName() + RiaVariableMapper::variableToken(); QString variableValue = summaryCase->displayCaseName(); variableMapper.addVariable( variableName, variableValue ); summaryCase->setCustomCaseName( variableName ); } } for ( auto gridCase : allGridCases() ) { if ( gridCase->displayNameType() == RimCaseDisplayNameTools::DisplayName::CUSTOM ) { // At this point, after the replace of variables into caf::FilePath objects, the variable name is // stored in the summary case object. Read out the variable name and append "_name" for custom // summary variables. QString variableName = gridCase->gridFileName(); variableName = variableName.remove( RiaVariableMapper::variableToken() ); variableName = RiaVariableMapper::variableToken() + variableName + RiaVariableMapper::postfixName() + RiaVariableMapper::variableToken(); QString variableValue = gridCase->caseUserDescription(); variableMapper.addVariable( variableName, variableValue ); gridCase->setCustomCaseName( variableName ); } } variableMapper.replaceVariablesInValues(); m_globalPathList = variableMapper.variableTableAsText(); } //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- QString RimProject::updatedFilePathFromPathId( QString filePath, RiaVariableMapper* pathListMapper /*=nullptr*/ ) const { std::unique_ptr<RiaVariableMapper> internalMapper; if ( pathListMapper == nullptr ) { internalMapper = std::make_unique<RiaVariableMapper>( m_globalPathList ); pathListMapper = internalMapper.get(); } QString returnValue = filePath; QString pathIdCandidate = filePath.trimmed(); QStringList pathIdComponents = pathIdCandidate.split( RiaVariableMapper::variableToken() ); if ( pathIdComponents.size() == 3 && pathIdComponents[0].size() == 0 && pathIdComponents[1].size() > 0 && pathIdComponents[2].size() == 0 ) { bool isFound = false; QString path = pathListMapper->valueForVariable( pathIdCandidate, &isFound ); if ( isFound ) { returnValue = path; } } return returnValue; } //-------------------------------------------------------------------------------------------------- /// //-------------------------------------------------------------------------------------------------- void RimProject::distributePathsFromGlobalPathList() { RiaVariableMapper pathListMapper( m_globalPathList() ); for ( caf::FilePath* filePath : allFilePaths() ) { filePath->setPath( updatedFilePathFromPathId( filePath->path(), &pathListMapper ) ); } for ( auto summaryCase : allSummaryCases() ) { if ( summaryCase->displayNameType() == RimCaseDisplayNameTools::DisplayName::CUSTOM ) { auto variableName = summaryCase->displayCaseName(); bool isFound = false; QString variableValue = pathListMapper.valueForVariable( variableName, &isFound ); if ( isFound ) { summaryCase->setCustomCaseName( variableValue ); } else if ( variableName.contains( RiaVariableMapper::postfixName() + RiaVariableMapper::variableToken() ) ) { // The variable name is not found in the variable list, but the name indicates a variable. Reset // to full case name. summaryCase->setDisplayNameOption( RimCaseDisplayNameTools::DisplayName::FULL_CASE_NAME ); } } } for ( auto gridCase : allGridCases() ) { if ( gridCase->displayNameType() == RimCaseDisplayNameTools::DisplayName::CUSTOM ) { auto variableName = gridCase->caseUserDescription(); bool isFound = false; QString variableValue = pathListMapper.valueForVariable( variableName, &isFound ); if ( isFound ) { gridCase->setCustomCaseName( variableValue ); } else if ( variableName.contains( RiaVariableMapper::postfixName() + RiaVariableMapper::variableToken() ) ) { // The variable name is not found in the variable list, but the name indicates a variable. Reset // to full case name. gridCase->setDisplayNameType( RimCaseDisplayNameTools::DisplayName::FULL_CASE_NAME ); } } } }

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Enable command path for multiplexed updates Connected fields are added to fieldsToUpdate but then the command path is skipped when any are present, breaking undo/redo for updates that include multiplexed fields. Allow the command to execute regardless, and ensure non-editable connected fields are filtered out before issuing the command. This preserves undo/redo while supporting multiplexing. Fwk/AppFwk/cafProjectDataModel/cafPdmUiCore/cafPdmUiCommandSystemProxy.cpp [122-140] std::vector<caf::PdmFieldHandle*> connectedFields; if ( m_fieldChangedMultiplexer ) { connectedFields = m_fieldChangedMultiplexer->connectedFields( editorField ); for ( auto connectedField : connectedFields ) { - if ( std::find( fieldsToUpdate.begin(), fieldsToUpdate.end(), connectedField ) == fieldsToUpdate.end() ) + if ( connectedField && std::find( fieldsToUpdate.begin(), fieldsToUpdate.end(), connectedField ) == fieldsToUpdate.end() ) { fieldsToUpdate.push_back( connectedField ); } } } -if ( m_commandInterface && !fieldsToUpdate.empty() && connectedFields.empty() ) +// Filter out non-writable fields to avoid issuing commands for read-only targets +fieldsToUpdate.erase( + std::remove_if(fieldsToUpdate.begin(), fieldsToUpdate.end(), + [&](caf::PdmFieldHandle* f){ return !m_commandInterface || !m_commandInterface->isFieldWritable(f); }), + fieldsToUpdate.end()); + +if ( m_commandInterface && !fieldsToUpdate.empty() ) { auto firstField = fieldsToUpdate.front(); if ( m_commandInterface->isFieldWritable( firstField ) ) { ... [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the condition && connectedFields.empty() breaks the undo/redo functionality for multiplexed fields and proposes a valid fix, which is a critical correction.	High
	Prevent cycles and ensure safe conversion This can recurse or thrash if destination is also multiplexed back to the source, and it lacks type checking. Add a guard to prevent cycles and only propagate when the value actually changes, converting types safely before setting. This avoids infinite loops and QVariant type mismatches. ApplicationLibCode/ProjectDataModel/Tools/RimValueMultiplexerCollection.cpp [123-141] void RimValueMultiplexerCollection::notifyFieldChanged( caf::PdmObject* source, const QString& fieldName, QVariant newValue ) { + // Simple cycle guard using a static thread_local flag + static thread_local bool inPropagation = false; + if ( inPropagation ) return; + inPropagation = true; + for ( auto multiplexer : m_valueMultiplexers ) { if ( multiplexer->source() == source && multiplexer->sourceFieldName() == fieldName ) { if ( auto destinationField = multiplexer->destinationField() ) { - auto fh = destinationField->uiCapability(); + QVariant old = destinationField->toQVariant(); - auto old = destinationField->toQVariant(); + // Skip if unchanged + if ( old == newValue ) + continue; - destinationField->setFromQVariant( newValue ); + // Attempt safe conversion to destination type + QVariant converted = newValue; + if ( converted.metaType() != old.metaType() ) + { + converted.convert( old.metaType() ); + } - fh->notifyFieldChanged( old, newValue ); + destinationField->setFromQVariant( converted ); + + if ( auto uiCap = destinationField->uiCapability() ) + { + uiCap->notifyFieldChanged( old, converted ); + } } } } + + inPropagation = false; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential for infinite recursion in notifyFieldChanged and proposes a valid cycle guard, which is a critical improvement for robustness.	Medium
General	Validate selection and field names Creating a multiplexer with empty field names will silently fail later. Validate that both objects are selected and prompt the user or abort if either field name is empty. At minimum, early-return when objects are null to avoid adding invalid entries. ApplicationLibCode/Commands/ToolsCommands/RicCreateValueMultiplexerFeature.cpp [33-52] -void RicCreateValueMultiplexerFeature::onActionTriggered( bool isChecked ) +void RicCreateValueMultiplexerFeature::onActionTriggered( bool /*isChecked*/ ) { - auto generalObjects = caf::selectedObjectsByType<caf::PdmObject*>(); - caf::PdmObject* firstObject = nullptr; - caf::PdmObject* secondObject = nullptr; + auto generalObjects = caf::selectedObjectsByType<caf::PdmObject*>(); + if ( generalObjects.size() < 2 ) + return; - if ( !generalObjects.empty() ) - { - firstObject = generalObjects.front(); - } + caf::PdmObject* firstObject = generalObjects.front(); + caf::PdmObject* secondObject = generalObjects.at( 1 ); + if ( !firstObject || !secondObject ) + return; - if ( generalObjects.size() > 1 ) - { - secondObject = generalObjects.at( 1 ); - } + // TODO: Acquire concrete field names via UI; for now, avoid creating invalid mappings + const QString firstFieldName; + const QString secondFieldName; + if ( firstFieldName.isEmpty() || secondFieldName.isEmpty() ) + return; auto multiplexerCollection = RimProject::current()->valueMultiplexerCollection(); - multiplexerCollection->addMultiplexer( firstObject, "", secondObject, "" ); - multiplexerCollection->updateAllRequiredEditors(); + if ( multiplexerCollection ) + { + multiplexerCollection->addMultiplexer( firstObject, firstFieldName, secondObject, secondFieldName ); + multiplexerCollection->updateAllRequiredEditors(); + } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that creating a multiplexer with null objects and empty field names is problematic, and the proposed validation improves the robustness of the feature.	Medium
More