feat: added Language support #1341
Closed
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
…gemagic-app into recover-savedbadges
Reviewer's GuideThis PR adds Flutter localization with runtime locale switching across the app, extends HomeScreen and SavedBadgeProvider to support editing of saved badges (by persisting and retrieving their original text), and hardens file I/O by introducing synchronization locks, cache consolidation, and robust JSON parsing; it also introduces a BadgeTextStorage utility for managing original badge messages. Class diagram for localization and language switching integrationclassDiagram
class MyApp {
- Locale? _locale
+ void setLocale(Locale locale)
}
class SettingsScreen {
+ void Function(Locale)? onLocaleChange
}
class AppLocalizations {
<<Flutter gen-l10n>>
+ static AppLocalizations? of(BuildContext context)
+ static LocalizationsDelegate<AppLocalizations> delegate
}
MyApp <|-- _MyAppState
SettingsScreen --> MyApp : onLocaleChange
MyApp ..> AppLocalizations : uses
Class diagram for BadgeTextStorage utility and saved badge editingclassDiagram
class BadgeTextStorage {
+ static Future<void> saveOriginalText(String badgeFilename, String originalText)
+ static Future<String> getOriginalText(String badgeFilename)
+ static Future<void> deleteOriginalText(String badgeFilename)
}
class SavedBadgeProvider {
+ Future<void> applySavedBadgeDataToUI(...)
+ Future<void> updateBadgeData(...)
}
class HomeScreen {
+ Map<String, dynamic>? savedBadgeData
+ String? savedBadgeFilename
}
HomeScreen --> SavedBadgeProvider : uses for editing
SavedBadgeProvider ..> BadgeTextStorage : uses
Class diagram for FileHelper synchronization and cache updateclassDiagram
class FileHelper {
+ static final Lock _badgeFileLock
+ Future<File> saveBadgeData(...)
+ Future<dynamic> readBadgeData(...)
+ void _updateSavedBadgeCache(String filename, Map<String, dynamic> jsonData)
}
class InlineImageProvider {
+ List<MapEntry<String, dynamic>> savedBadgeCache
}
FileHelper --> InlineImageProvider : updates cache
FileHelper ..> Lock : synchronizes file I/O
File-Level Changes
Possibly 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 @nope3472 - I've reviewed your changes - here's some feedback:
- HomeScreen has grown huge and mixes UI, state management, and saved‐badge logic—consider breaking it into smaller widgets and moving saving/loading operations into dedicated services or providers for better readability and testability.
- SavedBadgeProvider and FileHelper contain UI side‐effects (toasts, context usage) and heavy file logic; you should extract toast calls and any context‐dependent code into the widget layer or a coordinator to keep providers pure.
- Using pushAndRemoveUntil in SaveBadgeCard clears the entire navigation stack, which may prevent normal back navigation—use a standard push or pop to replace only the necessary routes instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- HomeScreen has grown huge and mixes UI, state management, and saved‐badge logic—consider breaking it into smaller widgets and moving saving/loading operations into dedicated services or providers for better readability and testability.
- SavedBadgeProvider and FileHelper contain UI side‐effects (toasts, context usage) and heavy file logic; you should extract toast calls and any context‐dependent code into the widget layer or a coordinator to keep providers pure.
- Using pushAndRemoveUntil in SaveBadgeCard clears the entire navigation stack, which may prevent normal back navigation—use a standard push or pop to replace only the necessary routes instead.
## Individual Comments
### Comment 1
<location> `lib/view/homescreen.dart:437` </location>
<code_context>
+ if (state == AppLifecycleState.resumed) {
+ inlineimagecontroller.clear();
+ previousText = '';
+ animationProvider.stopAllAnimations.call(); // If method exists
+ animationProvider.initializeAnimation.call(); // If method exists
+ if (mounted) setState(() {});
+ } else if (state == AppLifecycleState.paused ||
</code_context>
<issue_to_address>
Calling methods with .call() may cause runtime errors if not callable.
Using `.call()` assumes these are callable objects, not standard methods. Remove `.call` and invoke them directly as methods: `animationProvider.stopAllAnimations();` and `animationProvider.initializeAnimation();`.
</issue_to_address>
### Comment 2
<location> `lib/providers/saved_badge_provider.dart:82` </location>
<code_context>
+ // Set the text in the controller
+ inlineimagecontroller.text = badgeText;
+
+ // Set animation effects
+ if (message.flash) {
+ animationProvider.addEffect(effectMap[1]); // Flash effect
+ }
+ if (message.marquee) {
+ animationProvider.addEffect(effectMap[2]); // Marquee effect
+ }
+ // Set inversion if applicable
+ if (savedData.containsKey('invert') && savedData['invert'] == true) {
+ animationProvider.addEffect(effectMap[0]); // Invert effect
</code_context>
<issue_to_address>
Effect addition does not clear previous effects, which may cause effect leakage.
Currently, effects from previous badges may persist when loading new badge data. To prevent this, call `animationProvider.clearAllEffects()` before adding new effects.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Set the text in the controller
inlineimagecontroller.text = badgeText;
// Set animation effects
if (message.flash) {
animationProvider.addEffect(effectMap[1]); // Flash effect
}
if (message.marquee) {
animationProvider.addEffect(effectMap[2]); // Marquee effect
}
// Set inversion if applicable
if (savedData.containsKey('invert') && savedData['invert'] == true) {
animationProvider.addEffect(effectMap[0]); // Invert effect
}
=======
// Set the text in the controller
inlineimagecontroller.text = badgeText;
// Clear previous animation effects before adding new ones
animationProvider.clearAllEffects();
// Set animation effects
if (message.flash) {
animationProvider.addEffect(effectMap[1]); // Flash effect
}
if (message.marquee) {
animationProvider.addEffect(effectMap[2]); // Marquee effect
}
// Set inversion if applicable
if (savedData.containsKey('invert') && savedData['invert'] == true) {
animationProvider.addEffect(effectMap[0]); // Invert effect
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `lib/providers/saved_badge_provider.dart:93` </location>
<code_context>
+ // Set animation mode
</code_context>
<issue_to_address>
No validation for modeValue index may cause out-of-range errors.
Validate that modeValue is within the bounds of animationMap before using it as an index to prevent potential out-of-range errors from invalid or corrupted data.
</issue_to_address>
### Comment 4
<location> `lib/providers/saved_badge_provider.dart:61` </location>
<code_context>
+ /// Save the original text for a badge
+ static Future<void> saveOriginalText(
+ String badgeFilename, String originalText) async {
+ try {
+ // Get the existing text storage or create a new one
+ Map<String, String> textStorage = await _getTextStorage();
</code_context>
<issue_to_address>
No validation for grid size may cause UI inconsistencies.
Validate the grid size after converting the hex string to a binary array to prevent rendering issues from malformed data.
</issue_to_address>
### Comment 5
<location> `lib/bademagic_module/utils/file_helper.dart:22` </location>
<code_context>
+
class FileHelper {
+ // Static lock to synchronize file access and prevent race conditions
+ static final Lock _badgeFileLock = Lock();
final InlineImageProvider imageCacheProvider =
GetIt.instance<InlineImageProvider>();
</code_context>
<issue_to_address>
File locking is introduced for badge file operations, but not all file accesses are covered.
Other operations like listing or deleting badge files may not be synchronized. Please review all file access points to ensure thread safety.
Suggested implementation:
```
static Future<File> _writeToFile(String filename, String data) async {
final path = await _getFilePath(filename);
logger.d('Writing to file: $path');
return await _badgeFileLock.synchronized(() async {
return File(path).writeAsString(data);
```
```
static Future<String> _readFromFile(String filename) async {
final path = await _getFilePath(filename);
logger.d('Reading from file: $path');
return await _badgeFileLock.synchronized(() async {
return File(path).readAsString();
});
}
```
```
static Future<void> _deleteFile(String filename) async {
final path = await _getFilePath(filename);
logger.d('Deleting file: $path');
await _badgeFileLock.synchronized(() async {
final file = File(path);
if (await file.exists()) {
await file.delete();
}
});
}
```
```
static Future<List<FileSystemEntity>> _listBadgeFiles(String directory) async {
final dir = Directory(directory);
logger.d('Listing files in: $directory');
return await _badgeFileLock.synchronized(() async {
return dir.list().toList();
});
}
```
If there are other badge file operations (such as file renaming, copying, or moving) in this file, you should also wrap them in `_badgeFileLock.synchronized(...)` in a similar manner to ensure all badge file accesses are thread-safe.
</issue_to_address>
### Comment 6
<location> `lib/view/homescreen.dart:46` </location>
<code_context>
- @override
- Widget build(BuildContext context) {
- super.build(context);
- InlineImageProvider inlineImageProvider =
- Provider.of<InlineImageProvider>(context);
-
- return MultiProvider(
</code_context>
<issue_to_address>
Redefining inlineImageProvider in build may shadow the class property.
Consider renaming the local variable or using the class property to avoid confusion and potential bugs from shadowing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+437
to
+438
| animationProvider.stopAllAnimations.call(); // If method exists | ||
| animationProvider.initializeAnimation.call(); // If method exists |
There was a problem hiding this comment.
issue (bug_risk): Calling methods with .call() may cause runtime errors if not callable.
Using .call() assumes these are callable objects, not standard methods. Remove .call and invoke them directly as methods: animationProvider.stopAllAnimations(); and animationProvider.initializeAnimation();.
Comment on lines
+79
to
+92
| // Set the text in the controller | ||
| inlineimagecontroller.text = badgeText; | ||
|
|
||
| // Set animation effects | ||
| if (message.flash) { | ||
| animationProvider.addEffect(effectMap[1]); // Flash effect | ||
| } | ||
| if (message.marquee) { | ||
| animationProvider.addEffect(effectMap[2]); // Marquee effect | ||
| } | ||
| // Set inversion if applicable | ||
| if (savedData.containsKey('invert') && savedData['invert'] == true) { | ||
| animationProvider.addEffect(effectMap[0]); // Invert effect | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Effect addition does not clear previous effects, which may cause effect leakage.
Currently, effects from previous badges may persist when loading new badge data. To prevent this, call animationProvider.clearAllEffects() before adding new effects.
Suggested change
| // Set the text in the controller | |
| inlineimagecontroller.text = badgeText; | |
| // Set animation effects | |
| if (message.flash) { | |
| animationProvider.addEffect(effectMap[1]); // Flash effect | |
| } | |
| if (message.marquee) { | |
| animationProvider.addEffect(effectMap[2]); // Marquee effect | |
| } | |
| // Set inversion if applicable | |
| if (savedData.containsKey('invert') && savedData['invert'] == true) { | |
| animationProvider.addEffect(effectMap[0]); // Invert effect | |
| } | |
| // Set the text in the controller | |
| inlineimagecontroller.text = badgeText; | |
| // Clear previous animation effects before adding new ones | |
| animationProvider.clearAllEffects(); | |
| // Set animation effects | |
| if (message.flash) { | |
| animationProvider.addEffect(effectMap[1]); // Flash effect | |
| } | |
| if (message.marquee) { | |
| animationProvider.addEffect(effectMap[2]); // Marquee effect | |
| } | |
| // Set inversion if applicable | |
| if (savedData.containsKey('invert') && savedData['invert'] == true) { | |
| animationProvider.addEffect(effectMap[0]); // Invert effect | |
| } |
Comment on lines
+93
to
+102
| // Set animation mode | ||
| int modeValue = 0; // Default to left animation | ||
| try { | ||
| // Handle different mode formats - could be enum or int | ||
| if (message.mode is int) { | ||
| modeValue = message.mode as int; | ||
| } else { | ||
| // Try to extract the mode value from the enum | ||
| String modeString = message.mode.toString(); | ||
| // If it's in format "Mode.left", extract just the mode name |
There was a problem hiding this comment.
issue (bug_risk): No validation for modeValue index may cause out-of-range errors.
Validate that modeValue is within the bounds of animationMap before using it as an index to prevent potential out-of-range errors from invalid or corrupted data.
Comment on lines
+61
to
+70
| try { | ||
| if (savedBadgeFilename != null) { | ||
| // Get the original text from BadgeTextStorage | ||
| badgeText = await BadgeTextStorage.getOriginalText(savedBadgeFilename); | ||
| // If we couldn't find the original text, use the filename as a fallback | ||
| if (badgeText.isEmpty) { | ||
| badgeText = | ||
| savedBadgeFilename.substring(0, savedBadgeFilename.length - 5); | ||
| // If the filename is a timestamp, use a generic text | ||
| if (badgeText.contains(":") && badgeText.contains("-")) { |
There was a problem hiding this comment.
suggestion (bug_risk): No validation for grid size may cause UI inconsistencies.
Validate the grid size after converting the hex string to a binary array to prevent rendering issues from malformed data.
Comment on lines
32
to
34
|
|
||
| static Future<File> _writeToFile(String filename, String data) async { | ||
| final path = await _getFilePath(filename); |
There was a problem hiding this comment.
suggestion (bug_risk): File locking is introduced for badge file operations, but not all file accesses are covered.
Other operations like listing or deleting badge files may not be synchronized. Please review all file access points to ensure thread safety.
Suggested implementation:
static Future<File> _writeToFile(String filename, String data) async {
final path = await _getFilePath(filename);
logger.d('Writing to file: $path');
return await _badgeFileLock.synchronized(() async {
return File(path).writeAsString(data);
static Future<String> _readFromFile(String filename) async {
final path = await _getFilePath(filename);
logger.d('Reading from file: $path');
return await _badgeFileLock.synchronized(() async {
return File(path).readAsString();
});
}
static Future<void> _deleteFile(String filename) async {
final path = await _getFilePath(filename);
logger.d('Deleting file: $path');
await _badgeFileLock.synchronized(() async {
final file = File(path);
if (await file.exists()) {
await file.delete();
}
});
}
static Future<List<FileSystemEntity>> _listBadgeFiles(String directory) async {
final dir = Directory(directory);
logger.d('Listing files in: $directory');
return await _badgeFileLock.synchronized(() async {
return dir.list().toList();
});
}
If there are other badge file operations (such as file renaming, copying, or moving) in this file, you should also wrap them in _badgeFileLock.synchronized(...) in a similar manner to ensure all badge file accesses are thread-safe.
Comment on lines
+46
to
+55
| class _HomeScreenState extends State<HomeScreen> | ||
| with | ||
| TickerProviderStateMixin, | ||
| AutomaticKeepAliveClientMixin, | ||
| WidgetsBindingObserver { | ||
| late final TabController _tabController; | ||
| AnimationBadgeProvider animationProvider = AnimationBadgeProvider(); | ||
| late SpeedDialProvider speedDialProvider; | ||
| BadgeMessageProvider badgeData = BadgeMessageProvider(); | ||
| ImageUtils imageUtils = ImageUtils(); |
There was a problem hiding this comment.
nitpick: Redefining inlineImageProvider in build may shadow the class property.
Consider renaming the local variable or using the class property to avoid confusion and potential bugs from shadowing.
There was a problem hiding this comment.
Hey @nope3472 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Calling methods with .call() may cause runtime errors if not callable. (link)
General comments:
- HomeScreen is handling multiple concerns and growing very large—consider extracting saved‐badge application logic and localization widget building into separate classes or widgets to improve readability and maintainability.
- In didChangeAppLifecycleState you’re invoking methods via
.call()(e.g.stopAllAnimations.call())—double-check that these methods are actually executed and replace with direct calls if necessary. - The routing setup mixes an
initialRoutewith a customonGenerateRoutethat repeats most routes—consider consolidating or simplifying your route definitions to avoid duplication and potential conflicts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- HomeScreen is handling multiple concerns and growing very large—consider extracting saved‐badge application logic and localization widget building into separate classes or widgets to improve readability and maintainability.
- In didChangeAppLifecycleState you’re invoking methods via `.call()` (e.g. `stopAllAnimations.call()`)—double-check that these methods are actually executed and replace with direct calls if necessary.
- The routing setup mixes an `initialRoute` with a custom `onGenerateRoute` that repeats most routes—consider consolidating or simplifying your route definitions to avoid duplication and potential conflicts.
## Individual Comments
### Comment 1
<location> `lib/view/homescreen.dart:437` </location>
<code_context>
+ if (state == AppLifecycleState.resumed) {
+ inlineimagecontroller.clear();
+ previousText = '';
+ animationProvider.stopAllAnimations.call(); // If method exists
+ animationProvider.initializeAnimation.call(); // If method exists
+ if (mounted) setState(() {});
+ } else if (state == AppLifecycleState.paused ||
</code_context>
<issue_to_address>
Calling methods with .call() may cause runtime errors if not callable.
Using `.call()` here will cause a runtime error if these are regular methods. Invoke them directly as `stopAllAnimations()` and `initializeAnimation()` instead.
</issue_to_address>
### Comment 2
<location> `lib/view/homescreen.dart:46` </location>
<code_context>
- @override
- Widget build(BuildContext context) {
- super.build(context);
- InlineImageProvider inlineImageProvider =
- Provider.of<InlineImageProvider>(context);
-
- return MultiProvider(
</code_context>
<issue_to_address>
Redefinition of inlineImageProvider in build may shadow the class field.
Consider renaming the local variable or using the class field to avoid confusion and potential bugs from shadowing.
</issue_to_address>
### Comment 3
<location> `lib/providers/saved_badge_provider.dart:82` </location>
<code_context>
+ // Set the text in the controller
+ inlineimagecontroller.text = badgeText;
+
+ // Set animation effects
+ if (message.flash) {
+ animationProvider.addEffect(effectMap[1]); // Flash effect
+ }
+ if (message.marquee) {
+ animationProvider.addEffect(effectMap[2]); // Marquee effect
+ }
+ // Set inversion if applicable
+ if (savedData.containsKey('invert') && savedData['invert'] == true) {
+ animationProvider.addEffect(effectMap[0]); // Invert effect
</code_context>
<issue_to_address>
Effect addition does not clear previous effects, possibly leading to duplicates.
Currently, effects are added without first clearing existing ones, which may cause duplicates if the provider is reused. Please clear existing effects before adding new ones.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Set animation effects
if (message.flash) {
animationProvider.addEffect(effectMap[1]); // Flash effect
}
if (message.marquee) {
animationProvider.addEffect(effectMap[2]); // Marquee effect
}
// Set inversion if applicable
if (savedData.containsKey('invert') && savedData['invert'] == true) {
animationProvider.addEffect(effectMap[0]); // Invert effect
}
=======
// Set animation effects
animationProvider.clearEffects(); // Clear previous effects to avoid duplicates
if (message.flash) {
animationProvider.addEffect(effectMap[1]); // Flash effect
}
if (message.marquee) {
animationProvider.addEffect(effectMap[2]); // Marquee effect
}
// Set inversion if applicable
if (savedData.containsKey('invert') && savedData['invert'] == true) {
animationProvider.addEffect(effectMap[0]); // Invert effect
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `lib/providers/saved_badge_provider.dart:93` </location>
<code_context>
+ // Set animation mode
</code_context>
<issue_to_address>
No null check for animationMap[modeValue] may cause runtime error.
Add validation or a fallback for `modeValue` to prevent passing null to `setAnimationMode`.
</issue_to_address>
### Comment 5
<location> `lib/providers/saved_badge_provider.dart:145` </location>
<code_context>
+ }
+ animationProvider.setAnimationMode(animationMap[modeValue]);
+
+ // Set speed using Speed.getIntValue to ensure correct dial value
+ try {
+ int speedDialValue = 1; // Default
+ // Use the static helper method to get the correct dial value
+ speedDialValue = Speed.getIntValue(message.speed);
+ logger.i("Setting speed dial to: $speedDialValue from �[33m");
+ speedDialProvider.setDialValue(speedDialValue);
+ } catch (e) {
+ logger.e("Failed to set speed dial value: $e");
+ speedDialProvider.setDialValue(1); // Fallback to default
+ }
+ // Store the filename for saving back to the same file
</code_context>
<issue_to_address>
No null check for message.speed may cause exception.
Since `Speed.getIntValue(message.speed)` can throw if `message.speed` is null or invalid, please add a null check or handle the exception appropriately.
</issue_to_address>
### Comment 6
<location> `lib/bademagic_module/utils/file_helper.dart:22` </location>
<code_context>
+
class FileHelper {
+ // Static lock to synchronize file access and prevent race conditions
+ static final Lock _badgeFileLock = Lock();
final InlineImageProvider imageCacheProvider =
GetIt.instance<InlineImageProvider>();
</code_context>
<issue_to_address>
File operations are now synchronized, but error handling is minimal.
Add try/catch blocks to handle and log file write errors to improve robustness.
</issue_to_address>
### Comment 7
<location> `lib/bademagic_module/utils/file_helper.dart:203` </location>
<code_context>
+ /// Save the original text for a badge
+ static Future<void> saveOriginalText(
+ String badgeFilename, String originalText) async {
+ try {
+ // Get the existing text storage or create a new one
+ Map<String, String> textStorage = await _getTextStorage();
</code_context>
<issue_to_address>
Logging for file read errors uses info instead of error level.
Use `logger.e` instead of `logger.i` to log file read errors for better error visibility.
</issue_to_address>
### Comment 8
<location> `lib/main.dart:33` </location>
<code_context>
}
-class MyApp extends StatelessWidget {
+class MyApp extends StatefulWidget {
const MyApp({super.key});
+ @override
+ State<MyApp> createState() => _MyAppState();
+}
+
</code_context>
<issue_to_address>
App locale is managed in state but not persisted.
Consider persisting the user's locale selection to maintain their language preference across app restarts.
Suggested implementation:
```
import 'package:provider/provider.dart';
import 'package:flutter_localizations/flutter_localizations.dart';
import 'package:shared_preferences/shared_preferences.dart';
import 'l10n/app_localizations.dart';
import 'globals/globals.dart' as globals;
```
```
class MyApp extends StatefulWidget {
const MyApp({super.key});
@override
State<MyApp> createState() => _MyAppState();
}
class _MyAppState extends State<MyApp> {
Locale? _locale;
@override
void initState() {
super.initState();
_loadLocale();
}
Future<void> _loadLocale() async {
final prefs = await SharedPreferences.getInstance();
final localeCode = prefs.getString('locale');
if (localeCode != null) {
setState(() {
_locale = Locale(localeCode);
});
}
}
Future<void> _setLocale(Locale locale) async {
final prefs = await SharedPreferences.getInstance();
await prefs.setString('locale', locale.languageCode);
setState(() {
_locale = locale;
});
}
@override
Widget build(BuildContext context) {
return MaterialApp(
locale: _locale,
supportedLocales: AppLocalizations.supportedLocales,
localizationsDelegates: const [
AppLocalizations.delegate,
GlobalMaterialLocalizations.delegate,
GlobalWidgetsLocalizations.delegate,
GlobalCupertinoLocalizations.delegate,
],
// Pass _setLocale to wherever the user can change the locale
// Example: home: HomeScreen(onLocaleChanged: _setLocale),
);
}
}
```
- You must ensure that wherever the user changes the locale (e.g., a language picker), you call `_setLocale(newLocale)`.
- If you already have a mechanism for changing the locale, update it to use `_setLocale`.
- Add `shared_preferences` to your `pubspec.yaml` dependencies if not already present.
</issue_to_address>
### Comment 9
<location> `lib/main.dart:73` </location>
<code_context>
+ onGenerateRoute: (settings) {
</code_context>
<issue_to_address>
onGenerateRoute returns null for unknown routes, which may cause blank screens.
Consider adding a fallback or error route to handle unknown routes and prevent blank screens.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+437
to
+438
| animationProvider.stopAllAnimations.call(); // If method exists | ||
| animationProvider.initializeAnimation.call(); // If method exists |
There was a problem hiding this comment.
issue (bug_risk): Calling methods with .call() may cause runtime errors if not callable.
Using .call() here will cause a runtime error if these are regular methods. Invoke them directly as stopAllAnimations() and initializeAnimation() instead.
Comment on lines
+46
to
+55
| with | ||
| TickerProviderStateMixin, | ||
| AutomaticKeepAliveClientMixin, | ||
| WidgetsBindingObserver { | ||
| late final TabController _tabController; | ||
| AnimationBadgeProvider animationProvider = AnimationBadgeProvider(); | ||
| late SpeedDialProvider speedDialProvider; | ||
| BadgeMessageProvider badgeData = BadgeMessageProvider(); | ||
| ImageUtils imageUtils = ImageUtils(); | ||
| InlineImageProvider inlineImageProvider = |
There was a problem hiding this comment.
nitpick: Redefinition of inlineImageProvider in build may shadow the class field.
Consider renaming the local variable or using the class field to avoid confusion and potential bugs from shadowing.
Comment on lines
+82
to
+92
| // Set animation effects | ||
| if (message.flash) { | ||
| animationProvider.addEffect(effectMap[1]); // Flash effect | ||
| } | ||
| if (message.marquee) { | ||
| animationProvider.addEffect(effectMap[2]); // Marquee effect | ||
| } | ||
| // Set inversion if applicable | ||
| if (savedData.containsKey('invert') && savedData['invert'] == true) { | ||
| animationProvider.addEffect(effectMap[0]); // Invert effect | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Effect addition does not clear previous effects, possibly leading to duplicates.
Currently, effects are added without first clearing existing ones, which may cause duplicates if the provider is reused. Please clear existing effects before adding new ones.
Suggested change
| // Set animation effects | |
| if (message.flash) { | |
| animationProvider.addEffect(effectMap[1]); // Flash effect | |
| } | |
| if (message.marquee) { | |
| animationProvider.addEffect(effectMap[2]); // Marquee effect | |
| } | |
| // Set inversion if applicable | |
| if (savedData.containsKey('invert') && savedData['invert'] == true) { | |
| animationProvider.addEffect(effectMap[0]); // Invert effect | |
| } | |
| // Set animation effects | |
| animationProvider.clearEffects(); // Clear previous effects to avoid duplicates | |
| if (message.flash) { | |
| animationProvider.addEffect(effectMap[1]); // Flash effect | |
| } | |
| if (message.marquee) { | |
| animationProvider.addEffect(effectMap[2]); // Marquee effect | |
| } | |
| // Set inversion if applicable | |
| if (savedData.containsKey('invert') && savedData['invert'] == true) { | |
| animationProvider.addEffect(effectMap[0]); // Invert effect | |
| } |
Comment on lines
+93
to
+102
| // Set animation mode | ||
| int modeValue = 0; // Default to left animation | ||
| try { | ||
| // Handle different mode formats - could be enum or int | ||
| if (message.mode is int) { | ||
| modeValue = message.mode as int; | ||
| } else { | ||
| // Try to extract the mode value from the enum | ||
| String modeString = message.mode.toString(); | ||
| // If it's in format "Mode.left", extract just the mode name |
There was a problem hiding this comment.
issue (bug_risk): No null check for animationMap[modeValue] may cause runtime error.
Add validation or a fallback for modeValue to prevent passing null to setAnimationMode.
Comment on lines
+145
to
+154
| // Set speed using Speed.getIntValue to ensure correct dial value | ||
| try { | ||
| int speedDialValue = 1; // Default | ||
| // Use the static helper method to get the correct dial value | ||
| speedDialValue = Speed.getIntValue(message.speed); | ||
| logger.i("Setting speed dial to: $speedDialValue from [33m"); | ||
| speedDialProvider.setDialValue(speedDialValue); | ||
| } catch (e) { | ||
| logger.e("Failed to set speed dial value: $e"); | ||
| speedDialProvider.setDialValue(1); // Fallback to default |
There was a problem hiding this comment.
issue (bug_risk): No null check for message.speed may cause exception.
Since Speed.getIntValue(message.speed) can throw if message.speed is null or invalid, please add a null check or handle the exception appropriately.
Comment on lines
32
to
34
|
|
||
| static Future<File> _writeToFile(String filename, String data) async { | ||
| final path = await _getFilePath(filename); |
There was a problem hiding this comment.
suggestion: File operations are now synchronized, but error handling is minimal.
Add try/catch blocks to handle and log file write errors to improve robustness.
Comment on lines
203
to
+212
| try { | ||
| final path = await _getFilePath(filename); | ||
| final file = File(path); | ||
|
|
||
| if (await file.exists()) { | ||
| final content = await file.readAsString(); | ||
| final dynamic decodedData = jsonDecode(content); | ||
|
|
||
| // Automatically return decoded JSON as a dynamic type | ||
| return decodedData; | ||
| } else { | ||
| logger.d('File not found: $filename'); | ||
| return null; | ||
| } | ||
| return await _badgeFileLock.synchronized(() async { | ||
| if (await file.exists()) { | ||
| final content = await file.readAsString(); | ||
| return jsonDecode(content); | ||
| } else { | ||
| logger.i('File not found: $path'); | ||
| return null; |
There was a problem hiding this comment.
nitpick: Logging for file read errors uses info instead of error level.
Use logger.e instead of logger.i to log file read errors for better error visibility.
Comment on lines
+33
to
+37
| class MyApp extends StatefulWidget { | ||
| const MyApp({super.key}); | ||
|
|
||
| @override | ||
| State<MyApp> createState() => _MyAppState(); |
There was a problem hiding this comment.
suggestion: App locale is managed in state but not persisted.
Consider persisting the user's locale selection to maintain their language preference across app restarts.
Suggested implementation:
import 'package:provider/provider.dart';
import 'package:flutter_localizations/flutter_localizations.dart';
import 'package:shared_preferences/shared_preferences.dart';
import 'l10n/app_localizations.dart';
import 'globals/globals.dart' as globals;
class MyApp extends StatefulWidget {
const MyApp({super.key});
@override
State<MyApp> createState() => _MyAppState();
}
class _MyAppState extends State<MyApp> {
Locale? _locale;
@override
void initState() {
super.initState();
_loadLocale();
}
Future<void> _loadLocale() async {
final prefs = await SharedPreferences.getInstance();
final localeCode = prefs.getString('locale');
if (localeCode != null) {
setState(() {
_locale = Locale(localeCode);
});
}
}
Future<void> _setLocale(Locale locale) async {
final prefs = await SharedPreferences.getInstance();
await prefs.setString('locale', locale.languageCode);
setState(() {
_locale = locale;
});
}
@override
Widget build(BuildContext context) {
return MaterialApp(
locale: _locale,
supportedLocales: AppLocalizations.supportedLocales,
localizationsDelegates: const [
AppLocalizations.delegate,
GlobalMaterialLocalizations.delegate,
GlobalWidgetsLocalizations.delegate,
GlobalCupertinoLocalizations.delegate,
],
// Pass _setLocale to wherever the user can change the locale
// Example: home: HomeScreen(onLocaleChanged: _setLocale),
);
}
}
- You must ensure that wherever the user changes the locale (e.g., a language picker), you call
_setLocale(newLocale). - If you already have a mechanism for changing the locale, update it to use
_setLocale. - Add
shared_preferencesto yourpubspec.yamldependencies if not already present.
Comment on lines
+73
to
+82
| onGenerateRoute: (settings) { | ||
| if (settings.name == '/settings') { | ||
| return MaterialPageRoute( | ||
| builder: (context) => SettingsScreen( | ||
| onLocaleChange: setLocale, | ||
| ), | ||
| ); | ||
| } | ||
| // fallback to default | ||
| final routes = <String, WidgetBuilder>{ |
There was a problem hiding this comment.
suggestion (bug_risk): onGenerateRoute returns null for unknown routes, which may cause blank screens.
Consider adding a fallback or error route to handle unknown routes and prevent blank screens.
added 10 commits
June 20, 2025 10:08
|
@nope3472 we were thinking of integrating weblate for language suppport if I am right @adityastic |
Yep, that's correct |
|
@nope3472 Will you please Clean your all commits by sqashing then or using interactive rebase ? |
|
Hey @adityastic, I checked out Weblate for our translations, but as a contributor I can’t spin up a new project there. Only the owner or maintainer can do that, and for open-source projects we even need the Weblate team to sign off. |
|
@nope3472 will you please resolve common build and Failing CI on this PR ? also there are conflicts i think. |
There was a problem hiding this comment.
@nope3472 Do we need to translate everything in our app into another language like this ? Is there any thing better other than weblate or are you working on weblate ?
There was a problem hiding this comment.
I am taking care of weblate. Will add your to the FOSSASIA team on Weblate after sorting out an issue there.
mariobehling
requested changes
Jul 14, 2025
|
@nope3472 please update the branch to get it ready to merge |
samruddhi-Rahegaonkar
suggested changes
Jul 31, 2025
There was a problem hiding this comment.
@nope3472 Go through the Sourcery Comments and Please resolve it and the failing Build then we can move forward this PR.
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
Fixes #<1332>
Changes
app_localizations.dartand relevant files for multi-language support.l10n.yamlfor supported languages.Screenshots / Recordings
Checklist
constants.dartwithout hard-coding any values.flutter analyze) and tests (flutter test).Summary by Sourcery
Add multi-language support and enable editing of saved badges, refactoring file I/O and providers to improve reliability and separation of concerns
New Features:
Bug Fixes:
Enhancements:
Build: