1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips.

Status: RESOLVED FIXED

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

Description

[greatwall2686]

* Description: Add video without tip size limit when add message attachments. (add music, pictures did not prompt)


* Reproduction steps:
  1. New a message
  2. Press new message icon
  3. Press add attachment icon
  5. From files choose a video
  6. Check result

* Expected result:
  When choose the file is too large should be a size limit. 

* Actual result:
  Prompt “The file you have selected is too large” but no tip size limit.

* Reproduction build:(Buri - Firefox OS V1.4 outside)
 - Gaia      17b102ee8d9a724b62285547cc5f1c5d30bfb01c
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/95be84248033
 - BuildID   20140520000201
 - Version   30.0

* Reproduction Frequency
  - 10/10

Comment 1

[Julien Wajsberg [:julienw]]

This is an UX issue, so needinfo our UX designers.

Comment 2

[Omega Feng [:Omega] [:馮於懋]]

Is this related to bug 990481?

Comment 3

[Julien Wajsberg [:julienw]]

Omega, I think that this issue is only about the error text that is displayed to the user.

Comment 4

[Jenny Lee]

Error text appears when 
1. attaching a single file that exceeds 300 KB, this error text appears as a dialogue box. 

 Original: The file you have selected is too large.
 Modified: The file you have selected exceeds the 300 KB size limit of a single multimedia message.

2. attaching a file that cause the MMS to exceed size limit, this error text appears as a sticky toast. 

 Original: You've exceeded the maximum size. Remove attachments or shorten the message to send it. 
 Modified: You've exceeded the 300 KB size limit of a single multimedia message. Remove attachments or shorten the message to send it.

Comment 5

[kumar rishav (:rishav_)]

Attachment: Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Hi Julienw
Assuming this a confimed bug.
Here is the attached Patch 
Thanks

Comment 6

[Julien Wajsberg [:julienw]]

Hey Matej, can you please just confirm the texts in comment 4? Thanks !

Comment 7

[kumar rishav (:rishav_)]

Yeah...one more thing.
For case 1..
Is it only for this
**Original: The file you have selected is too large
or for this also
**Original: The files you have selected are too large.

Thanks

Comment 8

[Julien Wajsberg [:julienw]]

Hey Jenny, I just want to confirm with you: we use the "configured limit - 5KB" because there is an overhead with MMS. That means the default limit is 295KB instead of 300. Would you like that we display 295 or 300 in the error message?

Comment 9

[Julien Wajsberg [:julienw]]

Jenny, NI also for comment 7 :) when sharing several images from the Gallery app.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

* You need to change the l10n key when you change the value.
* You need to take the limit value stored in Settings
* You missed some texts

Thank you :)

Comment 11

[Matej Novak [:matej]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Hey Matej, can you please just confirm the texts in comment 4? Thanks !

+1

Thanks!

Comment 12

[Jenny Lee]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Hey Jenny, I just want to confirm with you: we use the "configured limit -
> 5KB" because there is an overhead with MMS. That means the default limit is
> 295KB instead of 300. Would you like that we display 295 or 300 in the error
> message?

Hi there! Although it's a little weird to show 295KB, I still think it's better to reflect the actual size limit, so let's go with 295KB. Thanks!

Comment 13

[Jenny Lee]

(In reply to kumar rishav (:rishav_) from comment #7)
> Yeah...one more thing.
> For case 1..
> Is it only for this
> **Original: The file you have selected is too large
> or for this also
> **Original: The files you have selected are too large.
> 
> Thanks

The message should also include multiple selection, let's simply add a "(s)" to the original error message:
"The file(s) you have selected exceeds the 300 KB size limit of a single multimedia message."

Thanks!

Comment 14

[kumar rishav (:rishav_)]

Hi Julien
Is it necessary to change the l10n key when we change the value? I asked this because i found existing one is fine. Say.. for L10n key : "files-too-large"   or "message-exceeded-max-length" . 
I didn't find a better L10n key.Please suggest some.
Thanks

Comment 15

[kumar rishav (:rishav_)]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Hi Julien
Updated the PR. Have a look.
Thanks

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Jenny Lee from comment #13)
> (In reply to kumar rishav (:rishav_) from comment #7)
> > Yeah...one more thing.
> > For case 1..
> > Is it only for this
> > **Original: The file you have selected is too large
> > or for this also
> > **Original: The files you have selected are too large.
> > 
> > Thanks
> 
> The message should also include multiple selection, let's simply add a "(s)"
> to the original error message:
> "The file(s) you have selected exceeds the 300 KB size limit of a single
> multimedia message."
> 
> Thanks!

We don't need to add "(s)", we can change the message depending on the number of files (we already do this presently) :)

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

some minimal nits :)

please ask review when ready.

thanks again!

Comment 18

[kumar rishav (:rishav_)]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Hi Julien
Updated the PR. Small nits are done. Hope it's okay now :)
Thanks

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Some more nits, sorry about this :)

Comment 20

[kumar rishav (:rishav_)]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Hi Julienw
Hope it's ok now.
Thanks

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Please remove the unused "if" block in showMaxLengthNotice :)

And then please ask review to Steve when you're ready!

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Hi, kumar, if we want to display number of max size, I think it might be better with your patch in bug 1014234 that could handle the unit properly. And the rest of part looks good, thanks!

Comment 23

[kumar rishav (:rishav_)]

Hi Steve
Discussed with julien and planned to do this separately. For this bug, leave it like this. For size i will file separate bug (a generic function which will return size and unit), and will do modification for bug 1014234 and bug 1018111 in this bug. 
What you say steve?

Thanks

Comment 24

[Steve Chung [:steveck]]

Sorry for the late reply, I'm fine with leaving the file size unit to follow up(bug 1113480). Just one nit about the file size: It seems not necessary to display the decimal in the dialog(comment 13 didn't display decimal either). So still keep the review since the fixing should be really quick.

Comment 25

[kumar rishav (:rishav_)]

Done.

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8505149 [details] [review]
Bug 1018111 - [Flame][v1.4][MMS] Edit text messages, add video no size limit tips

Just one nit and overall looks good, thanks.

Comment 27

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/d733b56f2bc857ed84a0411f3da0d6c5a70c0b73

Comment 28

[Francesco Lodolo [:flod]]

Strings in this bug landed with poor English (easy to miss with copy and paste)

When there are multiple "files", they "exceed", not "exceeds". No need to change the string ID to fix this, I guess someone could just land a quick fix without filing a new bug. Otherwise let me know and I'll file a follow-up bug.
One more nit: I know that [zero] is never used, but technically it's plural, not singular ("zero files, not zero file", that would be French or Portuguese).

Comment 29

[kumar rishav (:rishav_)]

Hi Flod
these nits(exceed in place of exceeds for multiple files and Zero files ) will land in Bug 1113480.
Hope it's okay now?
Thanks

Comment 30

[Francesco Lodolo [:flod]]

Yes