-
Notifications
You must be signed in to change notification settings - Fork 348
Add Additional App Settings #566
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
Conversation
WalkthroughIntroduces app-wide preferences for language and Whisper transcription model. Adds persistent storage for selected locale and model on startup, a new App Preferences screen for changing them, routing to that screen, localization keys/getters for related UI, and updates the transcription controller to use the current selected Whisper model. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (main.dart)
participant Storage as SecureStorage
participant GetX as GetX/Locale
participant Const as constants.dart
participant Ctrl as WhisperTranscriptionController
App->>Storage: read("languageLocale")
Storage-->>App: locale or null
App->>GetX: set Locale (default "en" if null)
App->>Storage: read("whisperModel")
Storage-->>App: modelName or null
App->>Const: set currentWhisperModel (from name or base)
App->>Ctrl: initialize controller
Ctrl->>Const: use currentWhisperModel.value
sequenceDiagram
autonumber
participant User
participant Pref as AppPreferencesScreen
participant Const as constants.dart
participant Storage as SecureStorage
participant UI as App UI
User->>Pref: Select language
Pref->>UI: Get.updateLocale(Locale(code))
Pref->>Storage: write("languageLocale", code)
User->>Pref: Select Whisper model
Pref->>Const: currentWhisperModel.value = selected
Pref->>Storage: write("whisperModel", selected.name)
Note right of Pref: UI reflects selected model (card state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🎉 Welcome @M4dhav!
We appreciate your contribution! 🚀 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (5)
lib/utils/constants.dart (1)
107-108: Document and encapsulate global state (lib/utils/constants.dart:107-108)
- Add doc comments explaining the initialization flow and persistence of
languageLocaleandcurrentWhisperModel.- Move these into an
AppPreferencesController(GetxController) that initializes, owns, and persists them beforerunApp.lib/main.dart (1)
61-69: Harden and streamline secure storage reads (reuse instance + try/catch)Avoid multiple allocations and handle potential PlatformExceptions gracefully.
Apply:
- languageLocale = - await FlutterSecureStorage().read(key: "languageLocale") ?? "en"; - final String? savedModel = await FlutterSecureStorage().read( - key: "whisperModel", - ); + final storage = FlutterSecureStorage(); + try { + languageLocale = await storage.read(key: "languageLocale") ?? "en"; + final String? savedModel = await storage.read(key: "whisperModel"); + currentWhisperModel.value = WhisperModel.values.firstWhere( + (model) => model.modelName == (savedModel ?? "base"), + orElse: () => WhisperModel.base, + ); + } catch (_) { + languageLocale = "en"; + currentWhisperModel.value = WhisperModel.base; + } - currentWhisperModel.value = WhisperModel.values.firstWhere( - (model) => model.modelName == (savedModel ?? "base"), - orElse: () => WhisperModel.base, - );lib/views/screens/landing_screen.dart (1)
50-67: Mark constant widgets as const at call sitesSmall perf/readability win; constructor is const.
- LandingImage( + const LandingImage( imagePath: AppImages.landingFirstImage, initialHeight: UiSizes.height_82, imageHeight: UiSizes.height_246, imageWidth: UiSizes.width_45, ), - LandingImage( + const LandingImage( imagePath: AppImages.landingSecondImage, initialHeight: UiSizes.height_90, imageHeight: UiSizes.height_246, imageWidth: UiSizes.width_40, ), - LandingImage( + const LandingImage( imagePath: AppImages.landingThirdImage, initialHeight: UiSizes.height_90, imageHeight: UiSizes.height_246, imageWidth: UiSizes.width_56, ),lib/views/screens/app_preferences_screen.dart (2)
110-110: Prefer localized “App Preferences” titleUse the new key to match screen purpose.
- appBar: AppBar(title: Text(AppLocalizations.of(context)!.appSettings)), + appBar: + AppBar(title: Text(AppLocalizations.of(context)!.appPreferences)),
139-145: Reuse a single FlutterSecureStorage instanceAvoid repeated allocations inside callbacks.
Add to _AppPreferencesScreenState:
final _storage = FlutterSecureStorage();Then update writes:
- await FlutterSecureStorage().write( + await _storage.write( key: "languageLocale", value: language.isoCode, );- await FlutterSecureStorage().write( + await _storage.write( key: "whisperModel", value: modelData['model'].modelName, );Also applies to: 210-214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
lib/controllers/whisper_transcription_controller.dart(1 hunks)lib/l10n/app_en.arb(1 hunks)lib/l10n/app_hi.arb(1 hunks)lib/l10n/app_localizations.dart(1 hunks)lib/l10n/app_localizations_en.dart(1 hunks)lib/l10n/app_localizations_gu.dart(1 hunks)lib/l10n/app_localizations_hi.dart(1 hunks)lib/main.dart(3 hunks)lib/routes/app_pages.dart(2 hunks)lib/routes/app_routes.dart(1 hunks)lib/utils/constants.dart(2 hunks)lib/views/screens/app_preferences_screen.dart(1 hunks)lib/views/screens/landing_screen.dart(1 hunks)lib/views/screens/settings_screen.dart(1 hunks)project_structure.txt(2 hunks)untranslated.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Software Tests
🔇 Additional comments (10)
lib/routes/app_pages.dart (1)
148-151: LGTM!The route configuration is correct and follows the existing pattern. The use of a const constructor is appropriate for this stateless screen.
lib/views/screens/settings_screen.dart (1)
84-89: LGTM!The App Preferences menu item is correctly integrated into the Settings screen, following the existing pattern and properly placed in the App Settings section.
lib/routes/app_routes.dart (1)
37-37: LGTM!The route constant is properly defined and follows the existing naming convention.
untranslated.txt (1)
3-18: Gujarati translations are incomplete.The untranslated keys are properly tracked. Users selecting Gujarati locale will see untranslated English strings for the new App Preferences features until these translations are completed.
Consider creating a follow-up issue to complete the Gujarati translations for the new App Preferences and Whisper model options.
lib/l10n/app_localizations_hi.dart (1)
319-369: Hindi localizations are properly structured.All new localization getters are correctly implemented with proper
@overrideannotations. The translations include technical details such as model sizes, which is helpful for users.Note: Translation accuracy cannot be verified without Hindi language expertise.
lib/l10n/app_hi.arb (1)
405-468: New HI localization keys look consistent and completeKeys and descriptions align with EN; no structural issues spotted.
lib/l10n/app_localizations_gu.dart (1)
319-371: Gujarati getters for new preferences/model keys added correctlySignatures align with AppLocalizations; fine to translate later.
Please confirm if a translation pass is planned for these new strings.
lib/l10n/app_localizations.dart (1)
697-792: Abstract API extended; ensure all locales implement new gettersConfirm app_localizations_hi.dart implements these, or builds will fail.
lib/l10n/app_localizations_en.dart (1)
320-372: EN implementations for new preference/model strings look goodMatches ARB and abstract interface.
lib/l10n/app_en.arb (1)
405-468: New EN localization keys are well-formed and completeEntries and descriptions align with usage across the app.
| final Whisper whisper = Whisper( | ||
| model: WhisperModel.base, | ||
| model: currentWhisperModel.value, | ||
| downloadHost: "https://huggingface.co/ggerganov/whisper.cpp/resolve/main", | ||
| ); |
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.
Whisper model changes won't take effect until controller is recreated.
The Whisper instance is initialized once with currentWhisperModel.value at construction time. If a user changes the Whisper model preference in the App Preferences screen, the change won't take effect for this controller until the app restarts or the controller is recreated.
Consider one of these approaches:
Solution 1: Make Whisper instance reactive (preferred if whisper_flutter_new supports it):
class WhisperTranscriptionController extends GetxController {
- final Whisper whisper = Whisper(
- model: currentWhisperModel.value,
- downloadHost: "https://huggingface.co/ggerganov/whisper.cpp/resolve/main",
- );
+ late Whisper whisper;
+
+ @override
+ void onInit() {
+ super.onInit();
+ _initWhisper();
+ ever(currentWhisperModel, (_) => _initWhisper());
+ }
+
+ void _initWhisper() {
+ whisper = Whisper(
+ model: currentWhisperModel.value,
+ downloadHost: "https://huggingface.co/ggerganov/whisper.cpp/resolve/main",
+ );
+ }Solution 2: Recreate on each transcription (simpler but may have performance implications):
class WhisperTranscriptionController extends GetxController {
- final Whisper whisper = Whisper(
+ Whisper _createWhisper() => Whisper(
model: currentWhisperModel.value,
downloadHost: "https://huggingface.co/ggerganov/whisper.cpp/resolve/main",
);
Future<String> transcribeChapter(String chapterId) async {
log('Starting to Transcribe');
final convertResult = await convertMp4ToWav(chapterId);
if (convertResult) {
final storagePath = await getApplicationDocumentsDirectory();
+ final whisper = _createWhisper();
final transcription = await whisper.transcribe(🤖 Prompt for AI Agents
In lib/controllers/whisper_transcription_controller.dart around lines 11 to 14,
the Whisper instance is created once with currentWhisperModel.value so changes
to the preference won't take effect; fix by making the Whisper client follow the
reactive model or by recreating it when needed: either (preferred) track
currentWhisperModel changes (add a listener/obx/reactive binding) and
rebuild/reassign the Whisper instance whenever the model value changes
(dispose/close the old instance if required), or (simpler) remove the long-lived
Whisper field and instantiate a new Whisper with the currentWhisperModel.value
each time you start a transcription (ensuring any streaming/cleanup is handled
to avoid leaks); choose one approach and implement proper disposal/exception
handling.
| color: Theme.of(context).brightness == Brightness.light | ||
| ? Colors.black.withValues(alpha: 0.04) | ||
| : Colors.white.withValues(alpha: 0.04), | ||
| ); |
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.
Color.withValues causes compile errors; use withOpacity
Color does not expose withValues; replace with withOpacity.
- color: Theme.of(context).brightness == Brightness.light
- ? Colors.black.withValues(alpha: 0.04)
- : Colors.white.withValues(alpha: 0.04),
+ color: Theme.of(context).brightness == Brightness.light
+ ? Colors.black.withOpacity(0.04)
+ : Colors.white.withOpacity(0.04),- ).colorScheme.outline.withValues(alpha: 0.3),
+ ).colorScheme.outline.withOpacity(0.3),- .onPrimaryContainer
- .withValues(alpha: 0.7)
+ .onPrimaryContainer
+ .withOpacity(0.7)Also applies to: 128-133, 253-256
🤖 Prompt for AI Agents
In lib/views/screens/app_preferences_screen.dart around lines 101-104 (and also
update lines 128-133 and 253-256), the code calls non-existent Color.withValues
which causes compile errors; replace those calls with Color.withOpacity and pass
the same alpha value as a decimal between 0.0 and 1.0 (e.g., 0.04) so the color
construction uses withOpacity instead of withValues. Ensure you update all three
locations to use .withOpacity(alpha) on the base color expression.
|
✅ PR Closed - Thank You, @M4dhav!
We appreciate your effort and look forward to more contributions from you! 🤝 |
Description
This PR Adds additional settings to the App Setting which allows users to change the App Locales and Model used for transcriptions.
Related to #563 and #482
Type of change
How Has This Been Tested?
Software tests are passing and functionalities are working
Checklist:
Maintainer Checklist
Summary by CodeRabbit
New Features
Localization
Improvements