fix(wechaty-bro): resolved emit RECALLED type msg (fix #8) #744

binsee · 2017-08-18T22:34:24Z

fix #8

binsee · 2017-08-18T22:38:44Z

There is a small problem with this way, after the recall message, the web page displays a blank message.
But does not affect the use.

coveralls · 2017-08-18T23:07:42Z

Coverage remained the same at 54.688% when pulling 6976262 on binsee:fix-recalled into d3aa251 on Chatie:master.

coveralls · 2017-08-18T23:54:36Z

Coverage decreased (-0.09%) to 54.599% when pulling 4a7dff8 on binsee:fix-recalled into d3aa251 on Chatie:master.

huan

Your implement is clean and straight forward, Awesome job!

And the message usage is what I thought it should be: a message with RECALLED MsgType set.

I leave some questions & suggestions in the review message, please let me know what you think.

Thanks!

huan · 2017-08-19T12:58:38Z

src/puppet-web/wechaty-bro.js

@@ -76,6 +76,7 @@
    glueToAngular()
    connectSocket()
    hookEvents()
+    hookProcess()


I'd like to suggest we name this function to hookRecalledMsgProcess because that would look more straight forward than hookProcess.

I use hookProcess because I think in the future may add other hooks. But for now, the term is not clear, I will modify it.

huan · 2017-08-19T13:00:36Z

src/puppet-web/wechaty-bro.js

+
+  function hookProcess() {
+    var chatFactory = WechatyBro.glue.chatFactory
+    var rootScope = WechatyBro.glue.rootScope


rootScope is not needed at here.

i will remove it.

huan · 2017-08-19T13:02:54Z

src/puppet-web/wechaty-bro.js

+    // hook chatFactory._recalledMsgProcess, resolve emit RECALLED type msg
+    chatFactory.__recalledMsgProcess = chatFactory._recalledMsgProcess
+    chatFactory._recalledMsgProcess = function (msg) {
+      var _this = this


What's the reason that you save this to _this?

IMHO, It seems not necessary to do that inside this function?

huan · 2017-08-19T13:05:57Z

src/puppet-web/wechaty-bro.js

+    chatFactory.__recalledMsgProcess = chatFactory._recalledMsgProcess
+    chatFactory._recalledMsgProcess = function (msg) {
+      var _this = this
+      _this.__recalledMsgProcess(msg)


How about use chatFactory at here instead of this?

Like: chatFactory.__recalledMsgProcess and __recalledMsgProcess.addChatMessage

I modified it, it can work:

chatFactory._recalledMsgProcess = function (msg) { chatFactory.__recalledMsgProcess(msg) chatFactory.addChatMessage(msg) }

huan · 2017-08-19T13:06:53Z

src/message.ts

+   * })
+   * ```
+   */
+  public getRevokeId(): string {


Why we need to add this new method?

I did not think it's necessary.

Please correct me if I'm wrong.

RECALLED msg, content:

<sysmsg type="revokemsg"><revokemsg><session>binsee</session><oldmsgid>1056815127</oldmsgid><msgid>6991717488370193149</msgid><replacemsg><![CDATA["杉木" 撤回了一条消息]]></replacemsg></revokemsg></sysmsg>

use UtilLib.unescapeHtml(this.obj.content)

// the content has been formatted <sysmsg type="revokemsg"> <revokemsg> <session>binsee</session> <oldmsgid>1056815127</oldmsgid> <msgid>6991717488370193149</msgid> <replacemsg> <![CDATA["杉木" 撤回了一条消息]]> </replacemsg> </revokemsg> </sysmsg>

We need to use getRevokeId () package to extract the code msgId

const ret = UtilLib.unescapeHtml(this.obj.content).match(/<msgid>(\w+)<\/msgid>/)

modify `_this` to `chatFactory`

binsee · 2017-08-19T14:43:06Z

RECALLED msg, content:

&lt;sysmsg type="revokemsg"&gt;&lt;revokemsg&gt;&lt;session&gt;binsee&lt;/session&gt;&lt;oldmsgid&gt;1056815127&lt;/oldmsgid&gt;&lt;msgid&gt;6991717488370193149&lt;/msgid&gt;&lt;replacemsg&gt;&lt;![CDATA["杉木" 撤回了一条消息]]&gt;&lt;/replacemsg&gt;&lt;/revokemsg&gt;&lt;/sysmsg&gt;

use UtilLib.unescapeHtml(this.obj.content)

// the content has been formatted
<sysmsg type="revokemsg">
    <revokemsg>
        <session>binsee</session>
        <oldmsgid>1056815127</oldmsgid>
        <msgid>6991717488370193149</msgid>
        <replacemsg>
            <![CDATA["杉木" 撤回了一条消息]]>
        </replacemsg>
    </revokemsg>
</sysmsg>

We need to use getRevokeId () package to extract the code msgId

const ret = UtilLib.unescapeHtml(this.obj.content).match(/<msgid>(\w+)<\/msgid>/)

huan · 2017-08-19T14:48:24Z

I see.

As my understanding now, the parameter of _recalledMsgProcess() is <sysmsg type="revokemsg">...</sysmsg>, right?

If so, I think the better design to do this might be(in wechaty-bro.js):

find the original message for that msgid, then
set the message with MsgType.RECALLED, then
use WechatBro.emit() to emit that message again. (see: https://github.com/Chatie/wechaty/blob/master/src/puppet-web/wechaty-bro.js#L366)

The above design could let the end-user receive a message event, with the original message, but with MsgType.RECALLED set. (and no new method need to be introduced)

How about this idea, what do you think?

binsee · 2017-08-19T14:58:15Z

msg.content='<sysmsg type="revokemsg"><revokemsg><session>binsee</session><oldmsgid>1056815127</oldmsgid><msgid>6991717488370193149</msgid><replacemsg><![CDATA["杉木" 撤回了一条消息]]></replacemsg></revokemsg></sysmsg>'

must use UtilLib.unescapeHtml(this.obj.content), return xml text, and use match to get msgId.
So, still need to use the new method.

coveralls · 2017-08-19T15:01:54Z

Coverage decreased (-0.1%) to 54.545% when pulling 3a6d5b0 on binsee:fix-recalled into d3aa251 on Chatie:master.

…essage

huan · 2017-08-19T15:13:39Z

Could you do those works in the browser, as my last suggestion?

binsee · 2017-08-19T15:14:12Z

Sorry, I read your reply repeatedly and found that I did not fully understand what you meant. I will think again and then update it

coveralls · 2017-08-19T15:29:37Z

Coverage decreased (-0.008%) to 54.68% when pulling 3d66014 on binsee:fix-recalled into d3aa251 on Chatie:master.

binsee · 2017-08-19T17:44:26Z

In the wechaty-bro to achieve the RECALLED message handling, it looks good results.
But encountered a question that needs to be discussed:

The order in which the code is called will produce a different return effect. Which do we choose?

Description: dev_tag is my added field for debugging notes

Option 1

First chatFactory._recalledMsgProcess again WechatyBro.emit

Recalled the WebWxApp received the message, rawObj data:

msg.type() return MsgType.RECALLED
msg.content() return 杉木撤回了一条消息

msg.rawObj.MMRecall == true

{"MsgId":"3066610148686027451","FromUserName":"@d4d830178e30550380be2175c454951f","ToUserName":"@ae39ddd7f22214f8f330b68aaa8ec0190702c0c33467b2a23338a77221e6f431",
"MsgType":10002,
"Content":"接收到的消息内容","Status":3,"ImgStatus":1,"CreateTime":1503162116,"VoiceLength":0,"PlayLength":0,"FileName":"","FileSize":"","MediaId":"","Url":"","AppMsgType":0,"StatusNotifyCode":0,"StatusNotifyUserName":"","RecommendInfo":{"UserName":"","NickName":"","QQNum":0,"Province":"","City":"","Content":"","Signature":"","Alias":"","Scene":0,"VerifyFlag":0,"AttrStatus":0,"Sex":0,"Ticket":"","OpCode":0},"ForwardFlag":0,"AppInfo":{"AppID":"","Type":0},"HasProductId":0,"Ticket":"","ImgHeight":0,"ImgWidth":0,"SubMsgType":0,"NewMsgId":3066610148686027300,"OriContent":"","MMPeerUserName":"@d4d830178e30550380be2175c454951f",
"MMDigest":"杉木撤回了一条消息","MMIsSend":false,"MMIsChatRoom":false,"MMUnread":true,"LocalID":"3066610148686027451",
"ClientMsgId":"3066610148686027451",
"MMActualContent":"杉木撤回了一条消息",
"MMActualSender":"@d4d830178e30550380be2175c454951f","MMDigestTime":"1:01","MMDisplayTime":1503162116,"MMTime":"1:01","_h":0,
"MMRecall":true,
"dev_tag":"这是基于原先的消息体"}

Recalled the WebWxApp did not received the message, rawObj data:

msg.type() return MsgType.RECALLED
msg.content() return 杉木撤回了一条消息

{"MsgId":"443731713080375307","FromUserName":"@d4d830178e30550380be2175c454951f","ToUserName":"@ae39ddd7f22214f8f330b68aaa8ec0190702c0c33467b2a23338a77221e6f431",
"MsgType":10002,
"Content":"杉木 撤回了一条消息","Status":4,"ImgStatus":1,"CreateTime":1503162122,"VoiceLength":0,"PlayLength":0,"FileName":"","FileSize":"","MediaId":"","Url":"","AppMsgType":0,"StatusNotifyCode":0,"StatusNotifyUserName":"","RecommendInfo":{"UserName":"","NickName":"","QQNum":0,"Province":"","City":"","Content":"","Signature":"","Alias":"","Scene":0,"VerifyFlag":0,"AttrStatus":0,"Sex":0,"Ticket":"","OpCode":0},"ForwardFlag":0,"AppInfo":{"AppID":"","Type":0},"HasProductId":0,"Ticket":"","ImgHeight":0,"ImgWidth":0,"SubMsgType":0,"NewMsgId":1357981904162237700,"OriContent":"","MMPeerUserName":"@d4d830178e30550380be2175c454951f",
"MMDigest":"","MMIsSend":false,"MMIsChatRoom":false,"MMUnread":true,"LocalID":"1357981904162237688",
"ClientMsgId":"1357981904162237688",
"MMActualContent":"杉木 撤回了一条消息",
"MMActualSender":"@d4d830178e30550380be2175c454951f","MMDigestTime":"1:02","MMDisplayTime":1503162116,"MMTime":"",
"dev_tag":"这是基于撤消操作消息体"}

Option 2

First WechatyBro.emit again chatFactory._recalledMsgProcess

Recalled the WebWxApp received the message, rawObj data:

msg.type() return MsgType.RECALLED
msg.content() return 接收到的消息内容

{"MsgId":"5026368840711665152","FromUserName":"@d4d830178e30550380be2175c454951f","ToUserName":"@ae39ddd7f22214f8f330b68aaa8ec0190702c0c33467b2a23338a77221e6f431",
"MsgType":10002,
"Content":"接收到的消息内容","Status":3,"ImgStatus":1,"CreateTime":1503162474,"VoiceLength":0,"PlayLength":0,"FileName":"","FileSize":"","MediaId":"","Url":"","AppMsgType":0,"StatusNotifyCode":0,"StatusNotifyUserName":"","RecommendInfo":{"UserName":"","NickName":"","QQNum":0,"Province":"","City":"","Content":"","Signature":"","Alias":"","Scene":0,"VerifyFlag":0,"AttrStatus":0,"Sex":0,"Ticket":"","OpCode":0},"ForwardFlag":0,"AppInfo":{"AppID":"","Type":0},"HasProductId":0,"Ticket":"","ImgHeight":0,"ImgWidth":0,"SubMsgType":0,"NewMsgId":5026368840711666000,"OriContent":"","MMPeerUserName":"@d4d830178e30550380be2175c454951f",
"MMDigest":"接收到的消息内容","MMIsSend":false,"MMIsChatRoom":false,"MMUnread":false,"LocalID":"5026368840711665152",
"ClientMsgId":"5026368840711665152",
"MMActualContent":"接收到的消息内容","MMActualSender":"@d4d830178e30550380be2175c454951f","MMDigestTime":"1:07","MMDisplayTime":1503162297,"MMTime":"","_index":33,"_h":56,"_calcing":false,"_offsetTop":1853,"$$hashKey":"0CO",
"dev_tag":"这是基于原先的消息体"}

Recalled the WebWxApp did not received the message, rawObj data:

msg.type() return MsgType.RECALLED
msg.content() return 杉木撤回了一条消息

{"MsgId":"3684395441945462470","FromUserName":"@d4d830178e30550380be2175c454951f","ToUserName":"@ae39ddd7f22214f8f330b68aaa8ec0190702c0c33467b2a23338a77221e6f431",
"MsgType":10002,
"":"杉木 撤回了一条消息","Status":4,"ImgStatus":1,"CreateTime":1503162479,"VoiceLength":0,"PlayLength":0,"FileName":"","FileSize":"","MediaId":"","Url":"","AppMsgType":0,"StatusNotifyCode":0,"StatusNotifyUserName":"","RecommendInfo":{"UserName":"","NickName":"","QQNum":0,"Province":"","City":"","Content":"","Signature":"","Alias":"","Scene":0,"VerifyFlag":0,"AttrStatus":0,"Sex":0,"Ticket":"","OpCode":0},"ForwardFlag":0,"AppInfo":{"AppID":"","Type":0},"HasProductId":0,"Ticket":"","ImgHeight":0,"ImgWidth":0,"SubMsgType":0,"NewMsgId":8699756819766069000,"OriContent":"","MMPeerUserName":"@d4d830178e30550380be2175c454951f",
"MMDigest":"","MMIsSend":false,"MMIsChatRoom":false,"MMUnread":true,"LocalID":"8699756819766068805",
"ClientMsgId":"8699756819766068805",
"MMActualContent":"杉木 撤回了一条消息","MMActualSender":"@d4d830178e30550380be2175c454951f","MMDigestTime":"1:07","MMDisplayTime":1503162479,"MMTime":"1:07",
"dev_tag":"这是基于撤消操作消息体"}

I think Recalled type of message, using type () to judge, only through the msg.id to obtain the recalled msgId, do not deal with other information, or only as a reference.
Especially when the withdrawn message is not TEXT type.

I recommend choosing Option 1

remove Message.getRevokeId function

binsee · 2017-08-19T18:16:38Z

I think the code is perfect, no need to change.
Please check the code, please tell me if there is a problem.

huan · 2017-08-20T02:34:29Z

Yes, the code is very good, that's exactly what's I suggest. 👍

And about the two options, I would like to think it by intuition: when a message is revoked, the orignal message should be fired again, with the MsgType set to RECALLED.

In detail:

When we received a normal message, let's say, an image. The on('message', cb) event will be fired, with a message of that image as the parameter of cb, and the message should be instance of MediaMessage.
After a while, the sender decides to REVOKE that image. Then the on('message', cb) event will be fired again, for the revoked message. The message should be as same as the last message, except that the MsgType is set to RECALLED.

huan

The code looks great to me.

I'll merge this PR after you get another two approvements from our contributors.

Thank you for implementing issue #8 !

lijiarui

Thanks!

binsee · 2017-09-02T04:51:41Z

@hczhcz
Can I invite you to take the time to review this pr?
Thanks!

hczhcz

The code looks good to me.

huan · 2017-09-03T08:55:29Z

Thanks all for reviewing the code!

fix(wechaty-bro): resolved emit RECALLED type msg

6976262

fix #8

add(message): add getRevokeId(), Return the recalled message id

4a7dff8

huan requested changes Aug 19, 2017

View reviewed changes

huan requested review from JasLin, xinbenlv, Gcaufy, lijiarui, mukaiu and TingYinHelen August 19, 2017 13:16

fix(wechaty-bro): modify hookProcess to hookRecalledProcess

3a6d5b0

modify `_this` to `chatFactory`

huan changed the title ~~fix(wechaty-bro): resolved emit RECALLED type msg~~ fix(wechaty-bro): resolved emit RECALLED type msg (#8) Aug 19, 2017

huan changed the title ~~fix(wechaty-bro): resolved emit RECALLED type msg (#8)~~ fix(wechaty-bro): resolved emit RECALLED type msg (fix #8) Aug 19, 2017

fix(wechaty-bro): Remain a blank message after repairing the recall m…

3d66014

…essage

fix(wechaty-bro): Unify the data returned by the RECALLED type message

5efcc72

remove Message.getRevokeId function

huan approved these changes Aug 20, 2017

View reviewed changes

lijiarui approved these changes Aug 27, 2017

View reviewed changes

lijiarui reviewed Aug 27, 2017

View reviewed changes

huan requested a review from hczhcz September 1, 2017 16:44

hczhcz approved these changes Sep 3, 2017

View reviewed changes

Merge branch 'master' into fix-recalled

31d37c4

huan merged commit 174b677 into wechaty:master Sep 3, 2017

binsee deleted the fix-recalled branch September 3, 2017 15:47

huan mentioned this pull request Sep 6, 2017

Invite @binsee to write a blog about what he like. wechaty/docusaurus#42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(wechaty-bro): resolved emit RECALLED type msg (fix #8) #744

fix(wechaty-bro): resolved emit RECALLED type msg (fix #8) #744

binsee commented Aug 18, 2017

binsee commented Aug 18, 2017

coveralls commented Aug 18, 2017 •

edited

Loading

coveralls commented Aug 18, 2017 •

edited

Loading

huan left a comment •

edited

Loading

huan Aug 19, 2017

binsee Aug 19, 2017

huan Aug 19, 2017

binsee Aug 19, 2017

huan Aug 19, 2017

huan Aug 19, 2017

binsee Aug 19, 2017

huan Aug 19, 2017

binsee Aug 19, 2017

binsee commented Aug 19, 2017

huan commented Aug 19, 2017 •

edited

Loading

binsee commented Aug 19, 2017 •

edited

Loading

coveralls commented Aug 19, 2017 •

edited

Loading

huan commented Aug 19, 2017 •

edited

Loading

binsee commented Aug 19, 2017

coveralls commented Aug 19, 2017

binsee commented Aug 19, 2017 •

edited

Loading

binsee commented Aug 19, 2017

huan commented Aug 20, 2017

huan left a comment

lijiarui left a comment

binsee commented Sep 2, 2017

hczhcz left a comment

huan commented Sep 3, 2017

fix(wechaty-bro): resolved emit RECALLED type msg (fix #8) #744

fix(wechaty-bro): resolved emit RECALLED type msg (fix #8) #744

Conversation

binsee commented Aug 18, 2017

binsee commented Aug 18, 2017

coveralls commented Aug 18, 2017 • edited Loading

coveralls commented Aug 18, 2017 • edited Loading

huan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binsee commented Aug 19, 2017

huan commented Aug 19, 2017 • edited Loading

binsee commented Aug 19, 2017 • edited Loading

coveralls commented Aug 19, 2017 • edited Loading

huan commented Aug 19, 2017 • edited Loading

binsee commented Aug 19, 2017

coveralls commented Aug 19, 2017

binsee commented Aug 19, 2017 • edited Loading

Option 1

Option 2

binsee commented Aug 19, 2017

huan commented Aug 20, 2017

huan left a comment

Choose a reason for hiding this comment

lijiarui left a comment

Choose a reason for hiding this comment

binsee commented Sep 2, 2017

hczhcz left a comment

Choose a reason for hiding this comment

huan commented Sep 3, 2017

coveralls commented Aug 18, 2017 •

edited

Loading

coveralls commented Aug 18, 2017 •

edited

Loading

huan left a comment •

edited

Loading

huan commented Aug 19, 2017 •

edited

Loading

binsee commented Aug 19, 2017 •

edited

Loading

coveralls commented Aug 19, 2017 •

edited

Loading

huan commented Aug 19, 2017 •

edited

Loading

binsee commented Aug 19, 2017 •

edited

Loading