[Infra] Adding a button to snapshot the current Media Browse Tree #28
Conversation
There was a problem hiding this comment.
I added some comments, but overall I think this really most of this needs to be split into a separate file and more, smaller methods. A method for doing the DFS, a method for printing a media item, etc...
I'm also not sure why the method requires Android N+? In generally I'd really like to keep the minSdkVersion=21 (I'd actually be fine raising it to 23, for various reasons, but not 24 yet). So I'd also prefer if you could guard the limited things that actually need Android N+ with a Build.VERSION.SDK_INT check around those bits, rather than annotating the entire method.
...controller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java
Outdated
Show resolved
Hide resolved
...controller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java
Outdated
Show resolved
Hide resolved
...controller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java
Outdated
Show resolved
Hide resolved
modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaBrowseTreeSnapshot.kt modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaBrowseTreeSnapshot.kt
There was a problem hiding this comment.
I was taking a quick look and wanted to provide this feedback early since it may require digging into suspend functions a bit more than you expected.
(You should be able to convert the callback into a suspendCoroutine -- Roman gives and example of doing that here: https://stackoverflow.com/questions/48552925/existing-3-function-callback-to-kotlin-coroutines/48562175#48562175 )
| import java.util.concurrent.Executors | ||
| import java.util.concurrent.Semaphore | ||
|
|
||
| class MediaBrowseTreeSnapshot(private val mBrowser: MediaBrowserCompat, private val mContext: Context) { |
There was a problem hiding this comment.
Kotlin doesn't use 'm' prefixes in parameters or member variables.
nit: Ideally context should be the first parameter
| val loaded = Semaphore(1) | ||
| val executorService = Executors.newFixedThreadPool(4) | ||
| val mItems: MutableList<MediaBrowserCompat.MediaItem> = ArrayList() | ||
| executorService.execute { |
There was a problem hiding this comment.
Since this is in Kotlin, you should use suspend functions, not executors, to do the background work.
modified: mediacontroller/src/main/AndroidManifest.xml modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppListAdapter.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaBrowseTreeSnapshot.kt modified: mediacontroller/src/main/AndroidManifest.xml modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppListAdapter.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaBrowseTreeSnapshot.kt
modified: mediacontroller/build.gradle modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaBrowseTreeSnapshot.kt modified: mediacontroller/src/main/res/layout/media_browse_tree.xml modified: mediacontroller/build.gradle modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaAppControllerActivity.java modified: mediacontroller/src/main/java/com/example/android/mediacontroller/MediaBrowseTreeSnapshot.kt modified: mediacontroller/src/main/res/layout/media_browse_tree.xml
I believe that I have fixed all the issues you have brought up. |
There was a problem hiding this comment.
Thanks :)
I don't know how much work it would be (or if it would be worth it), but I could also see value in having all the file i/o together in one bit, so the code visited all the nodes and collected them, and then there was a single spot that everything was actually output.
I'm going to mark it as approved (since the only other comment was whether you think it might make sense to do the I/O bit on Dispatchers.IO rather than Dispatchers.DEFAULT), but if you did want to do those updates (either now or in a follow up), I'd be happy to review them again.
Thank you!
| * item nodes. | ||
| */ | ||
| private suspend fun runDFSOnBrowseTree(mediaItems: MutableList<MediaBrowserCompat.MediaItem>, outputStream: OutputStream) { | ||
| val printWriter = PrintWriter(outputStream) |
There was a problem hiding this comment.
Consider whether this bit (the output part) should be wrapped in a withContext(Dispatchers.IO) block.
There was a problem hiding this comment.
Sounds good! I will merge this then update the functionality soon.
The MCT app already has a section that lists the single level contents of a media browse tree. This section can be accessed by clicking the control button then swiping to the media items tab. This page displays the list of media items at the current level of the media browse tree. There are two buttons, up and top. The Up button brings you one level higher on the browse tree and the top button takes you to the root node. This page is useful but only gives us a single level of the tree and not the whole tree itself. We propose to add a button next to the “Top” and “Up” that will be labeled “Save to File”.
This button will start at the root node and run a depth first search tree traversal printing out each of the browse tree elements in the following format. Each subsequent level will have an added tab character in front of the media items description. One possible output could look like this: