Skip to content

Replaces push button editors with dynamic buttons#619

Open
magnesj wants to merge 1 commit intodevfrom
use-dynamic-push-button-02
Open

Replaces push button editors with dynamic buttons#619
magnesj wants to merge 1 commit intodevfrom
use-dynamic-push-button-02

Conversation

@magnesj
Copy link
Owner

@magnesj magnesj commented Jan 23, 2026

User description

Replaces usage of the cafPdmUiPushButtonEditor with dynamic buttons created directly in the UI ordering. This simplifies the code and allows for more flexible button behavior.
The change improves the user interface by providing more context-aware button labels and actions.

Removes the following files as a result of the change:

  • cafPdmUiPushButtonEditor.h

PR Type

Enhancement


Description

  • Replace cafPdmUiPushButtonEditor with dynamic buttons in UI ordering

  • Move button logic from fieldChangedByUi to lambda callbacks in defineUiOrdering

  • Convert boolean PDM fields to local member variables where appropriate

  • Extract complex button handlers into dedicated private methods


Diagram Walkthrough

flowchart LR
  A["cafPdmUiPushButtonEditor<br/>Fields & Handlers"] -->|Remove| B["Boolean PDM Fields"]
  A -->|Replace with| C["Dynamic Buttons<br/>addNewButton"]
  B -->|Convert to| D["Local Member<br/>Variables"]
  C -->|Inline Logic| E["Lambda Callbacks<br/>in defineUiOrdering"]
  C -->|Extract Logic| F["Private Methods<br/>for Complex Handlers"]
  E --> G["Simplified UI Code"]
  F --> G
Loading

File Walkthrough

Relevant files
Enhancement
37 files
RimFlowCharacteristicsPlot.cpp
Replace push button fields with dynamic buttons                   
+79/-104
RimEnsembleCurveSet.cpp
Replace push button editors with dynamic buttons                 
+85/-88 
RimTernaryLegendConfig.cpp
Replace three push button fields with dynamic buttons       
+30/-74 
RimGeoMechCase.cpp
Replace element property file command buttons                       
+39/-59 
RimWellConnectivityTable.cpp
Replace time step and tracer selection buttons                     
+4/-34   
RimCustomObjectiveFunctionWeight.cpp
Replace objective function address selection button           
+36/-46 
RimStatisticsContourMap.cpp
Replace compute statistics push button with dynamic button
+34/-51 
RimCloudDataSourceCollection.cpp
Replace authentication and data source buttons                     
+14/-55 
RimTextAnnotation.cpp
Replace pick point button fields with local variables       
+24/-33 
RimAbstractCorrelationPlot.cpp
Replace summary address selection push button                       
+35/-44 
RimWellTargetMapping.cpp
Replace generate and reset default buttons                             
+32/-61 
RimEnsembleCurveFilter.cpp
Replace objective function address selection button           
+38/-46 
RimGridCalculation.cpp
Replace edit result address push button                                   
+17/-28 
RimExtrudedCurveIntersection.cpp
Replace edit polygon push button with dynamic button         
+3/-21   
RimWellAllocationOverTimePlot.cpp
Replace apply time step selections button                               
+1/-16   
RimRegularLegendConfig.cpp
Replace reset user defined values button                                 
+6/-24   
RimEclipseStatisticsCase.cpp
Replace compute/edit statistics button                                     
+13/-28 
RimReachCircleAnnotation.cpp
Replace center point pick button with local variable         
+11/-24 
RimPolygonFilter.cpp
Replace edit polygon push button                                                 
+3/-22   
RimPolygon.cpp
Replace edit polygon button with dynamic button                   
+9/-34   
RimWellPathGeometryDef.cpp
Replace pick points button with local variable                     
+9/-26   
RimUserDefinedCalculation.cpp
Replace help button with dynamic button                                   
+6/-23   
RimGeoMechFaultReactivationResult.cpp
Replace create fault reactivation plot button                       
+6/-27   
RimFractureTemplate.cpp
Replace scale apply button with dynamic button                     
+1/-14   
RimDeltaSummaryEnsemble.cpp
Replace swap ensembles button with dynamic button               
+7/-24   
RimPolygonInView.cpp
Replace select polygon button with dynamic button               
+1/-18   
RimEnsembleCurveFilterCollection.cpp
Replace new filter button with dynamic button                       
+8/-30   
RimEllipseFractureTemplate.cpp
Remove scale apply button field reference                               
+1/-3     
RimFlowCharacteristicsPlot.h
Add private method declaration for show region                     
+1/-2     
RimEnsembleCurveSet.h
Add private method declarations for address selection       
+3/-2     
RimGridCalculation.h
Add private method for edit result address button               
+1/-1     
RimExtrudedCurveIntersection.h
Add cafPdmUiPushButtonEditor include for compatibility     
+1/-1     
RimCustomObjectiveFunctionWeight.h
Add method declaration and remove button field                     
+1/-1     
RimEnsembleCurveFilter.h
Add method declaration for address selection                         
+1/-1     
RimTextAnnotation.h
Convert button fields to local member variables                   
+2/-2     
RimReachCircleAnnotation.h
Convert button field to local member variable                       
+1/-1     
RimWellPathGeometryDef.h
Convert button field to local member variable                       
+1/-1     
Additional files
28 files
RimPlotDataFilterItem.cpp +0/-1     
RimPolygonFilter.h +0/-1     
RimCloudDataSourceCollection.h +0/-3     
RimFractureTemplate.h +0/-1     
RimMeshFractureTemplate.cpp +0/-6     
RimStatisticsContourMap.h +0/-1     
RimAbstractCorrelationPlot.h +0/-1     
RimCorrelationMatrixPlot.cpp +0/-5     
RimWellAllocationOverTimePlot.h +0/-1     
RimWellConnectivityTable.h +0/-4     
RimGeoMechCase.h +0/-3     
RimGeoMechFaultReactivationResult.h +0/-3     
RimOpmFlowJob.cpp +0/-1     
RimPolygon.h +0/-2     
RimPolygonInView.h +0/-1     
RimEclipseStatisticsCase.h +0/-2     
RimRegularLegendConfig.h +0/-1     
RimResultSelectionUi.cpp +0/-1     
RimSimWellInViewCollection.cpp +0/-1     
RimTernaryLegendConfig.h +0/-3     
RimUserDefinedCalculation.h +0/-1     
RimWellTargetMapping.h +0/-3     
RimStimPlanModel.cpp +0/-1     
RimStimPlanModelTemplate.cpp +0/-1     
RimDeltaSummaryEnsemble.h +0/-1     
RimEnsembleCurveFilterCollection.h +0/-2     
RimSummaryCurveAutoName.cpp +0/-2     
RimSummaryTable.cpp +0/-1     

Replaces usage of the `cafPdmUiPushButtonEditor` with dynamic buttons created directly in the UI ordering. This simplifies the code and allows for more flexible button behavior.
The change improves the user interface by providing more context-aware button labels and actions.

Removes the following files as a result of the change:
* `cafPdmUiPushButtonEditor.h`
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Null pointer dereference

Description: showRegionInView() can dereference m_flowDiagSolution() without a preceding null check
(e.g., line 838 calls m_flowDiagSolution()->flowDiagResults()->maxAbsPairFlux(timeStep)
even when m_flowDiagSolution may be null), which can be triggered via the new dynamic
"Show Region" button and cause a crash/denial-of-service.
RimFlowCharacteristicsPlot.cpp [792-852]

Referred Code
void RimFlowCharacteristicsPlot::showRegionInView()
{
    if ( !m_case ) return;

    if ( m_cellFilter() == RigFlowDiagResults::CELLS_ACTIVE ) return;

    RimEclipseView* view =
        RicSelectOrCreateViewFeatureImpl::showViewSelection( m_case, "FlowCharacteristicsLastUsedView", "RegionView", "Show Region in View" );

    if ( view == nullptr ) return;

    view->faultCollection()->setActive( false );
    view->cellResult()->setResultType( RiaDefines::ResultCatType::FLOW_DIAGNOSTICS );
    view->cellResult()->setFlowDiagTracerSelectionType( RimEclipseResultDefinition::FlowTracerSelectionType::FLOW_TR_BY_SELECTION );
    view->cellResult()->setSelectedTracers( m_selectedTracerNames );

    if ( m_cellFilter() == RigFlowDiagResults::CELLS_COMMUNICATION )
    {
        view->cellResult()->setResultVariable( RigFlowDiagDefines::communicationResultName() );
    }
    else


 ... (clipped 40 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Misspelled identifier: The newly added variable name candicateAddress is misspelled, reducing clarity and
self-documentation.

Referred Code
RimSummaryEnsemble*             candidateEnsemble = m_yValuesSummaryEnsemble();
RifEclipseSummaryAddress        candicateAddress  = m_yValuesSummaryAddress->address();

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Null dereference risk: The new showRegionInView() method dereferences m_flowDiagSolution() without ensuring
m_flowDiagSolution is non-null, which can crash the UI action.

Referred Code
int timeStep = 0;
if ( m_timeStepSelectionType() == ALL_AVAILABLE )
{
    if ( m_flowDiagSolution )
    {
        std::vector<int> timeSteps = m_flowDiagSolution()->flowDiagResults()->calculatedTimeSteps( RigFlowDiagResultAddress::PHASE_ALL );
        if ( !timeSteps.empty() )
        {
            timeStep = timeSteps[0];
        }
    }
}
else
{
    if ( !m_selectedTimeStepsUi().empty() )
    {
        timeStep = m_selectedTimeStepsUi()[0];
    }
}

// Ensure selected time step has computed results


 ... (clipped 2 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Auth flow review: The new dynamic authentication button triggers requestTokenWithCancelButton() and
subsequent data-source actions, which appear safe but require confirmation that
permission/authorization checks and secure token handling are enforced outside this diff.

Referred Code
void RimCloudDataSourceCollection::defineUiOrdering( QString uiConfigName, caf::PdmUiOrdering& uiOrdering )
{
    auto authGroup = uiOrdering.addNewGroup( "Authentication" );

    bool    isGranted = m_sumoConnector && m_sumoConnector->isGranted();
    QString text      = "Authentication Status: ";
    text += isGranted ? "<font color='#228B22'>✔ Granted</font>" : "<font color='#FFA500'>❌ Not Granted</font>";

    if ( !isGranted )
    {
        authGroup->addNewButton( text,
                                 [this]()
                                 {
                                     if ( m_sumoConnector )
                                     {
                                         m_sumoConnector->requestTokenWithCancelButton();
                                     }
                                 } );
    }

    if ( isGranted )


 ... (clipped 10 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Refresh UI after apply

Call updateConnectedEditors() within the "Apply" button's lambda function after
updating m_selectedTimeSteps to ensure the UI reflects the changes.

ApplicationLibCode/ProjectDataModel/Flow/RimFlowCharacteristicsPlot.cpp [414-426]

 uiOrdering.addNewButton( "Apply",
                           [this]()
                           {
                               if ( m_flowDiagSolution )
                               {
                                   // Compute any missing time steps...
                                   m_selectedTimeSteps = m_selectedTimeStepsUi;
+                                  updateConnectedEditors();
                               }
                           } );

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that updateConnectedEditors() is missing to refresh the UI after a state change, which is a common pattern in the codebase and improves UI responsiveness.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments