-
Notifications
You must be signed in to change notification settings - Fork 41
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
优先选择偏好类型数据源,不需要等其他源查询完毕 #1002
base: main
Are you sure you want to change the base?
优先选择偏好类型数据源,不需要等其他源查询完毕 #1002
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
需要增加测试, 当一个源完成后就立即选择
app/shared/app-data/src/commonMain/kotlin/data/source/media/fetch/MediaFetcher.kt
Outdated
Show resolved
Hide resolved
app/shared/app-data/src/commonMain/kotlin/data/source/media/fetch/MediaFetcher.kt
Outdated
Show resolved
Hide resolved
app/shared/app-data/src/commonMain/kotlin/data/source/media/fetch/MediaFetcher.kt
Outdated
Show resolved
Hide resolved
app/shared/app-data/src/commonMain/kotlin/data/source/media/selector/MediaSelector.kt
Outdated
Show resolved
Hide resolved
不清楚怎么测试,感觉比较难观察 hasCompleted 的变化 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, 但是需要有 test 确保以后不会 regress
需要测试:
- prefer BT 时, WEB 完成了, 应当继续等待
- prefer BT 时, BT 完成了, 应当自动选择
- prefer BT 时, 没有启用 BT 源, 会怎么样?
- prefer BT 时, 但 BT 没有搜到东西, 是什么行为?
- 没有 preferefence, BT 完成了但 web 没有完成, 应当继续等待
- 没有 preferefence, 都完成了, 自动选择 (应该是现有 case?)
test 中协程是单线程的, 你可以控制的 在数据源实现中, 用 |
@JvmInline | ||
value class CompletedConditions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JvmInline | |
value class CompletedConditions( | |
class CompletedConditions( |
这个东西看起来会用在被 box 的地方, 那其实不需要 value
states.all { it is MediaSourceFetchState.Disabled } -> null | ||
states.all { it is MediaSourceFetchState.Completed || it is MediaSourceFetchState.Disabled } -> true | ||
else -> false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么要有 null
? 不能用 false
吗
} | ||
} | ||
|
||
combine(map) { pairs -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
目前两个 combine 的算法会导致启动数倍多的协程
用一个 combine 就足够解决问题了, 让 combine 的 lambda 返回 ImmutableEnumMap<MSK, Boolean>
override val hasCompleted = if (mediaSourceResults.isEmpty()) { | ||
flowOf(true) | ||
flowOf(CompletedConditions.AllCompleted) | ||
} else { | ||
combine(mediaSourceResults.map { it.state }) { states -> | ||
states.all { it is MediaSourceFetchState.Completed || it is MediaSourceFetchState.Disabled } | ||
val map = MediaSourceKind.entries.map { kind -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
目前这样改会为那些原本总是期待所有数据源加载完成的代码增加计算量 (计算是否完成并构造 EnumMap 等), 导致性能变差
这是可以避免的, 我觉得可能要做一个另外的属性来支持中间状态
null | ||
} | ||
|
||
fun copy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
未使用, 没必要有
val mediaList = mutableListOf<DefaultMedia>() | ||
if (webEnabled) mediaList.addAll(TestMediaList.map { it.copy(kind = MediaSourceKind.WEB) }) | ||
if (btEnabled) mediaList.addAll(TestMediaList.map { it.copy(kind = MediaSourceKind.BitTorrent) }) | ||
this.mediaList.value = mediaList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这看起来可能会意外地影响 test 的结果
我感觉应当至少保持一个其他类型的
@Test | ||
fun `awaitCompletedAndSelectDefault selects one when prefer bt and bt done`() = runTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我觉得要新加一个 section 表示这些是来测试 "
优先选择偏好类型数据源,不需要等其他源查询完毕" 的, 否则未来的人可能不会知道这些 case 的用途
), | ||
), | ||
), | ||
flowContext = EmptyCoroutineContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memory leak
val mediaFetcher = MediaSourceMediaFetcher( | ||
configProvider = { MediaFetcherConfig.Default }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
当用处超过两个就最好弄一个 createMediaSourceFetcher
. 否则未来给 MediaSourceMediaFetcher
增加参数时要修改很多地方. 如果有 createMediaSourceFetcher 就只需要改一个地方
btEnabled: Boolean = true, | ||
webEnabled: Boolean = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btEnabled: Boolean = true, | |
webEnabled: Boolean = true, | |
addBtSources: Boolean = true, | |
addWebSources: Boolean = true, |
说明它的直接意图
resolve #816