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

Enable media message forwarding #2236

Closed
wants to merge 1 commit into from
Closed

Enable media message forwarding #2236

wants to merge 1 commit into from

Conversation

McLoo
Copy link
Contributor

@McLoo McLoo commented Dec 18, 2014

For images this works without handing the content type through the intents, as it is set to ContentType.IMAGE_JPEG hardcoded.
This also unifies AudioSlide's and VideoSlide's constructPartFromUri as they were edited anyway.

Fixes #1362 and #1914

I strongly recommend merging #2234 and #2232 as without them forwarding message is not funny...

@McLoo McLoo changed the title Enable media messages forwarding Enable media message forwarding Dec 18, 2014
@mcginty
Copy link
Contributor

mcginty commented Jan 1, 2015

I'm thinking we should just change ConversationActivity to accept a DRAFT_TEXT, DRAFT_MEDIA, and DRAFT_MEDIA_TYPE, and have it determine the slide type from the media type passed in.

@McLoo
Copy link
Contributor Author

McLoo commented Jan 1, 2015

@mcginty I like how you use the word 'just' 😀

@McLoo
Copy link
Contributor Author

McLoo commented Jan 1, 2015

Works fine so far.
Maybe some prettifying is needed... but hey a minus diff

@McLoo
Copy link
Contributor Author

McLoo commented Jan 4, 2015

fixed non media forward :)
EDIT: and now we make use of ContentType.java

@McLoo
Copy link
Contributor Author

McLoo commented Jan 12, 2015

I are hates git burgerz
grilled this PR. for now :/

fixed. git push -f origin HEAD:ff_av is thy friend

@McLoo
Copy link
Contributor Author

McLoo commented Feb 16, 2015

needs heavy refactoring after #2331

@rhodey
Copy link
Contributor

rhodey commented Mar 25, 2015

@McLoo I started looking into this a few days ago before noticing that you already had a pull open. With some of the updates that have been included since you're pull was opened this seems to be a lot easier now.

In ConversationFragment.handleForwardMessage() you should be able to add a check for message.isMms(). Then if this is true call MediaMmsMessageRecord.fetchMediaSlide() and pass in a listener that onSuccess(Slide result) starts ConversationActivity with one of the following extras ConversationActivity.DRAFT_IMAGE_EXTRA, ConversationActivity.DRAFT_AUDIO_EXTRA, or ConversationActivity.DRAFT_VIDEO_EXTRA.

The MediaMmsMessageRecord.fetchMediaSlide() call is technically asynchronous but MediaMmsMessageRecord's SlideDeck should have definitely been resolved by now and we're already assuming this is the case here. If the listener gets an onFailure() call just treat it as a forwarded text-only message instead.

I defer to @mcginty for his ImageSlide and SlideDeck expertise.

@McLoo
Copy link
Contributor Author

McLoo commented Mar 26, 2015

@rhodey if you're fine with it, you can PR this as a new one. I'm a bit busy these days. If not... well I guess there's always some time around 😀

Still don't know if this should be touched again before #2331 #2792
Edit: though there should be less changes if we just don't implement DRAFT_MEDIA,and DRAFT_MEDIA_TYPE as proposed by @mcginty

@McLoo
Copy link
Contributor Author

McLoo commented Mar 26, 2015

@rhodey just checked your fix. You'll need to do some content type handling. I admit I don't understand it completely, but the changes to AudioSlide and VideoSlide where necessary to avoid crashes.
Don't think yours will work out of the box.

@rhodey
Copy link
Contributor

rhodey commented Mar 26, 2015

@McLoo thanks for the tip! I hadn't tested that POC branch for audio and video yet, will check it out :)

@McLoo
Copy link
Contributor Author

McLoo commented May 19, 2015

@rhodey is fixing this anywhere on your agenda? I might have a second look, will be another PR for sure. sooo many changed 😮

@McLoo McLoo closed this May 19, 2015
@rhodey
Copy link
Contributor

rhodey commented May 19, 2015

@McLoo it is not on my todo list but probably my todo-if-everything-else-magically-works-out list. I was also waiting to let the Glide refactor mature, seems like things are getting pretty stable with that and last time I checked all the changes made things significantly easier-- don't let me stall you on that if you feel like starting again :)

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

Successfully merging this pull request may close these issues.

[bug] Forwarding media messages only forwards text
3 participants