Description: Attaching a video that is too large from the video app prompts the user a confusing error message. The user will be prompted "You've exceeded the maximum length. Remove special characters or shorten the message to send it". Repro Steps: 1) Updated Buri to Build ID: 20131126004001 2) Tap on the "Video" app. 3) Tap on a video that is too large in size send as a MMS. 4) Tap on the "Share" button on the video. 5) Tap on "Messages". Actual: The user is prompted a confusing error message. They may think that they need to delete some of the characters in the video title name to share it. Expected: The user is prompted that the video is too large to share. Environmental Variables Device: Buri v 1.2.0 COM RIL Build ID: 20131126004001 Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/21e2ad082d85 Gaia: 264c6044b941437ac3c4b28fe4ca392d2bc78445 Platform Version: 26.0 RIL Version: 01.02.00.019.102 Notes: Repro frequency: 100% See attached: screenshot, logcat

This issue does not occur on Leo v 1.1.0 COM RIL Environmental Variables Device: Leo v 1.1.0 COM RIL Build ID: 20131024041202 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/e4cb5a852e3d Gaia: 39b0203fa9809052c8c4d4332fef03bbaf0426fc Platform Version: 18.1 RIL Version: 01.01.00.019.264 The user only has the option to share a video via bluetooth through the video app.

Minor, not a blocking issue.

Right, we should change this text now. We used to display it when the user switched the encoding of a SMS and as a result exceeded the max size, but now we switch to MMS in this case. Therefore, the only case this message is displayed is when we attach a too big attachment. The current text is: "You've exceeded the maximum length. Remove special characters or shorten the message to send it." Here is my proposition to change it: "You've exceeded the maximum size. Remove attachments or shorten the message to send it." Tyler, can you please proofread this? :)

Let's simplify the language a bit: "The message is too big. Remove attachments or shorten the text to send it."

Mmm I kept "shorten the message" on purpose in my proposal, because I don't think shortening the text will ever be enough to go below the MMS size limit. Maybe the message should be more centered on attachments, something like: "The message is too big. Please remove or replace attachments to send it."

Ah ok, by "shorten the message" I understood that we were referring to the text. Reading the previous comments and explanation better, I'm unclear on something: Based on the explanation, it sounds like this only happens when a video is attached to a message. But the attached screenshot shows two files attached. Is there a specific size limit that we can simply tell the user? If so, the message should be even simpler: "Messages cannot be bigger than X MB." In what cases is this error thrown, and what can we tell the user about it?

When images are attached, we automatically resize them so that it fits the expected size. But we don't do this for video or music attachments, so the message size can exceed the max size when attaching such attachments. We can actually tell the user the max size, but this would require some code change in addition to the string change, and I don't know if that's that meaningful for the user after all. Ayman, what do you think?

From a communication/clarity standpoint, it is *much* more meaningful to tell the user a specific number that they can clearly and easily compare to the size of the files they've attached. If we only tell them "You're over the limit", we leave them with one option: Blindly try different sizes of files until one, by pure luck, does not throw an error. Imagine writing out a text message and then suddenly seeing a message telling you "The message is too long. Make it shorter." It would be extremely frustrating to have to cut words and letters little by little until the error message finally stops appearing...

But we don't display the current message size. Maybe we need to show both in the error message: the current size and the max size.

If it's easy to show the current size, that wouldn't be bad, but also doesn't seem necessary. I'm assuming the size only includes the message text and the attachments. But relative to the max size, the text is trivial, right? That's to say, if the max size of a message is 5MB, the chances are extremely low that removing a couple words from the text is going to make a difference in regards to the size limit. It seems that it will basically always be a question of attachment size. If that's correct, then by simply telling the user that "Attachments can't exceed 5MB." they will be able to compare their attachment size and quickly know what to change. Some tiny amount of extra space could even be left in the real limit compared to what we tell the users, just to give some space for the text. Are my assumptions correct? Does this seem like a decent solution?

Just for information: the max size is about 300 or 600KB (depending on the countries). Also, we need a shorter message than what we used to have (see bug 946474). So I'd be generally happy with your proposal. But I'd like Ayman to comment here.

passing to jose

(In reply to Julien Wajsberg [:julienw] from comment #10) > But we don't display the current message size. Maybe we need to show both in > the error message: the current size and the max size. We tried this, but it didn't work at all. It becomes very noisy easily. Our proposal is: - If you add an attachment that exceeds the limit or the last file that you add exceeds the total amount: last file must not be attached and an alert message is displayed (instead of using an in-app message) - Following Tyler advice, the message would be: "Sorry, attachments can’t exceed [max-size] per message" Please let me know your thoughts.

Adding a dot at the end of the sentence. Thanks Tyler!

This proposal makes a lot of sense, thanks!

Great to hear that ;)

I am taking up with these issue. Will soon upload the patch.

Kindly assign these issue to me as I am not able to assign it to myself.

Hi ankit, I just assigned the bug to you. Please ask if you have any question, and please say it if you're blocked or don't work on this for any reason, so that we can reassign. Thanks!

Julien I am done with this bug. I ll upload the patch by today itself.

I think this issue is already resolved. The phone on which I am working already supports these. because When I added a 1Mb video then I got an alert("File is to Large") and the file was not attached to the compose part. Comments?

There are 2 ways to add an attachment: * either you're in the "Video" app and you press "share" and then "Messages" * either you're in the "Messages" app, and you press the "attach" button, choose "Video", and choose the video you want to attach One way shows the alert (I think this is done in the Video app) but the other way does not. Anyway we have no check about this in the SMS app so that means that if an external app does not check it, nobody does. This bug is about adding this check to the SMS app for both ways. Is it clearer?

Hey Julien I checked the scenario. Actually what is happening is as follows:- i> First Scenario - I tried to attach a video 1Mb in size through the SMS attach icon & it displays the alert message saying that the file is too big & it didn't attach. ii> Second Scenario - I tried to attach a 290kb video through the message attach icon. It added successfully. Then again I tried to add another video of size 290kb through the message attach icon. These time it displayed the message "You've exceeded the maximum length of characters. Remove any special characters or shorter the message to send it" & the video was appended to the compose. What I feel is this - If the attachment file size is more than the required attachment limit then automatically the alert system gets activated & If the attachment size is below the attachment limit but if total size of all the attachment is more than the required attachment limit then the file is appended to the compose and a toast message "You've exceeded the maximum length of characters. Remove any special characters or shorter the message to send it" is displayed. what to do?

Yep, you're right, what happens is that we send the "max message size" information to the activity, see [1]. But I think this is wrong: I'm not sure we should send this information. It's accurate for video and audio, but not for images, because we're able to resize them. So now it happens that the Gallery and the Camera just ignores this and everything works. But in the same time, Video and Camera _coud_ use it to change their resolution or the length of the video. I think we should be able to send a different information for each requested mime type, but sadly we can't for now. I'll file a bug about this. So here is my proposal: * don't change this line for now * but check this at the activity return in the SMS app. You also didn't test my first scenario: pressing "share" from the Video app. I didn't test yet but I think this shows no warning, because it doesn't even have the max message size information, and this should be fixed in this bug too, using the same check that I asked above. What do you think? [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/compose.js#L562-L564

Julien I ll do it right away & let you know..

Julien I checked for the scenario to share via the video app. I took a 1.9mb file & tried to share it. It was appended to the message & a toast message was displayed saying "You've exceeded the maximum length. Remove any special character or shorten the length to proceed".

Heh, see :) Now, let's implement the dialog error in the app for these cases!

Hey Julien Kindly have a look at the patch. It covers all the cases be it from video app sharing or message app attach button. Still one case is not covered. The case when the user go through the message app-> attach icon-> select video-> file less than 300kb.. repeat these steps.. When the attachment size crosses the maximum size then a toast is displayed & the file is appended to compose.. Instead a alert box should had been raised & the file shouldn't have been added to compose. I tried a lot working on this case too but couldn't succeed as of now. I will try for this case too! However If u suggest any idea about how to go through this case then it would be great! thanks!

Comment on attachment 8358908 [details] [diff] [review] 943976.patch I'm redirecting the request to steve who will handle the feedback and review process. Thanks!

Comment on attachment 8358908 [details] [diff] [review] 943976.patch Review of attachment 8358908 [details] [diff] [review]: ----------------------------------------------------------------- an early feedback: * we need some tests * we need a pull request * we need a good indentation Steve will handle the review. ::: apps/sms/js/attachment.js @@ +147,5 @@ > */ > + if(this.blob.size > Settings.mmsSizeLimitation){ > + alert(navigator.mozL10n.get('file-too-large')); > + return; > + } wrong indentation ::: apps/sms/js/compose.js @@ +410,4 @@ > request.onsuccess = this.append.bind(this); > request.onerror = function(err) { > if (err === 'file too large') { > + alert(navigator.mozL10n.get('file-too-large')); the indentation is wrong, we use spaces and not tabs. @@ +449,4 @@ > }).bind(this); > request.onerror = function(err) { > if (err === 'file too large') { > + alert(navigator.mozL10n.get('file-too-large')); same ::: apps/sms/js/thread_ui.js @@ +396,4 @@ > }, > > messageComposerInputHandler: function thui_messageInputHandler(event) { > + some trailing spaces, please remove them @@ +396,5 @@ > }, > > messageComposerInputHandler: function thui_messageInputHandler(event) { > + > + if(Compose.size > Settings.mmsSizeLimitation){ please add a space after `if` and before `{`. @@ +397,5 @@ > > messageComposerInputHandler: function thui_messageInputHandler(event) { > + > + if(Compose.size > Settings.mmsSizeLimitation){ > + //alert(navigator.mozL10n.get('file-too-large')); please remove useless comments @@ +400,5 @@ > + if(Compose.size > Settings.mmsSizeLimitation){ > + //alert(navigator.mozL10n.get('file-too-large')); > + this.updateCounterForMms(); > + return; > + } please fix the indentation ::: apps/sms/locales/sms.en-US.properties @@ +38,4 @@ > converted-to-mms = Converting to multimedia message. > converted-to-sms = Converting to text message. > messages-max-length-text = You've reached the maximum length. > +messages-exceeded-length-text = You've exceeded the maximum size. Remove attachments or shorten the message to send it. You need to change the l10n key as well when you change the value.

Minor, not a blocker.

thanks Julien For your time.. I ll do it as suggested!

Comment on attachment 8358908 [details] [diff] [review] 943976.patch Review of attachment 8358908 [details] [diff] [review]: ----------------------------------------------------------------- Except for the wrong indentation and unnecessary comment/spaces, please move the file-too-large part to proper place as comment said instead of in attachment.js, thanks. ::: apps/sms/js/attachment.js @@ +147,5 @@ > */ > + if(this.blob.size > Settings.mmsSizeLimitation){ > + alert(navigator.mozL10n.get('file-too-large')); > + return; > + } It seems not the best place for checking the attachment size and return. Since attachment picker part already displayed the file-too-large message, the only case we should handle the file-too-large message is from file sharing. Please add this block in activity_handler.js -> share function instead of here.

As per your request I have modified the patch. Hey Julien = As said to change the L10n key but what should be the keyword for it? I dont think we should add a different keyword as there is no need for it.

(In reply to ankit93040 from comment #39) > Hey Julien = As said to change the L10n key but what should be the keyword > for it? > > I dont think we should add a different keyword as there is no need for it. Hey, I'm not making the rules :) There is no need for it when we read only the code, but there is a need for it because of the l10n rules we have. Adding a new key gives localizers a marker that a new string needs to be localized. Otherwise they have no mean to know this. Hope this is clearer!

Comment on attachment 8360259 [details] [diff] [review] 943976_modified.patch Review of attachment 8360259 [details] [diff] [review]: ----------------------------------------------------------------- As Julien said, we need to change key if the string changes, please 1) change the string for l10n, 2) remove the unnecessary logic and 3) Add a test for activity_handler share changes.(Please ping julien or me if you have no idea about the test) Thanks! ::: apps/sms/js/thread_ui.js @@ +398,5 @@ > messageComposerInputHandler: function thui_messageInputHandler(event) { > + if (Compose.size > Settings.mmsSizeLimitation) { > + this.updateCounterForMms(); > + return; > + } We already called updateCounterForMms inside the messageComposerInputHandler, so there there is no need to check here(and it's not a correct timing to check here because it's only for mms and we should not check it when resizing the image).

Hey Steve Changed the L10n key. Kindly reveiwe! thanks

Comment on attachment 8360875 [details] [diff] [review] 2.patch Review of attachment 8360875 [details] [diff] [review]: ----------------------------------------------------------------- Hi ankit, this patch still lack of test case for the changes in activity_handler.js, which will break current unit test. Please set activity_handler_test.js -> "share activity" suite, you have to: 1) Include mock settings for testing. 2) Add another test with exceed size blob contained in shareActivity 3) Spy the alert function and verify alert is called when blob size exceed the limitation. Please ref MDN[1] for running the unit test in gaia and sinon.js[2] for writing a test case, and feel free to ask any question you faced while writing a test case! [1]https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests [2]http://sinonjs.org/docs/ ::: apps/sms/js/thread_ui.js @@ +870,4 @@ > if (Compose.size > Settings.mmsSizeLimitation) { > Compose.lock = true; > navigator.mozL10n.localize(this.maxLengthNotice.querySelector('p'), > + 'message-max-length'); If you change the key here, please also apply the changes to pass test case here: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/test/unit/thread_ui_test.js#L1057 ::: apps/sms/locales/sms.en-US.properties @@ +39,3 @@ > converted-to-sms = Converting to text message. > messages-max-length-text = You've reached the maximum length. > messages-exceeded-length-text = You've exceeded the maximum length. Remove special characters or shorten the message to send it. Please remove the unused l10n key/string. @@ +39,4 @@ > converted-to-sms = Converting to text message. > messages-max-length-text = You've reached the maximum length. > messages-exceeded-length-text = You've exceeded the maximum length. Remove special characters or shorten the message to send it. > +message-max-length = You've exceeded the maximum size. Remove attachments or shorten the message to send it. nit: maybe 'message-exceeded-max-length' would be better?

Thanks! Steve for the feedback. I ll upload the changes with the Unit tests.

ankit, do you think you'll be able to finish this soon? I think we need to fix this for the MWC Demo.

Also, I'd like to have this on 1.3, so we need to land this on master ASAP so that we can request approval.

Hi julien The patch is already reviewed by you & steve Comment # 43. I don't have proper idea about the test cases. Once that is done I ll create pull request for the same. thanks!

ankit: I think Steve said quite clearly what you had to do in comment 43. Can you tell me what's blocking you ? If necessary we can take over and write the tests ourselves but that would be good that you do it :)

Hi Julien I'll try for the unit test cases. I'll inform you by tomorrow Monday IST 5pm. I'll either inform you about the issue that i faced while writing the unit test cases or will upload the unit test cases as applicable. Attached here is the final patch for which i' ll write the Unit test cases (same as in comment # 43 already reviewed by steveck). thanks!

Comment on attachment 8372962 [details] [diff] [review] latest.patch Don't need an additional review if Steve already did one. Please request review from Steve once the unit tests are done, thanks !

Hey Julien I tried with the Unit test cases but couldn't succeed as in I couldn't understand how to write it. You may proceed with writing of the unit test cases. I ll create the pull request then. thanks!

Steve, can you help Ankit on this? Ayman would like that we fix this for 1.3 ;)

(In reply to Julien Wajsberg [:julienw] from comment #54) > Steve, can you help Ankit on this? Ayman would like that we fix this for 1.3 > ;) Sure, I will add a patch for unit test later.

Hi Julien, here is the unit test changes for this patch. Any feedback about test patch?

Hi ankit, please also add 'Settings' in activity_handler.js global comments (at line #4) for passing jshint checking.

Comment on attachment 8374151 [details] [diff] [review] 0001-unit-test.patch Review of attachment 8374151 [details] [diff] [review]: ----------------------------------------------------------------- mostly nits, but this looks good now we should do a full pull request and land this ! ::: apps/sms/test/unit/activity_handler_test.js @@ +124,3 @@ > } > }; > this.prevAppend = Compose.append; I think you can remove also this, because we use `this.sinon` to change Compose.append, so it's probably a left over. @@ +129,5 @@ > > teardown(function() { > window.location.hash = this.prevHash; > Compose.append = this.prevAppend; > + Settings.mmsSizeLimitation = this.prevLimit; Why do we need this ? the defaut mmsSizeLimitation is set at "mSetup" time by the mock so we shouldn't need to reset it ? @@ +156,5 @@ > }); > > + test('Attachment size over mms limitation should not be appended', > + function() { > + Settings.mmsSizeLimitation = -1; maybe add a comment explaining this because "-1" is usually used to disable something, which is not the case here (I understand you want to trigger the behavior). To make it a little more readable, you can use 5 or 10 here, and make one of the Blob contain something? @@ +163,5 @@ > + > + MockNavigatormozSetMessageHandler.mTrigger('activity', shareActivity); > + sinon.assert.notCalled(Compose.append); > + assert.equal(Mockalert.mLastMessage, > + navigator.mozL10n.get('file-too-large')); we're using the l10n mock here, so you just need to compare with 'file-too-large'.

Here is the unit test patch update. Hi ankit, please commit the latest patch to github including the unit test patch. If you have no idea how to do this or have no time for this, please reply here and we will handle the patch, thanks.

Hi steve I ll do it.. By tomorrow IST(2/14/2014) 12pm it will be done. thanks.

Hi Steve, Please find the updated patch along with the tests. Thanks for writing test cases. Thanks!

Comment on attachment 8376212 [details] Pointer to Pull Request.html Hi ankit, Please apply my latest unit test patch attachment 8375369 [details] [diff] [review] in your commit, and don't forget comment 57(You could refer to my patch -> activity_handler_test.js, the top of the global comment).

Ankit, I saw you left some message on IRC and you got another pull request on github(https://github.com/mozilla-b2g/gaia/pull/16977) with some test failed. It's wierd because the settings test seems apply the mock settings which is loaded in activity_handler test and the original settings.js is ignored. Hi Julien, is this possible a regression of unit test mechenism, or something missed in the unit test?

That's because the "Settings" object is specified twice in the MocksHelper constructor. I opened Bug 980981 and did a PR there to detect this error early, so thanks for doing it in the first place ;)

Hi Steve & Julien Please find here the attached Pull Request. Kindly merge the same to the master. thanks!:)

Comment on attachment 8388339 [details] Pointer to Pull Request.html I'll leave this to Steve who did the last review already.

Comment on attachment 8388339 [details] Pointer to Pull Request.html Only the conflict issue, please set review again once you fix it, thanks!

Hi Steve I didn't find any conflict with the latest master code. Kindly merge the same to the master. thanks!

(In reply to ankit93040 from comment #68) > Created attachment 8390309 [details] > Pointer to Pull Request.html > > Hi Steve > > I didn't find any conflict with the latest master code. > > Kindly merge the same to the master. > > thanks! Could you please try to fetch upstream and rebase/send pull request again? 1) git fetch upstream (if you add the remote mozilla b2g to 'upstream') 2) git rebase upstream/master 3) Fix the conflicts if any. After conflicts fixed, just git add -u and git rebase --continue 4) pit push -f original YOUR_BRANCH_NAME If you have any changes in current branch but you want to keep the commit history clean, you can try to use rebase to squash the unnecessary commit log instead of creating a new branch.

Hi Steve For me still there is no conflict. I tried to create a new pull request & travis failed here:- https://travis-ci.org/mozilla-b2g/gaia/jobs/21084010 I don't think it failed because of our patch. However I am attaching the patch (including everything for this bug).

Comment on attachment 8393422 [details] [diff] [review] 943976.patch Now it's ok to megre to master and these error in Travis should be unrelated. Thanks for the contribution! in master: 16b7f7c8d313917517ec834dbda05db117ec141c

Hey Steve, next time, please take care that the commit log is correct. Also, I'm not sure at all about the change. The added check in activity_handler.js will prevent adding bigger images that would be redimensionned, because we don't resize before calling append... IMO we should not do the check if the attachment is an image. Should we file a follow-up or should we backout?

I can also change this in the rebased version of bug 929027 that I'm doing right now.

Ok, I changed it in bug 929027.

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #72) > Hey Steve, next time, please take care that the commit log is correct. > Hey Julien, sorry I'm not quite sure about your concern about the commit log. Do you mean we should avoid the dash in the log?

Ref: https://github.com/mozilla-b2g/gaia/commit/46671d68e3f19b2a03c3695afa502d380a17b02a The commit log should read: "Bug XXXXXX - title" Here we had the bug number so it's not that bad, but a little more consistency always help :)