fix: changes the implementation of the display selection screen #23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request refactors the display selection screen to remove the previous JSON implementation and filtering logic. It replaces the dynamic display grid with a hardcoded list of Updated class diagram for DisplaySelectionScreenclassDiagram
class DisplaySelectionScreen {
-List<Epd> displays
-int? selectedIndex
+Widget build(BuildContext context)
-Widget _buildContinueButton(BuildContext context)
}
class _DisplaySelectionScreenState {
-List<Epd> displays
-int? selectedIndex
+Widget build(BuildContext context)
-Widget _buildContinueButton(BuildContext context)
}
DisplaySelectionScreen -- _DisplaySelectionScreenState
class Epd {
<<abstract>>
+int width
+int height
+List~Image Function(img.Image)~ processingMethods
+String name
+String modelId
+String aspectRatio
+String driverName
+String imgPath
+List~Color~ colors
+Driver controller
}
class Gdey037z03 {
+int width
+int height
+String name
+String modelId
+String aspectRatio
+String driverName
+String imgPath
+List~Color~ colors
}
class Gdey037z03BW {
+int width
+int height
+String name
+String modelId
+String aspectRatio
+String driverName
+String imgPath
+List~Color~ colors
}
Epd <|-- Gdey037z03
Epd <|-- Gdey037z03BW
class DisplayCard {
-Epd display
-bool isSelected
-VoidCallback onTap
+Widget build(BuildContext context)
-String _getColorName(Color color)
-Widget _buildSpecRow(String label, String value)
}
DisplaySelectionScreen -- DisplayCard
note for DisplaySelectionScreen "Removes JSON implementation and filtering logic.
Replaces dynamic display grid with a hardcoded list of Epd objects.
Implements simple state management for display selection."
note for DisplayCard "Updated to work with the new Epd model."
note for Epd "Abstract class for ePaper displays."
note for Gdey037z03 "Concrete implementation of Epd for a specific display."
note for Gdey037z03BW "Concrete implementation of Epd for a specific display."
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Vishveshwara - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a
ListView.builderinstead ofGridView.builderif you expect a small number of displays, as it might be more performant. - The
_getColorNamemethod inDisplayCardcould be moved to theEpdclass to promote code reuse.
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.
| onTap: onTap, | ||
| highlightColor: Colors.redAccent, | ||
| borderRadius: BorderRadius.circular(12), | ||
| splashColor: Colors.redAccent.withValues(alpha: 0.2), |
There was a problem hiding this comment.
issue (typo): Potential typo in splashColor method usage.
It appears that withValues(alpha: 0.2) might be intended to set opacity. Consider replacing with withOpacity(0.2) to correctly apply the desired transparency.
Suggested change
| splashColor: Colors.redAccent.withValues(alpha: 0.2), | |
| splashColor: Colors.redAccent.withOpacity(0.2), |
AsCress
reviewed
Apr 21, 2025
There was a problem hiding this comment.
@Vishveshwara Nice work on the refactoring ! This code is now much more simplified and makes much more sense.
Thank you for the review! 🙏 I appreciate your guidance in helping simplify the architecture. |
kienvo
requested changes
Apr 23, 2025
lib/util/epd/gdey037z03.dart
Outdated
| @override | ||
| String get aspectRatio => '26:15'; | ||
| @override | ||
| String get driverName => 'UC8253'; |
There was a problem hiding this comment.
I think this driverName should be moved to util/epd/driver /driver.dart.
aspectRatio is not so important, can we remove it?
lib/view/widget/display_card.dart
Outdated
Comment on lines
19
to
21
| if (color == Colors.black) return 'Black'; | ||
| if (color == Colors.white) return 'White'; | ||
| if (color == Colors.red) return 'Red'; |
There was a problem hiding this comment.
Not just Black, White, Red, but also Yellow, Blue, Green, and Orange.
kienvo
approved these changes
Apr 23, 2025
|
@mariobehling This is good to go. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #18
Screenshot
Changes:
Summary by Sourcery
Refactor the display selection screen by removing JSON, filtering logic, and provider-based state management, and simplify the implementation to use a more direct approach for selecting e-paper displays.
Bug Fixes:
Enhancements:
Chores: