Closed Bug 929027 Opened 6 years ago Closed 6 years ago

[Messages][Gallery] The option "Share with: Messages" are missing, when more than one file is selected

Categories

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

x86
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: sarsenyev, Assigned: sasikala.paruchuri8)

References

Details

(Whiteboard: [mentor=schung][ready-to-land-after-1.4-branching] [m+])

Attachments

(4 files, 12 obsolete files)

91.93 KB, image/png
Details
85.37 KB, image/png
Details
46 bytes, text/x-github-pull-request
steveck
: review+
Details | Review
46 bytes, text/x-github-pull-request
steveck
: review+
flod
: feedback+
Details | Review
Description:
When choosing to share multiple files from the "Gallery" or from the "Video" library, the "Share with message" option is missing when sharing multiple files

Prerequisites: Multiple image or video files in "Library"

Repro Steps:
1) Updated Buri to BuildID: 20131021004006
2) Open the "Gallery"
3) Tap the "Edit" icon 
4) Select more than one image 
5) Tap the "Share" button

Actual:
"Share with message" bar is missing

Expected:
"Share with message" bar is presents along with other sharing options

Environmental Variables:
Device:  Buri v1.2 Aurora Mozilla RIL)
BuildID: 20131021004006
Gaia: 1fd315337d8ae891c3024e4c682c4c50797ea40e
Gecko: d585fe28cd55
Version: 26.0a2
Firmware Version: US_20130930
Notes:

Repro frequency: 100%

See attached: (screenshot
Attached image Issue(no SMS)
Attached image Expected(with SMS)
blocking-b2g: --- → koi?
Component: Gaia::Video → Gaia
QA Contact: nkot
regression range:

-last working build-
Gaia   59c7f4a59157a0bc1c45d705294395f988c509b2
SourceStamp 76820c6dff7b
BuildID 20130624031223
Version 24.0a1

-first broken build-
Gaia   a8252a4a096431ca7f39b0ce9307957945f77075
SourceStamp c5ce065936fa
BuildID 20130629031257
Version 25.0a1

//there are no release builds between 6/24-6/29 to get a deeper regression range
koi+ as this feature has worked in the past - sharing multiple files.
blocking-b2g: koi? → koi+
here are my findings: 

This limitation to not allow to show option 'share with messages' on selecting multiple files is set in sms manifest , https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/manifest.webapp#L57 with fix of bug 873758 https://github.com/mozilla-b2g/gaia/pull/9854#issuecomment-18225028

Removing  "number": 1 from sms app manifest, started showing the option but not sure if sms app can handle all flows of sharing multiple images/videos. 

Steve, is 929027 a bug or it's intentional? please provide your inputs. Thanks
Flags: needinfo?(schung)
(In reply to Punam Dahiya from comment #5)
> here are my findings: 
> 
> This limitation to not allow to show option 'share with messages' on
> selecting multiple files is set in sms manifest ,
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/manifest.webapp#L57
> with fix of bug 873758
> https://github.com/mozilla-b2g/gaia/pull/9854#issuecomment-18225028
> 
> Removing  "number": 1 from sms app manifest, started showing the option but
> not sure if sms app can handle all flows of sharing multiple images/videos. 
> 
> Steve, is 929027 a bug or it's intentional? please provide your inputs.
> Thanks

It's not a regression. This mms limitation is always there from v1.1

Yes I limited the picked item number for some reason: mms total attachments size has limitation and it only around 300k ~ 600k, it's very easy to exceed the limitation if we allow multiple selection and sending action will be blocked. Another limitation in mms image sharing only allow at most 5 images per message for sending(in order to keep image quality). User have to delete the attached items one by one in order to fit the limitation(because we didn't show the actual size limitation in warning currently).
Image resizing for multiple images is another potential problem because resizing them all at once will have a serious OOM issue, but it is time-wasting if we doing this in sequence. Considering all these problem, that's why we only allow sharing single item for mms.

I suggest we move this feature to 1.3 or later because it's not a blocking issue nor regression. We defiantly need more ux input for multiple file sharing in mms, and need more time to verify the memory usage issue/use experience for multiple image sharing part, thanks.
blocking-b2g: koi+ → 1.3?
Flags: needinfo?(schung)
We don't need to block on this for now, so I'm going to pull the blocking flag generally then. We should notify product to add this to the product backlog however.

needinfo on Wilfred to add this to the comms backlog
blocking-b2g: 1.3? → ---
Component: Gaia → Gaia::SMS
Flags: needinfo?(wmathanaraj)
Keywords: regression
Wilfred mentioned over email he is going to add this to the product backlog.
Flags: needinfo?(wmathanaraj)
confirmed - i will add to backlog; we wont be able to support this in v1.3; we will discuss for v1.4 as a possible target to extend multiple attachments to MMS.
I mark this as a mentoring bug, because I don't think this is difficult with our current code.
Whiteboard: [mentor=:julienw]
Duplicate of this bug: 905480
Duplicate of this bug: 897837
Hi Julienw,

1.We tried removing the Restriction of sharing single image from Gallery inorder to show "share with:Messages" option when more than one file is selected.
  Code Changes: Removed "number":1 (restriction) in manifest.webapp

2.In MMS application we are accepting maximum of five images.
  Code Changes: "number":[1,2,3,4,5] in manifest.webapp

3.We modified the code in Messaging application to accept the multiple images.
  Code Changes:
  a. Added array in share function and pusing all the attachments to an array
  b. Added new function(appendAll) to perform the resizing operation for each image 

4.Currently this is working fine in Leo device. Will be doing more testing on this.

Is it okay to continue this way?

Thanks,
Flags: needinfo?(felash)
Hey sasikala,

Sounds good.

Some comments:
about: 3.b: why can't we do this in `append`? We can make "insert" handle an array parameter, right?

Also, we need to take care to do the resizing operation once.

The current behavior is: when we add a new image attachment, we resize all existing attachments. Therefore, if we keep the same behavior, we'll probably do the resizing several times.

So you really need to take care to do the resizing once for all. I don't exactly know the best way to do this with the current code, but again, the best is maybe to reuse the existing functions (append, insert, onContentChanged), and make them handle array parameters.

Hope this helps
Assignee: nobody → sasikala.paruchuri8
Flags: needinfo?(felash)
Hi Julienw,

Thanks for the Comments.

We added appendAll function inorder to avoid the multiple resize case.
We are checking the code again and will modify based on that.
Hi Julienw,

We tried to implement the multiple image sharing case in append function,but the following are some problems.

1.The entry point for the following two cases are different

  case 1) Sharing images from Gallery
     When sharing an images from the Gallery,we will receive as an array of blobs from Gallery.
	 
  case 2) Selecting images from Gallery to compose screen
     Selecting an attachment button from the MMS compose screen and adding an attachment is handled by   using pickactivity where we will receive a blob from Gallery.
	 
2.It's difficult to handle two cases in append() function as we will get two different inputs for both the cases.

So,the better way is to handle the Multiple image sharing case by adding new(appendAll) function

Please provide your opinion on this.

Thanks,
Sasikala
Please check Comment 16 and provide your comments.
Flags: needinfo?(felash)
Please do first with appendAll and we'll see how we can do better, if this is possible :)
Flags: needinfo?(felash)
These is a work in progress patch on behalf of sasikala.
Attachment #8355143 - Flags: feedback?(felash)
Depends on: 854795
Attached file Pointer to Pull Request.html (obsolete) —
Would you please review the attached pull request.
Attachment #8356064 - Flags: review?(felash)
Blocks: 958307
Attached file Pointer to Pull Request.html (obsolete) —
Hi,

There was a problem with the previous pull request(New function was not added).
Please consider this pull request.

Thanks
Attachment #8359063 - Flags: review?(felash)
Attachment #8356064 - Attachment is obsolete: true
Attachment #8356064 - Flags: review?(felash)
Attachment #8355143 - Attachment is obsolete: true
Attachment #8355143 - Flags: feedback?(felash)
Comment on attachment 8359063 [details]
Pointer to Pull Request.html

I did a small preliminary review.

There is especially too much copy/paste in this code.

Moreover, this does not prevent multiple resize operation from taking place in the same time.

Rick, can you please move on with this review ? Probably you'll need an updated patch but you can have a look already .

Sasikala, ankit, please request review from Rick for the next updates.

Thanks !
Attachment #8359063 - Flags: review?(waldron.rick)
Attachment #8359063 - Flags: review?(felash)
Attachment #8359063 - Flags: review-
Attached patch Modified patch (obsolete) — Splinter Review
Hi Rick,

I have modified some changes as per the Julien preliminary review comments.
1. Modified the limitation of allowing 5 images for MMS using 
   "number": {
     "max": 5
   }

2. Removed the previous check(if (arr.length == numbers) {) ) which was added in pull request which is not necessary.

3. Code for appendAll() function is same as existing append() function. So,i need help in remodifying the appendAll() function.

Thanks,
Sasikala
Attachment #8362426 - Flags: review?(waldron.rick)
We have tested this patch which is working fine without any crash but as per the review comments need help in modifying the changes
Comment on attachment 8362426 [details] [diff] [review]
Modified patch

Review of attachment 8362426 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/sms/js/activity_handler.js
@@ +87,1 @@
>        var blobs = activity.source.data.blobs,

Why the new array if activity.source.data.blobs is already an array and we're already iterating that array, why do we need the additional operation?

@@ +102,2 @@
>          });
> +        ThreadUI.cleanFields(true);

Is it not

@@ +102,5 @@
>          });
> +        ThreadUI.cleanFields(true);
> +        setTimeout(function() {
> +          Compose.appendAll(arr);
> +        },1000);

Why the whole 1 second delay?

::: apps/sms/js/compose.js
@@ +391,4 @@
>        return this;
>      },
>  
> +    appendAll: function(item) {

if this accepts an array, please make `item` => `items`

@@ +393,5 @@
>  
> +    appendAll: function(item) {
> +      var self = this;
> +      var lastItem;
> +      Array.forEach(item,function getMulti_attachments(value) {

Please do not use the non-standard static Array generics. Use: `items.forEach(...)`

@@ +412,5 @@
> +          self.scrollToTarget(dom.message.lastChild);
> +        }
> +        lastItem = value;
> +      });
> +      onContentChanged(lastItem);

I don't understand why you can't just call `Compose.append(value)`?
Attachment #8362426 - Flags: review?(waldron.rick) → review-
sasikala, can you put the patch in github please?
Summary: [B2G][Gallery] The option "Share with: Messages" are missing, when more than one file is selected → [Messages][Gallery] The option "Share with: Messages" are missing, when more than one file is selected
(In reply to Rick Waldron [:rwaldron] from comment #26)
Will modify the changes and add the pull request
> sasikala, can you put the patch in github please?
Attached file Pointer to Pull Request (obsolete) —
Hi Rick,

Please find the pull request with modified changes

1.Added local array to avoid the crashes happening when attaching multiple images(crashing when attaching more than 2 images)
2.When shared five images from Gallery -> without sending -> Click on home button -> share five more images 
 In this case inorder to clean old set of images and for replacing new set of  images added ThreadUI.cleanFields(true);
3.Added setTimeout() function inorder to avoid the delay and crash happening(This delay and crash is due to asynchronous calls in append() function)

Please review and provide your feedback on this
Thanks!
Attachment #8359063 - Attachment is obsolete: true
Attachment #8362426 - Attachment is obsolete: true
Attachment #8359063 - Flags: review?(waldron.rick)
Attachment #8362894 - Flags: feedback?(waldron.rick)
(In reply to sasikala from comment #28)
> Created attachment 8362894 [details]
> Pointer to Pull Request
> 
> Hi Rick,
> 
> Please find the pull request with modified changes
> 
> 1.Added local array to avoid the crashes happening when attaching multiple
> images(crashing when attaching more than 2 images)
> 2.When shared five images from Gallery -> without sending -> Click on home
> button -> share five more images 
>  In this case inorder to clean old set of images and for replacing new set
> of  images added ThreadUI.cleanFields(true);
> 3.Added setTimeout() function inorder to avoid the delay and crash
> happening(This delay and crash is due to asynchronous calls in append()
> function)

I still experience a crash with 5 attachments.
Hi Rick & Steve,

Please find the below comments for the changes

In existing Implementation:
(a) share function in activity_handler.js
 1. Only allowed sharing one item in a single action now.

(b)append() and imageAttachmentsHandling() in compose.js
 1. These two functions handling the received images concurrently

In Proposed Implementation:
(a)manifest.webapp
 Modified the code inorder to allow maximum of 5 images while sharing from  Gallery for MMS

(b)share function in activity_handler.js
 1. Calling Compose.append() in forEach may cause crash due to concurrent call
 2. To avoid the concurrent call (inorder to avoid crash),created new array, pushing all the attachments and finally calling append() by passing array.
 3.Added ThreadUI.cleanFields(true); inorder to replace the existing set of images when new set images are shared from Gallery, if the compose area already have images attached without sending.

(c)append() function in compose.js
 Modified the code to handle two cases
 1.Handling the received array of attachments and storing in one variable -> passing the last attachment to onContentChanged
 2.Handling the single attachment,during single attachment we wont receive an array,So no need of handling forEach.
 3.Moved the common code in both the cases to new function insertItem() to avoid same code.

(d) imageAttachmentsHandling() in compose.js
 1.Added resizedImg() inorder to make the process sequential

Tested the code,there is no crash happening.
We will write unit tests once the code is freezed.It would appreciate if the patch is reviewed and provide the comments

Thanks!
Attachment #8367222 - Flags: feedback?(waldron.rick)
Attachment #8367222 - Flags: feedback?(schung)
Note that the crashes might have been fixed by bug 959422 as well.

Sasikala, I thought of something. In the current code, when we have, say, 2 attached images that fills the available space (say: 300KB), and we want to attach another one, then we have a process that recompresses all images to make them smaller.

How does it work in your current code? Are we adding image 1, waiting, then image 2, possibly recompressing image 1, then image 3, possibly recompressing image 1 and 2, etc? I think we should try to compress them all only once, but I don't know how easy this is...
(In reply to Julien Wajsberg [:julienw] from comment #31)
> Note that the crashes might have been fixed by bug 959422 as well.
> 
We are using the latest code where bug 959422 changes are existing in the code.I have tested the code without our changes and only having the changes from manifest for sharing multiple images,Crash is happening(this might be due to concurrent call)

> Sasikala, I thought of something. In the current code, when we have, say, 2
> attached images that fills the available space (say: 300KB), and we want to
> attach another one, then we have a process that recompresses all images to
> make them smaller.
> 
> How does it work in your current code? Are we adding image 1, waiting, then
> image 2, possibly recompressing image 1, then image 3, possibly
> recompressing image 1 and 2, etc? I think we should try to compress them all
> only once, but I don't know how easy this is...

In Our proposed Implementation:
 1. We are resizing each image one after the other(Sequentially) but each image is resized only once

Thanks!
Attached file Pointer to Pull Request (obsolete) —
Hi,
Hi Rick & Steve,

Please find the below comments for the changes

In existing Implementation:
(a) share function in activity_handler.js
 1. Only allowed sharing one item in a single action now.

(b)append() and imageAttachmentsHandling() in compose.js
 1. These two functions handling the received images concurrently

In Proposed Implementation:
(a)manifest.webapp
 Modified the code inorder to allow maximum of 5 images while sharing from  Gallery for MMS

(b)share function in activity_handler.js
 1. Calling Compose.append() in forEach may cause crash due to concurrent call
 2. To avoid the concurrent call (inorder to avoid crash),created new array, pushing all the attachments and finally calling append() by passing array.
 3.Added ThreadUI.cleanFields(true); inorder to replace the existing set of images when new set images are shared from Gallery, if the compose area already have images attached without sending.

(c)append() function in compose.js
 Modified the code to handle two cases
 1.Handling the received array of attachments and storing in one variable -> passing the last attachment to onContentChanged
 2.Handling the single attachment,during single attachment we wont receive an array,So no need of handling forEach.
 3.Moved the common code in both the cases to new function insertItem() to avoid same code.

(d) imageAttachmentsHandling() in compose.js
 1.Added resizedImg() inorder to make the process sequential

Tested the code,there is no crash happening.
Please review the patch and provide your feedback.

Thanks!
Attachment #8362894 - Attachment is obsolete: true
Attachment #8367222 - Attachment is obsolete: true
Attachment #8362894 - Flags: feedback?(waldron.rick)
Attachment #8367222 - Flags: feedback?(waldron.rick)
Attachment #8367222 - Flags: feedback?(schung)
Attachment #8369034 - Flags: feedback?(waldron.rick)
Attachment #8369034 - Flags: feedback?(schung)
Attachment #8369034 - Flags: feedback?(felash)
Comment on attachment 8369034 [details]
Pointer to Pull Request

I give some feedback on github. If I don't get it wrong, onContentChanged will only triggered for the last appended item, which I think could be improved to be more robust for mixed type case.
One more minor thing about the resizing is this solution might produce different result than adding the images one by one. It won't break the functionality, so I think this differentiation might be acceptable? And please add some comments in imageAttachmentsHandling about why we don't resize the image parallel for saving more time.
Attachment #8369034 - Flags: feedback?(schung)
(In reply to Steve Chung [:steveck] from comment #34)
> Comment on attachment 8369034 [details]
> Pointer to Pull Request
> 
> I give some feedback on github. If I don't get it wrong, onContentChanged
> will only triggered for the last appended item, which I think could be
> improved to be more robust for mixed type case.
> One more minor thing about the resizing is this solution might produce
> different result than adding the images one by one. It won't break the
> functionality, so I think this differentiation might be acceptable? And
> please add some comments in imageAttachmentsHandling about why we don't
> resize the image parallel for saving more time.

Hi Steve,
Thank you for feedback on the changes.
I have replied on github for each comment. Please check and provide your opinion.
Thanks!
About imageAttachmentsHandling:
1.We are not handling any multipart/mixed in SMS application.Is there a plan to implement this in future.
2.We are not resizing the image in parallel because due to this crash is happening
Thanks!
Hi Steve,

I have modified the patch as per the review comments.
1.Moved the resizedImg(imgNodes[done]); after function declared
2.Removed done check (else if (done <= images)) from else as check is not needed.
3.Moved onContentChanged() ouside if and handled for different cases.
4.Added one flag to handle the multipart/mixed type.
5.Tested all the scenarios including multipart.

Please review the patch and provide your feedback.
Once the patch is fine,will add the pull request along with the tests.

Thanks!
Attachment #8372115 - Flags: feedback?(schung)
Comment on attachment 8372115 [details] [diff] [review]
Patch modified as per review comments

Review of attachment 8372115 [details] [diff] [review]:
-----------------------------------------------------------------

Some suggestion for the patch, but basically I'm ok with this flow.

::: apps/sms/js/compose.js
@@ +13,4 @@
>   * message content, and message size
>   */
>  var Compose = (function() {
> +  var isImage = false;

Is it necessary to put 'isImage' to global? You could put it in append function

@@ +97,5 @@
>      }
>  
>      // if the duck is an image attachment, handle resizes
> +    if (duck instanceof Attachment && duck.type === 'img' ||
> +        duck instanceof Attachment && isImage) {

As I commented in append function, we can have a polymorphic duck instead of additional isImage parameter. Having additional statement for checking the type of duck and value.

@@ +529,5 @@
> +            isImage = true;
> +        });
> +        itemValue = item[item.length-1];
> +      }
> +      onContentChanged(itemValue,isImage);

we can just set isImage as the only parameter for onContentChanged when attachment is array. There is no need to pass itemValue if we don't need it.
Attachment #8372115 - Flags: feedback?(schung) → feedback+
Attached patch Image_Sharing_Final.patch (obsolete) — Splinter Review
Hi Steve,
Please find the final patch with changes.
Thanks!
Attachment #8373213 - Flags: feedback?(schung)
Comment on attachment 8373213 [details] [diff] [review]
Image_Sharing_Final.patch

Review of attachment 8373213 [details] [diff] [review]:
-----------------------------------------------------------------

This patch contains some problem and I left some comments below:

::: apps/sms/js/compose.js
@@ +94,5 @@
>      if (ThreadUI.draft) {
>        ThreadUI.draft.isEdited = true;
>      }
>  
> +    if (isImage) {

You can not replace the duck with Boolean checking here. There are some other place trigger onContentChanged with attachment. You should add one more condition to the original statement:

if ((duck instanceof Attachment && duck.type === 'img') || (typeof duck === 'boolean' && duck))

@@ +514,4 @@
>      },
>  
> +    append: function(item) {
> +      var isImage = false;

I would prefer 'containsImage' or other name instead of isImage here.
Attachment #8373213 - Flags: feedback?(schung)
Attached file Pointer to Pull Request (obsolete) —
Hi,

I have modified the changes as per the review comments
1. The changes are described in comment # 33
2. Modified based on the comments given by Steve(for handling "multipart/mixed" case). Refer comment #37
3. Handled the new flag based on Steve comments for "multipart/mixed"
4. Added the tests.

Steve,reviewed the patch and he was okay with this, but as the original reviewer is Rick and mentor is Julien i am asking review from all.

Thanks!
Attachment #8369034 - Attachment is obsolete: true
Attachment #8373213 - Attachment is obsolete: true
Attachment #8369034 - Flags: feedback?(waldron.rick)
Attachment #8369034 - Flags: feedback?(felash)
Attachment #8373298 - Flags: review?(waldron.rick)
Attachment #8373298 - Flags: review?(schung)
Attachment #8373298 - Flags: review?(felash)
Comment on attachment 8373298 [details]
Pointer to Pull Request

I'm perfectly fine with Steve doing the review and I'll update the mentor flag :)
Attachment #8373298 - Flags: review?(felash)
Whiteboard: [mentor=:julienw] → [mentor=schung]
Comment on attachment 8373298 [details]
Pointer to Pull Request

A few comments on the PR, r? me when ready! Thanks :)
Attachment #8373298 - Flags: review?(waldron.rick) → review-
Attached file Pointer to Pull Request (obsolete) —
Hi Steve & Rick,

Thank you for the feedback.
I have modified the code based on your review comments.
Please review these changes.

Thanks!
Attachment #8373298 - Attachment is obsolete: true
Attachment #8373298 - Flags: review?(schung)
Attachment #8373994 - Flags: review?(waldron.rick)
Attachment #8373994 - Flags: review?(schung)
Comment on attachment 8373994 [details]
Pointer to Pull Request

Hi Sasikala, I put some comments on github. It's looks good to me overall(I would like to have a test for composer, but it seems not easy to write a test for that). The patch seems unable to merge, please fix the conflicts for landing.
Attached file Pointer to Pull Request (obsolete) —
Hi,

1.Created new pull request with all the review comments.
2.Fixed the conflicts.
3.Please review and land this on master.
Thanks!
Attachment #8373994 - Attachment is obsolete: true
Attachment #8373994 - Flags: review?(waldron.rick)
Attachment #8373994 - Flags: review?(schung)
Attachment #8374623 - Flags: review?(waldron.rick)
Attachment #8374623 - Flags: review?(schung)
Comment on attachment 8374623 [details]
Pointer to Pull Request

LGTM. Thanks for the great help on this practical feature :)
Attachment #8374623 - Flags: review?(schung) → review+
Steve I think you'll need to land it yourself if the PR is ready :)
Comment on attachment 8374623 [details]
Pointer to Pull Request

The new argument handling in `onContentChanged` needs to be made more explicit. I left a comment, please refactor `onContentChanged` to accept a plain object that operates based on the values provided by optional parameters (instead of making onContentChanged(true|false)` a new signature).
Attachment #8374623 - Flags: review?(waldron.rick) → review-
Comment on attachment 8374623 [details]
Pointer to Pull Request

Hi Rick,

Updated the changes and squash is done.
Please check and kindly review it.

Thanks!
Attachment #8374623 - Flags: review- → review?(waldron.rick)
Comment on attachment 8374623 [details]
Pointer to Pull Request

- Needs updates per IRC discussion (Compose.append(item, true) -> Compose.append(item, { ignoreChange: true }))
- Needs tests for Compose.append(array)
- Needs tests for Compose.append(array, { ignoreChange: true })
Attachment #8374623 - Flags: review?(waldron.rick) → review-
Comment on attachment 8374623 [details]
Pointer to Pull Request

Hi Rick,
Thanks for feedback on changes.
Updated all the changes as per the review comments and added new tests.
Please review the changes.
Hope these changes will cover all the comments and will be the final :)
Thanks!
Attachment #8374623 - Flags: review- → review?(waldron.rick)
Comment on attachment 8374623 [details]
Pointer to Pull Request

Almost there! The tests need work, I provided another example to help you work through it.
Attachment #8374623 - Flags: review?(waldron.rick) → review-
Comment on attachment 8374623 [details]
Pointer to Pull Request

Hi Rick,
Thank you for help in writing unit tests.
Updated the changes as per the review comments and added new tests.
Kindly review the changes.
Thanks!
Attachment #8374623 - Flags: review- → review?(waldron.rick)
Still waiting for the tree to reopen.
Hi Rick,

Will you please update the review status here and also let us know whether you are able to push the changes(land this on master)
Thanks!
Sorry, as I explained to you on IRC, we can't land any new feature on master right now.
(In reply to sasikala from comment #56)
> Hi Rick,
> 
> Will you please update the review status here and also let us know whether
> you are able to push the changes(land this on master)
> Thanks!


As I did last night? 

Anyway, just checking in again, the tree is still closed.
Now that the tree is open (https://bugzilla.mozilla.org/show_bug.cgi?id=974564) I've pushed a copy of this to my origin and opened a new PR just to run through travis before landing in master.

https://travis-ci.org/mozilla-b2g/gaia/builds/19353258
Reverted https://github.com/mozilla-b2g/gaia/commit/a6ecff38bc2afc45e2e316e535558c3c8757b358

This will land when we have the "ok" to land more "risky" patches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/155e203e398c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S2 (28feb)
Keeping the reopened status.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [mentor=schung] → [mentor=schung][ready-to-land-after-1.4-branching]
Hi,

As 1.4 branching is already done,Will you please land this feature.

Thanks!
Whiteboard: [mentor=schung][ready-to-land-after-1.4-branching] → [mentor=schung][ready-to-land-after-1.4-branching] [m+]
Yes Sasikala, will do it today or tomorrow.
I was in holiday next week and that's why I didn't do it ;)
Flags: needinfo?(felash)
Sorry, will be tomorrow, I don't have a phone to test 1.5 here right now.
Attached file rebased github PR
Hey Steve, can you please check these changes?

I rebased and added some more changes in a separate commit. I also changed one test during the rebase and that's why it's part of the first commit.

Especially I fixed the issue I outlined in Bug 943976.
Attachment #8372115 - Attachment is obsolete: true
Attachment #8374623 - Attachment is obsolete: true
Attachment #8374623 - Flags: review?(waldron.rick)
Attachment #8397174 - Flags: review?(schung)
Flags: needinfo?(felash)
Comment on attachment 8397174 [details] [review]
rebased github PR

Oh! Apology for my carelessness in Bug 943976 :( , and thanks for the re-submit this patch. Overall looks great, I only has a concern about the behavior[1] and I'll let UX to make the final decision. 

Hi Omage, could you please help provide some comment about the attachment sharing behavior when total size over the limitation? I'll discuss the detail with you offline.

[1]: https://github.com/julienw/gaia/commit/2f945c5bbda6f6ff5650d4a9303b45c23a470993#commitcomment-5814759
Flags: needinfo?(ofeng)
(In reply to Steve Chung [:steveck] from comment #68)
> Comment on attachment 8397174 [details] [review]
> rebased github PR
> 
> Oh! Apology for my carelessness in Bug 943976 :( , and thanks for the
> re-submit this patch. Overall looks great, I only has a concern about the
> behavior[1] and I'll let UX to make the final decision. 
> 
> Hi Omage, could you please help provide some comment about the attachment
> sharing behavior when total size over the limitation? I'll discuss the
> detail with you offline.
> 
> [1]:
> https://github.com/julienw/gaia/commit/
> 2f945c5bbda6f6ff5650d4a9303b45c23a470993#commitcomment-5814759

I think we should attach nothing and inform user when attaching some files and the total size exceeds the size limitation, no matter each of them is small enough to be attached.
Flags: needinfo?(ofeng)
Comment on attachment 8397174 [details] [review]
rebased github PR

Since UX suggests that we should cancel all the files attaching if total size exceeded, I have no question about the patch. Really appreciate for re-commit this feature missed in 1.4 branching chaos and additional bug fixing!
Attachment #8397174 - Flags: review?(schung) → review+
Omega, the current displayed text is: "The file you have selected is too large". Do you think it should be changed when we have several files? Would "The files you have selected are too large" be ok when we have several files?
Flags: needinfo?(ofeng)
Comment on attachment 8397174 [details] [review]
rebased github PR

Hey Steve,

sorry to ask you again, I changed some other things. I added them all in a separate commit so that you can review them more easily.

Thanks !
Attachment #8397174 - Flags: review+ → review?(schung)
(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #71)
> Omega, the current displayed text is: "The file you have selected is too
> large". Do you think it should be changed when we have several files? Would
> "The files you have selected are too large" be ok when we have several files?

Yeah, I think "The files you have selected are too large" is OK for multiple files.
Flags: needinfo?(ofeng)
(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #72)
> Comment on attachment 8397174 [details] [review]
> rebased github PR
> 
> Hey Steve,
> 
> sorry to ask you again, I changed some other things. I added them all in a
> separate commit so that you can review them more easily.
> 
> Thanks !

Having another alert message for multiple attachments sharing make sense here, thanks! BTW, could you rebase again for conflicts fixing?
Flags: needinfo?(felash)
Steve, I rebased and pushed. I didn't see any conflicts here though!
Flags: needinfo?(felash)
Hey Steve, can you have a look at it? I'd like to merge this so that Oleg can move forward on Bug 936729 :)
Flags: needinfo?(schung)
Comment on attachment 8397174 [details] [review]
rebased github PR

r=me, Sorry for the waiting and I might need to wait for your commit squash bebore merging...

in master: 501cc2526f493c4311f3fb1b67804d5e5d7a2930
Attachment #8397174 - Flags: review?(schung) → review+
Flags: needinfo?(schung)
I mean, I should wait for your commit squashed but I already merge it. Just hope that it won't block your progress.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Thanks for the merge !
I'm confused by the strings introduced in this bug. Why there's no number in the string? That's not the right approach (see for example bug 658191 and bug 476833 for some background).
Flags: needinfo?(felash)
Francesco, we don't want to display the number itself, the number controls only which form we use (plural or non-plural). Do you think it's wrong? How would you do this?
Flags: needinfo?(felash)
(In reply to Steve Chung [:steveck] from comment #78)
> I mean, I should wait for your commit squashed but I already merge it. Just
> hope that it won't block your progress.

Yeah a squash would have been better but now it's done ;)
(In reply to Julien Wajsberg [:julienw] from comment #81)
> Francesco, we don't want to display the number itself, the number controls
> only which form we use (plural or non-plural). Do you think it's wrong? How
> would you do this?

Without the number this is not going to work: noun depends on how many items you're referring to, it doesn't make sense (i.e. it would be grammatically wrong) if the number is missing. Is there a specific reason for not showing it?
Duplicate of this bug: 991931
(In reply to Francesco Lodolo [:flod] from comment #83)
> (In reply to Julien Wajsberg [:julienw] from comment #81)
> > Francesco, we don't want to display the number itself, the number controls
> > only which form we use (plural or non-plural). Do you think it's wrong? How
> > would you do this?
> 
> Without the number this is not going to work: noun depends on how many items
> you're referring to, it doesn't make sense (i.e. it would be grammatically
> wrong) if the number is missing.

I don't really get it. The message is referring to the files the user tried to attach, in a generic way. Is there a language this can't be translated as-is ?

> Is there a specific reason for not showing it?

It would not bring any value to the user. At least in english (and in french) it's unnecessary. Now, I suppose a language translator could decide to show it, since it's still available using {{n}}.
(In reply to Julien Wajsberg [:julienw] from comment #85)
> It would not bring any value to the user. At least in english (and in
> french) it's unnecessary. 

As a general rule, plural forms should not be used to differentiate one vs many.

Take a look at this page (and the bugs I pointed to in previous comments). 
https://developer.mozilla.org/en/docs/Localization_and_Plurals

It's kind of strange coming from languages like French, English, Italian, etc. 

Let's imagine English uses "0 files, 1 file, 2 filess, 3 filesss, undefined amount of filesssss". Displaying a message "The filesss you selected are too big" would be simply wrong without a "3" in the sentence.

> Now, I suppose a language translator could decide to show it, since it's still available using {{n}}.
Great to know, but how is a localizer supposed to know this? Can we please start using localization comments to make their life easier? That would be a general request for Gaia devs.

Something like: "If your locale requires to display the number of files to create a correct sentence, it's possible to use {{n}} in the string".
Attachment #8401815 - Flags: review?(schung)
Attachment #8401815 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8401815 [details] [review]
github PR for adding a localization note

Thanks, looks good to me. (nit: English, not english).
Attachment #8401815 - Flags: feedback?(francesco.lodolo) → feedback+
Nit fixed, thanks !
Comment on attachment 8401815 [details] [review]
github PR for adding a localization note

It's good to know the possibility of plural with number parameter is necessary for other language translation. Thanks for the remider and notes!
Attachment #8401815 - Flags: review?(schung) → review+
master: bca51aba1e940854f0ea4addab58299393e1667b
Depends on: 992891
You need to log in before you can comment on or make changes to this bug.