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

Update actions #230

Merged
merged 5 commits into from Apr 1, 2024
Merged

Update actions #230

merged 5 commits into from Apr 1, 2024

Conversation

uhooi
Copy link
Owner

@uhooi uhooi commented Mar 28, 2024

  • データソースやリポジトリのメソッドを非同期にした
    • 非同期で実行しやすくするため
  • データのリフレッシュを ViewModel の init から、Screen の task で実行するように変更した
    • Screen の deinit 時にリフレッシュをキャンセルできるようにするため
    • リフレッシュが重い場合に画面が固まるのを防ぐため

@uhooi uhooi self-assigned this Mar 28, 2024
@@ -63,6 +67,9 @@ package struct SakatsuListScreen: View {
error: viewModel.uiState.sakatsuListError,
onDismiss: { viewModel.send(.screen(.onErrorAlertDismiss)) }
)
.task {
await viewModel.sendAsync(.screen(.task))
Copy link
Owner Author

Choose a reason for hiding this comment

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

send(_:) メソッドは同期処理用なので、非同期用に sendAsync(_:) メソッドと ○○AsyncAction を用意しました。

Copy link
Owner Author

Choose a reason for hiding this comment

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

今回は task のみですが、 ○○AsyncAction は task 以外にも refreshable などが思いつきます。

@@ -68,8 +73,6 @@ final class SakatsuListViewModel: ObservableObject {
self.uiState = SakatsuListUiState()
self.onSettingsButtonClick = onSettingsButtonClick
self.repository = repository

refreshSakatsus()
Copy link
Owner Author

Choose a reason for hiding this comment

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

リフレッシュ処理を ViewModel の init に書くとキャンセルできないため、task へ移動しました。

@@ -133,16 +138,45 @@ final class SakatsuListViewModel: ObservableObject {
}
}
}

nonisolated
Copy link
Owner Author

Choose a reason for hiding this comment

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

ViewModel 全体に @MainActor を付けているため、メインアクター外で非同期メソッドを実行するために nonisolated を付けています。
付けないと処理が重い場合に画面が固まってしまいます。

Copy link
Owner Author

Choose a reason for hiding this comment

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

nonisolated を付けるより、ViewModel 全体から @MainActor を外し、必要なプロパティやメソッドのみに @MainActor を付けるほうがいいでしょうか?
意見募集しています!

Copy link
Owner Author

Choose a reason for hiding this comment

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

あとリフレッシュ中に画面を固まらせない場合、インジケータを表示したり、ビューを disabled にしたりしたほうがいいでしょうか?
こちらも意見募集しています!

Copy link
Owner Author

Choose a reason for hiding this comment

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

非同期メソッドの実装次第ですが、 nonisolated を付けなくてもメインアクター外で実行できたので消しました…。 c9b45fd

Copy link
Owner Author

Choose a reason for hiding this comment

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

リフレッシュ中は特に制御しなくてもよさそうです。
ただ保存時はビューを disabled にしたほうがいいと感じました。

uiState.sakatsus = sakatsus
}
} catch is CancellationError {
return
Copy link
Owner Author

Choose a reason for hiding this comment

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

キャンセルはエラー扱いにしません。

@@ -4,7 +4,7 @@ import LogCore

// MARK: UI state

struct SakatsuInputUiState {
struct SakatsuInputUiState: Sendable {
Copy link
Owner Author

Choose a reason for hiding this comment

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

SakatsuSakatsuInputErrorSendable に準拠していないのに、 SakatsuInputUiStateSendable に準拠できるのってどういう理由でしたっけ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

○○UiStateSendable に準拠させるのは、メインアクター外からゲットしたいためです。
セットは Task { @MainActor in ... } 内で行えばいいです。

Choose a reason for hiding this comment

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

SakatsuとSakatsuInputErrorすべてがSendableプロトコルに準拠しているなら、struct SakatsuInputUiStateをSendableプロトコルに準拠してもコンパイルエラーが起こらないので、SakatsuとSakatsuInputErrorもSendableプロトコルに暗黙的に準拠していると考えられます!Takeshiさんの本参考にさせてもらいました。
https://github.com/SatoTakeshiX/first-step-swift-concurrency/blob/main/try-concurrency.playground/Pages/5-1-sendable.xcplaygroundpage/Contents.swift

Choose a reason for hiding this comment

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

public出ないなら暗黙的に準拠できると書いてありました。
なんでpublicだったら暗黙的に準拠できているかまではわからなかったので、また教えてください!

Copy link
Owner Author

Choose a reason for hiding this comment

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

それだー! ありがと 🙏
暗黙的に準拠しているなら Sendable を明示的に付けるのをやめました! c9b45fd

Copy link
Owner Author

Choose a reason for hiding this comment

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

public だと暗黙的に準拠しない理由は…わからないw
ライブラリとして提供する場合にわかりづらいからとか、、?

Copy link

@hinakkograshi hinakkograshi left a comment

Choose a reason for hiding this comment

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

SakatsuとSakatsuInputErrorもSendableプロトコルに暗黙的に準拠していると考えられます。

@@ -4,7 +4,7 @@ import LogCore

// MARK: UI state

struct SakatsuInputUiState {
struct SakatsuInputUiState: Sendable {

Choose a reason for hiding this comment

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

SakatsuとSakatsuInputErrorすべてがSendableプロトコルに準拠しているなら、struct SakatsuInputUiStateをSendableプロトコルに準拠してもコンパイルエラーが起こらないので、SakatsuとSakatsuInputErrorもSendableプロトコルに暗黙的に準拠していると考えられます!Takeshiさんの本参考にさせてもらいました。
https://github.com/SatoTakeshiX/first-step-swift-concurrency/blob/main/try-concurrency.playground/Pages/5-1-sendable.xcplaygroundpage/Contents.swift

@@ -4,7 +4,7 @@ import LogCore

// MARK: UI state

struct SakatsuInputUiState {
struct SakatsuInputUiState: Sendable {

Choose a reason for hiding this comment

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

public出ないなら暗黙的に準拠できると書いてありました。
なんでpublicだったら暗黙的に準拠できているかまではわからなかったので、また教えてください!

Base automatically changed from feature/refactor_views to main April 1, 2024 00:47
@uhooi uhooi merged commit 98d8e38 into main Apr 1, 2024
1 of 2 checks passed
@uhooi uhooi deleted the feature/update_actions branch April 1, 2024 16:02
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.

None yet

2 participants