Android GUI: Migrate Rx Java to Kotlin Coroutines

[cjee21]

Resolve #1284 while migrating fully to Kotlin Coroutines and reducing the number of dependencies at the same time.

Also fix #1288

[Copilot]

Pull request overview

This PR migrates the Android GUI data layer and UI observers from RxJava to Kotlin coroutines/Flow to address issue #1284 (Room EmptyResultSetException) and reduce dependencies.

Changes:

• Converted Room DAO APIs from RxJava (Single/Flowable) to suspend and Flow.
• Updated ViewModel/Activities/Fragments to use lifecycleScope coroutines and Flow collection instead of Rx subscriptions.
• Reduced coroutine/Rx dependencies in the Android app Gradle config.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
Source/GUI/Android/app/src/main/java/net/mediaarea/mediainfo/ReportViewModel.kt	Replaced Rx-based DB operations with suspend calls and Flow exposure; adjusted navigation logic after inserts.
Source/GUI/Android/app/src/main/java/net/mediaarea/mediainfo/ReportListActivity.kt	Replaced Rx subscriptions with coroutine launches and Flow collection for list updates and title loading.
Source/GUI/Android/app/src/main/java/net/mediaarea/mediainfo/ReportDetailFragment.kt	Replaced Rx fetching/export flow with coroutine-based fetching and background conversion.
Source/GUI/Android/app/src/main/java/net/mediaarea/mediainfo/ReportDetailActivity.kt	Replaced Rx list observation with Flow collection to drive pager updates.
Source/GUI/Android/app/src/main/java/net/mediaarea/mediainfo/ReportDao.kt	Migrated DAO methods to suspend and Flow.
Source/GUI/Android/app/build.gradle	Removed RxJava deps and adjusted coroutine dependency.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[cjee21]

Core.createReport will pass to MediaInfoLib's OpenFd which will close the fd there. Gemini recommends using fd.use so that any remaining 'shell' after detaching will be cleaned up properly.

[cjee21]

Is there an issue if cleanup of UI is run first while report is deleting? We're deleting all anyway.

[cjee21]

This bug is already there before migrating to coroutines. Will see what can be done.

[cjee21]

Let's see if there are anymore issues. I do not know about pager adapter stuff so that is almost 100% Google Gemini AI written, not sure if that is fine.

[JeromeMartinez]

snapshot (but before the last update).

[JeromeMartinez]

@copilot please review the latest changes and author's answers.

[JeromeMartinez]

Snapshot

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

Source/GUI/Android/app/src/main/java/net/mediaarea/mediainfo/ReportDetailFragment.kt:133

• The fallback text "Error fetching report." is hard-coded and not localized. If this is meant to be shown to users (it’s rendered into the WebView), it should come from a string resource (and ideally be consistent with existing error handling like onError()).

                val content = withContext(Dispatchers.IO) {
                    val reportObject = activityListener.getReportViewModel().getReport(id)
                    val report: String = if (reportObject != null ) Core.convertReport(reportObject.report, view) else "Error fetching report."
                    var content = ""
                    if (view != "HTML") {

[cjee21]

Well, it built fine. "should not compile." is wrong. When we assign pageChangeListener = PageChangeListener(reports) Kotlin is smart enough to smart cast and know it will be non-null at the next line. Suggested change just introduces another variable.

[cjee21]

Then the initial suggestion is wrong too as inside saveReport, there are onError() too. But it can be solved.

[cjee21]

I just follow the original style of doing this.

[JeromeMartinez]

No idea about the technic part, but testing as a user, I can say that it is very smooth, also on hundreds of videos files, and switching between files is nice.

[cjee21]

No idea about the technic part, but testing as a user, I can say that it is very smooth, also on hundreds of videos files, and switching between files is nice.

That's good then. I think I have addressed all issues found by Copilot. The list should be even smoother with nice fluid animations when adding and deleting reports after the other PR (which now likely needs some work to rebase after this PR).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

[cjee21]

Gemini: In practice, no, it will likely never exceed Int.MAX_VALUE in your specific app, but it depends entirely on how your database is used.Int.MAX_VALUE is 2,147,483,647. To break that limit, a user would have to import or generate over 2.1 billion reports on a single device. Even if an automated script inserted 10,000 reports every single day, it would take over 580 years to exceed that limit.However, there is one technical nuance with SQLite that you should be aware of:The SQLite rowid LifecycleIn SQLite, if you do not specify a value for a primary key, it uses an internal rowid that can automatically scale up to a 64-bit Long ($9 \times 10^{18}$).Because your Report entity explicitly uses an Int for its primary key, Room enforces a 32-bit ceiling on the database column itself. If the database did somehow reach 2,147,483,647, the next insertion would cause an auto-increment overflow error (SQLiteException: database or disk is full) long before insertReport could return a value that would wrap during a .toInt() conversion.

[JeromeMartinez]

2 billions files is not expected in reality, right :). But who knows, in an hypothetical future...
I am fine to keep that for the moment, until someone wants to fix that.

[cjee21]

Because your Report entity explicitly uses an Int for its primary key, Room enforces a 32-bit ceiling on the database column itself. If the database did somehow reach 2,147,483,647, the next insertion would cause an auto-increment overflow error (SQLiteException: database or disk is full) long before insertReport could return a value that would wrap during a .toInt() conversion.

So we cannot do anything anyway. Unless changing existing ID from Int to Long which I do not know if will break existing databases as I am no database expert.

[cjee21]

I'll let this be. Unlikely to ever happen. Previously getReport can't even return null and would have exception.

[cjee21]

Gemini confirmed what I thought, most of these protections and handling are actually not really needed due to the app not having any list manipulation while viewing the viewpager.

---

Given your specific application architecture, no, it is not worth adding the extra complexity of an asynchronous differ here.

The code review comment is a standard, textbook recommendation for RecyclerView lists, but it overlooks two critical real-world details about how your specific screen works:

1. Your Workflow Minimizes Changes

Operations that mutate this list (like adding or deleting reports) happen on the Main Activity, not while the user is actively swiping through pages on this detail screen. Because updates on this screen are incredibly rare, the chances of triggering a background-sync diff calculation mid-swipe are close to zero.

2. ViewPager Lists Are Naturally Small

AsyncListDiffer is built for infinite scrolling feeds (like a social media timeline or an inventory catalog) with thousands of items. A ViewPager2 detail view typically holds a manageable number of items (e.g., dozens or low hundreds).

Running DiffUtil on a couple hundred items using only lightweight metadata fields takes less than 1 millisecond, which easily fits into the 16ms frame budget of the Main Thread without causing any visual stutter (jank).

[JeromeMartinez]

snapshot.

[cjee21]

Most of the edge cases we are handling likely will never happen since we cannot actually get to the ViewPager activity when there are no reports.

Anyway, asked Gemini to review again and made some more suggested improvements that look fine. Gemini also found some other issues that Copilot did not. Also fixed the export file part by switching between two contexts depending on whether doing file operations or showing error in UI as well as better handling closing file stream as recommended by Gemini.

[JeromeMartinez]

@cjee21 thank you, I'll ask a last time to Copilot in case there is something useful, then @g-maxime will test on several old Android in order to be sure all is fine, then... We'll see in production if we have less crashes!

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

[cjee21]

Ah this is what Copilot missed initially but Gemini found and I just forgot to fix this one after fixing the one above. Fixed now.

[cjee21]

This is old bug. Why did it only see it now? Anyway this is already fixed in the other PR.

[JeromeMartinez]

Anyway this is already fixed in the other PR.

The other PR will be stalled until the abandon of old Android, is it possible to migrate everything not related to SDK updates to this PR?

[cjee21]

Probably difficult. Since I only started noticing and fixing these after changing the structure of the list to the card look. Also fixing this required redesigning the entire recyclerview handling IIRC.

[cjee21]

Trying to port it. Maybe possible. See if it builds and works....

[JeromeMartinez]

Trying to port it. Maybe possible. See if it builds and works....

🧡

[cjee21]

Quick test seems to work. Now adding and removing items have smooth animations and list should maintain position. Will also auto scroll to current report when opening new report in two pane. Also includes some logic that is not used at the moment but will be used when implementing the other PR such as highlighting selected report and rounding first and last cards.

[cjee21]

I think just port this one here. The other changes are not affecting functionality or fixing bugs and just design change, minor code fixes and adding new libcurl feature that can wait till min API increase.

[cjee21]

Still so many issues with that file!

Details

Here is a comprehensive code review of your original ReportDetailFragment implementation. This review consolidates the architectural, lifecycle, thread-safety, and code-quality issues present in the code, along with targeted recommendations on how to fix them.

---

1. Lifecycle & View Integrity Issues

Multiple Layout Inflations (Wasted Resources & Potential Bugs)

In onAttach, you call ReportDetailBinding.inflate(layoutInflater). Then, in onCreateView, you overwrite reportDetailBinding with a brand-new inflation:

override fun onAttach(context: Context) {
    ...
    reportDetailBinding = ReportDetailBinding.inflate(layoutInflater) // First inflation (Discarded)
}

override fun onCreateView(...): View {
    reportDetailBinding = ReportDetailBinding.inflate(inflater, container, false) // Second inflation
    return reportDetailBinding.root
}

• The Problem: The view hierarchy inflated in onAttach is never used, never attached to a window, and immediately discarded. This wastes CPU and memory cycles.
• The Fix: Remove the inflation logic from onAttach completely. Layout layout inflation belongs strictly inside onCreateView.

Memory Leaks via Strong ViewBinding Reference

You are holding a hard reference to private lateinit var reportDetailBinding: ReportDetailBinding.

• The Problem: Fragment instances often outlive their views (for example, when a fragment is placed on the backstack). Because you hold a strong reference to the binding object, the entire UI view hierarchy cannot be garbage collected when the view is destroyed, leading to a memory leak.
• The Fix: Use a nullable backing property that clears itself in onDestroyView():

private var _binding: ReportDetailBinding? = null
private val binding get() = _binding!!

override fun onDestroyView() {
    super.onDestroyView()
    _binding = null
}

---

2. Asynchronous Programming & Thread Safety

Calling UI Methods from a Background Thread

Inside saveReport, you switch to a background thread using withContext(Dispatchers.IO). Inside the try-catch block, you call onError() on the background thread:

private suspend fun saveReport(directory: DocumentFile, report: Report) {
    withContext(Dispatchers.IO) {
        ...
        try {
            val document = directory.createFile(...) ?: run {
                withContext(Dispatchers.Main) { onError() } // Correctly switches threads
                return@withContext
            }
            ...
        } catch (_: Exception) {
            onError() // DANGER: This runs on Dispatchers.IO!
        }
    }
}

• The Problem: If an exception is thrown during file writing, onError() is called directly on Dispatchers.IO. Inside onError(), you display a Toast. In Android, touching UI elements or displaying Toasts from a non-UI thread causes a CalledFromWrongThreadException crash.
• The Fix: Move the error handling out of the background worker entirely by making saveReport return a Boolean success status.

Unsafe Lifecycle Execution for File Writing

In your file-saving flow, you trigger saveReport from inside lifecycleScope.launch.

• The Nuance: Using lifecycleScope here is actually appropriate because file exporting shouldn't terminate mid-write just because a user shifts focus to another app. However, because it keeps running in the background, executing UI functions like onError() blindly upon completion is dangerous if the user has navigated away and destroyed the fragment views.
• The Fix: Before displaying UI feedback (like the Toast in onError), check if the UI is still alive by verifying viewLifecycleOwnerLiveData.value != null.

---

3. Architecture & Clean Code

Fragile Multi-Fragment Coupling

Inside the onMenuItemSelected handler, you loop through all fragments in the parentFragmentManager and force-cast them to recreate them:

parentFragmentManager.fragments.forEach {
    val fragment = it as? ReportDetailFragment
    if (fragment != null) {
        ...
        parentFragmentManager.beginTransaction().detach(it).commit()
        parentFragmentManager.beginTransaction().attach(it).commit()
    }
}

• The Problem: One fragment is directly controlling, manipulating, and forcing lifecycle cycles on adjacent fragments. If you add different types of fragments, use nested fragments, or implement a view pager layout, this code will become fragile, unpredictable, and cause layout bugs.
• The Fix: Move the active view state into a Shared ViewModel. Fragments should simply observe a state flow. When the user changes the layout mode, update the state flow, and any active detail fragments will cleanly re-render themselves without tearing down and rebuilding fragments manually.

Code Readability and Scope Shadows

• The Arrow Anti-Pattern: Inside your registerForActivityResult callback, you have deeply nested if/else checks (id.let -> report == null -> currentContext == null -> directory == null -> !directory.canWrite()). This creates an "arrow" shape that reduces readability. Use guard clauses with clean, early returns (return@registerForActivityResult) to flatten out the script.
• Variable Shadowing: In onViewCreated(view: View, ...), the incoming UI object is named view. In the same block, you point to your global string variable using this@ReportDetailFragment.view. This duplication is highly error-prone. Rename your class string to something explicit like currentViewMode.

---

Final Recommended Blueprint

Applying all the changes discussed above results in a lean, safe, and modern implementation:

class ReportDetailFragment : Fragment() {
    private lateinit var activityListener: ReportActivityListener
    private var sharedPreferences: SharedPreferences? = null
    
    // Leak-safe view binding setup
    private var _binding: ReportDetailBinding? = null
    private val binding get() = _binding!!
    
    // Explicitly renamed to avoid variable shadowing with Android UI View
    private var currentViewMode: String = "HTML"
    private var reportId: Int? = null

    private val saveFile = registerForActivityResult(ActivityResultContracts.OpenDocumentTree()) { uri ->
        val id = reportId
        if (uri == null || id == null) {
            onError()
            return@registerForActivityResult
        }

        // Using fragment lifecycleScope so the data operation finishes even if app goes to background
        viewLifecycleOwner.lifecycleScope.launch {
            val report = activityListener.getReportViewModel().getReport(id)
            val currentContext = context
            
            // Clean guard clauses to flatten code layout
            if (report == null || report.report.isEmpty() || currentContext == null) {
                onError()
                return@launch
            }

            val directory = DocumentFile.fromTreeUri(currentContext, uri)
            if (directory == null || !directory.canWrite()) {
                onError()
                return@launch
            }

            // Save report returns a completion flag
            val isSavedSuccessfully = saveReport(directory, report)

            // Ensure UI updates run securely on the Main thread and check lifecycle availability
            withContext(Dispatchers.Main) {
                if (!isSavedSuccessfully && viewLifecycleOwnerLiveData.value != null) {
                    onError()
                }
            }
        }
    }

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        arguments?.let {
            if (it.containsKey(Core.ARG_REPORT_ID)) {
                val newId = it.getInt(Core.ARG_REPORT_ID)
                if (newId != -1) reportId = newId
            }
        }
    }

    override fun onAttach(context: Context) {
        super.onAttach(context)
        try {
            activityListener = activity as ReportActivityListener
        } catch (_: Throwable) {
            throw ClassCastException("$activity must implement ReportActivityListener")
        }

        sharedPreferences = getDefaultSharedPreferences(context)
        val oldSharedPreferences = activity?.getSharedPreferences(getString(R.string.preferences_key), Context.MODE_PRIVATE)
        val key = getString(R.string.preferences_view_key)

        if (sharedPreferences?.contains(key) == false && oldSharedPreferences?.contains(key) == true) {
            sharedPreferences?.edit {
                putString(key, oldSharedPreferences.getString(key, "HTML"))
            }
        }

        currentViewMode = sharedPreferences?.getString(key, "HTML") ?: "HTML"
    }

    override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View {
        // Layout inflation happens only once here
        _binding = ReportDetailBinding.inflate(inflater, container, false)
        return binding.root
    }

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        displayReport()
        setupMenu()
    }

    private fun displayReport() {
        val id = reportId ?: return
        viewLifecycleOwner.lifecycleScope.launch {
            val content = withContext(Dispatchers.IO) {
                val reportObject = activityListener.getReportViewModel().getReport(id)
                val report = if (reportObject != null) Core.convertReport(reportObject.report, currentViewMode) else ""
                
                if (currentViewMode != "HTML") {
                    buildString {
                        append("<html><head><style>:root { color-scheme: var(--color-scheme, light); } @media (prefers-color-scheme: dark) { :root { --color-scheme: dark; } }</style></head><body><pre>")
                        append(report.replace("\t", "    ").htmlEncode())
                        append("</pre></body></html>")
                    }
                } else {
                    report
                }
            }
            binding.reportDetail.loadDataWithBaseURL(null, content, "text/html", "utf-8", null)
        }
    }

    private fun setupMenu() {
        val menuHost: MenuHost = requireActivity()
        menuHost.addMenuProvider(object : MenuProvider {
            override fun onCreateMenu(menu: Menu, menuInflater: MenuInflater) {
                menuInflater.inflate(R.menu.menu_detail, menu)
                val viewMenu: SubMenu = menu.findItem(R.id.action_change_view).subMenu ?: return
                
                Core.views.forEachIndexed { index, current ->
                    val desc = if (current.desc == "Text") resources.getString(R.string.text_output_desc) else current.desc
                    viewMenu.add(R.id.menu_views_group, index, Menu.NONE, desc).apply {
                        isCheckable = true
                        isChecked = (current.name == currentViewMode)
                    }
                }
                viewMenu.setGroupCheckable(R.id.menu_views_group, true, true)
            }

            override fun onMenuItemSelected(menuItem: MenuItem): Boolean {
                return when (menuItem.itemId) {
                    R.id.action_export_report -> {
                        saveFile.launch(null)
                        true
                    }
                    else -> if (menuItem.groupId == R.id.menu_views_group) {
                        val selectedView = Core.views[menuItem.itemId].name
                        if (currentViewMode != selectedView) {
                            currentViewMode = selectedView
                            sharedPreferences?.edit { putString(getString(R.string.preferences_view_key), currentViewMode) }
                            
                            // Refresh presentation directly instead of fragment churning
                            displayReport()
                        }
                        true
                    } else {
                        false
                    }
                }
            }
        }, viewLifecycleOwner, Lifecycle.State.RESUMED)
    }

    // Completely separated from UI logic. Focused entirely on File I/O operations.
    private suspend fun saveReport(directory: DocumentFile, report: Report): Boolean = withContext(Dispatchers.IO) {
        val reportText = Core.convertReport(report.report, currentViewMode, true)
        val filename = String.format(Locale.US, "%s.%s", report.filename, currentViewMode)
        val mime = Core.views.find { it.name == currentViewMode }?.mime ?: "text/plain"

        return@withContext try {
            val document = directory.createFile(mime, filename) ?: return@withContext false
            context?.contentResolver?.openOutputStream(document.uri)?.use { ostream ->
                ostream.write(reportText.toByteArray())
                ostream.flush()
            }
            true
        } catch (_: Exception) {
            false
        }
    }

    private fun onError() {
        activity?.applicationContext?.let {
            Toast.makeText(it, R.string.error_write_text, Toast.LENGTH_LONG).show()
        }
    }

    override fun onDestroyView() {
        super.onDestroyView()
        _binding = null // Releasing view binding structure to prevent memory leaks
    }
}

[cjee21]

Should be fine if this works properly in actual test. Already went through few rounds with Gemini and it is different every time. The last review seems no major issues. Never ending if keep requesting review from AI. Even it's own suggestions will be criticized by itself the next round.

[cjee21]

Switching views doesn't work now....

[JeromeMartinez]

Never ending if keep requesting review from AI. Even it's own suggestions will be criticized by itself the next round.

No worry, I use it only for hint, chasing potential crashes.
But I need no regression...

[cjee21]

Looks like there may be some real issues with the lifecycle stuff or memory leaks and may crash in rare case if AI is correct, it seems to make sense. But the refactoring is causing many regressions. So maybe revert the last commit and let it be if no issues in actual testing. Wasted time on refactoring but at least know it is not so easy and changing fragment code can also break deleting reports for some reason that I do not know.

[cjee21]

Even after revert still have regression. I do not know which part started causing troubles now.

[JeromeMartinez]

if you didn't keep history, I have some source snapshots at https://mediaarea.net/download/snapshots/branches/source/mediainfo/

[JeromeMartinez]

So maybe revert the last commit and let it be if no issues in actual testing.

I am fine with that if you are confident that the version before the last commit is better than without the PR. I let you decide what you think to be the best version.

[cjee21]

Ok, not wasted. the refactoring seems to work now after some more changes.

[cjee21]

I think just go with this if no one finds any more issues.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

[cjee21]

Looks like it has nothing more to say other than the code comments :)

[cjee21]

if you didn't keep history, I have some source snapshots at https://mediaarea.net/download/snapshots/branches/source/mediainfo/

FYI can easily revert to any commit using git reset with hash. If do not know hash can refer to git reflog for recent hashes. If local copy is gone (garbage collected) can usually fetch any pushed commit from GitHub if know the full hash which can be obtained from PR history. GitHub seems to keep commits forever (so reminder to be careful not to accidentally push secrets).

[JeromeMartinez]

Snapshot

[cjee21]

Found and fixed bug after the refactoring where view selection menu does not update the checked item when changed.

[JeromeMartinez]

Snapshot.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

[cjee21]

I think low risk? Loading is usually way faster than a user can open the menu and select another mode. In rare case that it is slow and older job overwrites, user can just tap the desired mode again in the menu.

[JeromeMartinez]

I think low risk?

I agree.

[JeromeMartinez]

@cjee21 no pressure about Copilot feedback, it is just if it hints something you consider useful, else code can stay as is.

[cjee21]

Static analysis doesn't detect any issues beyond code style as well. So I think should be fine if actual testing finds no issues. I added another two commits from the other PR to reduce the commits of the other PR. Just minor changes that doesn't affect functionality, no review needed.