From c953168eb28a0863e005ad263506efcae27d7acb Mon Sep 17 00:00:00 2001 From: JP Date: Sat, 30 May 2026 21:20:04 -0700 Subject: [PATCH] Fix 6 code review issues: subtitle robustness, user feedback, lint warnings Critical fixes: - Fixed state mutation in remember block (PlayerScreen.kt): extracted subtitle detection to testable function - Fixed regex case-sensitivity: changed from ([a-z]{2}) to (?i)([a-zA-Z]{2}) to match uppercase language codes - Fixed empty displayLanguage fallback: properly fall back to language code for unknown locales - Removed unused baseNameNoExt variable Medium fixes: - Added snackbar confirmation when sample torrents are added (MainScreen.kt) - Converted SharedPreferences.edit() to KTX extension for cleaner syntax Code quality improvements: - Created PlayerScreenTest with 6 unit tests for subtitle detection logic - Fixed all lint warnings: removed redundant qualifiers, suppressed Java deprecation warnings - Converted legacy Long delay to Duration (3.seconds) - Converted redundant boolean comparisons to range check - Updated README.md with Recent Improvements section All 24 unit tests pass, lint clean, tested on R37 emulator. --- README.md | 6 ++ .../simpletorrent/data/DataRepository.kt | 3 +- .../com/jpcottin/simpletorrent/theme/Theme.kt | 2 + .../simpletorrent/ui/main/MainScreen.kt | 48 +++++---- .../simpletorrent/ui/player/PlayerScreen.kt | 97 +++++++++++-------- .../ui/player/PlayerScreenTest.kt | 95 ++++++++++++++++++ 6 files changed, 191 insertions(+), 60 deletions(-) create mode 100644 app/src/test/java/com/jpcottin/simpletorrent/ui/player/PlayerScreenTest.kt diff --git a/README.md b/README.md index 6920072..6d2d2f7 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,12 @@ Android NDK and the libtorrent-rasterbar C++ library. - Adaptive multi-column grid — 1 column on phones, 2 on foldables, 3 on tablets - Full edge-to-edge UI with proper IME insets, Material You theming +## Recent Improvements + +- **Subtitle support robustness**: Case-insensitive language code matching, proper fallback for unknown locales, fixed Compose state mutation +- **User feedback**: Sample torrents now show confirmation snackbar when added +- **Code quality**: Extracted testable subtitle detection logic, added unit tests for edge cases + ## Demo https://github.com/user-attachments/assets/6182efdb-9862-47ee-9fa7-011a64ee87eb diff --git a/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt b/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt index 29b7248..8dd0678 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt @@ -1,5 +1,6 @@ package com.jpcottin.simpletorrent.data +import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flow @@ -19,7 +20,7 @@ class DefaultDataRepository : DataRepository { override val torrents: Flow> = flow { while (true) { emit(TorrentManager.getTorrents()) - delay(3_000L) + delay(3.seconds) } } diff --git a/app/src/main/java/com/jpcottin/simpletorrent/theme/Theme.kt b/app/src/main/java/com/jpcottin/simpletorrent/theme/Theme.kt index cd8f484..e225bb7 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/theme/Theme.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/theme/Theme.kt @@ -1,5 +1,6 @@ package com.jpcottin.simpletorrent.theme +import android.annotation.SuppressLint import android.os.Build import androidx.compose.foundation.isSystemInDarkTheme import androidx.compose.material3.MaterialTheme @@ -29,6 +30,7 @@ private val LightColorScheme = */ ) +@SuppressLint("NewApi") @Composable fun SimpleTorrentTheme( darkTheme: Boolean = isSystemInDarkTheme(), diff --git a/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt b/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt index 6509c40..07dd26c 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt @@ -15,43 +15,46 @@ import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.PaddingValues import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.WindowInsets +import androidx.compose.foundation.layout.consumeWindowInsets import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height -import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.safeDrawing +import androidx.compose.foundation.layout.size import androidx.compose.foundation.lazy.grid.GridCells import androidx.compose.foundation.lazy.grid.LazyVerticalGrid import androidx.compose.foundation.lazy.grid.items -import androidx.compose.foundation.layout.WindowInsets -import androidx.compose.foundation.layout.consumeWindowInsets -import androidx.compose.foundation.layout.safeDrawing -import androidx.compose.material3.Scaffold import androidx.compose.foundation.text.KeyboardActions import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.Add import androidx.compose.material.icons.filled.Delete import androidx.compose.material.icons.filled.Description -import androidx.compose.material.icons.filled.PlayCircle import androidx.compose.material.icons.filled.ExpandLess import androidx.compose.material.icons.filled.ExpandMore import androidx.compose.material.icons.filled.FolderOpen import androidx.compose.material.icons.filled.Pause import androidx.compose.material.icons.filled.PlayArrow -import androidx.compose.material.icons.filled.Share +import androidx.compose.material.icons.filled.PlayCircle import androidx.compose.material.icons.filled.PlaylistAdd +import androidx.compose.material.icons.filled.Share import androidx.compose.material3.AlertDialog -import androidx.compose.material3.ExperimentalMaterial3Api -import androidx.compose.material3.ModalBottomSheet +import androidx.compose.material3.Button import androidx.compose.material3.Card import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.DropdownMenu import androidx.compose.material3.DropdownMenuItem +import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.material3.HorizontalDivider import androidx.compose.material3.Icon import androidx.compose.material3.IconButton import androidx.compose.material3.LinearProgressIndicator +import androidx.compose.material3.ModalBottomSheet import androidx.compose.material3.OutlinedTextField +import androidx.compose.material3.Scaffold +import androidx.compose.material3.SnackbarHost +import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable @@ -74,9 +77,11 @@ import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp import androidx.core.content.FileProvider +import androidx.core.content.edit import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.viewmodel.compose.viewModel import androidx.navigation3.runtime.NavKey +import com.jpcottin.simpletorrent.Player import com.jpcottin.simpletorrent.data.DefaultDataRepository import com.jpcottin.simpletorrent.data.FileInfo import com.jpcottin.simpletorrent.data.PeerInfo @@ -99,9 +104,11 @@ fun MainScreen( val createState by viewModel.createState.collectAsStateWithLifecycle() var magnetInput by remember { mutableStateOf("") } var showSampleSheet by remember { mutableStateOf(false) } + val snackbarHostState = remember { SnackbarHostState() } val context = LocalContext.current + val scope = rememberCoroutineScope() - fun resolveTreeToPath(uri: android.net.Uri): String? { + fun resolveTreeToPath(uri: Uri): String? { val treeDocId = android.provider.DocumentsContract.getTreeDocumentId(uri) val colon = treeDocId.indexOf(':') if (colon < 0) return null @@ -200,6 +207,7 @@ fun MainScreen( Scaffold( modifier = modifier, contentWindowInsets = WindowInsets.safeDrawing, + snackbarHost = { SnackbarHost(snackbarHostState) }, ) { innerPadding -> Column( modifier = Modifier @@ -247,17 +255,17 @@ fun MainScreen( onPause = { viewModel.pause(torrent.infoHash) }, onResume = { viewModel.resume(torrent.infoHash) }, onRemove = { - val savePath = com.jpcottin.simpletorrent.data.TorrentManager.currentSavePath - val prefs = context.getSharedPreferences("player_positions", android.content.Context.MODE_PRIVATE) - prefs.edit().apply { + val savePath = TorrentManager.currentSavePath + val prefs = context.getSharedPreferences("player_positions", Context.MODE_PRIVATE) + prefs.edit { torrent.fileList.forEach { remove("$savePath/${it.name}") } - }.apply() + } viewModel.remove(torrent.infoHash, deleteFiles = true) }, onPlay = { relPath, title -> viewModel.setSequentialDownload(torrent.infoHash, true) - val savePath = com.jpcottin.simpletorrent.data.TorrentManager.currentSavePath - onItemClick(com.jpcottin.simpletorrent.Player("$savePath/$relPath", title)) + val savePath = TorrentManager.currentSavePath + onItemClick(Player("$savePath/$relPath", title)) }, ) } @@ -274,6 +282,9 @@ fun MainScreen( onAdd = { magnet -> showSampleSheet = false viewModel.addMagnet(magnet) + scope.launch { + snackbarHostState.showSnackbar("Adding torrent to downloads") + } }, ) } @@ -305,8 +316,9 @@ internal fun MagnetInputBar( keyboardOptions = KeyboardOptions(imeAction = ImeAction.Done), keyboardActions = KeyboardActions(onDone = { onAdd() }), ) - androidx.compose.material3.Button(onClick = onAdd) { Text("Add") } + Button(onClick = onAdd) { Text("Add") } IconButton(onClick = onSampleTorrents) { + @Suppress("DEPRECATION") Icon(Icons.Filled.PlaylistAdd, contentDescription = "Sample torrents") } Box { @@ -495,7 +507,7 @@ internal fun TorrentCard( } // Play / buffering indicator for single-file playable torrents if (torrent.fileList.size == 1 && torrent.fileList[0].name.isPlayable()) { - if (torrent.progress >= 0.01f && torrent.progress < 0.05f) { + if (torrent.progress in 0.01f..<0.05f) { CircularProgressIndicator(modifier = Modifier.size(24.dp).padding(2.dp)) } } diff --git a/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt b/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt index ddf92d7..67df0ac 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt @@ -17,6 +17,7 @@ import androidx.compose.material3.DropdownMenu import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.Icon import androidx.compose.material3.IconButton +import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.getValue @@ -24,18 +25,18 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment -import androidx.compose.ui.platform.LocalInspectionMode -import androidx.compose.ui.tooling.preview.Preview -import androidx.media3.common.Player import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Color import androidx.compose.ui.platform.LocalContext +import androidx.compose.ui.platform.LocalInspectionMode +import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.viewinterop.AndroidView +import androidx.core.content.edit import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleEventObserver import androidx.lifecycle.compose.LocalLifecycleOwner -import androidx.compose.material3.Text import androidx.media3.common.MediaItem +import androidx.media3.common.Player import androidx.media3.common.util.UnstableApi import androidx.media3.exoplayer.ExoPlayer import androidx.media3.ui.PlayerView @@ -52,33 +53,31 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { val prefs = remember { context.getSharedPreferences("player_positions", Context.MODE_PRIVATE) } var isBuffering by remember { mutableStateOf(true) } var showSubtitleMenu by remember { mutableStateOf(false) } - var subtitles by remember { mutableStateOf>>(emptyList()) } - val player = remember { - if (isPreview) return@remember null + val (player, subtitles) = remember(filePath) { + if (isPreview) return@remember null to emptyList>() val videoFile = File(filePath) + val srtConfigs = mutableListOf() val videoDir = videoFile.parentFile - val baseNameNoExt = videoFile.nameWithoutExtension + val subtitleLabels = findSubtitles(videoDir) - // Find all .srt files in the same directory - val srtConfigs = videoDir?.listFiles { file -> - file.isFile && file.extension == "srt" - }?.mapNotNull { srtFile -> - // Extract language code from filename pattern: *_xx.srt or subtitle_xx.srt - val match = Regex("([a-z]{2})\\.srt$").find(srtFile.name) + videoDir?.listFiles { file -> file.isFile && file.extension == "srt" }?.forEach { srtFile -> + val match = Regex("(?i)([a-zA-Z]{2})\\.srt$").find(srtFile.name) if (match != null) { - val languageCode = match.groupValues[1] - val displayLanguage = Locale(languageCode).displayLanguage - subtitles = subtitles + (languageCode to displayLanguage) - MediaItem.SubtitleConfiguration.Builder(Uri.fromFile(srtFile)) - .setMimeType("application/x-subrip") - .setLanguage(languageCode) - .setLabel(displayLanguage) - .build() - } else null - } ?: emptyList() + val languageCode = match.groupValues[1].lowercase() + @Suppress("DEPRECATION") + val displayLanguage = Locale(languageCode).displayLanguage.takeIf { it.isNotEmpty() } ?: languageCode + srtConfigs.add( + MediaItem.SubtitleConfiguration.Builder(Uri.fromFile(srtFile)) + .setMimeType("application/x-subrip") + .setLanguage(languageCode) + .setLabel(displayLanguage) + .build() + ) + } + } - ExoPlayer.Builder(context).build().apply { + val exoPlayer = ExoPlayer.Builder(context).build().apply { val mediaItem = MediaItem.Builder() .setUri(Uri.fromFile(videoFile)) .setSubtitleConfigurations(srtConfigs) @@ -90,36 +89,38 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { if (savedPosition > 0L) seekTo(savedPosition) playWhenReady = true } + exoPlayer to subtitleLabels } + val actualPlayer = player - DisposableEffect(player) { - if (player == null) return@DisposableEffect onDispose {} + DisposableEffect(actualPlayer) { + if (actualPlayer == null) return@DisposableEffect onDispose {} val listener = object : Player.Listener { override fun onPlaybackStateChanged(state: Int) { isBuffering = state == Player.STATE_BUFFERING } } - player.addListener(listener) - onDispose { player.removeListener(listener) } + actualPlayer.addListener(listener) + onDispose { actualPlayer.removeListener(listener) } } - DisposableEffect(lifecycleOwner, player) { - if (player == null) return@DisposableEffect onDispose {} + DisposableEffect(lifecycleOwner, actualPlayer) { + if (actualPlayer == null) return@DisposableEffect onDispose {} val observer = LifecycleEventObserver { _, event -> when (event) { Lifecycle.Event.ON_PAUSE -> { - prefs.edit().putLong(filePath, player.currentPosition).apply() - player.pause() + prefs.edit { putLong(filePath, actualPlayer.currentPosition) } + actualPlayer.pause() } - Lifecycle.Event.ON_RESUME -> player.play() + Lifecycle.Event.ON_RESUME -> actualPlayer.play() else -> {} } } lifecycleOwner.lifecycle.addObserver(observer) onDispose { - prefs.edit().putLong(filePath, player.currentPosition).apply() + prefs.edit { putLong(filePath, actualPlayer.currentPosition) } lifecycleOwner.lifecycle.removeObserver(observer) - player.release() + actualPlayer.release() } } @@ -128,11 +129,11 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { .fillMaxSize() .background(Color.Black), ) { - if (player != null) { + if (actualPlayer != null) { AndroidView( factory = { ctx -> PlayerView(ctx).apply { - this.player = player + this.player = actualPlayer setShowNextButton(false) setShowPreviousButton(false) } @@ -164,7 +165,7 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { tint = Color.White, ) } - if (subtitles.isNotEmpty() && player != null) { + if (subtitles.isNotEmpty() && actualPlayer != null) { Box( modifier = Modifier .align(Alignment.TopEnd) @@ -184,7 +185,7 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { DropdownMenuItem( text = { Text("None") }, onClick = { - player.trackSelectionParameters = player.trackSelectionParameters.buildUpon() + actualPlayer.trackSelectionParameters = actualPlayer.trackSelectionParameters.buildUpon() .setPreferredTextLanguages() .build() showSubtitleMenu = false @@ -194,7 +195,7 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { DropdownMenuItem( text = { Text(language) }, onClick = { - player.trackSelectionParameters = player.trackSelectionParameters.buildUpon() + actualPlayer.trackSelectionParameters = actualPlayer.trackSelectionParameters.buildUpon() .setPreferredTextLanguages(languageCode) .build() showSubtitleMenu = false @@ -207,6 +208,20 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { } } +internal fun findSubtitles(videoDir: File?): List> { + val subtitleLabels = mutableListOf>() + videoDir?.listFiles { file -> file.isFile && file.extension == "srt" }?.forEach { srtFile -> + val match = Regex("(?i)([a-zA-Z]{2})\\.srt$").find(srtFile.name) + if (match != null) { + val languageCode = match.groupValues[1].lowercase() + @Suppress("DEPRECATION") + val displayLanguage = Locale(languageCode).displayLanguage.takeIf { it.isNotEmpty() } ?: languageCode + subtitleLabels.add(languageCode to displayLanguage) + } + } + return subtitleLabels +} + @Preview(name = "Player — buffering", showBackground = true) @Composable private fun PlayerScreenBufferingPreview() { diff --git a/app/src/test/java/com/jpcottin/simpletorrent/ui/player/PlayerScreenTest.kt b/app/src/test/java/com/jpcottin/simpletorrent/ui/player/PlayerScreenTest.kt new file mode 100644 index 0000000..ddb948d --- /dev/null +++ b/app/src/test/java/com/jpcottin/simpletorrent/ui/player/PlayerScreenTest.kt @@ -0,0 +1,95 @@ +package com.jpcottin.simpletorrent.ui.player + +import org.junit.Test +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import java.io.File +import java.nio.file.Files + +class PlayerScreenTest { + + @Test + fun findSubtitles_matchesLowercaseCodes() { + val tempDir = Files.createTempDirectory("subtitles_test").toFile() + try { + File(tempDir, "movie_en.srt").writeText("") + File(tempDir, "movie_fr.srt").writeText("") + + val result = findSubtitles(tempDir) + + assertEquals(2, result.size) + assertTrue(result.any { it.first == "en" }) + assertTrue(result.any { it.first == "fr" }) + } finally { + tempDir.deleteRecursively() + } + } + + @Test + fun findSubtitles_matchesUppercaseCodes() { + val tempDir = Files.createTempDirectory("subtitles_test").toFile() + try { + File(tempDir, "movie_EN.srt").writeText("") + File(tempDir, "movie_FR.srt").writeText("") + + val result = findSubtitles(tempDir) + + assertEquals(2, result.size) + assertTrue(result.any { it.first == "en" }) + assertTrue(result.any { it.first == "fr" }) + } finally { + tempDir.deleteRecursively() + } + } + + @Test + fun findSubtitles_fallsBackToCodeForUnknownLocale() { + val tempDir = Files.createTempDirectory("subtitles_test").toFile() + try { + File(tempDir, "movie_xx.srt").writeText("") + + val result = findSubtitles(tempDir) + + assertEquals(1, result.size) + assertEquals("xx", result[0].second) + } finally { + tempDir.deleteRecursively() + } + } + + @Test + fun findSubtitles_ignoresNonSrtFiles() { + val tempDir = Files.createTempDirectory("subtitles_test").toFile() + try { + File(tempDir, "movie_en.srt").writeText("") + File(tempDir, "movie_fr.txt").writeText("") + File(tempDir, "readme.md").writeText("") + + val result = findSubtitles(tempDir) + + assertEquals(1, result.size) + assertEquals("en", result[0].first) + } finally { + tempDir.deleteRecursively() + } + } + + @Test + fun findSubtitles_handlesEmptyDirectory() { + val tempDir = Files.createTempDirectory("subtitles_test").toFile() + try { + val result = findSubtitles(tempDir) + + assertEquals(0, result.size) + } finally { + tempDir.deleteRecursively() + } + } + + @Test + fun findSubtitles_handlesNullDirectory() { + val result = findSubtitles(null) + + assertEquals(0, result.size) + } +}