Skip to content

Conversation

@magnesj
Copy link
Owner

@magnesj magnesj commented Dec 29, 2025

User description

Removes the concept of selection levels from the selection
manager. This simplifies the API and the internal logic of
the selection manager by only supporting a single selection level.

The changes include removing the selectionLevel parameter from
various methods in the SelectionManager class and updating the
call sites to use the simplified API.


PR Type

Enhancement


Description

  • Removes selection level concept from SelectionManager API

  • Simplifies method signatures by eliminating selectionLevel parameters

  • Updates all call sites across codebase to use simplified API

  • Removes SelectionLevel enum and related multi-level selection infrastructure


Diagram Walkthrough

flowchart LR
  A["SelectionManager<br/>with levels"] -- "Remove level<br/>parameters" --> B["SelectionManager<br/>single level"]
  C["Command classes<br/>with level params"] -- "Update calls" --> D["Command classes<br/>simplified"]
  E["UI editors<br/>with level logic"] -- "Remove level<br/>handling" --> F["UI editors<br/>simplified"]
  B --> D
  B --> F
Loading

File Walkthrough

Relevant files
Enhancement
27 files
RicDeleteCustomSegmentIntervalFeature.cpp
Remove selection level from method calls                                 
+3/-3     
RicDeleteDiameterRoughnessIntervalFeature.cpp
Remove selection level from method calls                                 
+3/-3     
RicDeleteOptionItemFeature.cpp
Remove selection level from method calls                                 
+2/-2     
RicNewOptionItemFeature.cpp
Remove selection level from method calls                                 
+1/-1     
RicDeletePressureTableItemFeature.cpp
Remove selection level from method calls                                 
+2/-2     
RicPolylineTarget3dEditor.cpp
Replace setSelectedItemAtLevel with setSelectedItem           
+1/-1     
RicWellTarget3dEditor.cpp
Replace setSelectedItemAtLevel with setSelectedItem           
+1/-1     
RicDeletePolylineTargetFeature.cpp
Remove selection level from method calls                                 
+2/-2     
RicDeleteWellPathAttributeFeature.cpp
Remove selection level from method calls                                 
+3/-3     
RicDeleteWellPathTargetFeature.cpp
Remove selection level from method calls                                 
+2/-4     
RicNewWellPathAttributeFeature.cpp
Remove selection level from method calls                                 
+3/-3     
RicNewWellPathListTargetFeature.cpp
Remove selection level from method calls                                 
+4/-4     
cafCmdSelectionChangeExec.cpp
Remove selection level parameter from redo/undo                   
+2/-4     
cafCmdSelectionHelper.cpp
Remove selection level from helper methods                             
+4/-7     
cafCmdDeleteItemFeature.cpp
Remove selection level from method calls                                 
+3/-3     
cafPdmUiCommandSystemProxy.cpp
Simplify selection level iteration logic                                 
+2/-9     
cafPdmUiSelection3dEditorVisualizer.cpp
Remove multi-level selection aggregation logic                     
+2/-7     
cafSelectionManager.cpp
Remove selection level parameters and multi-level methods
+10/-85 
cafPdmUiLineEditor.cpp
Remove selection level from method calls                                 
+1/-1     
cafPdmUiTableView.cpp
Remove table selection level methods                                         
+0/-16   
cafPdmUiTableViewEditor.cpp
Remove selection level member variables and logic               
+7/-28   
cafPdmUiTreeViewEditor.cpp
Remove selection level from method calls                                 
+1/-1     
cafCmdSelectionChangeExec.h
Remove selection level field from data class                         
+0/-2     
cafCmdSelectionHelper.h
Remove selection level parameters from declarations           
+2/-3     
cafSelectionManager.h
Remove SelectionLevel enum and level-based methods             
+13/-33 
cafPdmUiTableView.h
Remove selection level setter methods                                       
+0/-2     
cafPdmUiTableViewEditor.h
Remove selection level attributes and members                       
+1/-9     

Removes the concept of selection levels from the selection
manager. This simplifies the API and the internal logic of
the selection manager by only supporting a single selection level.

The changes include removing the `selectionLevel` parameter from
various methods in the `SelectionManager` class and updating the
call sites to use the simplified API.
@qodo-code-review
Copy link

qodo-code-review bot commented Dec 29, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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: Meaningful Naming and Self-Documenting Code

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

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: Security-First Input Validation and Data Handling

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

Status: Passed

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:
Duplicate condition: isCommandEnabled() contains two consecutive identical checks for
selectedItemOfType<RimWellPathAttributeCollection>(), which is likely unintended
logic that can mask missing edge-case handling or a forgotten second condition.

Referred Code
if ( caf::SelectionManager::instance()->selectedItemOfType<RimWellPathAttributeCollection>() )
{
    return true;
}
if ( caf::SelectionManager::instance()->selectedItemOfType<RimWellPathAttributeCollection>() )

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

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

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 29, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
The PR may introduce behavioral regressions

Removing selection levels merges previously separate selections (e.g., table
rows and their container) into one list. This change could cause regressions in
commands that now receive mixed object types instead of a specific type from a
dedicated selection level.

Examples:

Fwk/AppFwk/cafUserInterface/cafPdmUiTableViewEditor.cpp [407-438]
void PdmUiTableViewEditor::updateSelectionManagerFromTableSelection()
{
    if ( isSelectionRoleDefined() )
    {
        std::set<PdmUiItem*> selectedRowObjects;
        QModelIndexList      modelIndexList = m_tableView->selectionModel()->selectedIndexes();
        for ( const QModelIndex& mi : modelIndexList )
        {
            PdmObjectHandle* obj = m_tableModelPdm->pdmObjectForRow( mi.row() );


 ... (clipped 22 lines)
Fwk/AppFwk/cafProjectDataModel/cafPdmUiCore/cafSelectionManager.h [56-171]
class SelectionManager
{
public:
    static SelectionManager* instance();

    PdmUiItem*              selectedItem() const;
    std::vector<PdmUiItem*> selectedItems() const;

    void setSelectedItem( PdmUiItem* item );
    bool setSelectedItems( const std::vector<PdmUiItem*>& items );

 ... (clipped 106 lines)

Solution Walkthrough:

Before:

void PdmUiTableViewEditor::updateSelectionManagerFromTableSelection()
{
    // ... get selectedRowObjects
    std::vector<SelectionManager::SelectionItem> newCompleteSelection;

    // Add row items with row selection level
    for (auto item : selectedRowObjects) {
        newCompleteSelection.push_back({item, m_rowSelectionLevel});
    }

    // Add table container item with table selection level
    if (childArrayFieldHandle() && childArrayFieldHandle()->ownerObject()) {
        newCompleteSelection.push_back(
            {childArrayFieldHandle()->ownerObject()->uiCapability(), m_tableSelectionLevel});
    }

    // Set selection with distinct levels
    SelectionManager::instance()->setSelection(newCompleteSelection);
}

After:

void PdmUiTableViewEditor::updateSelectionManagerFromTableSelection()
{
    // ... get selectedRowObjects
    std::vector<PdmUiItem*> newCompleteSelection;

    // Add row items to a single list
    for (auto item : selectedRowObjects) {
        newCompleteSelection.push_back(item);
    }

    // Add table container item to the same list
    if (childArrayFieldHandle() && childArrayFieldHandle()->ownerObject()) {
        newCompleteSelection.push_back(childArrayFieldHandle()->ownerObject()->uiCapability());
    }

    // Set selection with all items merged into one level
    SelectionManager::instance()->setSelectedItems(newCompleteSelection);
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical design change where merging selection levels could lead to significant behavioral regressions, as commands might now receive a mix of object types they were not designed to handle.

High
Possible issue
Fix duplicate selection check
Suggestion Impact:The commit updates the first selection check in RicDeleteWellPathAttributeFeature::isCommandEnabled() to use RimWellPathAttribute instead of RimWellPathAttributeCollection, aligning with the suggested fix to restore correct enablement logic.

code diff:

-    if ( caf::SelectionManager::instance()->selectedItemOfType<RimWellPathAttributeCollection>() )
+    if ( caf::SelectionManager::instance()->selectedItemOfType<RimWellPathAttribute>() )

Fix a duplicated condition in isCommandEnabled by changing one of the checks
from RimWellPathAttributeCollection to RimWellPathAttribute to restore correct
command enablement logic.

ApplicationLibCode/Commands/WellPathCommands/RicDeleteWellPathAttributeFeature.cpp [36-43]

-if ( caf::SelectionManager::instance()->selectedItemOfType<RimWellPathAttributeCollection>() )
+if ( caf::SelectionManager::instance()->selectedItemOfType<RimWellPathAttribute>() )
 {
     return true;
 }
 if ( caf::SelectionManager::instance()->selectedItemOfType<RimWellPathAttributeCollection>() )
 {
     return true;
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies and fixes a logic bug introduced by the PR, where a check for RimWellPathAttribute was likely intended but was duplicated as RimWellPathAttributeCollection, breaking command enablement for single attributes.

Medium
General
Improve vector initialization from set

Initialize newCompleteSelection directly from selectedRowObjects iterators to
improve efficiency and simplify the code.

Fwk/AppFwk/cafUserInterface/cafPdmUiTableViewEditor.cpp [407-439]

 void PdmUiTableViewEditor::updateSelectionManagerFromTableSelection()
 {
     if ( isSelectionRoleDefined() )
     {
         std::set<PdmUiItem*> selectedRowObjects;
         QModelIndexList      modelIndexList = m_tableView->selectionModel()->selectedIndexes();
         for ( const QModelIndex& mi : modelIndexList )
         {
             PdmObjectHandle* obj = m_tableModelPdm->pdmObjectForRow( mi.row() );
 
             if ( obj && obj->uiCapability() )
             {
                 selectedRowObjects.insert( obj->uiCapability() );
             }
         }
 
-        std::vector<PdmUiItem*> newCompleteSelection;
-
-        for ( auto item : selectedRowObjects )
-        {
-            newCompleteSelection.push_back( item );
-        }
+        std::vector<PdmUiItem*> newCompleteSelection( selectedRowObjects.begin(), selectedRowObjects.end() );
 
         if ( childArrayFieldHandle() && childArrayFieldHandle()->ownerObject() )
         {
             newCompleteSelection.push_back( childArrayFieldHandle()->ownerObject()->uiCapability() );
         }
 
         m_isBlockingSelectionManagerChanged = true;
         SelectionManager::instance()->setSelectedItems( newCompleteSelection );
         m_isBlockingSelectionManagerChanged = false;
     }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 4

__

Why: The suggestion correctly proposes a more idiomatic and slightly more efficient way to construct a std::vector from a std::set, improving code readability.

Low
  • Update

magnesj and others added 2 commits December 30, 2025 08:30
…AttributeFeature.cpp

Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
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.

2 participants