[MSD-763][fix] adjust data sharing menu item - #3549
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe Help menu now includes a checkable “Share Data with Delmic” item. The main window initializes its reference. Sequence Diagram(s)sequenceDiagram
participant MainWindow
participant MenuController
participant DataSharingMenuItem
participant ToggleHandler
MainWindow->>MenuController: Initialize menu controller
MenuController->>DataSharingMenuItem: Check active role
alt meteor or mimas
MenuController->>DataSharingMenuItem: Synchronize checkbox state
MenuController->>ToggleHandler: Bind toggle handler
else Other role
MenuController->>DataSharingMenuItem: Remove and destroy item
end
Mergeability Score: ⚪ Minimal · up to This change only adjusts the Help menu presentation and system-specific data-sharing visibility; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/menu.py (1)
181-194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove
_append_data_sharing_menu_itemand initialize_consent_menu_item.No callers remain for
_append_data_sharing_menu_item, so delete it. The constructor never assignsself._consent_menu_item; clicking the item raisesAttributeErrorduring checkbox synchronization. Store the XRC item inself._consent_menu_item.🤖 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 `@src/odemis/gui/cont/menu.py` around lines 181 - 194, Remove the unused _append_data_sharing_menu_item method. In the constructor, initialize self._consent_menu_item by retrieving the existing XRC menu item, so checkbox synchronization can access it without raising AttributeError.
🤖 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 `@src/odemis/gui/cont/menu.py`:
- Around line 145-148: Update MenuController initialization and the role branch
around _on_toggle_data_sharing so the consent menu item is stored on
self._consent_menu_item, initializing it to None before the role check when
necessary. Use that instance attribute for the existing checkbox setup and
binding, ensuring the handler can synchronize the menu item without raising
AttributeError.
---
Outside diff comments:
In `@src/odemis/gui/cont/menu.py`:
- Around line 181-194: Remove the unused _append_data_sharing_menu_item method.
In the constructor, initialize self._consent_menu_item by retrieving the
existing XRC menu item, so checkbox synchronization can access it without
raising AttributeError.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df046101-eba3-4646-b7a2-54fd2ac52063
📒 Files selected for processing (3)
src/odemis/gui/cont/menu.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/frame_main.xrc
| consent_item.Check(self._data_collector.get_consent() is True) | ||
| main_frame.Bind(wx.EVT_MENU, self._on_toggle_data_sharing, id=consent_item.GetId()) | ||
| else: | ||
| menu = main_frame.menu_item_data_sharing.GetMenu() |
There was a problem hiding this comment.
This else block is a bit odd to me. Feels like we could have placed the instantiation of main_frame.menu_item_data_sharing in the if part and leave out the else part, unless there is a menu-specific constraint that I'm missing.
There was a problem hiding this comment.
It's always instantiated, from the XRC file. That's how all the menu items are done.
| <assign_var>1</assign_var> | ||
| </XRCED> | ||
| </object> | ||
| <object class="wxMenuItem" name="menu_item_data_sharing"> |
There was a problem hiding this comment.
The PR states that the order is changed, but I'm only seeing something added and not (re)moved.
There was a problem hiding this comment.
the same item was earlier appended in the list of menu items so it was done from the code. Now I have removed that and added here because with append it is the last item which we do not want
| consent_item.Check(self._data_collector.get_consent() is True) | ||
| main_frame.Bind(wx.EVT_MENU, self._on_toggle_data_sharing, id=consent_item.GetId()) | ||
| else: | ||
| menu = main_frame.menu_item_data_sharing.GetMenu() |
There was a problem hiding this comment.
It's always instantiated, from the XRC file. That's how all the menu items are done.
Change the order of data sharing in Help menu and remove data sharing for all system other than meteor and mimas
4ea0251 to
8beeb16
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the GUI Help menu to surface a data-sharing consent checkbox in a fixed position and ensures the option is only available on METEOR and MIMAS systems.
Changes:
- Added a new checkable “Share Data with Delmic” menu item to the Help menu XRC resources.
- Wired the new XRC menu item into
main_xrc.pyso the main frame exposes it asmenu_item_data_sharing. - Updated the menu controller to bind the consent toggle only for roles
meteorandmimas, and remove the menu item for all other roles.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/odemis/gui/xmlh/resources/frame_main.xrc |
Adds the new Help menu checkbox entry for data sharing. |
src/odemis/gui/main_xrc.py |
Exposes the new XRC menu item on the main frame (generated resource + lookup). |
src/odemis/gui/cont/menu.py |
Binds consent handling for METEOR/MIMAS and removes the menu item for other roles. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <object class="wxMenuItem" name="menu_item_data_sharing"> | ||
| <label>Share Data with Delmic</label> | ||
| <checkable>1</checkable> |
| <object class="wxMenuItem" name="menu_item_data_sharing"> | ||
| <label>Share Data with Delmic</label> | ||
| <checkable>1</checkable> |
Change the order of data sharing in Help menu and remove data sharing for all systems other than meteor and mimas