Implement subtitle parser and display it in VideoActivity view #46
Conversation
WalkthroughThis update introduces synchronized subtitle support for video playback within the application, aligning with elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months. A new data class for subtitles is added, and utility functions are provided to load and parse SRT files. The video player activity and view model are extended to manage subtitle loading, synchronization, and display. The user interface is updated to include a dedicated TextView for showing subtitles at the bottom of the video screen. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VideoActivity
participant VideoViewModel
participant SubtitleUtil
participant SubtitleTextView
User->>VideoActivity: Start video playback
VideoActivity->>VideoViewModel: loadSubtitles("sample.srt")
VideoViewModel->>SubtitleUtil: loadSubtitles(assetManager, "sample.srt")
SubtitleUtil-->>VideoViewModel: List<Subtitle>
VideoViewModel-->>VideoActivity: subtitles StateFlow update
VideoActivity->>VideoActivity: synchronizeSubtitles()
loop every 500ms
VideoActivity->>VideoViewModel: getCurrentSubtitle(currentPosition)
VideoViewModel-->>VideoActivity: Subtitle?
VideoActivity->>SubtitleTextView: Update text if subtitle changed
end
Assessment against linked issues
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
data-video/src/main/java/ai/elimu/filamu/data/video/viewmodel/VideoViewModelImpl.kt (1)
1-94: 💡 Verification agent🧩 Analysis chain
Documentation needed for the Subtitle model class
The code references a
Subtitlemodel class that's imported but not documented. Adding documentation for this class would improve code maintainability and help others understand the subtitle implementation.Let's check if the Subtitle class has proper documentation:
🏁 Script executed:
#!/bin/bash # Find and display the Subtitle model class echo "Looking for Subtitle model class definition:" find . -name "*.kt" -exec grep -l "class Subtitle" {} \; | xargs catLength of output: 290
Add KDoc to the
Subtitledata classTo keep the code clear and maintainable (and in line with elimu.ai’s mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months), please add documentation to the
Subtitlemodel.File to update:
data-video/src/main/java/ai/elimu/filamu/ui/video/model/Subtitle.ktSuggested KDoc:
/** * Represents a subtitle segment displayed during video playback. * * @property startTime timestamp in milliseconds when the subtitle appears * @property endTime timestamp in milliseconds when the subtitle disappears * @property text the subtitle text content */ data class Subtitle( val startTime: Long, val endTime: Long, val text: String )
🧹 Nitpick comments (6)
app/src/main/res/layout/activity_video.xml (1)
17-29: Consider enhancing subtitle visibility and accessibilityThe subtitle TextView implementation is a great step towards enhancing content accessibility, aligning with elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months. However, a few improvements could make this feature more effective:
- Consider using a semi-transparent background (e.g.,
#80000000) instead of solid black to maintain visibility of video content behind the subtitles- Add content description for accessibility support
- Consider supporting larger text sizes for younger viewers or children with visual impairments
- Add elevation to ensure the TextView stays on top of other elements
<TextView android:id="@+id/subtitle_text_view" android:layout_width="match_parent" android:layout_height="wrap_content" android:layout_gravity="bottom" - android:background="@android:color/black" + android:background="#80000000" android:textColor="@android:color/white" android:padding="8dp" android:textSize="16sp" android:gravity="center" + android:contentDescription="@string/subtitle_content_description" + android:elevation="2dp" + android:lineSpacingExtra="4dp" app:layout_constraintBottom_toBottomOf="parent" app:layout_constraintStart_toStartOf="parent" app:layout_constraintEnd_toEndOf="parent"/>app/src/main/java/ai/elimu/filamu/ui/video/model/Subtitle.kt (1)
1-3: Add documentation and consider additional helper methodsThis Subtitle data class provides a solid foundation for implementing the subtitle feature, supporting elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months. However, adding documentation and helper methods would improve maintainability.
package ai.elimu.filamu.ui.video.model +/** + * Represents a single subtitle entry with start and end time boundaries. + * All time values are in milliseconds since the start of the video. + * + * @property startTime The time (in ms) when this subtitle should start displaying + * @property endTime The time (in ms) when this subtitle should stop displaying + * @property text The actual subtitle content to be displayed + */ -data class Subtitle(val startTime: Long, val endTime: Long, val text: String) +data class Subtitle(val startTime: Long, val endTime: Long, val text: String) { + /** + * Checks if the given playback position falls within this subtitle's time range + * + * @param position Current playback position in milliseconds + * @return True if this subtitle should be displayed at the given position + */ + fun isVisibleAt(position: Long): Boolean = position in startTime..endTime +}data-video/src/main/java/ai/elimu/filamu/data/video/viewmodel/VideoViewModel.kt (2)
29-29: Import the Subtitle class to avoid using fully qualified nameUsing the fully qualified name makes the code more verbose and harder to maintain. Importing the class would improve readability.
package ai.elimu.filamu.data.video.viewmodel import ai.elimu.model.v2.gson.content.VideoGson +import ai.elimu.filamu.ui.video.model.Subtitle import android.graphics.Bitmap import kotlinx.coroutines.flow.StateFlow ... interface VideoViewModel { val uiState: StateFlow<LoadVideosUiState> - val subtitles: StateFlow<List<ai.elimu.filamu.ui.video.model.Subtitle>> + val subtitles: StateFlow<List<Subtitle>>
34-35: Improve error handling and documentation for subtitle methodsAdding proper documentation and error handling for the subtitle-related functions would improve usability and maintainability, supporting elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
- fun loadSubtitles(fileName: String) - fun getCurrentSubtitle(currentPosition: Long): ai.elimu.filamu.ui.video.model.Subtitle? + /** + * Load subtitles from the specified file in the assets directory. + * The result will be emitted through the [subtitles] StateFlow. + * + * @param fileName The name of the subtitle file in the assets directory + * @return Boolean indicating if the loading was initiated successfully + */ + fun loadSubtitles(fileName: String): Boolean + + /** + * Get the subtitle that should be displayed at the current playback position. + * + * @param currentPosition Current video playback position in milliseconds + * @return The subtitle to display, or null if no subtitle should be shown + */ + fun getCurrentSubtitle(currentPosition: Long): Subtitle?app/src/main/java/ai/elimu/filamu/ui/video/VideoActivity.kt (1)
121-131: Subtitle synchronization polling interval could be optimizedThe current implementation uses a polling approach with a fixed 500ms interval. While functional, this approach may not be optimal for performance or battery life on mobile devices.
Consider using a more adaptive approach:
private fun synchronizeSubtitles() { synchronizationJob?.cancel() synchronizationJob = lifecycleScope.launch { while (true) { val currentPosition = videoPlayer.currentPosition val subtitle = videoViewModel.getCurrentSubtitle(currentPosition) - binding.subtitleTextView.text = subtitle?.text ?: "" - delay(500) // Check subtitle updates every 500ms + val currentText = subtitle?.text ?: "" + if (binding.subtitleTextView.text != currentText) { + binding.subtitleTextView.text = currentText + } + + // Calculate optimal delay based on next subtitle timing + val nextSubtitle = videoViewModel.getNextSubtitle(currentPosition) + val delayTime = if (nextSubtitle != null) { + // Wait until 100ms before next subtitle or at most 1000ms + minOf(nextSubtitle.startTime - currentPosition - 100, 1000L).coerceAtLeast(100) + } else { + 500 // Default delay if no next subtitle + } + delay(delayTime) } } }This would require adding a
getNextSubtitlemethod to the ViewModel.data-video/src/main/java/ai/elimu/filamu/data/video/viewmodel/VideoViewModelImpl.kt (1)
89-93: Optimize subtitle search for better performanceThe current implementation searches the entire list of subtitles on each call, which could be inefficient for large subtitle files and impact the smooth learning experience that elimu.ai aims to provide.
Consider optimizing by tracking the last matched subtitle position or using binary search:
+ private var lastSubtitleIndex = 0 override fun getCurrentSubtitle(currentPosition: Long): Subtitle? { + val subtitles = _subtitles.value + + // Quick check if we're still in the same subtitle + if (lastSubtitleIndex < subtitles.size) { + val current = subtitles[lastSubtitleIndex] + if (current.startTime <= currentPosition && current.endTime >= currentPosition) { + return current + } + + // Check next subtitle + if (lastSubtitleIndex + 1 < subtitles.size) { + val next = subtitles[lastSubtitleIndex + 1] + if (next.startTime <= currentPosition && next.endTime >= currentPosition) { + lastSubtitleIndex++ + return next + } + } + } + + // Fall back to full search if quick check fails + for (i in subtitles.indices) { + val subtitle = subtitles[i] + if (subtitle.startTime <= currentPosition && subtitle.endTime >= currentPosition) { + lastSubtitleIndex = i + return subtitle + } + } + + return null } + // Add method to get next subtitle for optimized polling + fun getNextSubtitle(currentPosition: Long): Subtitle? { + val subtitles = _subtitles.value + return subtitles.find { it.startTime > currentPosition } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/ai/elimu/filamu/ui/video/VideoActivity.kt(4 hunks)app/src/main/java/ai/elimu/filamu/ui/video/model/Subtitle.kt(1 hunks)app/src/main/java/ai/elimu/filamu/util/SubtitleUtil.kt(1 hunks)app/src/main/res/layout/activity_video.xml(1 hunks)data-video/src/main/java/ai/elimu/filamu/data/video/viewmodel/VideoViewModel.kt(1 hunks)data-video/src/main/java/ai/elimu/filamu/data/video/viewmodel/VideoViewModelImpl.kt(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test (28)
- GitHub Check: test (29)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (macos-latest, 17)
🔇 Additional comments (5)
app/src/main/java/ai/elimu/filamu/ui/video/VideoActivity.kt (3)
36-36: Good practice using a nullable Job property for coroutine management.The nullable
synchronizationJobproperty correctly allows for tracking and cancellation of the subtitle synchronization coroutine, which is essential for proper lifecycle management. This aligns with elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
111-119: Good synchronization trigger pointTriggering subtitle synchronization when the player reaches the
STATE_READYstate is appropriate. This ensures subtitles start displaying only when the video is ready to play, supporting elimu.ai's mission to provide effective learning tools for children.
135-135: Good cleanup practiceProperly cancelling the synchronization job in
onDestroy()prevents memory leaks and unnecessary background work, which is important for maintaining application performance. This careful resource management supports elimu.ai's mission to build effective learning software for children with potentially limited device resources.data-video/src/main/java/ai/elimu/filamu/data/video/viewmodel/VideoViewModelImpl.kt (2)
22-26: Proper dependency injection for the Application contextAdding the Application parameter through dependency injection is the correct approach for accessing Android resources. This structured approach to accessing resources aligns with elimu.ai's mission to build robust learning software that empowers out-of-school children.
35-36: Well-structured state management using StateFlowUsing a private MutableStateFlow with a public immutable StateFlow accessor follows best practices for state management in Kotlin. This approach provides a clean, reactive way to handle subtitle data changes.
| fun parseTime(time: String): Long { | ||
| val parts = time.split(",", ":").map { it.toInt() } | ||
| return parts[0] * 3600000 + parts[1] * 60000 + parts[2] * 1000 + parts[3] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to parseTime method
The time parsing function lacks error handling which could lead to crashes if the subtitle file contains malformed timestamps.
- fun parseTime(time: String): Long {
- val parts = time.split(",", ":").map { it.toInt() }
- return parts[0] * 3600000 + parts[1] * 60000 + parts[2] * 1000 + parts[3]
+ fun parseTime(time: String): Long {
+ return try {
+ val parts = time.split(",", ":").map { it.toInt() }
+ if (parts.size != 4) {
+ Timber.w("Invalid time format: $time, expected hh:mm:ss,ms")
+ 0L
+ } else {
+ parts[0] * 3600000 + parts[1] * 60000 + parts[2] * 1000 + parts[3]
+ }
+ } catch (e: NumberFormatException) {
+ Timber.e(e, "Failed to parse time: $time")
+ 0L
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun parseTime(time: String): Long { | |
| val parts = time.split(",", ":").map { it.toInt() } | |
| return parts[0] * 3600000 + parts[1] * 60000 + parts[2] * 1000 + parts[3] | |
| } | |
| fun parseTime(time: String): Long { | |
| return try { | |
| val parts = time.split(",", ":").map { it.toInt() } | |
| if (parts.size != 4) { | |
| Timber.w("Invalid time format: $time, expected hh:mm:ss,ms") | |
| 0L | |
| } else { | |
| parts[0] * 3600000 + parts[1] * 60000 + parts[2] * 1000 + parts[3] | |
| } | |
| } catch (e: NumberFormatException) { | |
| Timber.e(e, "Failed to parse time: $time") | |
| 0L | |
| } | |
| } |
| object SubtitleUtil { | ||
|
|
||
| suspend fun loadSubtitles(assetManager: AssetManager, fileName: String): List<Subtitle> { | ||
| return withContext(Dispatchers.IO) { | ||
| val subtitles = mutableListOf<Subtitle>() | ||
| try { | ||
| val inputStream = assetManager.open(fileName) | ||
| val reader = BufferedReader(InputStreamReader(inputStream)) | ||
| val pattern = Pattern.compile("(\\d+)\\n(\\d{2}:\\d{2}:\\d{2},\\d{3}) --> (\\d{2}:\\d{2}:\\d{2},\\d{3})\\n(.*)") | ||
|
|
||
| var line: String? | ||
| val subtitleBuilder = StringBuilder() | ||
|
|
||
| while (reader.readLine().also { line = it } != null) { | ||
| if (line.isNullOrBlank()) { | ||
| val matcher = pattern.matcher(subtitleBuilder.toString()) | ||
| if (matcher.find()) { | ||
| val startTime = parseTime(matcher.group(2)!!) | ||
| val endTime = parseTime(matcher.group(3)!!) | ||
| val text = matcher.group(4)!! | ||
| subtitles.add(Subtitle(startTime, endTime, text)) | ||
| } | ||
| subtitleBuilder.clear() | ||
| } else { | ||
| subtitleBuilder.append(line).append("\n") | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Timber.e("Error loading subtitles: ${e.message}") | ||
| } | ||
| subtitles | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix subtitle parsing pattern to handle multi-line subtitles and add proper resource handling
The current implementation has a few issues that could impact subtitle parsing reliability, which is important for elimu.ai's mission to build innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months.
- The regular expression pattern only captures single-line subtitles, but SRT files often contain multi-line text
- Resource leaks: InputStreams should be closed properly using
useextension function - The final subtitle block might not be captured if the file doesn't end with a blank line
- Error handling could be more specific
object SubtitleUtil {
suspend fun loadSubtitles(assetManager: AssetManager, fileName: String): List<Subtitle> {
return withContext(Dispatchers.IO) {
val subtitles = mutableListOf<Subtitle>()
try {
- val inputStream = assetManager.open(fileName)
- val reader = BufferedReader(InputStreamReader(inputStream))
- val pattern = Pattern.compile("(\\d+)\\n(\\d{2}:\\d{2}:\\d{2},\\d{3}) --> (\\d{2}:\\d{2}:\\d{2},\\d{3})\\n(.*)")
+ assetManager.open(fileName).use { inputStream ->
+ BufferedReader(InputStreamReader(inputStream)).use { reader ->
+ // Pattern that captures subtitle index, timing and multi-line text content
+ val pattern = Pattern.compile("(\\d+)\\n(\\d{2}:\\d{2}:\\d{2},\\d{3}) --> (\\d{2}:\\d{2}:\\d{2},\\d{3})\\n((?:.*\\n?)+)")
var line: String?
val subtitleBuilder = StringBuilder()
while (reader.readLine().also { line = it } != null) {
if (line.isNullOrBlank()) {
val matcher = pattern.matcher(subtitleBuilder.toString())
if (matcher.find()) {
val startTime = parseTime(matcher.group(2)!!)
val endTime = parseTime(matcher.group(3)!!)
val text = matcher.group(4)!!
subtitles.add(Subtitle(startTime, endTime, text))
}
subtitleBuilder.clear()
} else {
subtitleBuilder.append(line).append("\n")
}
}
+ // Process the last subtitle block if it exists
+ if (subtitleBuilder.isNotEmpty()) {
+ val matcher = pattern.matcher(subtitleBuilder.toString())
+ if (matcher.find()) {
+ val startTime = parseTime(matcher.group(2)!!)
+ val endTime = parseTime(matcher.group(3)!!)
+ val text = matcher.group(4)!!
+ subtitles.add(Subtitle(startTime, endTime, text))
+ }
+ }
+ }
+ }
} catch (e: Exception) {
- Timber.e("Error loading subtitles: ${e.message}")
+ Timber.e(e, "Error loading subtitles from %s", fileName)
}
subtitles
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| object SubtitleUtil { | |
| suspend fun loadSubtitles(assetManager: AssetManager, fileName: String): List<Subtitle> { | |
| return withContext(Dispatchers.IO) { | |
| val subtitles = mutableListOf<Subtitle>() | |
| try { | |
| val inputStream = assetManager.open(fileName) | |
| val reader = BufferedReader(InputStreamReader(inputStream)) | |
| val pattern = Pattern.compile("(\\d+)\\n(\\d{2}:\\d{2}:\\d{2},\\d{3}) --> (\\d{2}:\\d{2}:\\d{2},\\d{3})\\n(.*)") | |
| var line: String? | |
| val subtitleBuilder = StringBuilder() | |
| while (reader.readLine().also { line = it } != null) { | |
| if (line.isNullOrBlank()) { | |
| val matcher = pattern.matcher(subtitleBuilder.toString()) | |
| if (matcher.find()) { | |
| val startTime = parseTime(matcher.group(2)!!) | |
| val endTime = parseTime(matcher.group(3)!!) | |
| val text = matcher.group(4)!! | |
| subtitles.add(Subtitle(startTime, endTime, text)) | |
| } | |
| subtitleBuilder.clear() | |
| } else { | |
| subtitleBuilder.append(line).append("\n") | |
| } | |
| } | |
| } catch (e: Exception) { | |
| Timber.e("Error loading subtitles: ${e.message}") | |
| } | |
| subtitles | |
| } | |
| } | |
| object SubtitleUtil { | |
| suspend fun loadSubtitles(assetManager: AssetManager, fileName: String): List<Subtitle> { | |
| return withContext(Dispatchers.IO) { | |
| val subtitles = mutableListOf<Subtitle>() | |
| try { | |
| assetManager.open(fileName).use { inputStream -> | |
| BufferedReader(InputStreamReader(inputStream)).use { reader -> | |
| // Pattern that captures subtitle index, timing and multi-line text content | |
| val pattern = Pattern.compile( | |
| "(\\d+)\\n" + | |
| "(\\d{2}:\\d{2}:\\d{2},\\d{3}) --> " + | |
| "(\\d{2}:\\d{2}:\\d{2},\\d{3})\\n" + | |
| "((?:.*\\n?)+)" | |
| ) | |
| var line: String? | |
| val subtitleBuilder = StringBuilder() | |
| while (reader.readLine().also { line = it } != null) { | |
| if (line.isNullOrBlank()) { | |
| val matcher = pattern.matcher(subtitleBuilder.toString()) | |
| if (matcher.find()) { | |
| val startTime = parseTime(matcher.group(2)!!) | |
| val endTime = parseTime(matcher.group(3)!!) | |
| val text = matcher.group(4)!!.trimEnd() | |
| subtitles.add(Subtitle(startTime, endTime, text)) | |
| } | |
| subtitleBuilder.clear() | |
| } else { | |
| subtitleBuilder.append(line).append("\n") | |
| } | |
| } | |
| // Process the last subtitle block if it exists | |
| if (subtitleBuilder.isNotEmpty()) { | |
| val matcher = pattern.matcher(subtitleBuilder.toString()) | |
| if (matcher.find()) { | |
| val startTime = parseTime(matcher.group(2)!!) | |
| val endTime = parseTime(matcher.group(3)!!) | |
| val text = matcher.group(4)!!.trimEnd() | |
| subtitles.add(Subtitle(startTime, endTime, text)) | |
| } | |
| } | |
| } | |
| } | |
| } catch (e: Exception) { | |
| Timber.e(e, "Error loading subtitles from %s", fileName) | |
| } | |
| subtitles | |
| } | |
| } | |
| } |
|
|
||
| // Load subtitles | ||
| videoViewModel.loadSubtitles("sample.srt") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded subtitle filename limits flexibility
Using a hardcoded subtitle filename "sample.srt" limits the application's ability to load different subtitle files for different videos. Consider making this dynamic based on the video being played.
- // Load subtitles
- videoViewModel.loadSubtitles("sample.srt")
+ // Load subtitles based on video ID
+ val subtitleFileName = "video_${videoId}_subtitles.srt"
+ videoViewModel.loadSubtitles(subtitleFileName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Load subtitles | |
| videoViewModel.loadSubtitles("sample.srt") | |
| // Load subtitles based on video ID | |
| val subtitleFileName = "video_${videoId}_subtitles.srt" | |
| videoViewModel.loadSubtitles(subtitleFileName) |
| private fun synchronizeSubtitles() { | ||
| synchronizationJob?.cancel() | ||
| synchronizationJob = lifecycleScope.launch { | ||
| while (true) { | ||
| val currentPosition = videoPlayer.currentPosition | ||
| val subtitle = videoViewModel.getCurrentSubtitle(currentPosition) | ||
| binding.subtitleTextView.text = subtitle?.text ?: "" | ||
| delay(500) // Check subtitle updates every 500ms | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for subtitle synchronization
The synchronization coroutine doesn't include any error handling. An uncaught exception could terminate the subtitle display.
private fun synchronizeSubtitles() {
synchronizationJob?.cancel()
synchronizationJob = lifecycleScope.launch {
+ try {
while (true) {
val currentPosition = videoPlayer.currentPosition
val subtitle = videoViewModel.getCurrentSubtitle(currentPosition)
binding.subtitleTextView.text = subtitle?.text ?: ""
delay(500) // Check subtitle updates every 500ms
}
+ } catch (e: Exception) {
+ Timber.tag(TAG).e("Subtitle synchronization error: ${e.message}")
+ // Retry synchronization after a delay
+ delay(1000)
+ synchronizeSubtitles()
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun synchronizeSubtitles() { | |
| synchronizationJob?.cancel() | |
| synchronizationJob = lifecycleScope.launch { | |
| while (true) { | |
| val currentPosition = videoPlayer.currentPosition | |
| val subtitle = videoViewModel.getCurrentSubtitle(currentPosition) | |
| binding.subtitleTextView.text = subtitle?.text ?: "" | |
| delay(500) // Check subtitle updates every 500ms | |
| } | |
| } | |
| } | |
| private fun synchronizeSubtitles() { | |
| synchronizationJob?.cancel() | |
| synchronizationJob = lifecycleScope.launch { | |
| try { | |
| while (true) { | |
| val currentPosition = videoPlayer.currentPosition | |
| val subtitle = videoViewModel.getCurrentSubtitle(currentPosition) | |
| binding.subtitleTextView.text = subtitle?.text ?: "" | |
| delay(500) // Check subtitle updates every 500ms | |
| } | |
| } catch (e: Exception) { | |
| Timber.tag(TAG).e("Subtitle synchronization error: ${e.message}") | |
| // Retry synchronization after a delay | |
| delay(1000) | |
| synchronizeSubtitles() | |
| } | |
| } | |
| } |
| override fun loadSubtitles(fileName: String) { | ||
| ioScope.launch { | ||
| val loadedSubtitles = SubtitleUtil.loadSubtitles(application.assets, fileName) | ||
| _subtitles.emit(loadedSubtitles) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for subtitle loading
The subtitle loading lacks error handling. If the subtitle file doesn't exist or is malformed, this could crash the application.
override fun loadSubtitles(fileName: String) {
ioScope.launch {
+ try {
val loadedSubtitles = SubtitleUtil.loadSubtitles(application.assets, fileName)
_subtitles.emit(loadedSubtitles)
+ } catch (e: Exception) {
+ // Log error and emit empty list to avoid crashing
+ Timber.e("Failed to load subtitles: ${e.message}")
+ _subtitles.emit(emptyList())
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun loadSubtitles(fileName: String) { | |
| ioScope.launch { | |
| val loadedSubtitles = SubtitleUtil.loadSubtitles(application.assets, fileName) | |
| _subtitles.emit(loadedSubtitles) | |
| } | |
| } | |
| override fun loadSubtitles(fileName: String) { | |
| ioScope.launch { | |
| try { | |
| val loadedSubtitles = SubtitleUtil.loadSubtitles(application.assets, fileName) | |
| _subtitles.emit(loadedSubtitles) | |
| } catch (e: Exception) { | |
| // Log error and emit empty list to avoid crashing | |
| Timber.e("Failed to load subtitles: ${e.message}") | |
| _subtitles.emit(emptyList()) | |
| } | |
| } | |
| } |
| override fun getCurrentSubtitle(currentPosition: Long): Subtitle? { | ||
| return _subtitles.value.find { | ||
| it.startTime <= currentPosition && it.endTime >= currentPosition | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add unit tests for the subtitle handling logic
The subtitle synchronization is a critical feature for educational videos, but there are no unit tests for this functionality. To ensure a reliable learning experience for children using elimu.ai's software, comprehensive testing is essential.
Please verify if there are any unit tests for the subtitle functionality by running:
🏁 Script executed:
#!/bin/bash
# Check for tests related to subtitle functionality
echo "Looking for subtitle-related test files..."
find . -path "*/src/test/*" -name "*Subtitle*Test*.kt" -o -name "*Test*Subtitle*.kt"
echo "Checking for subtitle-related test methods in VideoViewModel tests..."
find . -path "*/src/test/*" -name "*VideoViewModel*Test*.kt" | xargs grep -l "subtitle" || echo "No subtitle tests found in VideoViewModel test files"Length of output: 533
Add unit tests for subtitle synchronization logic
To uphold elimu.ai’s mission of building innovative learning software that empowers out-of-school children to teach themselves basic reading📖, writing✍🏽 and math🔢 within 6 months, we need to ensure that subtitle timing is bullet-proof. No existing tests cover getCurrentSubtitle, so let’s add them.
• Create a new test class, e.g. VideoViewModelImplSubtitleTest.kt
• Target getCurrentSubtitle(currentPosition: Long) in VideoViewModelImpl.kt
• Cover scenarios:
– Position before any subtitle (expect null)
– Position exactly at start/end boundaries
– Position within a subtitle interval
– Multiple subtitles with overlapping times
– Empty subtitle list
Issue Number
Purpose
Implement subtitle parser and display it in VideoActivity view
Regression Tests
UI Tests
Content Tests
eng.elimu.ai)hin.elimu.ai)tgl.elimu.ai)tha.elimu.ai)vie.elimu.ai)Summary by CodeRabbit
New Features
User Interface