Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?

https://developer.apple.com/tutorials/swiftui/handling-user-input

このチュートリアルの最後の場面で、LandmarkDetail画面で、お気に入りのスターを表示する処理と、タップによってスターをオン/オフする処理が実装される。

この画面の設計が気持ち悪い。

まず、LandmarkDetailはinitでlandmark: Landmarkを受けてvarに保存している。 それを、self.landmark.nameなどの表示に使っている。

一方、LandmarkDetailは@EnvironmentObjectuserData: UserDataも暗黙に受け取っている。 そして、self.landmark.idを使ってself.userData.landmarksからインデックスを引いてきて、self.userData.landmarks[index]として、Landmark型を取り出す処理がある。 こうやって取り出した方のLandmarkは、isFavoriteの方の表示に使っている。

このように、引数で受けたLandmarkを静的な値の表示、UserDataの中のLandmarkを動的な値の表示にと使い分けている。この作りは、値が静的か動的かどうかという必要のない関心をコードに生じさせてしまっていると思う。

また、LandmarkDetailは一つのランドマークを表示するための画面なのに、UserData型固有のデータ構造に依存したコードになってしまっていて、これも必要のない関心が埋め込まれていると思う。

この画面は本来、@Binding var landmark: Landmarkなどとやって、表示するLandmark一つだけに依存させるべきで、画面表示側で、必要であればこれをUserData.landmarksと結合させる、といった構造になるべきだと思う。

ただ現状、SwiftUIにはlandmarks: [Landmark]からある一要素に対するBinding<Landmark>を作る標準の道具が無いような気がする?

@omochi

This comment has been minimized.

Copy link
Owner Author

commented Jul 17, 2019

頂いたコメントは下記のツイートにぶら下がっています
https://twitter.com/omochimetaru/status/1151326365624389632

@takasek

This comment has been minimized.

Copy link

commented Jul 18, 2019

引数で受けた Landmark を静的な値の表示、 UserData の中の Landmark を動的な値の表示にと使い分けている。この作りは、値が静的か動的かどうかという必要のない関心をコードに生じさせてしまっていると思う。

これは Landmark を引数で受けるのをやめて UserData に寄せればいいだけに見える

LandmarkDetail は一つのランドマークを表示するための画面なのに、 UserData 型固有のデータ構造に依存したコードになってしまっていて、これも必要のない関心が埋め込まれていると思う。

こっちはわかるー
UserDataがListに必要な情報とDetailに必要な情報をすべて持っているという前提が引っかかりますね
遷移する画面が増えるたびにUserDataが膨らみ、あらゆるデータを知る神になる
そこを切り離せばマシになりそう

たとえば以下の構造なら違和感がない?

  • UserData -> LandmarkListEnvironment と改める
  • Detailにinjectするenvironmentは、 LandmarkListEnvironment から生成した LandmarkDetailEnvironment
    • ここで Binding<Landmark> にできればもっといいんだけど…という話は多分おもちさんのほうでも検討済みなんでしょうね
    • ただ現状、SwiftUIにはlandmarks: [Landmark]からある一要素に対するBindingを作る標準の道具が無いような気がする?

    • という点が言及済みなので

ひとまずこんな感じで一応動きはした

final class LandmarkListEnvironment: BindableObject {
    let willChange = PassthroughSubject<Void, Never>()
    
    var showFavoritesOnly = false {
        willSet {
            willChange.send()
        }
    }

    var landmarks = landmarkData {
        willSet {
            willChange.send()
        }
    }

    func detailEnvironment(forId id: Int) -> LandmarkDetailEnvironment {
        func index(forId id: Int) -> Int! {
            landmarks.firstIndex(where: { $0.id == id })!
        }
        let env = LandmarkDetailEnvironment(
            landmark: landmarks[index(forId: id)]
        )
        _ = env.willChange
            .delay(for: 0.01, scheduler: DispatchQueue.main) // willChange時点では変化してないので苦し紛れ
            .sink { [weak self] in
                self?.landmarks[index(forId: id)] = $0.landmark
        }
        return env
    }
}

final class LandmarkDetailEnvironment: BindableObject {
    let willChange = PassthroughSubject<LandmarkDetailEnvironment, Never>()

    init(landmark: Landmark) {
        self.landmark = landmark
    }

    var landmark: Landmark {
        willSet {
            willChange.send(self)
        }
    }
}

struct LandmarkDetail: View {
    @EnvironmentObject var userData: LandmarkDetailEnvironment

    var body: some View {
        VStack {
            MapView(coordinate: userData.landmark.locationCoordinate)
                .edgesIgnoringSafeArea(.top)
                .frame(height: 300)
            
            CircleImage(image: userData.landmark.image(forSize: 250))
                .offset(x: 0, y: -130)
                .padding(.bottom, -130)
            
            VStack(alignment: .leading) {
                HStack {
                    Text(verbatim: userData.landmark.name)
                        .font(.title)
                    
                    Button(action: {
                        self.userData.landmark.isFavorite.toggle()
                    }) {
                        ...
@takasek

This comment has been minimized.

Copy link

commented Jul 18, 2019

あーー
でもこの対応策だと、ひとつ先の画面での変更を受けてデータを更新してるだけで、
原初MVCのような「モデルの更新がn個のviewに等しく反映される」世界は作れてないですね

たとえば、全然関係ない画面がTabBarとかで表示されてるとして、そこにlandmarkが出ていたら、それが更新されたときにListもDetailも更新したい…というようなとき難しい

willChangeを引き渡して共有する手もかんがえたんですが、listの要素はコピーなのでそれを引き戻す手段が絶対必要になるんですよね

難しい

@omochi

This comment has been minimized.

Copy link
Owner Author

commented Jul 19, 2019

コメントありがとう
そうですね、画面のコードが別画面の都合に依存してるのが嫌なところです

この方向性だとLandmarkDetailEnvironmentに実体を持たせずに、
リードは大本からの検索、ライトは大本への書き込みにする必要があって、
まあそれってやっぱりBindingがほしいって話だと思います。

@omochi

This comment has been minimized.

Copy link
Owner Author

commented Jul 19, 2019

あ、Bindingは外部からの変更を通知する機能は無いのか・・・?

@takasek

This comment has been minimized.

Copy link

commented Jul 19, 2019

「大本」があることをViewが知るべきじゃないと思う
先述したTabの例であれば、「大本」とは別個に、detailが通信で取得される可能性もあるでしょう
ほかにも「Listには含まれているけど、それとは関係なくディープリンクによって遷移したDetail」があったとしたら、その操作もListには反映させるべき

なので、「Viewが参照しているデータが単一の参照であり、それが変わればすべて連携する」という期待は持たず、
「グローバルな変更通知に対して、Identifiableなものがあればそれを適用」できる仕組みが欲しい気がする

SwiftUIそんなにしっかりキャッチアップできてないので見当外れなこと言ってるかもしれない

@omochi

This comment has been minimized.

Copy link
Owner Author

commented Jul 19, 2019

Binding型で受け取ると大本があることが暗喩されちゃうということ?
インターフェースとしては結局普通の型と同じでリード/ライトできるだけだけれど。
あと、通信で取得した場合でも、普通に値を保持しつつBindingに包む事自体はできるはず。
でも確かに微妙な気もする。

確かに明示的に値で持っておいて、必要に応じて変更通知を送信したり、それを受信して読み込んだりするほうが、
インターフェース的に自然な気はする。

ただIdentifiableをグローバルで考えちゃうと、環境を切ったり、小さい範囲で使いたい場合に面倒になるから、
殆どの場合にグローバルと同一であったとしても、なんらかのコンテキストオブジェクトは指定するようにしたほうが良いと思う。
例えばサブミット前の編集作業中の状態を表すオブジェクトと、それをPOSTして確定するまでのサーバ状態を知識として持っておくオブジェクトは、同じIDを持つけど状態としては独立させておきたい。

SwiftUI的にはそのようなコンテキストに当たるのがEnvironmentObjectだと思う。

そうすると現状のUserDataはまあまあ惜しくて、
landmarksでアクセスするんではなくて、
idを渡すgetter/setterと、idを渡してPublisherを返すAPIを追加してやるのがいいのかな。

そのへんはボイラープレート化しそうだから標準的な道具がやっぱり欲しいですね。

@takasek

This comment has been minimized.

Copy link

commented Jul 19, 2019

「大本」があることをViewが知るべきじゃないと思う

Binding型で受け取ると大本があることが暗喩されちゃうということ?

Binding型で受け取ることに対しては特に意見はなくて…というかそこまでBindingを理解できてなくて、

この方向性だとLandmarkDetailEnvironmentに実体を持たせずに、
リードは大本からの検索、ライトは大本への書き込みにする

という点が気になりました

https://twitter.com/omochimetaru/status/1151341934138408960

ForEach.initのcontentクロージャが、Data.Element.IdentifiedValueじゃなくてBinding<Data.Element.IdentifiedValue>をくれたらいい

の方針も、それだとViewが自らの認識の外の通知を受け取れる余地がないので…
となると結論は

なんらかのコンテキストオブジェクトは指定するようにしたほうが良いと思う。
SwiftUI的にはそのようなコンテキストに当たるのがEnvironmentObjectだと思う。

ってことになりますね

@omochi

This comment has been minimized.

Copy link
Owner Author

commented Jul 19, 2019

それだとViewが自らの認識の外の通知を受け取れる余地がないので…

確かにそうだけど、「大本(A)」以外からの変更通知を(Bで)受け取るシチュエーションはおかしいのでは?
他の場所(C)で変更が発生するとしても、それを(Bで)受け取る必要があるとしたら、同じ実体を意味しているはずなので、
そのほかの場所(C)から大本(A)に反映するような形にした上で、(Bでは)大本(A)経由で受け取るようにすると思います。

ただその大本の管理主体がコンテキストオブジェクトであるEnvironmentObjectとなるとは思います。

@takasek

This comment has been minimized.

Copy link

commented Jul 19, 2019

あー
大本=landmarks だと思って話してました

大本=A=EnvironmentObject
とすれば、認識一致してそう

landmarks: [Landmark]からある一要素に対するBindingを作る標準の道具

ができるときには、
引数としてEnvironmentを取りつつラクに書けると嬉しい世界なのかな

@omochi

This comment has been minimized.

Copy link
Owner Author

commented Jul 19, 2019

引数としてEnvironmentを取りつつラクに書けると嬉しい

そう思います。

@sidepelican

This comment has been minimized.

Copy link

commented Jul 30, 2019

(Twitter@iceman5499です。)

Xcode11 Beta5 くらいからいつの間にか Binding が自分で生成できるようになった( init(get: @escaping () -> Value, set: @escaping (Value) -> Void) が生えた)ので

landmarks: [Landmark]からある一要素に対するBindingを作る標準の道具

これができるようになったように見えます。
Bindingとしての取り出しはやや煩雑ですが、ある程度理想的な形でかけるようになりました

https://gist.github.com/sidepelican/330ca3255cffcf13cd5352d5935040c7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.