Closed Bug 1018111 Opened 7 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

(Reporter: greatwall2686, Assigned: rishav_, Mentored)

References

Details

(Whiteboard: [sms-papercuts])

Attachments

(1 file)

* 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
This is an UX issue, so needinfo our UX designers.
Flags: needinfo?(ofeng)
Flags: needinfo?(jelee)
Is this related to bug 990481?
Flags: needinfo?(ofeng)
Omega, I think that this issue is only about the error text that is displayed to the user.
Flags: needinfo?(ofeng)
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.
Flags: needinfo?(ofeng)
Flags: needinfo?(jelee)
Summary: [Flame][V1.4][MMS] Edit text messages, add video no size limit tips. → [Flame][v1.4][MMS] Edit text messages, add video no size limit tips.
Whiteboard: [sms-papercuts]
Hi Julienw
Assuming this a confimed bug.
Here is the attached Patch 
Thanks
Assignee: nobody → rishav006
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8505149 - Flags: review?(felash)
Hey Matej, can you please just confirm the texts in comment 4? Thanks !
Flags: needinfo?(Mnovak)
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
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?
Flags: needinfo?(jelee)
Jenny, NI also for comment 7 :) when sharing several images from the Gallery app.
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 :)
Attachment #8505149 - Flags: review?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #6)
> Hey Matej, can you please just confirm the texts in comment 4? Thanks !

+1

Thanks!
Flags: needinfo?(Mnovak)
(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!
Flags: needinfo?(jelee)
(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!
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
Flags: needinfo?(felash)
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
Flags: needinfo?(felash)
Attachment #8505149 - Flags: review?(felash)
(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 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!
Attachment #8505149 - Flags: review?(felash)
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
Attachment #8505149 - Flags: review?(felash)
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 :)
Attachment #8505149 - Flags: review?(felash)
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
Attachment #8505149 - Flags: review?(felash)
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!
Attachment #8505149 - Flags: review?(felash)
Mentor: schung
Attachment #8505149 - Flags: review?(schung)
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!
Attachment #8505149 - Flags: review?(schung)
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
Attachment #8505149 - Flags: review?(schung)
Blocks: 1113480
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.
Done.
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.
Attachment #8505149 - Flags: review?(schung) → review+
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/d733b56f2bc857ed84a0411f3da0d6c5a70c0b73
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S3 (9jan)
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).
Flags: needinfo?(schung)
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
Flags: needinfo?(francesco.lodolo)
Yes
Flags: needinfo?(schung)
Flags: needinfo?(francesco.lodolo)
Depends on: 1134178
You need to log in before you can comment on or make changes to this bug.