Skip to content

Fix SQL calls (android), library screen for narrow displays#202

Open
Y-PLONI wants to merge 5 commits intomigrationDB_V2from
migrationDB_V2+YPL
Open

Fix SQL calls (android), library screen for narrow displays#202
Y-PLONI wants to merge 5 commits intomigrationDB_V2from
migrationDB_V2+YPL

Conversation

@Y-PLONI
Copy link
Collaborator

@Y-PLONI Y-PLONI commented Mar 3, 2026

No description provided.

@gwngdwl
Copy link
Collaborator

gwngdwl commented Mar 4, 2026

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request effectively addresses the stated goals of fixing SQL calls for PRAGMA statements on Android/iOS and improving the library screen's responsiveness for narrow displays. The changes to the SeforimRepository correctly differentiate between rawQuery and execute for PRAGMA statements on specific platforms, which is a good practice for database interactions. UI adjustments in CustomTitleBar and _MemorialCardsGrid enhance the user experience on smaller screens by optimizing button sizes, spacing, and grid layout. Additionally, the default value for libraryShowPreview has been consistently updated across the application's settings, state, and tests.

@Y-PLONI Y-PLONI force-pushed the migrationDB_V2+YPL branch from 38ec1e3 to c59e3e4 Compare March 4, 2026 11:48
@Y-PLONI
Copy link
Collaborator Author

Y-PLONI commented Mar 4, 2026

@gemini-code-assist
צור סקירת קוד חדשה...

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces fixes for SQL calls on Android and improves the UI for narrow displays. The changes correctly handle PRAGMA statements on mobile platforms and add a fullscreen button for Android, which enhances usability on smaller screens. However, I've identified a couple of instances of code duplication that should be refactored to improve code maintainability. My detailed comments are below.

Note: Security Review did not run due to the size of the PR.

Comment on lines +489 to +511
IconButton(
icon: Icon(
settingsState.isFullscreen
? FluentIcons.full_screen_minimize_24_regular
: FluentIcons.full_screen_maximize_24_regular,
size: 18,
),
tooltip: settingsState.isFullscreen
? 'צא ממסך מלא'
: 'מסך מלא',
onPressed: () async {
final newFullscreenState = !settingsState.isFullscreen;
await FullscreenHelper.toggleFullscreen(
context,
newFullscreenState,
);
},
style: _kIconButtonStyle.copyWith(
foregroundColor: WidgetStatePropertyAll(
Theme.of(context).colorScheme.onSurfaceVariant,
),
),
),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This IconButton's implementation is a copy of the _buildAndroidFullscreenButton widget. Duplicating this logic can lead to maintenance issues if the button's behavior needs to be updated in the future. Please replace this with a call to the existing _buildAndroidFullscreenButton method to ensure code reusability.

Suggested change
IconButton(
icon: Icon(
settingsState.isFullscreen
? FluentIcons.full_screen_minimize_24_regular
: FluentIcons.full_screen_maximize_24_regular,
size: 18,
),
tooltip: settingsState.isFullscreen
? 'צא ממסך מלא'
: 'מסך מלא',
onPressed: () async {
final newFullscreenState = !settingsState.isFullscreen;
await FullscreenHelper.toggleFullscreen(
context,
newFullscreenState,
);
},
style: _kIconButtonStyle.copyWith(
foregroundColor: WidgetStatePropertyAll(
Theme.of(context).colorScheme.onSurfaceVariant,
),
),
),
_buildAndroidFullscreenButton(context, settingsState),

Comment on lines 1832 to 1843
Future<void> executeRawQuery(String sql) async {
final db = await _database.database;
final normalizedSql = sql.trim().toUpperCase();
final isPragma = normalizedSql.startsWith('PRAGMA ');

if (isPragma && (Platform.isAndroid || Platform.isIOS)) {
await db.rawQuery(sql);
return;
}

await db.execute(sql);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic in this method is now identical to _executeRawQuery after the changes. To avoid code duplication and improve maintainability, consider refactoring this. For example, this method could simply call _executeRawQuery(sql).

@Y-PLONI Y-PLONI force-pushed the migrationDB_V2+YPL branch from 1b8d25b to 9638a4f Compare March 5, 2026 11:33
@gwngdwl gwngdwl force-pushed the migrationDB_V2+YPL branch from ddaa291 to b907b55 Compare March 5, 2026 15:29
@Y-PLONI Y-PLONI force-pushed the migrationDB_V2+YPL branch from b907b55 to c26aae8 Compare March 12, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants