840069 - [MMS][User Story] Message preview including size

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Peter Dolanjski [:pdol]]

UCID: Messages-040

User Story:
As a user, I can preview a composed MMS message and see the size of the message (in kB) prior to sending to ensure the message is properly formatted and is a known size.

Comment 1

[Casey Yee]

I recommend that there should be a carrier configurable preference which allows for the carrier to set MMS billing either on a per-message basis or per-KB.

When the pref is set to per-KB we will give the user the ability to re-size images before attaching them to messages.   We will also display along side each attachment the file size so that the user knows how much data is being used for the message.

When the pref is set to a per-message basis, we automatically re-size image to the extents of the message limits and do not display the file sizes since they are not relevant in this billing scenario to the user.

All this is outlined in the MMS UX doc.

Comment 2

[Dietrich Ayala (:dietrich)]

Per partner and release-driver discussions, marking blocking- until all MMS functionality in bug 849867 is complete, allowing it all to be uplifted at once to avoid SMS bustage.

Comment 3

[Dan Heberden]

Got a good start on this, after muddling around with possible tests for existing functionality (bailed on that, since it was so DOM centric) and how to best achieve the placeholder text on a div[contentEditable].

Code so far: https://github.com/danheberden/gaia/commit/c3b117c430d015d6ef7f2996e009ba0eaa54d040

a) Abstracted the message composition into its own object to only expose sensible things and not perform direct DOM manipulation by other areas of Messaging.
b) Internalized .placeholder to use the `placeholder` attr of the element for i10n
c) Updated existing code to still work with ThreadUI.compose object

Not done, though - there's some things like `updateCounter` that perform messaging logic AND UI stuff. ThreadUI should just be UI, obviously, so I'll push this into the compose object. I just wanted to get *something* pushed tonight. Todo:

a) move sendMessage to utilize the form submission only
b) clean up the `updateHeight`, `sendEnabled` etc to not be so DOM dependent for testing
c) keep sending of the message non DOM dependent

Where I'm stuck is how to manage media inside of this element. There are a few use-cases to worry about:

a) user pastes media into input
b) user removes already included media
c) order of media inserted for MMS concatenation

My idea is this: have `ThreadUI.compose.getMessage()` return an array to be consumed by `sendMessage`. The array will be something like ['someText', Attachment, 'someMoreText']. The Attachment object will be the media attached, thumbnail, size, etc to display to the user. It will also have a DOM represenation, like, <div class="attachment"></div> that will be used to display the thumbnail, kB size, etc. If this element is deleted by the user using the backspace key, the message array from getMessage() will be updated accordingly. When an attachment is placed in the text area, the DOM representation will be added. 

This way, the MMS api can consume an Attachment however it wants/needs to. Feel free to PM me on IRC for discussion.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

leo+ as this is a part of MMS and part of v1.1 to be included in leo+ queries. No_UPLIFT for now before the whole MMS is completed

Comment 5

[Dan Heberden]

Hi! I finally got my last remaining issues figured out, so this is ready for an intense look. Here's what it looks like on a unagi: http://danheberden.com/share/90a3.png

Everything is in ThreadUI.compose:Object, which has things like append, insert, prepend. I also made Attachment, which creates attachments.

Attachment, however, needs to be worked on a bit to support proper types - like getting the preview from a video EXIF, etc. (Though, I'd like to see a Video constructor before that happens). 

One thing, though, after rebasing the hell out of this: I don't understand why every single element has an ID. It makes CSS harder with specificity weight, it makes the code bound to the DOM, and it makes for a lot more work. You can see how I handled the DOM in ThreadUI.compose and ThreadUI.compose.init. Feel free to hit me up on IRC if you'd like to discuss this with me (maybe i'm missing something or unaware of a particular problem?).

The work is all at https://github.com/danheberden/gaia/commits/840069_message_preview but i'll try to figure out how to add a .patch file.

Comment 6

[Dan Heberden]

Attachment: 840069 Message Preview with Attachment Support

https://github.com/danheberden/gaia/commits/840069_message_preview

Comment 7

[Paul Yang [:pyang] (away)]

moztrap #6542

Comment 8

[Steve Chung [:steveck]]

Hi Dan,
Could you also send a pull request to mozilla-b2g gaia? I will take a deep look later but it looks fine at first. Thanks for the great work!

Comment 9

[Borja Salguero [:borjasalguero]]

Hi Dan,
For landing a patch to Gaia you have to ask Julier or me to get a review of your code (that's why I've updated the flags). On the other hand, is this implementation based on any visual design from our UX Team?

Comment 10

[greg]

Attachment: pull request on github

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 741379 [details] [review]
pull request on github

taking the review

Comment 12

[Julien Wajsberg [:julienw]]

reviewed on the PR

Comment 13

[Dietrich Ayala (:dietrich)]

Is a new patch needed here?

Comment 14

[Julien Wajsberg [:julienw]]

new patches are constantly done on github's PR :)
(that's partly why PR are wrong, and partly why they're awesome).

Comment 15

[Dietrich Ayala (:dietrich)]

Yeah, realized that last week :P

How far is this from landing then?

Comment 16

[Borja Salguero [:borjasalguero]]

Comment on attachment 741379 [details] [review]
pull request on github

Adding me as well here for helping Julien as well (being in the same timezone is a good point here! ;) ).

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 741379 [details] [review]
pull request on github

borja, I'll let you finish that review, I think the patch is not far from being finished.

Comment 18

[Borja Salguero [:borjasalguero]]

We are quite close to land this! :)

Comment 19

[Borja Salguero [:borjasalguero]]

Attachment: Issue 1 - 'Blinking' text

Comment 20

[Borja Salguero [:borjasalguero]]

Attachment: Issue 2 - Broken layout

Comment 21

[Borja Salguero [:borjasalguero]]

Attachment: Issue 3 - Recipients container 'push up/pull down' effect

Comment 22

[Borja Salguero [:borjasalguero]]

Attachment: Issue 4 - Composer layout broken

Comment 23

[Borja Salguero [:borjasalguero]]

Attachment: Issue 5 - maxSegments bug reopened

Comment 24

[Borja Salguero [:borjasalguero]]

Comment on attachment 741379 [details] [review]
pull request on github

As we agreed, due to this code is risky, we are going to merge in a 'unstable' branch for moving forward. There are pending issues/regressions to be fixed, so we have to create a bug per each. Pending issues:
- **Tests are not working**:
 [sms] compose_test.js Message Composition Placeholder "before each" hook:
     TypeError: this._mozMobileMessage is undefined
- Composer input:
	+ Black & blinking text (Issue 1 attachment)
	+ 'setInputHeight' is not working as expected
	+ Layout is broken, and is breaking the App layout as well (not only the composer) (Issue 2 attachment)
- Composer, recipients container:
	+ Height has a 'pull-down' effect while typing. CSS is not working as expected. (Issue 3 attachment)
	+ Header is pushed up, so the layout of the app is broken
- Counter
	+ Counter style completely broken (Issue 4 attachment)
	+ maxSegmentsInfo is not working at all (it was a Tef+ https://bugzilla.mozilla.org/show_bug.cgi?id=865411 that now is reopened)
(Issue 5 attachment)
- Other comments to be addressed:
	+ Remove the bug https://bugzilla.mozilla.org/show_bug.cgi?id=869159 if is not in the code right now.
	+ https://github.com/mozilla-b2g/gaia/pull/9385/files#r4141001. Removing logs.

This is R+ **only** for merging in the dev-branch. All these pending issues should be addressed for considering this US done.

Comment 25

[Dan Heberden]

Created https://bugzilla.mozilla.org/show_bug.cgi?id=870057 for the Composer input bugs

Comment 26

[Corey Frang [:gnarf]]

Greg, FYI there has been some work from dan and I to get units passing again on https://github.com/gnarf37/gaia/compare/840069-squash - can you pick up this new squashed commit as your HEAD on the pull request?

Comment 27

[Borja Salguero [:borjasalguero]]

After testing the branch there are several more issues pending:
- Fixing the unit tests
- Send message & enable send is not working. STR: 
 + Create a recipient '123'
 + Go to input text
 + Tap on 'ENTER' several times
EXPECTED: Send button is not enabled

CURRENTLY: Send button is enabled, and when tapping on it nothing happens. 

Tomorrow morning I will take a look about the bugs that we need to create for fixing all the issues.

Comment 28

[Julien Wajsberg [:julienw]]

please include the commit hash and url here :)

Comment 29

[Corey Frang [:gnarf]]

https://github.com/mozilla-b2g/gaia/commit/081391ea5338351cc9ad5f03786ebdadd5246f67

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train : 513b375