feat: Single audio tracking

[elpafras]

• add function to play audio with highlighting verse

[yukuku]

/gemini review

[yukuku]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature: audio playback with verse highlighting. The implementation is extensive and covers both single and dual-language playback. While the feature seems well-integrated, there are several areas for improvement in terms of code structure, maintainability, and efficiency. Specifically, the main activity has become quite large, and some of the new logic is tightly coupled, which could make future maintenance difficult. I've provided several suggestions to improve separation of concerns, reduce code duplication, and enhance performance.

[gemini-code-assist]

This activity has grown quite large and is taking on many responsibilities, including complex audio playback logic. To improve maintainability and separation of concerns, consider refactoring the audio-related logic (e.g., ExoPlayerController instances, TimingUtil instances, coroutine jobs, and all the audio control functions like playAlternating, navigateVerse, etc.) into a separate AudioPlaybackManager class. The IsiActivity would then own and delegate to this manager, observing its state through a listener or Flow. This would make the activity cleaner and the audio logic more modular and testable.

[gemini-code-assist]

The buildAudioUrl function has a few areas that could be made more robust:

• The logic to determine if a book is in the Old or New Testament (plorpb) relies on the string name's index in a list. A more reliable method would be to use the book.bookId. Old Testament books typically have IDs from 0-38 and New Testament from 39-65.
• The URL format is hardcoded. This creates a tight coupling with the external media server. It would be better to move this URL construction logic into a dedicated class (e.g., SabdaMediaUrlBuilder) and define the base URL and path segments as constants.
• The when statement for special book name mappings could also be moved to a more centralized location, like BookAbbrManager.

[gemini-code-assist]

The function name ensureAudioReady is misleading. It calls the onReady lambda immediately at the beginning, before the audio player is actually prepared or timing files are loaded. The playback is initiated inside onReady, which means it's requested before the player is ready. While ExoPlayer might queue this, it makes the logic hard to follow. The onReady callback should be invoked only after all asynchronous preparation steps (like loading timing files) are complete.

[gemini-code-assist]

Calling notifyDataSetChanged() is inefficient as it forces the entire RecyclerView to redraw. This can cause performance issues and flickering, especially with frequent updates like verse highlighting during audio playback. It's much more efficient to update only the specific item that has changed using notifyItemChanged(position).

    fun updateHighlight(verseNumber: Int, color: Int) {
        Log.d(TAG, "updateHighlight - verseNumber1: $verseNumber, color: $color")
        if (color == 0) {
            highlightedVerses.remove(verseNumber)
        } else {
            highlightedVerses[verseNumber] = color
        }
        val position = data.getPositionFromVerse(verseNumber + 1)
        if (position != -1) {
            notifyItemChanged(position)
        }
    }

[gemini-code-assist]

The ExoplayerController is tightly coupled with the UI implementation in IsiActivity through the updateVerseHighlightUI method, which directly accesses views by ID. This violates separation of concerns and makes the controller less reusable and harder to test. The highlighting logic should be handled by the view layer (IsiActivity) through a listener. You already have a good pattern with ExoplayerCallback and TimingUtil.HighlightListener.

I suggest removing highlightVerses and updateVerseHighlightUI from this controller. IsiActivity.applyHighlight can be simplified to directly call the VersesAdapter's updateHighlight method on the relevant VersesController.

[gemini-code-assist]

Using a hardcoded android:layout_marginTop="40dp" on splitRoot to create space for the imageContainer is not a robust layout strategy. It can lead to UI issues on different screen sizes or with different font settings. A better approach would be to use a RelativeLayout as the parent of both imageContainer and splitRoot, and position splitRoot below imageContainer using android:layout_below="@id/imageContainer". This ensures the layout is flexible and adapts correctly.

[gemini-code-assist]

The functions navigateChapterSingle and navigateChapterDual contain a lot of duplicated code for navigating to the next or previous chapter/book. This logic could be extracted into a common helper function to reduce redundancy and improve maintainability. For example, you could have a function that determines the next (bookId, chapter_1) pair, and then the single/dual functions would just handle the UI/audio updates specific to their mode.

[gemini-code-assist]

The showSpeedMenu function uses magic numbers for menu item IDs and has repetitive when blocks. This can be simplified and made more maintainable. Consider defining the speed options in a data structure (e.g., a Map<Int, Float>) and creating the menu items in a loop. This would also simplify the setOnMenuItemClickListener. Alternatively, defining the menu in an XML resource (res/menu/speed_menu.xml) would be even better, following standard Android practices.

[gemini-code-assist]

The fromJsonArray function currently returns a List<Triple<Long, Long, Int>>. Using a Triple makes the code harder to read and maintain, as the meaning of first, second, and third is not immediately obvious. Since you already have the MTiming data class, it would be much clearer to return a List<MTiming>.

This will require updating the type of timingList in TimingUtil and other places from List<Triple<Long, Long, Int>> to List<MTiming>.

        fun fromJsonArray(jsonText: String): List<MTiming> {
            val timestampsArray = JSONObject(jsonText).getJSONArray("timestamps")
            return List(timestampsArray.length()) { i ->
                fromJson(timestampsArray.getJSONObject(i))
            }
        }

[gemini-code-assist]

Code Review

This pull request introduces a great new feature for audio playback with verse highlighting, supporting both single and dual-language modes. The use of coroutines for handling asynchronous operations is a good choice. However, there are several areas that need improvement, particularly regarding coroutine scope management which can lead to memory leaks, some fragile logic for URL building, and a layout issue that uses a magic number. I've left specific comments with suggestions on how to address these points. Overall, a solid feature addition with some important refinements needed.

[gemini-code-assist]

The logic for determining the testament (plorpb) and bookCode is fragile and inefficient. It relies on creating a list from map keys and finding indices of strings, which is not robust as map key order is not always guaranteed. This also creates a new list on every call, which is inefficient.

A better approach is to use book.bookId, which is a stable integer. Old Testament books have bookId < 39.

        val isOldTestament = book.bookId < 39
        val plorpb = if (isOldTestament) "pl" else "pb"

        val bookCode = if (isOldTestament) {
            String.format(Locale.US, "%02d", book.bookId + 1)
        } else {
            String.format(Locale.US, "%02d", book.bookId - 39 + 1)
        }

[gemini-code-assist]

Creating a new CoroutineScope with CoroutineScope(Dispatchers.Main) can lead to memory leaks if the Activity is destroyed but the coroutine is still running. It's better to use a lifecycle-aware scope. Since this is an Activity, you can use lifecycleScope.launch which will automatically cancel the coroutine when the activity is destroyed.

        dualJob = lifecycleScope.launch {

[gemini-code-assist]

Creating a new CoroutineScope with CoroutineScope(Dispatchers.IO) can lead to memory leaks if the owner (IsiActivity) is destroyed but the coroutine is still running. This class should accept a CoroutineScope in its constructor and use that to launch coroutines. IsiActivity can pass its lifecycleScope.

Example:

// In TimingUtil class
class TimingUtil(
    private val scope: CoroutineScope,
    // ... other params
) {
    // ...
    fun loadTimingFile(...) {
        scope.launch(Dispatchers.IO) { /* ... */ }
    }
    // ...
}

// In IsiActivity
timingUtil0 = TimingUtil(lifecycleScope, exoplayerController0, this, id = "controller0")

[gemini-code-assist]

Creating a new CoroutineScope with CoroutineScope(Dispatchers.Main) can lead to memory leaks if the Activity is destroyed but the coroutine is still running. It's better to use a lifecycle-aware scope. Since this controller is used within an Activity, you could pass the Activity's lifecycleScope to this controller and use it to launch coroutines. This will ensure that coroutines are automatically cancelled when the Activity is destroyed.

[gemini-code-assist]

Using a hardcoded android:layout_marginTop="40dp" on splitRoot to make space for imageContainer is a fragile layout practice. If the height of imageContainer changes, this margin will need to be updated manually.

A better approach is to change the parent FrameLayout (@+id/nontoolbar) to a RelativeLayout and position splitRoot below imageContainer using android:layout_below="@id/imageContainer". This will make the layout adjust automatically and more robust.

[gemini-code-assist]

This function has a couple of issues:

1. It redundantly sets isAudioVisible = false after toggleAudioBar() has already flipped its value from true to false.
2. It only stops exoplayerController0 and resets timingUtil0. In dual audio mode, exoplayerController1 will continue playing, which is likely not the intended behavior when hiding the audio bar.

You can call stopAudioForNavigation() here to correctly stop both players and reset highlights for both single and dual modes.

    fun hideAudioBarIfVisible() {
        if (isAudioVisible) {
            toggleAudioBar()
            stopAudioForNavigation()
        }
    }

[gemini-code-assist]

The playback speed is only set for exoplayerController0. When in dual audio mode, exoplayerController1 will continue to play at its own speed, which will be out of sync. The speed should be set for both controllers to ensure they play at the same rate.

            val speed = when (item.itemId) {
                1 -> 0.5f
                2 -> 0.8f
                3 -> 1.0f
                4 -> 1.1f
                5 -> 1.25f
                6 -> 1.5f
                7 -> 1.75f
                8 -> 2.0f
                else -> return@setOnMenuItemClickListener false
            }
            exoplayerController0.setPlaybackSpeed(speed)
            exoplayerController1.setPlaybackSpeed(speed)

[gemini-code-assist]

When the audio ends, startHighlightingVerses() and highlightVerse(1) are called on timingUtil0. In dual audio mode, timingUtil1 should also be handled to keep things in sync when repeating or moving to the next chapter.

        timingUtil0.startHighlightingVerses()
        timingUtil0.highlightVerse(1)
        if (cSplitVersion.isChecked) {
            timingUtil1.highlightVerse(1)
        }

[gemini-code-assist]

notifyDataSetChanged() is inefficient as it redraws the entire list. It's better to use notifyItemChanged() to update only the specific verse view that needs to be re-rendered. You'll need to find the position of the item from the verseNumber.

    fun updateHighlight(verseNumber: Int, color: Int) {
        Log.d(TAG, "updateHighlight - verseNumber1: $verseNumber, color: $color")
        highlightedVerses[verseNumber] = color
        val position = data.getPositionIgnoringPericopeFromVerse(verseNumber + 1)
        if (position != -1) {
            notifyItemChanged(position)
        } else {
            notifyDataSetChanged() // Fallback
        }
    }

[yukuku]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature for audio playback with verse highlighting. The implementation is quite extensive, involving new managers, controllers, and UI components. Overall, the structure is well-thought-out, especially the use of coroutines for handling playback logic. However, I've identified a few critical correctness issues, particularly in state management when navigating across books, and some areas for improvement in terms of maintainability and robustness. My review comments focus on fixing these bugs and improving the code's clarity and long-term health.

[gemini-code-assist]

There's a potential bug in the displayChapter lambda. It doesn't update activeSplit0.book with the new book when navigating chapters across different books. This will cause display() to show the new chapter in the old book. You should update activeSplit0 before calling display().

            displayChapter = { book, chapter ->
                activeSplit0 = activeSplit0.copy(book = book)
                display(chapter, 1)
            },

[elpafras]

For this issue, I changed the book to _ because I don't really need to set the book name yet.

[gemini-code-assist]

There is hardcoded logic to map the version "TB" to "tbsuara". This kind of version-specific logic is scattered across the codebase. It would be better to centralize this logic, perhaps in MediaList or a dedicated utility, to improve maintainability and avoid inconsistencies.

[elpafras]

updated in changes to be sent soon

[yukuku]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature for audio playback with verse highlighting. The implementation is quite comprehensive, covering single and dual (split) audio modes, playback controls (play/pause, navigation, speed, repeat), and integration with the existing UI. The use of coroutines for handling asynchronous operations is well done.

I've identified a few potential issues, mainly around edge cases and error handling, such as what happens when an audio URL or timing file can't be loaded. There are also some minor inconsistencies in logging. Addressing these points will make the feature more robust and maintainable.

Overall, this is a great addition to the app.

[gemini-code-assist]

In split mode, if activeSplit1 is null or its version has no short name, version1 falls back to version0. This will cause url1 to be the same as url0, and both ExoPlayer instances will be set up to play the same audio file. This is likely not the intended behavior and wastes resources.

It would be better to make version1 nullable and only set up the second player if version1 is not null.

Suggested change

	val version1 = activeSplit1?.version?.shortName ?: version0
	val version1 = activeSplit1?.version?.shortName

[gemini-code-assist]

The buildAudioUrl function can return an empty string if an audio version is not found. However, the returned URL is used in mediaKnownToExist and setAudioUrl without checking if it's empty. This could cause ExoPlayer to throw an exception when trying to prepare an empty URL. You should add a check to ensure the URL is not empty before using it to set up the player.

This applies to url0, url1 in dual mode, and the url in single audio mode (line 1613).

[gemini-code-assist]

For consistency with the rest of the project which uses a custom AppLog wrapper, please use AppLog.d instead of android.util.Log.d. This also applies to other Log.d calls on lines 1791 and 3304.

Suggested change

	Log.d("IsiActivity", "Initial audio_controls color = $color (isNight=$isNight)")
	AppLog.d(TAG, "Initial audio_controls color = $color (isNight=$isNight)")

[gemini-code-assist]

When loading the timing file fails, the error is logged, but the user is not notified. The playback proceeds without the verse highlighting feature. This is a reasonable fallback, but it's a silent failure. It would be a better user experience to provide some feedback, like a Toast message, indicating that verse highlighting is unavailable for the current chapter due to a loading error.

[gemini-code-assist]

Using a hardcoded delay(800) can be unreliable as it assumes the UI update will complete within 800ms. This could lead to race conditions on slower devices or under heavy load. Consider using a more robust synchronization mechanism, like a callback or a View.post() on a relevant view after navigateChapter has updated the UI, to ensure the UI is ready before proceeding with highlighting and playing the verse.

[gemini-code-assist]

It's a common convention to end files with a newline character. This file, and several other new files in this PR (AudioPlaybackManager.kt, BibleMediaManager.kt, MediaList.kt, NetworkUtil.kt, TimingUtil.kt), are missing it.

[gemini-code-assist]

For consistency with the rest of the project, please use the custom AppLog wrapper for logging instead of android.util.Log.

Suggested change

	Log.d(TAG, "onBindViewHolder - position: $position, verse: $index, highlightColor: $highlightColor")
	AppLog.d(TAG, "onBindViewHolder - position: $position, verse: $index, highlightColor: $highlightColor")