-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(data_hierarchy): implement changes for data type row #351
feat(data_hierarchy): implement changes for data type row #351
Conversation
…rarchy-editor-version-20-type-row-changes
- Code cleanup.
Reviewer's Guide by SourceryThis pull request implements significant changes to the data hierarchy editor dialog in the PASTA-ELN application. The changes focus on improving the functionality for creating and editing data types, enhancing the user interface, and refactoring the codebase for better maintainability. User journey diagram for data type row changesjourney
title User journey for data type row changes
section Data Type Row
User selects data type from combo-box: 5: User
User clicks Add button: 5: User
System displays form with data type, title, IRI, shortcut, icon: 5: System
User fills form and submits: 5: User
System saves new data type: 5: System
User clicks Edit button: 5: User
System displays form with read-only data type, icon, shortcut: 5: System
User modifies title and IRI: 5: User
System saves changes: 5: System
User clicks Delete button: 5: User
System checks if data type is structural: 5: System
System disables delete for structural types: 5: System
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jmurugan-fzj - I've reviewed your changes - here's some feedback:
Overall Comments:
- The refactoring to use a common TypeDialog base class for create and edit dialogs is a good improvement for code reuse and maintainability. Consider adding some high-level documentation explaining the class hierarchy and responsibilities.
- The new icon selection functionality looks good, but it may be worth adding some user documentation or tooltips to explain how to use this feature effectively.
Here's what I looked at during the review
- 🟡 General issues: 8 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 6 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…-changes # Conflicts: # pasta_eln/GUI/data_hierarchy/data_hierarchy_editor_dialog.py
…-changes # Conflicts: # pasta_eln/GUI/data_hierarchy/data_hierarchy_editor_dialog.py
…with 6.8.0. Separate issue already logged: #352.
- Other minor refactorings. - Corrected the failing tests.
@SteffenBrinckmann Could you please do the review, had fixed all the failing problems and addressed valid comments from Sourcery too! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmurugan-fzj Pyside6.8.0.1 was released today, just 5 days after 6.8.0 and they usually do not release 4-digit releases. It makes me think, did the issue you encounter, occur to them too and they have fixed it. Can you try to relax the requirements (without =6.7.3) and see if it works now?
For "Data type" row: Modified to include a combo-box (listing all doc type labels), Add, Edit and Delete buttons
Add functionality
Edit functionality
Delete functionality
Structure level 0 & Structure level 1 are only supported, delete functionality should be disabled for both. Edit form for "structure level type" displays icon and shortcut as read only items
Summary by Sourcery
Refactor the data hierarchy editor to separate the creation and editing of data types into distinct dialogs, enhancing the user interface and validation. Introduce a singleton class for managing QTA icons and add comprehensive tests for the new features.
New Features:
Enhancements:
Tests:
Additional Info: Pyside6 version has been fixed to 6.7.3 since there is some incompatibility between the dependent libraries with the latest PySide6 6.8.0, The issue is already backlogged: PASTA APP crashes with segmentation fault