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

メッセージ送受信の処理を実装 #33

Merged
merged 14 commits into from
Aug 8, 2020

Conversation

PictoMki
Copy link

@PictoMki PictoMki commented Jul 27, 2020

実装内容

・firesoteのライブラリの導入
・メッセージの送信機能(正常系のみ)
・メッセージの受信機能(正常系のみ)
・roomページのレイアウトを調整
・talkページからのroomデータの値渡しの修正

#24

注意点

メッセージ送信時のリアルタイムの更新はStreamBuilderを使って実装していますが、
builder直下で再度controllerのgetMessageを読んでるあたりがなんかしっくりこなかったので、
もしいい方法あったら教えてください!

あとMessageServiceとFirebaseMessageServiceに分けてるんですが、
Firebase使う前提なので、正直分けなくてもいいかなとも思ったのでその辺も意見あればください!

@PictoMki
Copy link
Author

メッセージの処理がかなり多そうだったので一旦issue切りました!
今回の修正は基盤部分の作成になります。

@PictoMki PictoMki requested review from ho2ri2s and thoth000 July 28, 2020 13:32
import 'package:chat_flutter/model/message.dart';
import 'package:chat_flutter/services/firebase_message_service.dart';

class MessageService implements MessageInterface {
Copy link

Choose a reason for hiding this comment

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

Interface切ってFirebaseに依存しないようにするの良いですね!!
設計よさげなので、(このアプリはFirebaseしか使わない想定ですが)このままいきましょう💪

@@ -74,4 +76,11 @@ class MessageListItem extends StatelessWidget {
),
);
}

String dateTimeToString(DateTime date) {
Copy link

Choose a reason for hiding this comment

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

おまかせしますが、このメソッドは汎用的なので、utilとして切り出しても良いかもしれませんね!

Future<void> sendMessage(String message) {
/// TODO Firebaseへの処理に置き換える
Future<void> sendMessage(String message, String roomId) async {
// ローカルに保存してそこから取得する?
Copy link

Choose a reason for hiding this comment

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

FirebaseAuthが実装できたら、FirebaseAuthから取得できそうですね〜

import 'package:flutter/material.dart';

class RoomController extends ChangeNotifier {
RoomController() {
getMessageList();
}
List<Message> messageList;
final MessageService _messageService = MessageService();
Copy link

Choose a reason for hiding this comment

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

MessageServiceはSingleton風に、 main.dart とかでProvideして、Controllerに引数で渡してあげると良さそうです🤔

Copy link
Author

Choose a reason for hiding this comment

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

MessageServiceをSingletonにまではわかるのですが、それ以降があまりイメージわかないのですが
どういう感じでしょうか?

Copy link

Choose a reason for hiding this comment

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

class RoomPage extends StatelessWidget {
  const RoomPage._({Key key}) : super(key: key);

  static Widget wrapped() {
    return ChangeNotifierProvider<RoomController>(
      create: (_) => RoomController(Provider.of<MessageService>(context, listen: false)),
      child: const RoomPage._(),
    );
  }
}

みたいに、Controllerをインスタンス化するときにMessageServiceをProvideしてあげるといい感じです
これでSingletonに保てるのと、引数として渡すことでControllerはcontextっていうUI情報を知らなくて良くなるので設計的にも良いと思います!

Copy link

Choose a reason for hiding this comment

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

MessageService自体はもっと親のほうで Provide.valueとかでProvideしてあげると、他の画面でもMessageServiceを使おうとしたときに同じインスタンスを使えますね(ご存知かもですが🙏)

Copy link
Author

Choose a reason for hiding this comment

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

なるほどですね!ありがとうございます!

Streamの件と絡むのですが、MessageServiceが正直いらないかなって思ったんですがどうでしょうか?
Streamの実装にあたって、下記の記事を参考にしたのですが
結局Firebaseに依存したものになる気がするので、逆にラップするとややこしい気がしてきました・・。
https://qiita.com/sarukun99/items/4057ae3c0801a3bfcfd7

どうでしょうか?

Copy link

Choose a reason for hiding this comment

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

あってもなくても、どっちでも良いと思いますが、あると変更に強い設計かなと思います!
やるとしたら、

  Stream<List<Entity>> streamList() => firebaseMessageService.streamList()

とするとStreamを横流しするだけなのでやりやすそうです!

Comment on lines 55 to 61
StreamBuilder<QuerySnapshot>(
stream: Firestore.instance
.collection('message/v1/rooms/${room.id}/transcripts')
.snapshots(),
builder: (context, snapshot) {
roomController.getMessageList();
return MessageList();
Copy link

Choose a reason for hiding this comment

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

ここはUI層でStreamを受け取るよりも、FirebaseMessageServiceでStreamを返すようにして、
Controller側でStreamが流れてくるたびに notifyListeners() を呼ぶやり方だとどうでしょう?

参考:https://medium.com/@babinsamrat123/fetch-cloud-firestore-data-stream-in-your-flutter-app-using-streamprovider-add-data-to-cloud-fa9971a5985c

@PictoMki
Copy link
Author

@ho2ri2s
MessageServiceの部分と、Streamの部分修正しました!
認識ズレていたら申し訳ないですが、再度確認をお願いします!
(何度もやり取り発生して申し訳ないです・・。)

Copy link
Collaborator

@thoth000 thoth000 left a comment

Choose a reason for hiding this comment

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

自分はインデントのチェックをしました。
気になるところが一つあったので、そこだけレビューです。
(わざとカンマを付けなかったならすいません!)

Comment on lines 87 to 88
await roomController.sendMessage(
textController.text, 'roomId');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
await roomController.sendMessage(
textController.text, 'roomId');
await roomController.sendMessage(
textController.text,
'roomId',
);

細かいですが、カンマがあるときれいになると思います。
(実装には直接関係ないのでお好みでお願いします^^)

tomoki inoue added 2 commits August 8, 2020 16:19
# Conflicts:
#	ios/Podfile
#	ios/Podfile.lock
#	lib/main.dart
@thoth000 thoth000 merged commit 8331c95 into master Aug 8, 2020
@thoth000 thoth000 deleted the feature/24/add_firestore_to_talk branch August 8, 2020 10:31
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.

3 participants