Skip to content
This repository has been archived by the owner on Aug 14, 2019. It is now read-only.

Photo messages #76

Closed
jamieomatthews opened this issue Nov 21, 2013 · 41 comments
Closed

Photo messages #76

jamieomatthews opened this issue Nov 21, 2013 · 41 comments

Comments

@jamieomatthews
Copy link

For my app, we need to display photo messages in addition to the text messages. I was digging through the innards of the the project last night trying to figure out how I would do it (without completely hacking it). My reasoning for writing this issue is to first ask if you have begun working on this, and two, to try to get a methodology in place for how it should be done, so I can proceed.

One attempt might make the JSBubbleView class handle either text or an image. This would definitely take some factoring, but is possible.

Another attempt might be to to add a new subclass of UIView, something like ImageBubbleView or something, that would be very similar to JSBubbleView but would be only for images. In that case, the JSBubbleMessageCell will decide, based on its config, whether or not to create a JSBubbleView or a JSImageBubbleVIew.

A third idea might even be that somehow you expose drawing the content inside the bubble to the Delegate (or at least that you could return a view in the delegate), and then it would get measured and redrawn behind the scenes.

Has anyone started this process, or have some advice?

@yesidi
Copy link

yesidi commented Nov 22, 2013

Good idea.

@carbocation
Copy link

Seconded.

@jessesquires
Copy link
Owner

documentation note : #16 has been closed as a duplicate of this. we can move that discussion here.

@jessesquires
Copy link
Owner

Hey guys — this is definitely a goal of mine for this control. Unfortunately, I won't have enough time in the near future to implement this. Also, this is going to be overly complicated trying to support iOS 6 and 7 (just a heads up for anyone trying). I don't have an immediate need for this personally, but I would be willing to help out after this control drops support for iOS 6 (which will probably be next year sometime once iOS 7 adoption increases).

Anyway, checkout the thread in #16 — a few decent references there. Also, you should check other forks of this project (though I'm not sure how well those are maintained).

@jessesquires
Copy link
Owner

@jamieomatthews — I like your idea for having a JSImageBubbleView and letting JSBubbleMessageCell decide which "type" of bubble to create.

My first thought is then to have a new datasource method imageForRowAtIndexPath: or something similar. However, one can quickly see problems arising from the conflicts with messageForRowAtIndexPath: — there would be lots of really ugly logic to make this smooth. (Checking if message is nil, or if the image is nil.)

I think the best, most modular solution would be for the datasource to provide a single method, dataForRowAtIndexPath: that returns id. Then check if this is an NSString or UIImage.

For the drawing the actual view, this should be easy. You should be able to mask/crop the image as a bubble. If you look at the UIImage category in the project, you can see how this kind of masking can be done. (For the iOS 7 style at least... support iOS 6 is going to be more difficult, but the references noted in #16 should help.)

Also, if you're starting on this, please make a new branch on your fork and we can pull that branch in here later. That way, if I'm able to help out (or anyone else), we can do all of the work there without affecting master (and still pull in bug fixes and refinements).

@robj
Copy link

robj commented Nov 24, 2013

I require this functionality but am not an Objective-C developer (Im using MessagesTableViewController in a RubyMotion project). Therefore I have put up an oDesk project to attract developers interested in adding the functionality.

https://www.odesk.com/jobs/Add-functionality-iMessage-style-iOS-control_~~76a19f159df997d0

@robj
Copy link

robj commented Nov 26, 2013

@jessesquires , How do you suggest the following use case is handled in the JSMessagesViewDataSource / JSMessagesViewDelegate protocols?

There are two types of photo bubbles. One normal, and one to represent video that will include a 'play button' mask. Protocol method(s) will be fired on tapping a bubble so the delegate can display the media.

eg.

JSMessagesViewDataSource:

mediaTypeForRowAtIndexPath

returning JSImage, JSVideo ?

JSMessagesViewDelegate:

bubblePressedAtIndexPath

@jessesquires
Copy link
Owner

I would like to avoid using anymore enums. (Already more than I'd like.) To accommodate non-text messages, the best, most modular approach would be to provide a single datasource method, dataForRowAtIndexPath: that either returns a JSMessageData object that encapsulates what the data is (a string, an image, or a video).

Separate (optional) delegates would be best: shouldViewImageAtIndexPath: and shouldViewVideoAtIndexPath:. This will defer implementation to consumers of this control. However, it would be nice (in my opinion) to (optionally?) provide this functionality. (i.e., JSMessagesViewController could know how to display images and play videos -- but that's definitely a much more long-term goal.)

@jessesquires
Copy link
Owner

Also, this entire control should really be re-written to use UICollectionView and auto-layout constraints. (Which would resolve a number of issues.) But that's way down the line, unfortunately. :/

@robj
Copy link

robj commented Nov 26, 2013

Interpreting your suggestions, how does something like this plan:-

JSMessagesViewDataSource Protocol will have new method dataForRowAtIndexPath returning JSMessageData

new JSMessageData Class

type - ( Text, Video, Image )
body - ( for plain text messages)
imageURL - ( the thumbnail URL of the Image, in the case of video the URL is of a thumbnail Image still representing the video)

if JSMessageData.type == Text
display normal text bubble

if JSMessageData.type == Image
display image bubble
shouldViewImageAtIndexPath in JSMessagesViewDelegate on tap

if JSMessageData.type == Video
display image bubble
add 'play button' mask over image
shouldViewVideoAtIndexPath in JSMessagesViewDelegate on tap

  • JSMessageData contains only the minimum required data to create a bubble.
  • Only the index is returned in the new JSMessagesViewDelegate methods, It is the responsibility of the consumer to fetch any additional data it needs straight from the datasource for displaying content (eg. full sized image urls, video url) MessagesTableViewController has no responsibility displaying content.

@jessesquires
Copy link
Owner

Roughly speaking, (and leaving certain implementation details out) yeah that sounds pretty good.

If you are starting work on this, please make a new feature branch, and push it to the main repo. That way, everyone involved can check on the progress, help in general, or help with bug fixes. :)

If you need me to create a branch, I can do that too. (I may have to, I'm not sure if you can submit a PR with a new, non-existent branch.)

@0xcodezero
Copy link

I would like to contribute in this feature request, but I prefer to start by Forking the repo, modifying code away from original repo. and after finalizing the Feature I would ask about pull request,

I will follow the discussion guidelines, Any discussion concerning the implementation will continue here.

I would share the Forked Repo link, later today.

@jessesquires
Copy link
Owner

@0xcodezero -- yes, exactly. But don't do the work on the master branch, do it on a feature branch.

@jessesquires
Copy link
Owner

Ok guys, I've created a new branch for this. You can either pull from upstream to work on this branch, or branch locally and submit your pull requests to this branch, not master.

These changes need to happen in small iterations, so please submit multiple small PRs, not a single giant one.

When it's ready, I'll merge this into master. :)

0xcodezero added a commit to 0xcodezero/MessagesTableViewController that referenced this issue Dec 2, 2013
…ure Header File ,Test Images, Attached Button Images
@powerqian
Copy link

Hi guys, I'm very looking forward to this feature as well. Meanwhile I hope you can take the Issue #75 into consideration when you design this new feature, which means you have to have some class method to determine the cell (bubble view) height based on provided content without using data source. Or a nicer way to solve this issue.

@jessesquires
Copy link
Owner

@powerqian -- I'm trying to find a nice solution to #75. :)

@ Everyone -- supporting image messages is going to be much nicer/easier if we drop iOS 6 support. How important is iOS 6 for each of you?

@robj
Copy link

robj commented Dec 4, 2013

I am not concerned with support for any less than iOS7

@carbocation
Copy link

I am only developing for ios7 and am not supporting ios<7 in my apps.

@jamieomatthews
Copy link
Author

Right now, its of medium importance to me. I think it would be nice if it had a fallback, even if it was limited in what it could do (IE, fixed size of the image, no masking), that would be nice.

@jessesquires by this, do you mean that this whole project would go iOS7 only, or this feature would be ios7 only?

@jessesquires
Copy link
Owner

@jamieomatthews -- the whole project. perhaps it's a bit too early for that. eventually though, this will happen. I'd also like to convert this to use a UICollectionView instead of UITableView -- but that will also take some heavy lifting.

@jessesquires
Copy link
Owner

thanks for the quick feedback everyone! @jamieomatthews -- also, another thing to think about... when this feature is finally done and ready for release, how important will iOS6 be then?

@powerqian
Copy link

I am using this control for my app which needs to meet the requirement of the customers of my company, so actually I made changes to make this control support iOS 5 :(. If it convert to use UICollectionView instead of UITableView I have to opt out….If you do so, please please make a branch which uses UITableView and stable so that I can make changes based on that.

@jessesquires
Copy link
Owner

@powerqian -- no worries! these major changes won't happen for awhile. :)

@vladimirGI
Copy link

agree with @jessesquires in regards to UICollectionView vs UITableView. This is the way regular SMS app works on ios7. The app we develop supports only ios7 + . So ios6 is not my concern right now.... Thanks

@scottcc
Copy link
Contributor

scottcc commented Dec 4, 2013

Unfortunately we still need iOS6 support for this.

On 2013-12-03, at 10:47 PM, Jesse Squires notifications@github.com wrote:

@powerqian -- I'm trying to find a nice solution to #75. :)

@everyone -- supporting image messages is going to be much nicer/easier if we drop iOS 6 support. How important is iOS 6 for each of you?


Reply to this email directly or view it on GitHub.

@jessesquires
Copy link
Owner

thanks @vladimirGI and @scottcc.

@scottcc -- do you need iOS 6 support in general, or specifically with the image message feature?

@jessesquires
Copy link
Owner

@ everyone — one last request, when you're finished with your apps that use this control, if they are public, please let me know so I can add them to the "Apps Using this Control" section of the README. :)

@scottcc
Copy link
Contributor

scottcc commented Dec 4, 2013

We need general support for iOS6 for now - photo support is something we'd eventually like but not critical (yet :)

I'd hate to do #ifdefs, or runtime checks, but it's something we're familiar with with a widely installed base of (private) users that upgrade on their own schedules. Heck, we have some applications that are still deployed and running on iOS 4.1 (not using your library though).

./scc

On 2013-12-04, at 11:38 AM, Jesse Squires notifications@github.com wrote:

thanks @vladimirGI and @scottcc.

@scottcc -- do you need iOS 6 support in general, or specifically with the image message feature?


Reply to this email directly or view it on GitHub.

@slycrel
Copy link

slycrel commented Dec 6, 2013

I've not been following this issue until now, but have plans to do this for our app that is in development. I had expected to get to it before now.. better late than never. =) Currently updating my project for the new main branch iOS 7 changes. (I really like the message bubble factory jesse, great job!) I'll be sure to reference the aforementioned branch for my development.

Also we are iOS 7 only as the app is not yet released.

@jessesquires
Copy link
Owner

@slycrel thanks!

@robj
Copy link

robj commented Dec 9, 2013

This branch will be renamed to issue76 as a candidate to pull for Image/Video photo bubbles implemented as discussed previously.

https://github.com/0xcodezero/MessagesTableViewController/tree/media-support-JSMessage

@robj
Copy link

robj commented Dec 9, 2013

minor note, the branch should be called issue_76_photoMessages, not issue_75_photoMessages

@jessesquires
Copy link
Owner

doh! just realized that i misnamed this feature branch.

just changed: issue_75_photoMessages --> issue_76_photoMessages

@ipeisong
Copy link

Why not create a 'CustomView' type to accept any subclass of UIView? That way actually one can create for any type of messages, photos, emotions, videos, even gifs and voice player.

@jessesquires
Copy link
Owner

Hello again everyone!

I want to "check the pulse" one more time regarding keeping support for iOS 6 for the photo messages feature.

It looks like iOS 7 adoption rates are around ~75%. I've recently begun looking into TextKit (iOS 7+) and there are some awesome features (possibly to use for adding the photo inside the input message view like iMessages). Supporting iOS 6 for this control (for this feature and in general) is going to be a pain — and judging by iOS 7 adoption rates, increasingly unnecessary. (Not to mention all the baggage that can be removed!)

It is extremely difficult for me to justify iOS 6 support, considering the above and the amount of time I have to dedicate to this project.

So... unless there's a really, really, really, really good argument out there... :) I think I'll draft one final release keeping iOS 6 support and update the pod.

@powerqian
Copy link

I agree the extra amount of time and pain to support both iOS 6 and iOS 7, but unfortunately the App I write needs to meet the customers requirement :(. So please at least make a branch or have a tag, including a stable release that support iOS 6, so that I can still fork that branch. Probably in the future I need to rewrite all your "iOS 7 only" code to support iOS 6.
Anyway, I believe I can still learn a lot of things from your code. Keep it up!

@jessesquires
Copy link
Owner

great idea @powerqian ! i will definitely make a new, stable iOS 6 branch. :)

@scottcc
Copy link
Contributor

scottcc commented Jan 14, 2014

+1 for the iOS 6 branch!

Sounds like at least a couple of us might become iOS 6 maintainers..

./scc

On Jan 14, 2014, at 2:57 AM, Jesse Squires notifications@github.com wrote:

great idea @powerqian ! i will definitely make a new, stable iOS 6 branch. :)


Reply to this email directly or view it on GitHub.

@vr-devil
Copy link

My app support for iOS7 only. I need this feature too. :)

@jessesquires jessesquires added this to the v5.0 beta release milestone Feb 23, 2014
@jessesquires
Copy link
Owner

Hello all! Please see #172

@jessesquires jessesquires removed this from the v5.0 beta release milestone Apr 11, 2014
@jessesquires
Copy link
Owner

Closing this issue.
New issue for media messages: #223

iOS6_support_stable branch available.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests