-
Notifications
You must be signed in to change notification settings - Fork 213
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: Added the logic to display the user drawn clipart to the badge. #981
feat: Added the logic to display the user drawn clipart to the badge. #981
Conversation
Reviewer's Guide by SourceryThis pull request enhances the logic to display user-drawn clipart on the badge. The changes include modifying the image cache to store filenames and keys, adding methods to read image data from files, and updating the conversion logic to handle these new data structures. Additionally, the retry logic in File-Level Changes
Tips
|
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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues 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 to tell me if it was helpful.
//storing the user drawn clipart to the badge in the form of a list | ||
//the first element of the list is the filename and the second element is the key | ||
//while parsing the vector we can take the filename and generate the hex for that vector | ||
//therefore transfering the vector to the physiacl badge will be easier. | ||
imageCacheProvider.imageCache[[filename, key]] = imageData; |
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.
suggestion: Consider improving the comment explaining the new cache structure
The current comment is a bit unclear. Consider explaining why this structure was chosen and how it benefits the system. For example: 'Store user-drawn clipart with both filename and key for easier vector-to-badge transfer. The filename allows hex generation, while the key maintains ordering.'
//storing the user drawn clipart to the badge in the form of a list | |
//the first element of the list is the filename and the second element is the key | |
//while parsing the vector we can take the filename and generate the hex for that vector | |
//therefore transfering the vector to the physiacl badge will be easier. | |
imageCacheProvider.imageCache[[filename, key]] = imageData; | |
/// Cache structure for user-drawn clipart: | |
/// Key: [filename, key] - Filename for hex generation, key for ordering | |
/// Value: imageData | |
/// This structure enables efficient vector-to-badge transfer | |
/// by providing both hex generation and order preservation. | |
imageCacheProvider.imageCache[[filename, key]] = imageData; |
await _addImageDataToCache(image, filename); | ||
} | ||
|
||
Future<List<List<int>>?> readFromFile(String filename) async { |
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.
suggestion: Consider enhancing error handling in readFromFile method
The current error handling returns null for all error cases. Consider differentiating between file not found and other IO errors. You might also want to propagate specific errors to the caller for better error handling upstream.
Future<List<List<int>>> readFromFile(String filename) async {
try {
// Existing implementation here
} on FileSystemException catch (e) {
if (e.osError?.errorCode == 2) {
throw FileNotFoundException('File not found: $filename');
}
throw FileReadException('Error reading file: $filename', e);
}
}
class FileNotFoundException implements Exception {
final String message;
FileNotFoundException(this.message);
}
class FileReadException implements Exception {
final String message;
final Exception originalException;
FileReadException(this.message, this.originalException);
}
logger.d("Retrying ($attempt/$_maxRetries)..."); | ||
await Future.delayed( | ||
const Duration(seconds: 2)); // Wait before retrying | ||
} else { |
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 (performance): Reconsider removing the delay between Bluetooth retry attempts
Removing the delay between retry attempts could lead to rapid, successive retries. This might overwhelm the Bluetooth stack or the device. Consider reintroducing a delay, possibly with an exponential backoff strategy for more robust error handling.
@@ -45,8 +45,6 @@ abstract class RetryBleState extends BleState { | |||
attempt++; | |||
if (attempt < _maxRetries) { | |||
logger.d("Retrying ($attempt/$_maxRetries)..."); | |||
await Future.delayed( |
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.
We should have some delay when retrying. You can reduce the delay but Retrying would not make sense if the failure is transient and you're trying again without waiting
#979
Summary by Sourcery
Add logic to display user-drawn clipart on the badge by storing clipart in the image cache with filenames and keys. Enhance image cache handling and update related methods and widgets to support the new functionality. Update tests to reflect changes.
New Features:
Enhancements:
addToCache
and_addImageDataToCache
methods to handle filenames for user-drawn clipart.Converters
class to read image data from files and convert it to LED hex format for user-drawn clipart.InlineImageProvider
to handle insertion of images using keys that can be either integers or lists.VectorGridView
widget to use the updated image cache keys.Tests:
Converters
test to accommodate the new parameter in theconvertBitmapToLEDHex
method.