#4 send image/video #337

mukaiu · 2017-03-19T06:47:24Z

Support Message Type of Image/Video for #4

Add
wechaty.uploadMedia(stream: NodeJS.ReadableStream, filename: string, toUserName: string)
wechaty.sendMedia(message: Message)

coveralls · 2017-03-19T07:04:43Z

Coverage decreased (-1.2%) to 54.596% when pulling 1bb2b9f on mukaiu:master into aab23c3 on Chatie:master.

coveralls · 2017-03-19T07:04:43Z

Coverage decreased (-1.2%) to 54.596% when pulling 1bb2b9f on mukaiu:master into aab23c3 on Chatie:master.

hailiang-wang · 2017-03-19T07:29:36Z

@zixia @lijiarui, @mukaiu has tested the functions. Please give some comments. Thanks.

huan · 2017-03-19T08:11:09Z

@Samurais Great, I'll have a look into it.

@mukaiu Good job! 👍

huan

Hi @mukaiu , Great job, the PR looks great!

I had made some comments for you, mainly is talking about where is the right place to put those functions.

Please follow my reviews, let's move on and make it ready today. :)

P.S. I'm just thinking about the API of Wechaty of sending/replying files(PDF/Video/Photo/Emotions), it seems that we use stream for doing this is not the best choice.

For example, when user want to send/reply a file, the simpliest way is just use the full file path, because that's straight forward, and no need to specify the MIME type and filename anymore(which is required when we using stream).

So, maybe we could consider to use file path name in the API, at lease we support both types(file path name & stream).

Then we can design the API like this:

contact.say('/tmp/file.jpg')
room.say('/tmp/document.pdf')
message.reply('/tmp/haha.gif'))

huan · 2017-03-19T08:23:13Z

example/ding-img-bot.ts

+ )
+
+ if (/^(ding|ping|bing)$/i.test(m.content()) && !m.self()) {
+ bot.uploadMedia(fs.createReadStream(__dirname '/../image/BotQrcode.png'), 'BotQrcode.png', m.from().id).then(mediaId => {


I think we can add this new feature(reply the QRCode image) to the classic ding-dong-bot example, instead of creating this new one, because send image should be a basic function for all users.

In Wechaty, we prefer to use await instead of then.

huan · 2017-03-19T08:32:12Z

src/message.ts

@@ -125,6 125,7 @@ export type MsgObj = {
 status: string,
 digest: string,
 date: string,
+ mediaId: string


In the current design of Message, there's two Message Classes: Message and MediaMessage.

the Message is the basic one, mainly for dealing the plain text;

the MediaMessage is extended from Message and will focus on dealing with all the media type messages(attachment files).

So I think we should better to move all the media logic to the MediaMessage instead of putting them directly into Message, which is more clear for the code.

huan · 2017-03-19T08:32:55Z

src/message.ts

@@ -370,6 372,17 @@ export class Message implements Sayable {
 return this.obj.content
 }

+ public mediaId(): string


Please move mediaId() to MediaMessage class.

huan · 2017-03-19T08:38:37Z

src/puppet-web/wechaty-bro.js

+ var m = chatFactory.createMessage({
+ ToUserName: ToUserName
+ , MediaId: MediaId
+ , MsgType: confFactory.MSGTYPE_IMAGE


Could we support other MSGTYPE(Such as PDF, VIDEO, etc...) as well? Because the function name is sendMedia. :)

huan · 2017-03-19T08:51:22Z

src/puppet.ts

@@ -30,6 30,8 @@ export abstract class Puppet extends EventEmitter implements Sayable {

 public abstract self(): Contact

+ public abstract getBaseRequest(): Promise<any>


The Abstract Puppet class is designed for abstract all the APIs at the high level for all puppets, such as PuppetWeb(which we are mainly developing on), PuppetAndroid(which we are hoping to develop), etc.

So we should only put the common methods into this abstract class.

The getBaseRequest() is highly related to the PuppetWeb. The other puppet, let's say PuppetAndroid, is more likely not need this method.

So we should better move getBaseRequest() to PuppetWeb and capsulate it inside.

huan · 2017-03-19T08:52:38Z

src/puppet.ts

@@ -30,6 30,8 @@ export abstract class Puppet extends EventEmitter implements Sayable {

 public abstract self(): Contact

+ public abstract getBaseRequest(): Promise<any>
+ public abstract sendMedia(message: Message): Promise<void>


Can we use send() to both Message & Media Message?

I purpose this form:

public abstract send(message: Message | MediaMessage)

huan · 2017-03-19T08:57:16Z

src/wechaty.ts

@@ -417,6 420,106 @@ export class Wechaty extends EventEmitter implements Sayable {
 return this.puppet.self()
 }

+ public async uploadMedia(stream: NodeJS.ReadableStream, filename: string, toUserName: string): Promise<string> {


The Wechaty Class is the highest abstract class for the FrameWork, it should not include any implement details, such as how to upload a media file.

We should move uploadMedia() from here to PuppetWeb instead, then we can call PuppetWeb.uploadMedia() to do the job.

huan · 2017-03-19T08:58:01Z

src/wechaty.ts

+ return mediaId as string
+ }
+
+ public async sendMedia(message: Message): Promise<void> {


As I mentioned before, I'd very like to use the same send() method to send both Message & MediaMessage.

huan · 2017-03-19T10:31:11Z

Update:

I realized that contact.say('/tmp/file.jpg') will not work in sudden, because that's a string...

Do you have any suggestion for using the file path name directly for this scanerio?

lijiarui · 2017-03-19T13:34:59Z

How about design a new type like

export type Content = {
    Type?: 'file' | 'pic' | 'voice' | ...
    Content: ''/tmp/file.jpg''
}

contact.say(para: Content | string)

Then, when we want to send voice or any other type of the content, just add Type

Even, we can add enum for Type

dcsan · 2017-03-19T16:01:39Z

I had a problem with this feature and created a ticket here:
#338

mukaiu · 2017-03-19T16:34:35Z

@dcsan
This is running ding-img-bot.ts?

dcsan · 2017-03-19T18:09:22Z

@mukaiu yes

ts-node example/ding-img-bot.ts

(run as an npm script) please see #338 for more details.

huan · 2017-03-21T10:22:08Z

@lijiarui Yes, a new type is needed with this scenario.

What I'm thinking about is to create a new Class for doing this instead of an Interface. I.e. contact.say(new File('/tmp/a.pdf')) because this class could set the MIME information of the file automatically instead of by hand, which will be more convenience.

@mukaiu @JasLin I want to hear from you too because this API will be needed after this PR is merged.

Thanks!

mukaiu · 2017-03-21T10:33:00Z

I think,

contact.say(MediaMessage)

huan · 2017-03-21T10:37:13Z

Yap, that's a better idea to solve this problem! :D

contact.say(new MediaMessage('/tmp/a.gif'))

👍 @mukaiu

coveralls · 2017-03-21T10:37:33Z

Coverage decreased (-0.4%) to 55.328% when pulling e1755a0 on mukaiu:master into aab23c3 on Chatie:master.

merge chatie

coveralls · 2017-03-21T17:11:57Z

Coverage decreased (-0.4%) to 55.328% when pulling ecd2ff8 on mukaiu:master into 0426d48 on Chatie:master.

contact.say(new MediaMessage('/tmp/file.jpg')) message.say(new MediaMessage('/tmp/haha.gif'))

mukaiu · 2017-03-23T18:32:24Z

@zixia
implement
contact.say(new MediaMessage('/tmp/file.jpg'))
message.say(new MediaMessage('/tmp/haha.gif'))

but the logic of room.say is uncertain.

mukaiu · 2017-03-23T18:53:34Z

test error,help~~~

dcsan · 2017-03-25T06:31:30Z

Is there a docker image with this version of Wechaty? It would make testing much easier...

huan

Hi @mukaiu ,

The new PR with enhanced media type upload is fantastic!

I just made some minor options in the comment; I think after finishing this we could be able to prepare the merging.

One more thing: about the circle dependency of Message and MediaMessage, I think we should consider solving this problem by merge message-media.ts to message.ts.

huan · 2017-03-25T05:11:06Z

src/contact.ts

@@ -578,20 581,23 @@ export class Contact implements Sayable {
 * await contact.say('welcome to wechaty!')
 * ```
 */
- public async say(content: string): Promise<void> {
- log.verbose('Contact', 'say(%s)', content)
+ public async say(content: string|MediaMessage): Promise<void> {


Using Typescript override syntax here:

public async say(text: string) public async say(mediaMessage: MediaMessage) public async say(textOrMedia: string | MessageMessage)

huan · 2017-03-25T05:13:15Z

src/contact.ts

+ if (typeof content === 'string') {
+ m = new Message()
+ m.content(content)
+ } else


} elseif (textOrMedia instanceof MediaMessage) { ... } else { throw new Error('not support args') } ```ts

huan · 2017-03-25T05:17:34Z

src/message-media.ts


 constructor(rawObj) {
- super(rawObj)


Use Typescript override syntax here:

constructor(rawObj: object) constructor(filePath: string) constructor(rawObjOrFilePath: object | string) {

huan · 2017-03-25T05:18:16Z

src/message-media.ts

+ if (typeof rawObj === 'string') {
+ super()
+ this.filepath = rawObj
+ } else


} else if (rawObj instanceof object) { ... } else { throw new Error('not supported construct param') }

huan · 2017-03-25T05:38:51Z

src/message-media.ts

 } from './message'
 import { UtilLib } from './util-lib'
 import { PuppetWeb } from './puppet-web/puppet-web'
 import { Bridge } from './puppet-web/bridge'

 export class MediaMessage extends Message {
 private bridge: Bridge
+ private filepath: string


Can we only keep the ReadableStream in MediaMessage?

I want to get rid of the absolute file path in the class because it's not necessary.

private fileStream: ReadableStream private fileName: string // 'music' private fileExt: string // 'mp3'

huan · 2017-03-25T07:36:31Z

src/message.ts

@@ -492,31 493,44 @@ export class Message implements Sayable {
 })
 }

- public say(content: string, replyTo?: Contact|Contact[]): Promise<any> {
+ public say(content: string | MediaMessage, replyTo?: Contact|Contact[]): Promise<any> {


Please use Override to declare say() at here.

say(textOrMedia: string | MediaMessage)

huan · 2017-03-25T07:46:19Z

src/message.ts

 }
- m.content(mentionList ' ' content)
+ } else {


} else if (textOrMedia instnaceof MediaMessage) {

huan · 2017-03-25T07:49:34Z

src/puppet-web/puppet-web.ts

+ FileMd5: md5,
+ FromUserName: this.self().id,
+ ToUserName: toUserName,
+ UploadType: 2,


Could you change this magic number to a enum value?

UploadType optional value 1 or 2, use unknown

huan · 2017-03-25T07:49:41Z

src/puppet-web/puppet-web.ts

+ ToUserName: toUserName,
+ UploadType: 2,
+ ClientMediaId: new Date,
+ MediaType: 4,


Could you change this magic number to a enum value?

huan · 2017-03-25T07:50:42Z

src/puppet-web/puppet-web.ts

+ let ext = path.extname(filepath)
+
+ let contentType = UtilLib.mime(ext)
+ let mediatype


Could you define a new type for MediaType like:

type MediaTpye = 'pic' | 'video' | 'doc'

I think here maybe 'file' is better than 'doc'
@zixia what do you think about it?

mukaiu · 2017-03-25T18:12:40Z

@dcsan docker pull mukaiu/wechaty

# Conflicts: # package.json

coveralls · 2017-03-27T20:09:31Z

Coverage decreased (-1.9%) to 54.59% when pulling 699f733 on mukaiu:master into d6a0e86 on Chatie:master.

coveralls · 2017-03-27T20:40:21Z

Coverage decreased (-1.9%) to 54.561% when pulling 699f733 on mukaiu:master into d6a0e86 on Chatie:master.

huan · 2017-03-28T11:34:46Z

Hi @mukaiu,

Thank you so much for this Pull Request!

Now I feel it's good enough for supporting attachment messages in Wechaty, and I believe it will help many developers who want to use this functionality!

I have no more reviews for your PR, and I'll approve the changes.

However, We all know it's not perfect. I'm afraid there's some part that we missed to review. Could you talk with other Reviewers, and let some of them to Approve Changes?

Let's do merge the PR after at least other two contributors approved it. Cheers!

dcsan · 2017-03-28T13:11:51Z

apologies for delay, I have been trying to check this out but haven't got a good docker usage development workflow, it takes too long to make a code change and test out, so I haven't been able to test it yet.

FWIW my flow is to build a container with mukai/wechaty fork as below
but then i have to rebuild the container, copy in my own code, run it...
since its a new container each time I also have to scan a QR code each dev loop.
this takes a few mins each code change.

is there a faster dev workflow you use?

I guess I could just run off a local build of wechaty, but I had problems with googlechrome and other deps, so aiming to use docker.

perhaps I can just exec bash on the docker container and work directly within it, but not sure how that works yet with the CMD [""] and ENTRYPOINT here
https://hub.docker.com/r/zixia/wechaty/~/dockerfile/

any other suggestions for fast workflow appreciated.

## using images fork
FROM mukaiu/wechaty
EXPOSE 8080
RUN mkdir -p /bot
WORKDIR /bot
COPY . /bot

CMD ["index.js"]

lijiarui · 2017-03-25T15:11:00Z

src/puppet-web/puppet-web.ts

+ let ext = path.extname(filepath)
+
+ let contentType = UtilLib.mime(ext)
+ let mediatype


I think here maybe 'file' is better than 'doc'
@zixia what do you think about it?

mukaiu · 2017-03-28T13:52:44Z

@dcsan
Try using Docker Volume to mount the local directory to the docker bot directory.

docker run --name  bot -d -v LOCAL_DIR:/bot zixia/wechaty mybot.js

After modifying the code, restart container

docker restart bot

Use the following command to display the container log

docker logs -f bot

dcsan · 2017-03-28T22:58:07Z

@mukaiu thanks i didn't know about docker volumes. perhaps if i combine that with hot reloading it will be much less painful to develop this way.

So i went to try and test this. in a wechaty checkout

  git checkout master
  git pull
  git fetch origin pull/337/head

but now it seems the ding-image bot doesnt exist, and there's no demo code in the ding-dong or media-file bot. Is there a usage example anywhere?

Since the API has changed, looking at your code above

message.say(new MediaMessage('/tmp/haha.gif'))

I modified ding-dong-bot.ts

import {
  MediaMessage,
} from '../'

// later
    if (/^(img)$/i.test(m.content()) ) {
      m.say(new MediaMessage('/image/wechaty-icon.png'))
      log.info('Bot', 'REPLY: dong')
    }

but gives TS compile error:

Argument of type 'MediaMessage' is not assignable to parameter of type 'string'.

I also looked in the /test directory for some examples but can't see anything:

grep -r Media test/*       
test/README.md:        # Subtest: Media download smoking test
test/README.md:    ok 2 - Media download smoking test # time=207.54ms
➜  wechaty git:(master) ✗

It would be great if there was some example code of how to use /test this functionality.

ts-node example/ding-dong-bot.ts 

/usr/local/lib/node_modules/ts-node/src/index.ts:319
          throw new TSError(formatDiagnostics(diagnosticList, cwd, ts, lineOffset))
                ^
TSError: ⨯ Unable to compile TypeScript
example/ding-dong-bot.ts (78,13): Argument of type 'MediaMessage' is not assignable to parameter of type 'string'. (2345)
    at getOutput (/usr/local/lib/node_modules/ts-node/src/index.ts:319:17)
    at /usr/local/lib/node_modules/ts-node/src/index.ts:350:18
    at Object.compile (/usr/local/lib/node_modules/ts-node/src/index.ts:509:17)
    at Module.m._compile (/usr/local/lib/node_modules/ts-node/src/index.ts:413:44)
    at Module._extensions..js (module.js:580:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/usr/local/lib/node_modules/ts-node/src/index.ts:416:12)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Function.Module.runMain (module.js:605:10)

which is since Message.say signature is:

  public say(content: string, replyTo?: Contact|Contact[]): Promise<any> {

what is the interface to send a MediaMessage?

since export class MediaMessage extends Message {
I also tried sending a MediaMessage with an empty string.

    if (/^(img)$/i.test(m.content()) ) {
      let img = new MediaMessage('/image/wechaty-icon.png')
      img.say("")
      log.info('Bot', 'REPLY: dong')
    }

but gives error:

07:14:39 ERR Bot on(message) exception: SyntaxError: Unexpected token / in JSON at position 0

dcsan · 2017-03-28T23:33:06Z

UPDATE

I tried just doing a clone of @mukaiu 's fork, and that seems to work very well in sending an image message. So at this point I'm not sure about this PR but the forked repo is functional.

https://github.com/mukaiu/wechaty/blob/master/example/ding-dong-bot.ts#L75

So, I guess my method for reviewing this PR isn't working.
Just FYI since I am not a maintainer of this repo, it's a bit more complicated to check out a fork.

this message signature extensions did not come down for me

  public async say(textOrMedia: string | MediaMessage): Promise<void> {

even though it shows up in the PR
https://github.com/Chatie/wechaty/pull/337/files#diff-91d335b29ba8ed8ebb1d29260610d794R584

  git checkout master
  git pull
  git fetch origin pull/337/head

update

git pull origin pull/337/head

now seems to get the latest from this PR
but that gives a different error on running code

07:48:12 ERR PuppetWeb send() exception: cannot say nothing
(node:74270) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: cannot say nothing
(node:74270) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

So anyway, mukais fork works fine. These problems are more about all the tooling/PRs/workflow on this project. I'll approve the PR as it doesn't introduce any new problems, and then do a clean checkout of the project where I can actually test this feature.

The other suggestion could be to make an unstable / edge branch and merge changes to that branch when these new features are being introduced.
Then you can get more people to easily test work in progress features.

dcsan

I can't get the checkout of the PR to work but the original fork is working very well. I'd like to get this into master (or an edge branch) asap so we can actually use it.

dcsan · 2017-03-28T23:55:00Z

if you did make an "edge" branch it would make it a lot easier for me to make a fork based off of that and start modifying / sending PRs. Making a PR based on someone else's PR on a repo that you don't own is just too much complication.

huan · 2017-03-30T09:50:36Z

Thanks @dcsan @lijiarui for approving this PR, it really helps.

@dcsan I like your idea of creating a edge branch to automaticaly merge the Pull Requests(which passed CI) for testers.

Do you know any web service or tools to do this kind of job(merge pull requests to a branch) automaticaly? That will be very efficiency.

dcsan · 2017-03-30T14:26:19Z

I'm not sure, but I dont think we have that many PRs now?
i have seen some other github projects with very active bots, so I will try to take a note next time I see this. Rails and many other projects keep edge branch.

dcsan · 2017-03-30T14:28:32Z

https://github.com/bors-ng/bors-ng
https://github.com/calmh/mergebot

mukaiu added 5 commits March 19, 2017 07:22

#1 发送图片测试

7acb473

tslint

c9c34d5

调整文件路径为stream

231a8ae

merge Chatie/wechaty

7f1295a

Merge branch 'Chatie-master'

1bb2b9f

huan requested changes Mar 19, 2017

View reviewed changes

huan mentioned this pull request Mar 19, 2017

invite @mukaiu to write a blog for introducing himself and talk about the send image PR(issue #4) wechaty/docusaurus#10

Closed

dcsan mentioned this pull request Mar 19, 2017

cannot send images / this.puppet.getBaseRequest is not a function #338

Closed

remove bl,mime

e1755a0

Merge pull request #3 from Chatie/master

ecd2ff8

merge chatie

#4 implement

04c2de7

contact.say(new MediaMessage('/tmp/file.jpg')) message.say(new MediaMessage('/tmp/haha.gif'))

clean message.mediaId

52867f8

huan requested changes Mar 25, 2017

View reviewed changes

huan requested a review from lijiarui March 25, 2017 08:00

huan added the enhancement label Mar 25, 2017

huan added this to the v0.9 Support All API in Web Wechat milestone Mar 25, 2017

huan requested review from dcsan, xinbenlv and Gcaufy March 26, 2017 02:24

mukaiu added 7 commits March 28, 2017 03:08

move MediaMessage to message.ts

12a71a2

package: add bl 1.2.0

20bcd84

merge Chatie-master

e915d66

Merge branch 'master' into pr/4

6bdbf42

# Conflicts: # package.json

Merge branch 'pr/4'

60bd553

Merge remote-tracking branch 'Chatie/master'

9fe5996

# Conflicts: # package.json

restore puppet-web.ts format

699f733

huan requested a review from hailiang-wang March 28, 2017 11:11

huan approved these changes Mar 28, 2017

View reviewed changes

lijiarui approved these changes Mar 28, 2017

View reviewed changes

dcsan approved these changes Mar 28, 2017

View reviewed changes

huan merged commit 11760a5 into wechaty:master Mar 30, 2017

		@@ -30,6 30,8 @@ export abstract class Puppet extends EventEmitter implements Sayable {

		public abstract self(): Contact

		public abstract getBaseRequest(): Promise<any>

#4 send image/video #337

#4 send image/video #337

Conversation

mukaiu commented Mar 19, 2017 • edited by huan Loading

coveralls commented Mar 19, 2017

coveralls commented Mar 19, 2017

hailiang-wang commented Mar 19, 2017

huan commented Mar 19, 2017

huan left a 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

huan Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huan commented Mar 19, 2017

lijiarui commented Mar 19, 2017 • edited Loading

dcsan commented Mar 19, 2017 • edited Loading

mukaiu commented Mar 19, 2017

dcsan commented Mar 19, 2017 • edited Loading

huan commented Mar 21, 2017 • edited Loading

mukaiu commented Mar 21, 2017

huan commented Mar 21, 2017 • edited Loading

coveralls commented Mar 21, 2017

coveralls commented Mar 21, 2017

mukaiu commented Mar 23, 2017

mukaiu commented Mar 23, 2017

dcsan commented Mar 25, 2017

huan left a comment • edited Loading

Choose a reason for hiding this comment

huan Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

huan Mar 25, 2017 • 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

Choose a reason for hiding this comment

mukaiu commented Mar 25, 2017

coveralls commented Mar 27, 2017 • edited Loading

coveralls commented Mar 27, 2017

huan commented Mar 28, 2017

dcsan commented Mar 28, 2017

Choose a reason for hiding this comment

mukaiu commented Mar 28, 2017 • edited Loading

dcsan commented Mar 28, 2017 • edited Loading

dcsan commented Mar 28, 2017 • edited Loading

dcsan left a comment

Choose a reason for hiding this comment

dcsan commented Mar 28, 2017

huan commented Mar 30, 2017

dcsan commented Mar 30, 2017

dcsan commented Mar 30, 2017

mukaiu commented Mar 19, 2017 •

edited by huan

Loading

huan Mar 19, 2017 •

edited

Loading

lijiarui commented Mar 19, 2017 •

edited

Loading

dcsan commented Mar 19, 2017 •

edited

Loading

dcsan commented Mar 19, 2017 •

edited

Loading

huan commented Mar 21, 2017 •

edited

Loading

huan commented Mar 21, 2017 •

edited

Loading

huan left a comment •

edited

Loading

huan Mar 25, 2017 •

edited

Loading

huan Mar 25, 2017 •

edited

Loading

coveralls commented Mar 27, 2017 •

edited

Loading

mukaiu commented Mar 28, 2017 •

edited

Loading

dcsan commented Mar 28, 2017 •

edited

Loading

dcsan commented Mar 28, 2017 •

edited

Loading