Skip to content

（提案）ActivityのViewModelのライフサイクルを用いてCoroutineのJobを管理する #578

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.

Sign up for GitHub

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

Jump to bottom
Open
magiepooh wants to merge 1 commit into
base: master
Choose a base branch
from
Open

（提案）ActivityのViewModelのライフサイクルを用いてCoroutineのJobを管理する #578

magiepooh wants to merge 1 commit into from

Conversation

@magiepooh
Copy link

@magiepooh magiepooh commented Jan 21, 2019

Issue

Overview (Required)

  • ActivityのViewModelと同じライフサイクルを保持するScreenViewModelを作成
  • ScreenViewModelのライフサイクルを提供するScreenLifecycleを作成
  • ScreenLifecycleからJobを管理
  • ActionCreatorで使っていたPageScopeのLifecycleをScreenLifecycleへ置き換え

Links

Screenshot

Before After

@jmatsu-bot
Copy link
Collaborator

jmatsu-bot commented Jan 21, 2019

Your app is deployed! Try it via https://deploygate.com/distributions/eaf4c330bf9539ad4b73fc53edd28a20f693a045

@jmatsu-bot
Copy link
Collaborator

jmatsu-bot commented Jan 21, 2019

Apk comparision results

Property Summary
New File Size 13948350 Bytes. (13.3 MB)
File Size Change -16733 Bytes. (-16.34 KB)
Download Size Change -14397 Bytes. (-14.06 KB)
New Method Reference Count 155251
Method Reference Count Change -219
New Number of dex file(s) 4
Number of dex file(s) Change 0

Generated by 🚫 Danger

jmatsu-bot
jmatsu-bot reviewed Jan 21, 2019
View reviewed changes
...o/github/droidkaigi/confsched2019/announcement/ui/actioncreator/AnnouncementActionCreator.kt
@@ -1,22 +1,23 @@
package io.github.droidkaigi.confsched2019.announcement.ui.actioncreator

import androidx.lifecycle.Lifecycle
Copy link
Collaborator

@jmatsu-bot jmatsu-bot Jan 21, 2019

Choose a reason for hiding this comment

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

🚫 Unused import

jmatsu-bot
jmatsu-bot reviewed Jan 21, 2019
View reviewed changes
...ain/java/io/github/droidkaigi/confsched2019/sponsor/ui/actioncreator/SponsorActionCreator.kt
@@ -1,22 +1,23 @@
package io.github.droidkaigi.confsched2019.sponsor.ui.actioncreator

import androidx.lifecycle.Lifecycle
Copy link
Collaborator

@jmatsu-bot jmatsu-bot Jan 21, 2019

Choose a reason for hiding this comment

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

🚫 Unused import

@jmatsu-bot
Copy link
Collaborator

jmatsu-bot commented Jan 21, 2019

3 Errors
🚫 feature/session/src/main/java/io/github/droidkaigi/confsched2019/session/ui/actioncreator/SessionContentsActionCreator.kt#L3 - Unused import
🚫 feature/session/src/main/java/io/github/droidkaigi/confsched2019/session/ui/actioncreator/SessionPagesActionCreator.kt#L3 - Unused import
🚫 feature/staff/src/main/java/io/github/droidkaigi/confsched2019/staff/ui/actioncreator/StaffSearchActionCreator.kt#L3 - Unused import

Generated by 🚫 Danger

takahirom
takahirom reviewed Jan 21, 2019
View reviewed changes
...nt/androidcomponent/src/main/java/io/github/droidkaigi/confsched2019/util/ScreenLifecycle.kt

import androidx.annotation.IntDef

class ScreenLifecycle {
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.

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

takahirom
takahirom reviewed Jan 21, 2019
View reviewed changes
...ain/java/io/github/droidkaigi/confsched2019/sponsor/ui/actioncreator/SponsorActionCreator.kt
import kotlinx.coroutines.launch
import javax.inject.Inject

class SponsorActionCreator @Inject constructor(
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.

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

@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.

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

jmatsu
jmatsu reviewed Jan 21, 2019
View reviewed changes
...androidcomponent/src/main/java/io/github/droidkaigi/confsched2019/util/ScreenLifecycleExt.kt
/**
* This implementation refers to https://github.com/Kotlin/kotlinx.coroutines/pull/760
*/
fun ScreenLifecycle.createJob(
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」が最長ライフサイクルである前提に立ってると思うんですが、実際には 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 •
edited
Loading

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 •
edited
Loading

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

@takahirom takahirom mentioned this pull request Feb 11, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@takahirom takahirom takahirom left review comments

@jmatsu jmatsu jmatsu left review comments

+1 more reviewer

@jmatsu-bot jmatsu-bot jmatsu-bot left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

awesome

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@magiepooh @jmatsu-bot @takahirom @jmatsu