Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Network awareness #168

Merged
merged 10 commits into from
May 17, 2022
Merged

Network awareness #168

merged 10 commits into from
May 17, 2022

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented May 16, 2022

WHAT

First import of network awareness module.

#128

WHY

Get a build in place and replace the in place code in the Media libraries.

Checklist 📋

  • Add explicit visibility modifier and explicit return types for public declarations
  • Run spotless check
  • Run tests
  • Update metalava's signature text files

yschimke added 2 commits May 16, 2022 08:06
Network Awareness module
@yschimke yschimke requested a review from wrkben May 16, 2022 08:08
@yschimke yschimke marked this pull request as ready for review May 16, 2022 10:26
@yschimke yschimke requested a review from luizgrp May 16, 2022 10:26
Copy link
Member

@luizgrp luizgrp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments that can be iterated over in subsequent prs

fun inject(mainActivity: MainActivity) {
mainActivity.mediaPlayerScreenViewModelFactory =
getMediaPlayerScreenViewModelFactory(getPlayerRepositoryImplFactory(getMediaDataSource()))
}

fun inject(navActivity: NavActivity) {
navActivity.dataRequestRepository = getDataRequestRepository()
navActivity.networkRepository = getNetworkRepository(navActivity, navActivity.lifecycleScope)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the NetworkRepository use the ViewModel scope instead? so in case of activity recreation, any request in progress wouldn't be stopped/started again

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - damn that's a nasty bug. Thanks for spotting.

NetworkRepository is likely application scoped in practice. A single OkHttp client shared by activities and services. But this sample is just working with the SampleAppDI. It's not a hilt/koin killer yet...

Copy link
Collaborator Author

@yschimke yschimke May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to put this as a TODO/Comment. Without adding Application Scope to your sample DI system, not sure I can do much here. But you are right, it's a bug as it stands, not so critical for the sample. I'll change the CoroutineScope to an application instance however.


public open class DelegatingEventListener(
private val delegate: EventListener
) : EventListener() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be able to make this class simpler using delegation:

public open class DelegatingEventListener(
    private val delegate: EventListener
) : EventListener() by delegate 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, only works for interfaces.

@yschimke
Copy link
Collaborator Author

@wrkben will follow up with any comments and document the existing TODOs in the github issue, but would like to work in smaller PRs.

@yschimke yschimke merged commit 5d568a0 into google:main May 17, 2022
@yschimke yschimke deleted the network_awareness branch June 20, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants