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

feat: add send for room by adding talkable trait #24

Merged
merged 4 commits into from
Jun 18, 2021

Conversation

7sDream
Copy link
Contributor

@7sDream 7sDream commented Jun 17, 2021

通过增加一个 Talkable trait 给 Room 增加了 send_xxx 系列方法,复用的 Contract 里的实现。

因为不知道 wechaty 的具体架构和协议,不太确定有没有问题,测试了 room.send_text 是没问题的。

另外不知道是客户端这边实现确实不对,还是 wechaty-puppet-wechat 实现有问题,用 wechaty docker 镜像 0.63 版本作为 puppet,扫码登录时会发一个 Heartbeat 类型的消息过来,但是 data 字段是个 Object(而不是代码里定义的 String),会导致 panic,这里顺便修复了下。

还有点小修改,比如给 QueryFilter 类型都加上了 Default trait 方便构造 filter。


以下是 HeartbeatdataObject 的 Log:

fix payload parse painc
fix filter structs not impl default trait
@CLAassistant
Copy link

CLAassistant commented Jun 17, 2021

CLA assistant check
All committers have signed the CLA.

@lucifer1004
Copy link
Collaborator

@7sDream Thanks a lot for your contribution!

@lucifer1004
Copy link
Collaborator

The data type of the heartbeat message may have been changed since there have been several months since I last worked on this project.

@lucifer1004
Copy link
Collaborator

Have you checked the message sending/receiving functions? Did they work?

@7sDream
Copy link
Contributor Author

7sDream commented Jun 17, 2021

Have you checked the message sending/receiving functions? Did they work?

Just tested the room.send_text, it works. Other type like send_contract/url/... has not been tested. But I can test it some time in today night.

Since my own bot didn't need receive message , I haven't tested it either. But I don’t think I changed any code for receiving messages.

@huan
Copy link
Member

huan commented Jun 17, 2021

Link to wechaty/PMC#16

@lucifer1004
Copy link
Collaborator

@huan What is the exact type of the data field in RPC messages right now?

@7sDream
Copy link
Contributor Author

7sDream commented Jun 17, 2021

Just added a example log screenshot for Heartbeat event data filed type is object.

@huan
Copy link
Member

huan commented Jun 17, 2021

@huan What is the exact type of the data field in RPC messages right now?

If you are referring to the payload, I believe it should be a string.

Please see the proto buffer definition at https://github.com/wechaty/grpc/blob/9c4d6c91540db9fafc74b9bc1b5fc616beaa4526/proto/wechaty/puppet/event.proto#L36

@lucifer1004
Copy link
Collaborator

@huan Please have a look at the screenshot posted by @7sDream, in which a Heartbeat message's payload had a non-string data field.

@huan
Copy link
Member

huan commented Jun 17, 2021

@huan Please have a look at the screenshot posted by @7sDream, in which a Heartbeat message's payload had a non-string data field.

According to the gRPC which is based on proto buffer protocol, the payload can only be string type.

It might be a stringified objects json string.

However, I do not know if there's any additional operations in the rust code. Please check it and let me know if there's anything I can help.

@7sDream
Copy link
Contributor Author

7sDream commented Jun 17, 2021

payload was defined in proto file, which should(and do) always be a string, a JSON string.

When parse(deserialize) payload into a struct, it has a data field.

In many condition, data field is a String, but in Heartbeat event triggered by scanning the login qrcode(but has not clicked confirm login button on the phone), data is a Object, just like screenshot shows.

The spec of this data field cannot be seen from the proto definition.

Copy link
Collaborator

@lucifer1004 lucifer1004 left a comment

Choose a reason for hiding this comment

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

LGTM

@huan
Copy link
Member

huan commented Jun 17, 2021

payload was defined in proto file, which should(and do) always be a string, a JSON string.

Yes, that's correct.

The spec of this data field cannot be seen from the proto definition.

I don't think we have a spec for this data field. Please correct me if I'm not right.

I believe the data could be any data which highly depends on the specific logic.

P.S. I agree that the heartbeat event should send simple data, I will inspect the related server code later.

@lucifer1004 lucifer1004 merged commit 2cf5545 into wechaty:main Jun 18, 2021
@huan
Copy link
Member

huan commented Jun 18, 2021

Thank you very much for your contribution!

You are welcome to join Wechaty Contributor Program

1. Join Wechaty Organization

You've invited 7sDream to Wechaty! They'll be receiving an email shortly. They can also visit https://github.com/wechaty to accept the invitation.

I have invited you to join our Wechaty GitHub Organization, please accept it by following the above message.

2. Update Your Wechaty Contributor Profile

  1. Please open Contributor Hall of Fame and add yourself to the end of the list, so that other contributors will know you better!
  2. Please add yourself to our Website Contributors by creating a PR and refer to this PR link as well.

3. Join The Contributor Only WeChat Room

We also have a WeChat room for contributors only which can discuss Wechaty at a deeper level, you are welcome to join and if you are interested.

Please add @lijiarui wechat: ruirui_0914 and send her this pr link. She will invite you into Wechaty Contributor Room

Cheers!

@7sDream
Copy link
Contributor Author

7sDream commented Jun 18, 2021

@huan

Thanks a lot for such a detailed guide text.

PR for create contributor profile: https://github.com/wechaty/wechaty.js.org/pull/961

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.

4 participants