Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,21 @@ package io.github.droidkaigi.confsched2019.announcement.ui.actioncreator
import androidx.lifecycle.Lifecycle
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Unused import

import io.github.droidkaigi.confsched2019.action.Action
import io.github.droidkaigi.confsched2019.data.repository.AnnouncementRepository
import io.github.droidkaigi.confsched2019.di.PageScope
import io.github.droidkaigi.confsched2019.dispatcher.Dispatcher
import io.github.droidkaigi.confsched2019.ext.android.coroutineScope
import io.github.droidkaigi.confsched2019.model.LoadingState
import io.github.droidkaigi.confsched2019.system.actioncreator.ErrorHandler
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle
import io.github.droidkaigi.confsched2019.util.coroutineScope
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import javax.inject.Inject

class AnnouncementActionCreator @Inject constructor(
override val dispatcher: Dispatcher,
private val announcementRepository: AnnouncementRepository,
@PageScope private val lifecycle: Lifecycle
) : CoroutineScope by lifecycle.coroutineScope,
private val screenLifecycle: ScreenLifecycle
) : CoroutineScope by screenLifecycle.coroutineScope,
ErrorHandler {

fun load() = launch {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
package io.github.droidkaigi.confsched2019.session.ui.actioncreator

import androidx.lifecycle.Lifecycle
import io.github.droidkaigi.confsched2019.action.Action
import io.github.droidkaigi.confsched2019.di.PageScope
import io.github.droidkaigi.confsched2019.dispatcher.Dispatcher
import io.github.droidkaigi.confsched2019.ext.android.coroutineScope
import io.github.droidkaigi.confsched2019.model.SearchResult
import io.github.droidkaigi.confsched2019.model.SessionContents
import io.github.droidkaigi.confsched2019.system.actioncreator.ErrorHandler
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle
import io.github.droidkaigi.confsched2019.util.coroutineScope
import kotlinx.coroutines.CoroutineScope
import javax.inject.Inject

@PageScope
class SearchActionCreator @Inject constructor(
override val dispatcher: Dispatcher,
@PageScope private val lifecycle: Lifecycle
) : CoroutineScope by lifecycle.coroutineScope,
private val screenLifecycle: ScreenLifecycle
) : CoroutineScope by screenLifecycle.coroutineScope,
ErrorHandler {
fun search(query: String?, sessionContents: SessionContents) {
// if we do not have query, we should show speakers and sessions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import io.github.droidkaigi.confsched2019.model.LoadingState
import io.github.droidkaigi.confsched2019.model.Session
import io.github.droidkaigi.confsched2019.session.R
import io.github.droidkaigi.confsched2019.system.actioncreator.ErrorHandler
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle
import io.github.droidkaigi.confsched2019.util.SessionAlarm
import io.github.droidkaigi.confsched2019.util.coroutineScope
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import timber.log.Timber
Expand All @@ -22,9 +24,9 @@ import javax.inject.Inject
class SessionContentsActionCreator @Inject constructor(
override val dispatcher: Dispatcher,
private val sessionRepository: SessionRepository,
@PageScope private val lifecycle: Lifecycle,
private val screenLifecycle: ScreenLifecycle,
private val sessionAlarm: SessionAlarm
) : CoroutineScope by lifecycle.coroutineScope,
) : CoroutineScope by screenLifecycle.coroutineScope,
ErrorHandler {
fun refresh() = launch {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ import io.github.droidkaigi.confsched2019.model.Room
import io.github.droidkaigi.confsched2019.model.Session
import io.github.droidkaigi.confsched2019.model.SessionPage
import io.github.droidkaigi.confsched2019.system.actioncreator.ErrorHandler
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle
import io.github.droidkaigi.confsched2019.util.coroutineScope
import kotlinx.coroutines.CoroutineScope
import javax.inject.Inject

@PageScope
class SessionPagesActionCreator @Inject constructor(
override val dispatcher: Dispatcher,
@PageScope private val lifecycle: Lifecycle
) : CoroutineScope by lifecycle.coroutineScope,
private val screenLifecycle: ScreenLifecycle
) : CoroutineScope by screenLifecycle.coroutineScope,
ErrorHandler {
fun load(sessions: List<Session>) {
dispatcher.launchAndDispatch(Action.SessionsLoaded(sessions))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,22 @@ package io.github.droidkaigi.confsched2019.sponsor.ui.actioncreator

import androidx.lifecycle.Lifecycle
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Unused import

import io.github.droidkaigi.confsched2019.action.Action
import io.github.droidkaigi.confsched2019.di.PageScope
import io.github.droidkaigi.confsched2019.dispatcher.Dispatcher
import io.github.droidkaigi.confsched2019.ext.android.coroutineScope
import io.github.droidkaigi.confsched2019.model.LoadingState
import io.github.droidkaigi.confsched2019.system.actioncreator.ErrorHandler
import io.github.droidkaigi.confsched2019.data.repository.SponsorRepository
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle
import io.github.droidkaigi.confsched2019.util.coroutineScope
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import javax.inject.Inject

class SponsorActionCreator @Inject constructor(
Copy link
Member

Choose a reason for hiding this comment

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

If you do not make a scope here, another SponsorActionCreator will be created when the screen rotates, is it difficult to make the scope?
画面回転時にもう一個インスタンスができてしまうんですが、スコープを付けるのって難しいですかね、、?

Copy link
Member

Choose a reason for hiding this comment

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

これは難しそうなので、とりあえずなくてもいいかもですね。。

override val dispatcher: Dispatcher,
private val sponsorRepository: SponsorRepository,
@PageScope private val lifecycle: Lifecycle
) : CoroutineScope by lifecycle.coroutineScope,
private val screenLifecycle: ScreenLifecycle
) : CoroutineScope by screenLifecycle.coroutineScope,
ErrorHandler {
fun load() = launch {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import io.github.droidkaigi.confsched2019.model.LoadingState
import io.github.droidkaigi.confsched2019.model.StaffContents
import io.github.droidkaigi.confsched2019.model.StaffSearchResult
import io.github.droidkaigi.confsched2019.system.actioncreator.ErrorHandler
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle
import io.github.droidkaigi.confsched2019.util.coroutineScope
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import javax.inject.Inject
Expand All @@ -18,8 +20,8 @@ import javax.inject.Inject
class StaffSearchActionCreator @Inject constructor(
override val dispatcher: Dispatcher,
private val staffRepository: StaffRepository,
@PageScope private val lifecycle: Lifecycle
) : CoroutineScope by lifecycle.coroutineScope, ErrorHandler {
private val screenLifecycle: ScreenLifecycle
) : CoroutineScope by screenLifecycle.coroutineScope, ErrorHandler {

fun load() = launch {
try {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package io.github.droidkaigi.confsched2019.di

import androidx.fragment.app.FragmentActivity
import androidx.lifecycle.ViewModelProviders
import dagger.Module
import dagger.Provides
import io.github.droidkaigi.confsched2019.ui.ScreenViewModel
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle

@Module
object ScreenModule {

@JvmStatic @Provides fun provideScreenViewModel(
activity: FragmentActivity
): ScreenViewModel {
return ViewModelProviders.of(activity).get(ScreenViewModel::class.java)
}

@JvmStatic @Provides fun provideScreenLifecycle(
screenViewModel: ScreenViewModel
): ScreenLifecycle {
return screenViewModel.lifecycle
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import io.github.droidkaigi.confsched2019.announcement.ui.AnnouncementFragment
import io.github.droidkaigi.confsched2019.announcement.ui.AnnouncementFragmentModule
import io.github.droidkaigi.confsched2019.databinding.ActivityMainBinding
import io.github.droidkaigi.confsched2019.di.PageScope
import io.github.droidkaigi.confsched2019.di.ScreenModule
import io.github.droidkaigi.confsched2019.ext.android.changed
import io.github.droidkaigi.confsched2019.floormap.ui.FloorMapFragment
import io.github.droidkaigi.confsched2019.floormap.ui.FloorMapFragmentModule
Expand Down Expand Up @@ -247,7 +248,7 @@ abstract class MainActivityModule {

@Module
abstract class MainActivityBuilder {
@ContributesAndroidInjector(modules = [MainActivityModule::class])
@ContributesAndroidInjector(modules = [MainActivityModule::class, ScreenModule::class])
abstract fun contributeMainActivity(): MainActivity
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package io.github.droidkaigi.confsched2019.ui

import androidx.lifecycle.ViewModel
import io.github.droidkaigi.confsched2019.util.ScreenLifecycle

class ScreenViewModel : ViewModel() {

val lifecycle: ScreenLifecycle = ScreenLifecycle()

init {
lifecycle.dispatchOnInit()
}

override fun onCleared() {
lifecycle.dispatchOnCleared()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package io.github.droidkaigi.confsched2019.util

import androidx.annotation.IntDef

class ScreenLifecycle {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to make ScreenLifecycle a Lifecycle of AAC?
ScreenLifecycleをAACのライフサイクルにすることって可能ですかね?


companion object {
const val NONE = 0
const val INIT = 1
const val CLEARED = 2

@Target(AnnotationTarget.TYPE, AnnotationTarget.PROPERTY)
@IntDef(
NONE,
INIT,
CLEARED
)
annotation class State
}

@State
internal var state: Int = NONE

private val onInitHooks = HashSet<(() -> Unit)>()
private val onDestroyHooks = HashSet<(() -> Unit)>()

fun onInit(r: (() -> Unit)) {
onInitHooks.add(r)
if (state == INIT) {
r()
}
}

fun onCleared(r: (() -> Unit)) {
onDestroyHooks.add(r)
if (state == CLEARED) {
r()
}
}

fun dispatchOnInit() {
state = INIT
onInitHooks.forEach { it() }
}

fun dispatchOnCleared() {
state = CLEARED
onDestroyHooks.forEach { it() }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package io.github.droidkaigi.confsched2019.util

import io.github.droidkaigi.confsched2019.ext.android.Dispatchers
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.launch

/**
* This implementation refers to https://github.com/Kotlin/kotlinx.coroutines/pull/760
*/
fun ScreenLifecycle.createJob(
Copy link
Contributor

Choose a reason for hiding this comment

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

この実装の思想は「画面回転を跨いだActivity」が最長ライフサイクルである前提に立ってると思うんですが、実際には Application が最大であり、それに対応できていないこと、また Supervisor を使ってのViewModelのインスタンススコープから外れた Global な実装は取扱を間違えると memory leak しそうです。正確には、この実装が暗黙的なライフサイクルとコールバックチェーンに支えられていて、コードだけ見ても memory leak しないという自信がないです。
また Dagger の Provide 部分が非同期になると memory cache に適切なsemaphoreがなくて、パッと見 memory leak 無しに動くか怪しいです。非同期で provide することは現状視野に入れてませんが、暗黙的にその制約がつくのはあまり好ましくないというのが正直な感想です。

Twitter でちらっと見た感じの印象としては ViewModel が直接 CoroutineScope (と必要であれば Dagger2 のComponent) を管理するイメージでした。(最新の ktx alpha の方式)
Pros/Cons を見たいんですが、この方式で最新の ktx alpha と比較したときの Pros/Cons ってなんでしょうか?

Copy link
Contributor

@jmatsu jmatsu Jan 21, 2019

Choose a reason for hiding this comment

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

ActivityのViewModelのライフサイクルを用いてCoroutineのJobを管理する

僕個人として、この考え自体には凄い賛成です。特にこのアプリはこの考えの恩恵をかなり受けられそうです。

Copy link
Member

@takahirom takahirom Jan 21, 2019

Choose a reason for hiding this comment

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

これの方式との比較が気になるという感じですかね?確かにAACのライフサイクル化よりそちらを先に考えたほうがいいかもですねー
https://qiita.com/kafumi/items/8a8764679df8646b764a

state: @ScreenLifecycle.Companion.State Int = ScreenLifecycle.NONE
): Job {
require(state != ScreenLifecycle.CLEARED) {
"CLEARED is a terminal state that is forbidden for createJob(…), to avoid leaks."
}
return SupervisorJob().also { job ->
when (state) {
ScreenLifecycle.CLEARED -> job.cancel()
else -> GlobalScope.launch(Dispatchers.Main) {
onCleared {
job.cancel()
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package io.github.droidkaigi.confsched2019.util

import io.github.droidkaigi.confsched2019.ext.android.Dispatchers
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import java.util.concurrent.ConcurrentHashMap

private val cachedLifecycleCoroutineScopes = ConcurrentHashMap<ScreenLifecycle, CoroutineScope>()
private val cachedLifecycleJobs = ConcurrentHashMap<ScreenLifecycle, Job>()

val ScreenLifecycle.coroutineScope: CoroutineScope
get() = cachedLifecycleCoroutineScopes[this] ?: job.let { job ->
val newScope = CoroutineScope(job + Dispatchers.Main)
if (job.isActive) {
cachedLifecycleCoroutineScopes[this] = newScope
job.invokeOnCompletion { cachedLifecycleCoroutineScopes -= this }
}
newScope
}

val ScreenLifecycle.job: Job
get() = cachedLifecycleJobs[this] ?: createJob().also {
if (it.isActive) {
cachedLifecycleJobs[this] = it
it.invokeOnCompletion { cachedLifecycleJobs -= this }
}
}