-
Notifications
You must be signed in to change notification settings - Fork 219
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
fix: updated the draw View logic feature. #1041
Conversation
Reviewer's Guide by SourceryThis pull request updates the draw View logic feature, primarily by replacing the BadgeWidget with CustomPaint and introducing separate paint classes for badge home and draw badge views. It also includes changes to the grid manipulation logic and some minor adjustments to improve the overall functionality. Updated class diagram for BadgeHomePaint and DrawBadgePaintclassDiagram
class BadgeHomePaint {
+List<List<bool>> grid
+BadgeHomePaint(List<List<bool>> grid)
+void paint(Canvas canvas, Size size)
+bool shouldRepaint(CustomPainter oldDelegate)
}
class DrawBadgePaint {
+List<List<bool>> grid
+DrawBadgePaint(List<List<bool>> grid)
+void paint(Canvas canvas, Size size)
+bool shouldRepaint(CustomPainter oldDelegate)
}
class CustomPainter
BadgeHomePaint --|> CustomPainter
DrawBadgePaint --|> CustomPainter
Updated class diagram for DrawBadgeProviderclassDiagram
class DrawBadgeProvider {
+List<List<bool>> homeViewGrid
+List<List<bool>> drawViewGrid
+List<List<bool>> getGrid()
+List<List<bool>> getDrawViewGrid()
+void setDrawViewGrid(int row, int col)
+void resetGrid()
}
File-Level Changes
Possibly linked issues
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 @Jhalakupadhyay - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding documentation to explain the custom painting approach, particularly in the BadgeHomePaint and DrawBadgePaint classes. This will help with future maintenance and understanding of the rendering logic.
- Please provide an explanation for the architectural change from BadgeWidget to custom painting. Understanding the rationale behind this decision would be helpful for the team.
- It's recommended to add unit tests for the new painting classes to ensure they behave correctly under various conditions and to prevent potential regressions in the future.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 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.
@@ -57,7 +57,7 @@ class DrawBadgeProvider extends ChangeNotifier { | |||
|
|||
//function to reset the state of the cell | |||
void resetGrid() { |
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.
issue (bug_risk): Potential bug in resetGrid method
The resetGrid method is now resetting drawViewGrid instead of homeViewGrid. Is this change intentional? If not, it could lead to unexpected behavior.
#1016
Summary by Sourcery
Update the draw view logic by replacing BadgeWidget with CustomPaint in badge home and draw badge views, and fix the grid update logic in DrawBadgeProvider.
Bug Fixes:
Enhancements: