1026384 - [MMS]Display switching of SMS/MMS composition screen

Status: RESOLVED FIXED

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

Description

[Naoya Matsumoto]

[User story]
1. Run message app
2. Create new messge
3. Type email adress as a recipient or attach the file.
4. Display MMS composition screen.

Comment 1

[Naoya Matsumoto]

This bug is derived from the following.
https://github.com/mozilla-b2g/gaia/pull/20439/files#r13754745

Comment 2

[Julien Wajsberg [:julienw]]

This is necessary for the 'send MMS to email' feature.

Comment 3

[Naoya Matsumoto]

Attachment: Display switching of SMS/MMS composition screen patch on github

Comment 4

[Naoya Matsumoto]

Hi, Julien and Steve.
I PR Attachment 8450883 [details].
Please review it.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8450883 [details] [review]
Display switching of SMS/MMS composition screen patch on github

This won"t work properly, we need something more involved in how we switch to 'mms'.

Basically, we want to be in 'mms' when (each one of these items are OR):
1. we have an attachment
2 [review]. we have a subject displayed and it's not empty
3. we have a too big text
4. we have an email as recipient (this is the new one)

Currently, 1. and 2. are done in Compose directly in 'updateType'.
3. is done in ThreadUI.onMessageTypeChange and ThreadUI.updateCounterForSms.

TBH I think some of the things we do in ThreadUI (and 3. is part of it) should move to Compose instead. But that's a quite big refactoring that maybe you don't want to do yourself.

One possible solution is to have a "contribution" model (Compose would request for 1. 2. 3. and 4. from the correct objects to set the type). But maybe this is overkill for what we really need here, so let me think about it over the weekend (and so I'm keeping the needinfo).

Comment 6

[Naoya Matsumoto]

Hi, Julien. Thanks for your review.
I see.
Amendment, such as the following how might it be?

 Recipients.prototype.checkmessagetype = function() {
     var list = data.get(this);
 
     for (var i = 0; i < list.length; i++) {
       if (Utils.isEmailAddress(list[i].number)) {
         Compose.type = 'mms';
         return this;
       }
     }
     Compose.updateType();
     ThreadUI.onMessageTypeChange();
     ThreadUI.updateCounterForSms();
     
     Compose.type = 'sms';
}

Comment 7

[Julien Wajsberg [:julienw]]

I gave some thought, and if you don't mind, I'd like that we do bug 944249 before this. We took it in the coming sprint and I'll try to start with it so that you don't stay blocked.

Does it work for you?

Comment 8

[Naoya Matsumoto]

There are no problems for me.
But I have concerned about who will fix Bug 944249.

Comment 9

[Julien Wajsberg [:julienw]]

Someone from Mozilla team (probably me, but maybe someone else).

Comment 10

[Naoya Matsumoto]

Attachment: Display switching of SMS/MMS composition screen patch on github

Comment 11

[Naoya Matsumoto]

Please review it.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8489959 [details] [review]
Display switching of SMS/MMS composition screen patch on github

I left some comments for you while I fixed bug 944249. Just look for "Bug 1026384" in the files. I think it should be easy to fix like I suggested in the comment.

Please also add unit tests.

Thanks !

Comment 13

[Naoya Matsumoto]

Thank you for your comment.
I have corrected it. Please review again.
But unit test(thread_ui_test.js) is error before I modify.

Comment 14

[Julien Wajsberg [:julienw]]

Naoya, what should I review? I see the previous PR is closed.

Can you attach the PR I should review to this bug and add the "review?" flag on the attachment? Thanks!

Comment 15

[Naoya Matsumoto]

Attachment: Display switching of SMS/MMS composition screen patch on github

Comment 16

[Naoya Matsumoto]

I'm sorry. I have replaced the attachment file.
Please review again

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8491436 [details] [review]
Display switching of SMS/MMS composition screen patch on github

You didn't apply all the comments of my previous review, please do.

Also, I still need unit tests, please. I checked myself that compose_test.js has no failure on master currently so you should not have any issue.

Please request review once you're ready and applied all the comments. Thanks!

Comment 18

[Salvador de la Puente González [:salva]]

Naoya, I was asked to help with bug 840515. I have a ready patch for this bug. Do you mind if I take it?

Comment 19

[Naoya Matsumoto]

Thank you Salvado.
Could you fix for this bug?

Comment 20

[Salvador de la Puente González [:salva]]

For sure. Taking the bug.

Comment 21

[Salvador de la Puente González [:salva]]

Attachment: Now listening to recipientschange for updating when an e-mail among the recipients.

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 8495752 [details] [review]
Now listening to recipientschange for updating when an e-mail among  the recipients.

I didn't test on the device yet so I'm keeping the r request. But you have some nits to fix + linter and unit tests failures.

Thanks :)

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: 2014-09-29-20-37-02.png

Hey Jenny, Fang, there is an issue when the user adds an email recipient and no text is added in composer yet.

In that case, we display "MMS" ahead of the send button, and this makes the composer bigger. Can you have a look to this picture and tell us if it's fine or if we should change it, and if yes, how we should change it?

Thanks !

Comment 24

[Julien Wajsberg [:julienw]]

Comment on attachment 8495752 [details] [review]
Now listening to recipientschange for updating when an e-mail among  the recipients.

Looks good, except for the issue I commented about, and the nits on github.

Thanks a lot for taking care of this!

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 8495752 [details] [review]
Now listening to recipientschange for updating when an e-mail among  the recipients.

oops, it was a feedback+ ;)

Comment 26

[Jenny Lee]

Comment on attachment 8497002 [details]
2014-09-29-20-37-02.png

Hi Julien, 
Fang will try a few layout and give feedback. Thanks!

Comment 27

[Fang Shih [:fang]]

Comment on attachment 8497002 [details]
2014-09-29-20-37-02.png

Hi Julien, 
After I tried few layouts, I think we should move MMS above of the composer. So the message can still align with send button and attachment icon, also it won't leave a big gap after the message moved down. 

Thanks!

Comment 28

[Fang Shih [:fang]]

Attachment: MMS_Spec.png

Attached the visual image. Hope it works for you. : ) Thanks!

Comment 29

[Julien Wajsberg [:julienw]]

Fang, there is a problem with your proposal: how does it work inside a thread ? the "MMS" text will be on top of messages :/

Comment 30

[Julien Wajsberg [:julienw]]

And there's also the case where the composer is as big as possible, in that case the top of the composer is adjacent to the bottom of the recipient panel.

Comment 31

[Fang Shih [:fang]]

Attachment: MMS.png

(In reply to Julien Wajsberg [:julienw] from comment #29)
> Fang, there is a problem with your proposal: how does it work inside a
> thread ? the "MMS" text will be on top of messages :/

Hi Julien, Thanks for mentioning. I've thought about that, so I was thinking to use the same method like "Subject MMS", give a height to it to avoid the problem of overlapping. 

And for the composer, we can also use the method. So the composer should have limited height when the message is expanded. 
Hope it works for you. Thanks!

Comment 32

[Julien Wajsberg [:julienw]]

Fang, so, let me summarize: we should always show the "MMS" indicator in the same place (with or without subject), and when we're in MMS mode we should always show the subject part?

Comment 33

[Julien Wajsberg [:julienw]]

NI Fang for comment 32.

Also NI Jenny because some UX is changed and I want to be sure she agrees with this.
Also how should behave the "add/remove subject" item when we're in MMS mode then?

Comment 34

[Salvador de la Puente González [:salva]]

(In reply to Julien Wajsberg [:julienw] from comment #32)
> Fang, so, let me summarize: we should always show the "MMS" indicator in the
> same place (with or without subject), and when we're in MMS mode we should
> always show the subject part?

Notice this is happening even when simply attaching a picture. I would recommend another different bug for this. WDYT?

Comment 35

[Salvador de la Puente González [:salva]]

Attachment: MMS mode when picture attached

Comment 36

[Salvador de la Puente González [:salva]]

Comment on attachment 8495752 [details] [review]
Now listening to recipientschange for updating when an e-mail among  the recipients.

Do you mind to review it again? Thank you!

Comment 37

[Julien Wajsberg [:julienw]]

(In reply to Salvador de la Puente González [:salva] from comment #34)
> (In reply to Julien Wajsberg [:julienw] from comment #32)
> > Fang, so, let me summarize: we should always show the "MMS" indicator in the
> > same place (with or without subject), and when we're in MMS mode we should
> > always show the subject part?
> 
> Notice this is happening even when simply attaching a picture. I would
> recommend another different bug for this. WDYT?

In that case, this is the (former) expected behavior.

I definitely agree to do this in another bug, but we can't enable "send MMS by email" before doing the change, otherwise this will look broken.

Comment 38

[Julien Wajsberg [:julienw]]

Comment on attachment 8495752 [details] [review]
Now listening to recipientschange for updating when an e-mail among  the recipients.

Redirecting to Steve as I'm away for a few days.

Hi Steve, to test the patch, you'll need to set Settings.supportEmailRecipient to true before flashing. You'll see we have tests that test some functions both with/without the feature. We need to still have these tests testing both cases until we enable the feature fully, so please take care that it is the case here.

As I said, we'll also need to figure out what to do with the MMS indicator before enabling the feature...

Comment 39

[Jenny Lee]

(In reply to Julien Wajsberg [:julienw] (PTO until 7th October, ask :steveck or :azasypkin for SMS stuff) from comment #33)
> NI Fang for comment 32.
> 
> Also NI Jenny because some UX is changed and I want to be sure she agrees
> with this.
> Also how should behave the "add/remove subject" item when we're in MMS mode
> then?

Hello Julien,

Looking at the screenshot you attached, we feel that the empty second line with the attachment button at the start really gives a broken visual experience. Fang and I discussed about it and decided to move the string and always show "MMS" at the end of subject line. Having a fixed position for the string can also avoid visual inconsistency that sometimes it appears in composer area but other times outside of it.
"add/remove subject" should behave the same way, the difference between with and without subject would be the appearance of "subject" placeholder. Thanks!

Comment 40

[Steve Chung [:steveck]]

Comment on attachment 8495752 [details] [review]
Now listening to recipientschange for updating when an e-mail among  the recipients.

Looks fine, but please create another bug for addressing the MMS label issue with solution in comment 39(so you will need to copy the UX suggestion to new bug's description) and set the bug as blocker to email meta bug. Thanks!

Comment 41

[Salvador de la Puente González [:salva]]

master: 700fd2a191bf270ac3e91848425b352b430a4fee