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

Add mention #362

Merged
merged 34 commits into from Apr 26, 2017

Conversation

Projects
None yet
7 participants
@lijiarui
Member

lijiarui commented Mar 26, 2017

continue with #287

I'm trying to reset the merge file. Becasue the ding error is not casued by this commit, so I should reset the commit 3c9e1ea Merge branch 'master' of https://github.com/wechaty/wechaty into add_mention

As I have said in #287, thanks god, finally I mocked room data successfully...

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Mar 26, 2017

Member

I feel a little bit strange for the continuous error:

src » message » ready() contact ready for ToUserName

I feel strange for two reasons:

  • it tests well on my computer, but it failed in the test.
  • I didn't change any function about m.ready() but it occurs test failed...
Member

lijiarui commented Mar 26, 2017

I feel a little bit strange for the continuous error:

src » message » ready() contact ready for ToUserName

I feel strange for two reasons:

  • it tests well on my computer, but it failed in the test.
  • I didn't change any function about m.ready() but it occurs test failed...
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 26, 2017

Coverage Status

Coverage increased (+0.4%) to 56.13% when pulling f81e60a on lijiarui:add_mention into 2f18936 on Chatie:master.

coveralls commented Mar 26, 2017

Coverage Status

Coverage increased (+0.4%) to 56.13% when pulling f81e60a on lijiarui:add_mention into 2f18936 on Chatie:master.

@zixia zixia requested review from dcsan, xinbenlv and mukaiu Mar 26, 2017

@zixia

This PR looks good to me.

Please follow my reviews, and add more unit test cases if you can.

Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
@zixia

One more thing...

const room11 = msg11.room()
if (room11) {
await room11.ready()
setTimeout(function () {

This comment has been minimized.

@zixia

zixia Mar 26, 2017

Member

You'd better find out why you need setTimeout at here, or you will be more likely to be bitten again and again.

@zixia

zixia Mar 26, 2017

Member

You'd better find out why you need setTimeout at here, or you will be more likely to be bitten again and again.

lijiarui added some commits Mar 27, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 27, 2017

Coverage Status

Coverage increased (+0.3%) to 56.066% when pulling e2a98e7 on lijiarui:add_mention into 2f18936 on Chatie:master.

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.3%) to 56.066% when pulling e2a98e7 on lijiarui:add_mention into 2f18936 on Chatie:master.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 16, 2017

Member

@JasLin @xinbenlv
this commit has handled contact name with @, like lijiarui@beijing@wechaty

besides this, @xinbenlv and I tested and get the following conclusion:

  • chrome web wechat doesn't have the function about red tips [You were mentioned]

  • client-side wechat have the function about red tips [You were mentioned], but it cannot identify the following cases:

    • two contacts with the same name(or roomAlias)
    • copy paste mention message
  • ios&android wechat have the function about red tips [You were mentioned], and can identify the following cases:

    • two contacts with the same name(or roomAlias)
    • copy paste mention message
Member

lijiarui commented Apr 16, 2017

@JasLin @xinbenlv
this commit has handled contact name with @, like lijiarui@beijing@wechaty

besides this, @xinbenlv and I tested and get the following conclusion:

  • chrome web wechat doesn't have the function about red tips [You were mentioned]

  • client-side wechat have the function about red tips [You were mentioned], but it cannot identify the following cases:

    • two contacts with the same name(or roomAlias)
    • copy paste mention message
  • ios&android wechat have the function about red tips [You were mentioned], and can identify the following cases:

    • two contacts with the same name(or roomAlias)
    • copy paste mention message
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 16, 2017

Coverage Status

Coverage decreased (-0.5%) to 54.257% when pulling 253025d on lijiarui:add_mention into 3c6b575 on Chatie:master.

coveralls commented Apr 16, 2017

Coverage Status

Coverage decreased (-0.5%) to 54.257% when pulling 253025d on lijiarui:add_mention into 3c6b575 on Chatie:master.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 16, 2017

Member

@lijiarui Could you write a summary of all the details about the mentioned feature to the issue or a new wiki page?

I'd like to see:

  1. how to identify the magic number
  2. what's the different behavior between the platforms(android/ios/web)

You could also consider writing a blog post about this story too, because it will be very interesting.

Member

zixia commented Apr 16, 2017

@lijiarui Could you write a summary of all the details about the mentioned feature to the issue or a new wiki page?

I'd like to see:

  1. how to identify the magic number
  2. what's the different behavior between the platforms(android/ios/web)

You could also consider writing a blog post about this story too, because it will be very interesting.

@xinbenlv

This comment has been minimized.

Show comment
Hide comment
@xinbenlv

xinbenlv Apr 16, 2017

Member

What does the number 3 means in
if (list.length < 3)

Member

xinbenlv commented on src/message.ts in 253025d Apr 16, 2017

What does the number 3 means in
if (list.length < 3)

@xinbenlv

This comment has been minimized.

Show comment
Hide comment
@xinbenlv

xinbenlv Apr 16, 2017

Member

Could you add in description what do you plan to do when there are multiple @ signs? - It seems a bit hard to interpret from the code.

Member

xinbenlv commented on 253025d Apr 16, 2017

Could you add in description what do you plan to do when there are multiple @ signs? - It seems a bit hard to interpret from the code.

Show outdated Hide outdated src/message.ts Outdated
@lijiarui

@xinbenlv Sorry for reply late...
description as follows:

Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 22, 2017

Member

about message mention event:

Web Mac PC Client iOS Mobile android Mobile
[You were mentioned] tip ([有人@我]提示)
Identify magic code (8197) by copy & paste in mobile
Identify magic code (8197) by programming
Identify two contacts with the same roomAlias by [You were mentioned] tip
Member

lijiarui commented Apr 22, 2017

about message mention event:

Web Mac PC Client iOS Mobile android Mobile
[You were mentioned] tip ([有人@我]提示)
Identify magic code (8197) by copy & paste in mobile
Identify magic code (8197) by programming
Identify two contacts with the same roomAlias by [You were mentioned] tip
@JasLin

JasLin approved these changes Apr 22, 2017

Show outdated Hide outdated src/message.ts Outdated
@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 22, 2017

Member

Thanks all for approving this PR.

@lijiarui Could you please add comments to document the function? I want you to at least include the following part:

  1. your message mention event table and
  2. your explaination(reply/review) of your PR

Because your code is still hard to read/understand, it will be better with the comments.

Member

zixia commented Apr 22, 2017

Thanks all for approving this PR.

@lijiarui Could you please add comments to document the function? I want you to at least include the following part:

  1. your message mention event table and
  2. your explaination(reply/review) of your PR

Because your code is still hard to read/understand, it will be better with the comments.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 24, 2017

Coverage Status

Coverage increased (+0.3%) to 55.059% when pulling 0c828c2 on lijiarui:add_mention into 3c6b575 on Chatie:master.

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.3%) to 55.059% when pulling 0c828c2 on lijiarui:add_mention into 3c6b575 on Chatie:master.

1 similar comment
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 24, 2017

Coverage Status

Coverage increased (+0.3%) to 55.059% when pulling 0c828c2 on lijiarui:add_mention into 3c6b575 on Chatie:master.

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.3%) to 55.059% when pulling 0c828c2 on lijiarui:add_mention into 3c6b575 on Chatie:master.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 24, 2017

Member

@zixia Could you merge this time or give some suggestion?
thanks

Member

lijiarui commented Apr 24, 2017

@zixia Could you merge this time or give some suggestion?
thanks

@zixia

Please see my comments and let me know what you think. Thanks.

Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
Show outdated Hide outdated src/message.ts Outdated
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 26, 2017

Coverage Status

Coverage increased (+0.3%) to 55.053% when pulling 5df7b78 on lijiarui:add_mention into 3c6b575 on Chatie:master.

coveralls commented Apr 26, 2017

Coverage Status

Coverage increased (+0.3%) to 55.053% when pulling 5df7b78 on lijiarui:add_mention into 3c6b575 on Chatie:master.

@zixia

zixia approved these changes Apr 26, 2017

The code looks great!

Can we use the magic mention character to enable mention someone by room.say(text, contact)?

@zixia zixia merged commit 6750d19 into Chatie:master Apr 26, 2017

2 of 5 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codacy/pr Good work! A positive pull request.
Details
security/snyk No new vulnerabilities
Details
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Apr 26, 2017

Member

I'll pr room.say() by magic code, but as mention table showed, it won't get red [You were mentioned] tip on mobile wechat. While anyway, it should change.

Member

lijiarui commented Apr 26, 2017

I'll pr room.say() by magic code, but as mention table showed, it won't get red [You were mentioned] tip on mobile wechat. While anyway, it should change.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Apr 26, 2017

Member

Awesome, thanks!

Member

zixia commented Apr 26, 2017

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment