[iOS] favorite(bookmark) scene

fummicc1 commented 4 years ago

Issue

close #497

Overview (Required)

add Bookmark feature.

Links

Figma

Screenshot

Before	After

Demo（old）

droid-kaigi-demo

fummicc1 commented 4 years ago

Now, I wonder if the approach that I chooses RealmSwift as LocalStorageClient is OK.

fummicc1 commented 4 years ago

@takahirom Nice to meet you. I am implementing a bookmark feature for the iOS app. And, I want to confirm that the approach that I choose RealmSwift as LocalStorageClient is good. if any other solutions are better, I want to know it and will implement it as so.

takahirom commented 4 years ago

Sorry we are talking about this 🙇

fummicc1 commented 4 years ago

I see. Thank you! I want to try this feature so I am waiting for your response. 😄

fummicc1 commented 4 years ago

IMO👀

I feel that Session::isFavorited is get-only, so I want to call the end-point which switch bookmark-state and manage each user's bookmarks on the cloud. But, I know that iOS-Client is not authenticated so how to distinguish each user in the back-end. I felt it is not realistic to use FirebaseAuth anonymously signin to distinguish each user because I do not know which Firebase plan is selected. So, I was trying to choose RealmSwift which can persist bookmarked sessions in local storage.

takahirom commented 4 years ago

@fummicc1 CC: @roana0229 Sorry for my late response. 🙇 I want to use the same model for Android and iOS as DroidKaigi. Currently, the methods in Kotlin's Session class cannot be called from iOS. Therefore, I think it is good to use Realm's Object when saving and reading in DB, but I would like to use SpeechSession and SserviceSession etc. in the application if possible. So, I want to put code to convert Realm Object into SpeechSession or SpecialSession in Swift. Is it possible? 🙏 Please let me know if you have any opinions.

fummicc1 commented 4 years ago

@takahirom cc @roana0229 Thank you for replying! Now, I would like to confirm a little bit.

Session is like a base model and I (as an ios-client) can use SpeechSession or ServiceSession instead of Session.
a SessionModel except for SpeechSession and ServiceSession may be added in the future?（I wonder what SpecialSession is.）
ServiceSession is like [Welcome Talk, Lunch] and SpeechSession is a session that has a speaker, right?

I want to confirm the above. Please⤴️

roana0229 commented 4 years ago

Session is like a base model and I (as an ios-client) can use SpeechSession or ServiceSession instead of Session.

Exactly. SpeechSession and ServiceSession are only SessionEntity in db, and it use converted to Session(SpeechSession or ServiceSession).

In reference.

a SessionModel except for SpeechSession and ServiceSession may be added in the future?（I wonder what SpecialSession is.）

Sorry. SpecialSession is mistake. 🙇 Now only SpeechSession and ServiceSession.

ServiceSession is like [Welcome Talk, Lunch] and SpeechSession is a session that has a speaker, right?

Almost good. SpeechSession has more than one speakers.

Please feel free to ask us if you have any questions. 😄

fummicc1 commented 4 years ago

@roana0229 Thank you!!

fummicc1 commented 4 years ago

@takahirom @roana0229 @ry-itto Hi, sorry for replying late at night... but the bookmark feature is coming to be done. If possible, I would like you to review my code.🙇‍♂️

roana0229 commented 4 years ago

Thank you for your quick work 👍 I will review tonight.

fummicc1 commented 4 years ago

AppBaseSession which is used in the iOS client is made from Session (SpeechSession or ServiceSession) and there is no converting AppBaseSession into Session... It is against that I should use the same model as the Android client, right?

Using a different model from API will need more changes in the future, I think.👀

roana0229 commented 4 years ago

I want to use the same SpeechSession and ServiceSession and more for Android and iOS. 🙏

I guess better that It's save as class Session: Realm.Object { ... }. And It convert to use SpeechSession and ServiceSession. I assumed that logic only have Kotlin. Also, I think that using the same DB configuration on Android / iOS has the advantage of being easy to understand.

This purpose is to share the method of the Android model. It's no problem, if SpeechSession andServiceSession are separated in DB. How about ?

（伝えきれないかも知れないので日本語でも書きます） Androidで使っているモデルが持つメソッドを共有したいため、iOSでも同じモデル利用したいです。私のイメージではiOSでもclass Session: Realm.Object { ... }として保存し、それを利用する際にKotlinのSpeechSession, ServiceSessionへ変換することで、ロジックをKotlin側にまとめられると想定していました。また、Android/iOSで同じDB構成にした方がわかりやすいというメリットはあると思います。

ただ、目的はAndroidのモデルが持つメソッドを共有することなので、DBへの保存する形はSpeechSession, ServiceSessionが別れていても問題ないと思います。どうでしょうか？

fummicc1 commented 4 years ago

@roana0229

Thank you for replying!

私のイメージではiOSでもclass Session: Realm.Object { ... }として保存し、それを利用する際にKotlinのSpeechSession, ServiceSessionへ変換することで、ロジックをKotlin側にまとめられると想定していました。

I see. I think so too, but somehow Session inherits KotlinBase class and it is hard to inherit Realm.Object. So, I decide on the approach.

(わかりました。ですが、SessionクラスはKotlinBaseを継承しており、新たにRealm.Objectを継承することが難しく、今はApp〇〇というRealm.Objectを継承したクラスを作成し、そのクラスをRealmに保存しています。修正範囲が増え、モデルクラスのコードも二倍に増えるのでいい実装ではないと思いますが、ローカルに保存するための方法が思いつかなく一度このように実装しました。)

roana0229 commented 4 years ago

すいません、自分の伝え方が悪かったかもしれないです。申し訳ないですが、日本語だけで書きます 🙇

ですが、SessionクラスはKotlinBaseを継承しており、新たにRealm.Objectを継承することが難しく

これはその通りだと思います！私がコメントした

私のイメージではiOSでもclass Session: Realm.Object { ... }として保存し、それを利用する際にKotlinのSpeechSession, ServiceSessionへ変換することで、ロジックをKotlin側にまとめられると想定していました。

のclass Session: Realm.Object { ... }部分は、KotlinBaseを継承しているSessionのことではなく、iOSで内部に保存する用の独自型を指していました。

class SessionEntity: Realm.Object {
  // AndroidのSessionEntityと同じプロパティを保持
}

Kotlin側に定義してあるSpeechSession,ServiceSessionをswiftからnewすることはできるので、このiOSでRealmを使って内部に保存したEntityを、Realmに保存したモデルから変換できれば、ロジックが共通化できていいなと思っています。このSpeechSession,ServiceSessionはSessionインターフェースを実装してあるので、View側のコードの変更はなしで行けそうだなと思っていました。

どう思いますか、懸念点などありますか？

fummicc1 commented 4 years ago

@roana0229 cc @ry-itto @takahirom Sorry, the reply was delayed. I fix and now can use the same Model as Kotlin-API and Realm but I wonder if my implementation is good.

In addition to it, I have a few contents to ask... sorry. 🙇‍♂️

I do not understand the difference between Message:: ResourceIdMessage and Message::TextMessage. Is there the same Entity such as MessageEntity.

The reason I am asking it is that SessionEntity needs to persist Session:: Message

SessionをSessionEntityから作成するコード

```swift static func toModel(sessionEntity session: SessionEntity, firstSessionSTime firstSTime: TimeInterval) -> Session? { let sessionTypeIds: [(String, SessionType)] = [ ("normal", SessionType.normal), ("welcome_talk", SessionType.welcomeTalk), ("reserved", SessionType.reserved), ("codelabs", SessionType.codelabs), ("fireside_chat", SessionType.firesideChat), ("lunch", SessionType.lunch), ("break", SessionType.breaktime), ("after_party", SessionType.afterParty), ("unknown", SessionType.unknown), ] let calendar = Calendar(identifier: .gregorian) // FIXME: I want to use same logic as Android. let startDay = calendar.dateComponents([.year, .month, .day], from: Date(timeIntervalSince1970: TimeInterval(session.stime) / 1000)).day ?? 0 let firstDay = calendar.dateComponents([.year, .month, .day], from: Date(timeIntervalSince1970: firstSTime / 1000)).day ?? 0 let dayNumber = startDay - firstDay + 1 guard let room = session.room else { return nil } let sessionTypeId = session.sessionType let sessionType = sessionTypeIds.first(where: { $0.0 == sessionTypeId })?.1 let category = session.category // MARK: I have no idea to convert message into primitive-type let message = session.message // if session.isServiceSession { return ServiceSession( id: SessionId(id: session.id), dayNumber: Int32(dayNumber), startTime: TimeInterval(session.stime), endTime: TimeInterval(session.etime), title: LocaledString(ja: session.title, en: session.enTitle), desc: session.desc, room: Room( id: Int32(room.id), name: LocaledString(ja: room.name, en: room.enName), sort: Int32(room.sort)), sessionType: sessionType ?? SessionType.unknown, isFavorited: session.isFavorited ) } else { return SpeechSession( id: SessionId(id: session.id), dayNumber: Int32(dayNumber), startTime: TimeInterval(session.stime), endTime: TimeInterval(session.etime), title: LocaledString(ja: session.title, en: session.enTitle), desc: session.desc, room: Room( id: Int32(room.id), name: LocaledString(ja: room.name, en: room.enName), sort: Int32(room.sort)) , lang: session.language == "ja" ? .ja : .en, category: Category( id: Int32(category?.id ?? 0), name: LocaledString( ja: category?.name ?? "", en: category?.enName ?? "") ), intendedAudience: session.intendedAudience, videoUrl: session.videoUrl, slideUrl: session.slideUrl, isInterpretationTarget: session.isInterpretationTarget, isFavorited: session.isFavorited, speakers: session.speakers.map({ toModel(speakerEntity: $0) }), message: nil ) } } ```

I have no idea to sort sessions that have same startTime.

Please Watch This Demo. demo

すいません、返事が遅れました。修正し、UI層ではデータベースに依存せずにSessionを使うことで統一できています。

ですが、いくつか問題点があり、共有させてください。

ios-combined.Session.Messageに TextMessageとResourceIdMessageがあり、仕様が理解できていなく、Realm側のエンティティ SessionEntityにメッセージを保存できていません。
同じ開始時刻のセッションをお気に入りにしたとき、同じ開始時刻内でのセッションの並びがソートできていません。

異なるデータベースから取得してきたデータを結合しているため、現在は手動でソートしています。そこでstartTimeが同じセッションについてのソートの方法が思いつかなく、UIが急にカタつくような体験になってしまっています。（デモ動画を参照してみてください。🙇‍♂️） demo

もしかしたら、付け足すかもしれないですが、今のところ上記2点を共有させてください。mm

takahirom commented 4 years ago

ios-combined.Session.Messageに TextMessageとResourceIdMessageがあり、仕様が理解できていなく、Realm側のエンティティ SessionEntityにメッセージを保存できていません。

val message: LocaledString? のようなので、気にしなくて良さそうです。そのクラスはAndroidでも使われていなそうです。

同じ開始時刻のセッションをお気に入りにしたとき、同じ開始時刻内でのセッションの並びがソートできていません。

一応部屋名を使えばなんとかなるはず。。です。部屋名がアルファベット順なので 👀

takahirom commented 4 years ago

LocaledStringは2つフィールドがあるだけのクラスです！

data class LocaledString(
    val ja: String,
    val en: String
)

fummicc1 commented 4 years ago

@takahirom

一応部屋名を使えばなんとかなるはず。。です。部屋名がアルファベット順なので 👀

この方法で対応しました。ありがとうございます！

val message: LocaledString? のようなので、気にしなくて良さそうです。そのクラスはAndroidでも使われていなそうです。

分かりました。👍

fummicc1 commented 4 years ago

@ry-itto @takahirom @roana0229

お疲れ様です！長引いてしまいましたが、お気に入り機能ができたと思います。コンフリクトを修正後のデモ動画・スクリーンショットに差し替えたので後ほどレビューをいただきたいです〜。mm

roana0229 commented 4 years ago

https://github.com/DroidKaigi/conference-app-2020/pull/668#discussion_r375393410

@fummicc1 Sounds reasonable. Could you please use first of local session stime ? And, I think that it don't have to wait for request.

see kotlin logic

I think it's okay to create another Issue or PR, what do you do?

fummicc1 commented 4 years ago

If possible, I want to do this because I also think it should be fixed.

roana0229 commented 4 years ago

Of course 👍 I look forward to your commit to this PR 😄

fummicc1 commented 4 years ago

@roana0229

Of course 👍 I look forward to your commit on this PR 😄

Thank You!

Sorry, I want to confirm some.

In my currect implementation, it cannnot be ensured that [SessionEntity] stored in Realm has firstDay（may be 2/20）.

So, to write such as sessionEntities.first().session.stime , all Sessions should be stored in local, I think.

fummicc1 commented 4 years ago

@roana0229 すいません、英語でどう書けばいいか分からなかったので日本語で書きます。mm

問題が、ローカルに保存したセッションの取得がリモートのセッションの取得の成功の可否に依存している点が問題という認識です。（せっかくRealmにエンティティごと保存しているのでオフラインでもお気に入りは取得できるようにしたいというモチベーションです。）

APIからセッションの取得が成功したときに関してはremoteSessions.first.startTimeを基にそれぞれのセッションがDay1なのかDay2なのかを判断し、もしネットワークが不安定などの理由でKotlin-Apiからデータを正常に取得できなかったら、iOS側にハードコーディグした 1582160400というUNIX時間からそれぞれのセッションがDay1なのかDay2なのかを判断する仕様にしてみました。

結果的にはオフライン時でもお気に入りが見れる仕様にはなっていると思います。どうでしょうか。👀

参考部分

参考部分のコード

```swift sessionsFetchFromApiRelay .asObservable() .flatMap { [unowned self] (sessions: [Session]) -> Observable<[Session]> in if sessions.isEmpty { return self.bookingSessionProvider.fetchBookedSessions() } return .just(sessions) } .filter { !$0.isEmpty } .compactMap { $0.first?.startTime } .flatMap(bookingSessionProvider.fetchBookedSessions) .bind(to: sessionsFetchFromLocalRelay) .disposed(by: disposeBag) ```

デモ動画（最初、ローカルに保存されたデータだけが見えて、その後リモートのデータが取得したら更新されれる感じです。）

demo

jmatsu-bot commented 4 years ago

No issue was reported. Cool!

Generated by :no_entry_sign: Danger

jmatsu-bot commented 4 years ago

Your apk has been deployed to https://deploygate.com/distributions/dc9e293a3e6d8224a3e7cbb21aac0f4ef3bd2206. Anyone can try your changes via the link.

Generated by :no_entry_sign: Danger

roana0229 commented 4 years ago

👀

roana0229 commented 4 years ago

Thank you always 😄 Awesome 👍 👍 👍

fummicc1 commented 4 years ago

@roana0229 I just discovered some bugs, but it takes a lot of time to fix it with my current technology (sorry...), so I'm glad to have it merged once in this state. Thank you very much.👍👍👍

roana0229 commented 4 years ago

@fummicc1 No worries. 👍 Even if it has unresolved problem, it can create another issue or pr. It is recommended that would you create a small PR to easy merging. 😄

DroidKaigi / conference-app-2020