Pull different steps in the codelab into modules #63

tunjid · 2021-06-25T15:46:10Z

A single PR against a copy of master with all the commits squashed; closes out #60, #61 & #62

Commit summary:

Initial kotlin conversion
Update kotlin to be more idiomatic
More idiomatic kotlin

dturner

This looks great. I tried pretty hard to find issues with the code but could only find one thing! I've asked borrelli@ to take a look as well.

Please don't merge until the codelab itself has been updated as well.

dturner · 2021-06-28T17:35:18Z

app/build.gradle

@@ -24,5 +25,6 @@ android {
 }

 dependencies {
-    implementation project(":player-lib")
+    // Switch out the code lab step to jump to different points in the code lab
+    implementation project(':exoplayer-codelab-04')


Suggested change

implementation project(':exoplayer-codelab-04')

implementation project(':exoplayer-codelab-00')

Done, and force pushed.

nic0lette

Looks good =D I made some comments about using some of the more helpful parts of AndroidX. Other stuff is much smaller.

nic0lette · 2021-06-29T09:24:30Z

exoplayer-codelab-01/src/main/java/com/example/exoplayer/PlayerActivity.kt

+    private fun initializePlayer() {
+        player = SimpleExoPlayer.Builder(this)
+            .build()
+            .also {


Consider an explicit name for it or even just getting rid of the also block, since this doesn't all need to be in one statement.

Opted for it based on conversations here to be more idiomatic: #62 as the more explicit names can be a bit noisy. It could also just be that I didn't choose very good explicit names lmao.

What would you suggest naming it more explicitly as?

Some alternatives:

Use apply instead of also. This would avoid using it.

Rename it to exoPlayer since it is the ExoPlayer implementation of the Player interface.

Opted for apply { -> exoPlayer } as the semantics of this got hard to discern because of the playWhenReady var .

It would have required:

apply { playWhenReady = this@PlayerActivity.playWhenReady }

nic0lette · 2021-06-29T09:25:31Z

exoplayer-codelab-01/src/main/java/com/example/exoplayer/PlayerActivity.kt

+    }
+
+    private fun releasePlayer() {
+        player?.let {


Similar to the above, providing a specific name for it

nic0lette · 2021-06-29T09:29:14Z

exoplayer-codelab-04/src/main/java/com/example/exoplayer/PlayerActivity.kt

+import com.google.android.exoplayer2.util.MimeTypes
+import com.google.android.exoplayer2.util.Util
+
+private val TAG = PlayerActivity::class.java.name


Suggest just putting in a static string here, which would allow making this a private const val

nic0lette · 2021-06-29T09:54:22Z

exoplayer-codelab-00/src/main/java/com/example/exoplayer/PlayerActivity.kt

-    super.onCreate(savedInstanceState);
-    setContentView(R.layout.activity_player);
-  }
+    private val viewBinding by lazy { ActivityPlayerBinding.inflate(layoutInflater) }


Consider moving this to be a local variable in onCreate -- I know it's used beyond onCreate in subsequent steps, so I'll explain more there.

nic0lette · 2021-06-29T10:10:41Z

exoplayer-codelab-01/src/main/java/com/example/exoplayer/PlayerActivity.kt

+    public override fun onStart() {
+        super.onStart()
+        if (Util.SDK_INT > 23) {
+            initializePlayer()


Instead of calling these inline like this, consider creating a LifecycleObserver. It could be initialized in onCreate and added as an observer to the activity with getLifecycle().addObserver(...).

If I were building it, I might consider having two parameters to the observer -- the 'init' and 'release' lifecycle states. Then, in the observer, check the state and call the appropriate method. (This way there wouldn't have to be duplicate code to init/release the player in onStart/onResume and onPause/onStop)

I considered this, however my major prompt was to keep it super simple so users can copy paste, and also to keep it as close to the original java codelab as possible as this is a first pass kotlin conversion.

After the convergence implementation, I can then explore using androidx dependencies like in lifecycle observers, and then even plausibly move the exoplayer instance to a ViewModel.

I can't do that at the moment as it would require extra dependencies, and even more edits to the codelab doc.

I think this is a good suggestion but agree that we might want to do this in a follow up PR. I would file a separate issue on this repo so that this suggestion doesn't get forgotten.

Created #65

nic0lette · 2021-06-29T10:12:40Z

exoplayer-codelab-01/build.gradle

-    delete rootProject.buildDir
-}
+    implementation 'androidx.appcompat:appcompat:1.2.0'
+    implementation 'com.google.android.exoplayer:exoplayer-core:2.12.0'


Maybe consider moving 2.12.0 to a gradle variable at the top level, since these are used in each of the modules, and they're all the same version (unless this might change in the future) -- even then, though, it might be useful to have a core, dash, and ui version in that case, so they only have to be updated in one place.

Will this affect the user's ability to copy paste? I considered doing this as well as I do it with the kotlin version, but as the user may be jumping in between modules, I decided to err on the side of copy pasting convenience.

I wouldn't get too caught up on copypastability. Maintainability and readability is also important. In this instance I agree with @nic0lette that moving to gradle variables is the better solution since it avoids duplicating the same version string throughout the code, although this could also be done in a follow up PR.

Will create an issue as an action item.

Created #66

dturner · 2021-07-07T14:38:53Z

exoplayer-codelab-04/src/main/java/com/example/exoplayer/PlayerActivity.kt

+    }
+
+    private fun releasePlayer() {
+        player?.let {


player?.run { playbackPosition = this.currentPosition currentWindow = this.currentWindowIndex playWhenReady = this.playWhenReady removeListener(playbackStateListener) release() }

Using run is marginally better here as it avoids it

dturner · 2021-07-08T12:12:15Z

@tunjid Thanks for all your hard work on this. Please rebase and merge as soon as the codelab has been updated.

tunjid requested a review from dturner June 25, 2021 15:46

This was referenced Jun 25, 2021

Pull different steps in the codelab into modules #60

Closed

Initial kotlin conversion #61

Closed

Update kotlin to be more idiomatic #62

Closed

dturner requested review from dturner and removed request for dturner June 28, 2021 17:30

dturner requested changes Jun 28, 2021

View reviewed changes

tunjid force-pushed the kotlin-squashed branch from 40f5423 to f7a2982 Compare June 28, 2021 18:18

nic0lette reviewed Jun 29, 2021

View reviewed changes

dturner reviewed Jul 7, 2021

View reviewed changes

dturner approved these changes Jul 8, 2021

View reviewed changes

Initial kotlin conversion

823f1b2

tunjid force-pushed the kotlin-squashed branch from 5c3c25a to 823f1b2 Compare July 8, 2021 13:09

tunjid merged commit 474f624 into android:kotlin-refresh Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull different steps in the codelab into modules #63

Pull different steps in the codelab into modules #63

tunjid commented Jun 25, 2021 •

edited

Loading

dturner left a comment

dturner Jun 28, 2021

tunjid Jun 28, 2021

nic0lette left a comment

nic0lette Jun 29, 2021

tunjid Jun 29, 2021 •

edited

Loading

dturner Jul 5, 2021 •

edited

Loading

tunjid Jul 7, 2021

nic0lette Jun 29, 2021

nic0lette Jun 29, 2021

nic0lette Jun 29, 2021

nic0lette Jun 29, 2021

tunjid Jun 29, 2021

dturner Jun 29, 2021

tunjid Jun 29, 2021

nic0lette Jun 29, 2021

tunjid Jun 29, 2021 •

edited

Loading

dturner Jun 29, 2021

tunjid Jun 29, 2021

tunjid Jun 29, 2021

dturner Jul 7, 2021

dturner Jul 7, 2021

dturner commented Jul 8, 2021

	implementation project(':exoplayer-codelab-04')
	implementation project(':exoplayer-codelab-00')

Pull different steps in the codelab into modules #63

Pull different steps in the codelab into modules #63

Conversation

tunjid commented Jun 25, 2021 • edited Loading

dturner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nic0lette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunjid Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

dturner Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunjid Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dturner commented Jul 8, 2021

tunjid commented Jun 25, 2021 •

edited

Loading

tunjid Jun 29, 2021 •

edited

Loading

dturner Jul 5, 2021 •

edited

Loading

tunjid Jun 29, 2021 •

edited

Loading