Take intersection of context menu options when selecting multiple workspaces #40577
Conversation
jhaigh0
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe change refactors workspace context-menu generation to support multiple selected workspaces. It adds a new Mantid::API::plotMenuActions struct and a QAction* m_separator. createWorkspaceContextMenu now accepts a QStringList of selections. Per-workspace builders were changed to return action vectors and plotMenuActions (for Matrix, MD, and groups) or vectors (for Table), and a new intersectionOfActions computes common actions. Menu construction now collects per-item actions, intersects them, and only inserts Plot and 3D submenus when common plot actions exist. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 Fix all issues with AI agents
In @qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp:
- Around line 128-129: There's a misspelled variable name: change
selectedWorksapceNames to selectedWorkspaceNames wherever it's declared and
used; specifically update the assignment from getSelectedWorkspaceNamesAsQList()
and the subsequent call to createWorkspaceContextMenu(selectedWorkspaceNames)
(and any other references in the same scope) so the identifier matches
correctly.
- Around line 318-332: The function
WorkspaceTreeWidgetSimple::intersectionOfActions can dereference an empty
actionVecs via actionVecs.front(), causing UB; fix by checking if
actionVecs.empty() (and possibly if any inner vector is empty) at the top and
return an empty std::vector<QAction*> immediately, then proceed with
sorting/combining as before; ensure std::erase_if on combinedActions is only
performed when combinedActions was initialized from a non-empty front() to avoid
crashes.
- Around line 238-270: The bug is that workspaces.push_back(workspace) is called
before type checks, so unhandled workspace types create size mismatches with
actionVecs and can make intersectionOfActions crash; fix by moving the
workspaces.push_back(workspace) into each handled branch (inside the
MatrixWorkspace, ITableWorkspace, IMDWorkspace, and WorkspaceGroup branches) so
only workspaces that produce corresponding actionVecs are added, and remove the
earlier unconditional push (or alternatively add an else { continue; } to skip
unhandled types).
🧹 Nitpick comments (3)
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.h (2)
22-25: MoveplotMenuActionsstruct inside the namespace.The struct is defined outside
MantidQt::MantidWidgetsnamespace, which pollutes the global namespace. Since it's only used byWorkspaceTreeWidgetSimple, it should be defined within the same namespace for consistency and encapsulation.♻️ Suggested refactor
-struct plotMenuActions { - std::vector<QAction *> plotActions = {}; - std::vector<QAction *> plot3DActions = {}; -}; - namespace MantidQt::MantidWidgets { + +struct plotMenuActions { + std::vector<QAction *> plotActions = {}; + std::vector<QAction *> plot3DActions = {}; +}; + class MantidDisplayBase;
111-111: Useconstreference for input parameter.
selectedWorkspacesis not modified by this function, so it should be passed asconst QStringList &for const-correctness.♻️ Suggested fix
- QMenu *createWorkspaceContextMenu(QStringList &selectedWorkspaces); + QMenu *createWorkspaceContextMenu(const QStringList &selectedWorkspaces);qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp (1)
233-233: Useconstreference for input parameter (consistency with header fix).The parameter should be
const QStringList &to match the suggested header change and ensure const-correctness.♻️ Suggested fix
-QMenu *WorkspaceTreeWidgetSimple::createWorkspaceContextMenu(QStringList &selectedWorkspaces) { +QMenu *WorkspaceTreeWidgetSimple::createWorkspaceContextMenu(const QStringList &selectedWorkspaces) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.hqt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,cc,cxx,c++,h,hpp,hxx}
📄 CodeRabbit inference engine (AGENTS.md)
Run clang-tidy for code linting using the configuration in .clang-tidy
Files:
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.hqt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
🧬 Code graph analysis (1)
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.h (1)
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp (8)
createWorkspaceContextMenu(233-316)createWorkspaceContextMenu(233-233)createMatrixWorkspacePlotMenu(448-473)createMatrixWorkspacePlotMenu(448-448)hasMultipleBins(33-45)hasMultipleBins(33-33)intersectionOfActions(318-332)intersectionOfActions(319-319)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: doxygen
🔇 Additional comments (6)
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.h (1)
130-131: LGTM on the newm_separatormember.Using a dedicated
QActionas a reusable separator is a reasonable approach for building dynamic menus.qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp (5)
69-69: LGTM on separator initialization.The
m_separatoraction is properly initialized and configured as a separator in the constructor.Also applies to: 104-104
276-291: Instrument view enablement logic looks correct.The code properly checks if
m_showInstrumentis in the combined actions before computing theenabledstate across all workspaces. Usingstd::all_ofensures the instrument view is only enabled when all selected workspaces support it.
293-310: Plot submenu construction is well-structured.The conditional building of Plot and 3D submenus only when actions are available is clean. The menu hierarchy (Plot → 3D) is properly established.
351-354: Single-selection check within per-workspace function.This check for
m_tree->selectedItems().size() == 1correctly restricts sample material/shape actions to single-selection scenarios. Since this is called for each workspace but the check is global, the actions will only be added when exactly one item is selected.
448-473: LGTM on refactored plot menu creation.The function now returns a
plotMenuActionsstruct instead of aQMenu*, which aligns with the new action-vector-based approach for building menus from intersections.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp:
- Around line 318-332: The function
WorkspaceTreeWidgetSimple::intersectionOfActions must guard against an empty
actionVecs to avoid calling actionVecs.front(); add an early check returning an
empty std::vector<QAction*> when actionVecs.empty(), then proceed with the
existing sort/combinedActions logic (using actionVecs.front() and std::erase_if)
for non-empty input; ensure you update only the body of intersectionOfActions
and keep the rest of the algorithm unchanged.
- Around line 128-129: There's a typo in the local variable name: change
selectedWorksapceNames to selectedWorkspaceNames where it's declared and any
subsequent uses (e.g., the call to getSelectedWorkspaceNamesAsQList() and the
argument passed into createWorkspaceContextMenu); update both the variable
declaration and its usage to the correctly spelled selectedWorkspaceNames to
avoid undefined symbol/compile errors.
- Around line 238-270: The loop currently always pushes the retrieved workspace
into workspaces but only appends to actionVecs for certain types, causing a
potential size mismatch and a crash in intersectionOfActions; fix by ensuring
workspaces and actionVecs stay in sync—either move the
workspaces.push_back(workspace) into each handled branch (the branches using
std::dynamic_pointer_cast<MatrixWorkspace>, ITableWorkspace, IMDWorkspace,
WorkspaceGroup that call
createMatrixWorkspaceActions/createTableWorkspaceActions/createMDWorkspaceActions/createWorkspaceGroupActions)
so you only add when you also add an actionVec, or skip/continue when no branch
matches (i.e., do not push the workspace for unhandled types), and additionally
add a guard after the loop to return/disable further processing if actionVecs is
empty before calling intersectionOfActions.
🧹 Nitpick comments (2)
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp (1)
276-291: CachegetInstrument()result to avoid redundant calls.Lines 279-280 call
workspace->getInstrument()twice. Caching the result would improve efficiency.♻️ Suggested refactor
const auto showInstrumentViewCheck = [](MatrixWorkspace_const_sptr workspace) { - return workspace->getInstrument() && !workspace->getInstrument()->getName().empty() && - workspace->getAxis(1)->isSpectra(); + const auto instrument = workspace->getInstrument(); + return instrument && !instrument->getName().empty() && workspace->getAxis(1)->isSpectra(); };qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.h (1)
22-25: Consider moving struct intoMantidQt::MantidWidgetsnamespace.The
plotMenuActionsstruct is currently in the global namespace. For better encapsulation and to avoid namespace pollution, consider moving it intoMantidQt::MantidWidgets.♻️ Suggested change
namespace Mantid::API { class Workspace; } -struct plotMenuActions { - std::vector<QAction *> plotActions = {}; - std::vector<QAction *> plot3DActions = {}; -}; - namespace MantidQt::MantidWidgets { + +struct plotMenuActions { + std::vector<QAction *> plotActions = {}; + std::vector<QAction *> plot3DActions = {}; +}; + class MantidDisplayBase;This would also require updating the return types in the function declarations to use the fully qualified name or bringing the struct into scope.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.hqt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,cc,cxx,c++,h,hpp,hxx}
📄 CodeRabbit inference engine (AGENTS.md)
Run clang-tidy for code linting using the configuration in .clang-tidy
Files:
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.hqt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
🧠 Learnings (1)
📚 Learning: 2025-10-22T13:08:14.586Z
Learnt from: GuiMacielPereira
Repo: mantidproject/mantid PR: 40194
File: Framework/Algorithms/src/CreateDetectorTable.cpp:64-68
Timestamp: 2025-10-22T13:08:14.586Z
Learning: In Mantid Framework, MatrixWorkspace::getInstrument() (inherited from ExperimentInfo) throws an exception if no instrument is found rather than returning nullptr. The method dereferences the internal instrument pointer, so null-checking the return value of getInstrument() is unnecessary. Only check the objects returned by methods called on the instrument (e.g., getSample()).
Applied to files:
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
🧬 Code graph analysis (1)
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.h (1)
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp (2)
hasMultipleBins(33-45)hasMultipleBins(33-33)
🔇 Additional comments (3)
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp (2)
69-69: LGTM: Separator initialization and usage.The separator action is correctly initialized and configured. Qt will manage its lifetime through the parent-child relationship.
Also applies to: 104-104
334-357: Well-structured refactoring to support action intersection.The refactored action creator functions correctly return action vectors/tuples instead of directly mutating menus. This enables the intersection logic to compute common actions across multiple selected workspaces, directly addressing the PR objective. Each workspace type appropriately returns its supported actions:
- MatrixWorkspace: Full plot menu for multi-bin data, limited for single values
- TableWorkspace: No plot actions (correct, as tables aren't plottable)
- MDWorkspace: Conditional slice viewer or 1D plot based on dimensions
- WorkspaceGroup: Actions derived from contained workspace types
Also applies to: 359-368, 370-407, 409-438, 448-473
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.h (1)
111-124: LGTM: Header declarations match implementation.The updated function signatures correctly reflect the refactored implementation:
createWorkspaceContextMenunow accepts multiple workspace names- Action creator functions return vectors/tuples for intersection computation
- New
intersectionOfActionshelper enables common action calculationm_separatormember supports clean action list formattingAlso applies to: 130-131
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
Outdated
Show resolved
Hide resolved
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
Outdated
Show resolved
Hide resolved
jhaigh0
added
ISIS: Core
There was a problem hiding this comment.
Thanks for implementing this. I've been trying with a few different combinations, everything seems to be working well, and the bug is not present.
I have added some comments in the code, but here are a few general suggestions:
- Release notes are missing.
- Since we are in this part of the code, could be add a small paragraph on the context menu docs with the dynamic behaviour of the context menu (workspacetreewidget.rst)
- Is it feasable to use cpp implementations of sets (std::sets) or function like std::set_intersection to find the intersections? Perhaps would simplify the code.
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
Outdated
Show resolved
Hide resolved
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
Outdated
Show resolved
Hide resolved
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
Outdated
Show resolved
Hide resolved
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
Outdated
Show resolved
Hide resolved
qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidgetSimple.cpp
Outdated
Show resolved
Hide resolved
qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidgetSimple.h
Outdated
Show resolved
Hide resolved
|
|
||
| const auto it = std::find(combinedActions.cbegin(), combinedActions.cend(), m_showInstrument); | ||
| if (it != combinedActions.cend()) { | ||
| const auto showInstrumentViewCheck = [](MatrixWorkspace_const_sptr workspace) { |
There was a problem hiding this comment.
can this lambda just be plug in on L284?
There was a problem hiding this comment.
I just did this for readability, it got too confusing when it was all one statement I think.
Co-authored-by: Adri Diaz <146007827+adriazalvarez@users.noreply.github.com>
jhaigh0
force-pushed
the
40401_disable_menu_options_for_multiple_workspace_types
branch
from
acd500e to
381a4d7
Compare
This is what I tried first. The main problem is that we want to preserve the order which the actions are listed in which isn't great for sets and also |
2 participants
Description of work
Previously, when selecting multiple workspaces and right-clicking, the context menu shown would be for the workspace which the mouse was over. However when selecting an option, all workspace names would be passed to that process. This could lead to problems like the one reported by a user in the original issue.
In this PR the system for generating the context menu is reworked so that only the actions which are available for all selected workspaces are added. This should stop any similar bugs.
Closes #40401
To test:
Create a few different kinds of workspaces e.g
RemoveInstrumentGeometry)Select different combinations of these and check that the options present in the context menu make sense (are the ones which are shared between them individually)
Also select multiple workspaces of the same type and check that everything is displayed normally, plus any other tests you can think of.
Reviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
mantid-developersormantid-contributorsteams, add a review commentrerun cito authorize/rerun the CIGatekeeper
As per the gatekeeping guidelines: