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

Method sendImage in Adapter API #996

Closed
wants to merge 1 commit into from
Closed

Conversation

lukefx
Copy link

@lukefx lukefx commented Jun 29, 2015

This PR is only to start a discussion, with the new Telegram bot API (https://github.com/lukefx/hubot-telegram) I've done an adapter to interact with the Telegram IM app, It's mainly used on mobile devices and it's possible to send and receive images.

At the moment it's not possible to do so with Hubot, you could send back and URL and usually the chat service will expand it to show the image.

Could we add the ability to do it natively? I've attached a proposal that works from my fork, but I would like to see what you think about it.

Example of scripts that reply with an image:

robot.respond /image me (.*)/i, (msg) ->
   msg.sendImage get_random_image(), caption

@arcturial
Copy link
Contributor

One problem I can foresee is that the images will have to be local to be able to upload them to Telegram (as an example). It might be better to have an option in the adapter like SEND_RAW_IMAGE=1 which will check the URL and if it's an image it will download it to a temporary storage and then forward it as an image. Doing it that way will allow existing Hubot scripts to benefit without breaking backwards compatibility or refactoring them.

@michaelansel
Copy link
Collaborator

Initial reaction: I'm nervous about expanding the Adapter API

I'll keep thinking on this and try to provide more thoughts when I have time...

@sdimkov
Copy link
Contributor

sdimkov commented Jun 30, 2015

In general an API call for sending local files would be handy. I can imagine many use-cases.

But this seems to be about sending images only and from a web source. That I guess is quite unique to Telegram. If that's the case no one else will implement it.

As to an API for sending local files I can imagine something like this:
sendFiles: (envelope, files..., callback) ->
where callback is called upon file(s) upload finished or got interrupted, or got rejected by other participant.
Such API could work for Facebook, Skype, Viber, HipChat etc..

@cycomachead
Copy link
Contributor

But this seems to be about sending images only and from a web source. That I guess is quite unique to Telegram. If that's the case no one else will implement it.

As to an API for sending local files I can imagine something like this:

👍 I was going to say the same thing -- a file sending API would be wondeful, especially for the clients that have some methods of file upload.

If enough services differentiate display of files by type, then I suppose that could be some optional parameter, but it seems like that logic would be adapter dependent and not necessarily need to be in Hubot.

@lukefx
Copy link
Author

lukefx commented Jul 2, 2015

I really like the idea of a generic sendFiles API like @sdimkov said.

sendFiles: (envelope, files..., callback) ->

The only problem I see now is that files should be an array of objects because every file can have metadata like filename, mime, etc. so we can find programmatically if we have to send a photo, video, audio...

@technicalpickles
Copy link
Member

Example of scripts that reply with an image:

In this example, what is get_random_image() returning? Is it just the URL, or is it an raw image? What would the adapter send to the service, just the URL or would it download the URL and post the contents of the image to the service?

I'm nervous about expanding the Adapter API

I get a little nervous too. I'm wondering how many scripts could be updated to use this instead of just sending a URL. I'm also wonder if the default implementation is to just send a URL of the thing.

@dcsan
Copy link

dcsan commented Aug 22, 2015

I'm new to hubot development, is there really no way for an adapter to declare some custom extension methods that can then be used (or maybe gracefully degraded) by custom scripts?
lukefx/hubot-telegram#15

the telegram API also allows you to send 'stickers' which is just a file_id of something that already exists on their system.

the telegram bot API is quite sophisticated with custom keyboards and many extensions beyond just text. It really is what hubot needs as a platform - they are fully supporting the bot revolution.

as well as Telegram, WeChat, LINE, KAKAOTalk and all the consumer messaging platforms are adding special features that add so much to a bot experience. Stickers for example are really just table stakes for an IM platform nowadays... lowest common denominator is good for inter-op but I would prefer if that decision was up to the library user. For a consumer app a text-only bot is a non-starter, so I guess that rules out using hubot outside of ascii apps.

@technicalpickles
Copy link
Member

I'm new to hubot development, is there really no way for an adapter to declare some custom extension methods that can then be used (or maybe gracefully degraded) by custom scripts?

There is no way as in no way currently implemented.

The methods have been proposed so far are all about sending something back to the user. That is the Response class where send, reply, etc live.

One thought I'm having is something like:

  • adapter creates a subclass of Response
  • adapter adds anything custom it wants
  • adapter updates robot.Response to use their subclass instead of the default

When listeners are called, they'd use robot.Response to create a response object, so it'd get the adapters custom one, which would have custom scripts. That means scripts could use adapter-specific behavior. If you are writing that kind of script with the intent to share, you'd need to do some feature sniffing to see if you have access to different methods.

At this point, there's no changes to hubot's core itself. At most, it might be making sure that robot.Response is used everywhere instead of requiring hubot's own Response. At the least, there could be more documentation in docs/adapters/development.md

I think the hard part is what happens when multiple adapters try to introduce the same methods that have different semantics.

@dcsan
Copy link

dcsan commented Aug 23, 2015

I'm wondering if we can have a graceful degradation adapter that would render to the best availability of the client. that's kind of what adapters usually do like a HAL . ie you don't only support the lowest common denominator but you let the adapter be smart about how it renders - text only? graphics?

eg the protocol may say:

choices: yes / no

in a text only client it shows just like that, and the user has to type in "Y" or "yes"

but in a richer client like Telegram with custom keyboards, we could actually render that as a real menu with choices.

For the next gen of Conversational UIs hubot could be a great core piece of infra, but without the capability to be extensible it's not usable.

I actually did a small video presentation on these scalable graphical Chat UIs at the meteor devshop in SF recently:

https://youtu.be/EGf665Fleko?t=4355

appreciate the brain trust here thoughts on good design patterns to enable something like these extensions without messing up the Hubot ecosystem!

@technicalpickles
Copy link
Member

I'm wondering if we can have a graceful degradation adapter that would render to the best availability of the client. that's kind of what adapters usually do like a HAL . ie you don't only support the lowest common denominator but you let the adapter be smart about how it renders - text only? graphics?

We do do this to some extent already, but very minimally. There is an emote function on the adapter, ie *hubot skims around the thread*. The default implementation just uses send so it comes out like any messages. The IRC adapter has it's own implementation of emote to send an actual emote.

in a text only client it shows just like that, and the user has to type in "Y" or "yes"

but in a richer client like Telegram with custom keyboards, we could actually render that as a real menu with choices.

Those custom keyboards are really interesting. I think the hard part with this is going to be coming up with a language for describing high level chat concepts, that gracefully degrade for simpler chat protocols (ie IRC).

In this case, I'd suggest something like prompt, ie response.prompt "Continue?", ["yes", "no"]. A text chat could just do a response.reply "Continue? Please reply yes or no", and telegram could use a custom keyboard you described.

The hard part with that particular workflow is that hubot would need a way have a one-off listener. Text clients would have to wait for text matching yes or no, but only once. There's been some issues and PRs around it, but nothing conclusive yet. Another potentially difficult thing about this is considering how the chat bot sees the selections from custom keyboard messages. Does it effectively send back text, or does it send a different message type? We already have a problem in hubot with adapters wanting or needing to do their own custom messages, and how to create listeners for them.

@dcsan
Copy link

dcsan commented Aug 24, 2015

thanks for the thoughtful response

Another potentially difficult thing about this is considering how the chat bot sees the selections from custom keyboard messages. Does it effectively send back text, or does it send a different message type?

in the case of telegram the response IS the text. WYSIWYG.
telegram also allows you to embed /commands in the text and those get highlit as clickable links that send that command. it's all very seamless and easy to author
just like html links can have href and visible text be different this is very desirable for the UX of chat UIs too.

the protocol could be text, such that at a worse case would just be presented as is, so its like a human legible protocol. you dont have JSON with menus and stuff, you just have:

choices: left / right

like an old unix usage: xx type prompt.

or, we could have content in chats marked as "metadata" for certain clients by just wrapping in { }
so you might see

are you ready?
{ buttons: ["Yes", "No"] }

then really dumb clients and old adapter codebase would not render the { buttons } stuff
slightly smarter adapters running on a simple client would somehow transform the {} markup into something textual like "type Yes or No"

and modern clients would render the buttons to make a cool UI. kind of like the whole modern browser thought train.

so this is more around the markup for how richer bots could be made, but that is quite a bottleneck for these projects.

@technicalpickles
Copy link
Member

@dcsan would you mind opening an issue to discuss response.prompt? I think we've sufficiently hijacked the discussion here from the original sendImage.

@technicalpickles
Copy link
Member

@lukefx you mentioned this is for the telegram adapter. Is this for their sendPhoto bot method? If so, it looks like that can take an uploaded file or an existing id. I don't see how supporting sendImage with a URL would help you there.

We've touched on it, but it seems like we need to just support something like response.upload that takes takes a file and uploads it. Maybe optionally it works from a URL to download and re-upload it?

If there are adapters that would want to customize how an image is uploaded, it might make sense then to add response.uploadImage (or uploadPhoto), with the default implementation just calling upload, but adapters can override it similar how we do for emote.

@dcsan
Copy link

dcsan commented Aug 25, 2015

opening an issue to discuss response.prompt

done: #1036
is that the content you had in mind?

@stale
Copy link

stale bot commented Jul 31, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Aug 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants