[Gallery][MMS] Unable to save the received cropped image

RESOLVED DUPLICATE of bug 895726

Status

P1
minor
RESOLVED DUPLICATE of bug 895726
5 years ago
5 years ago

People

(Reporter: leo.bugzilla.gaia, Unassigned)

Tracking

unspecified
1.1 QE4 (15jul)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+)

Details

(Whiteboard: [TD-58436], [u=commsapps-user c=messaging p=1], TaipeiMMS)

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
1. Title: Unable to save the received cropped image
2. Precondition: Should be able to send and receive the MMS messages
3. Tester's Action:  New - Message - Gallery - select image - Crop - send message
4. Detailed Symptom (ENG.) : The received cropped image cannot be saved
5. Expected:  Should be able to save the cropped image
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train: Reproduced on v1-train
8. Gaia Revision:  613e69ee0a009399130ad2cbb8b9d0462cd1fc70
9. Personal email id: sasikala.paruchuri8@gmail.com
(Reporter)

Updated

5 years ago
blocking-b2g: --- → leo+
Priority: -- → P1
Whiteboard: [TD-58436]
Target Milestone: --- → 1.1 QE4 (15jul)
Taking a look!
Assignee: nobody → fbsc

Updated

5 years ago
Whiteboard: [TD-58436] → [TD-58436], [u=commsapps-user c=messaging p=0]
(Reporter)

Comment 2

5 years ago
Created attachment 772002 [details]
Pointer to Pull Request: https://github.com/mozilla-b2g/gaia/pull/10833

Please review the attached patch.
(Reporter)

Updated

5 years ago
Attachment #772002 - Flags: review?(johu)
Comment on attachment 772002 [details]
Pointer to Pull Request: https://github.com/mozilla-b2g/gaia/pull/10833

Leo, 

I don't have the authority to review a bug. You have to request the review to the module owner, like David Flanagan or Dale Harvey. Since David is PTO now, I redirect the review to Dale.

Dale, 

Sorry, David is PTO now. I think the best reviewer is you.
Attachment #772002 - Flags: review?(johu) → review?(dale)
Comment on attachment 772002 [details]
Pointer to Pull Request: https://github.com/mozilla-b2g/gaia/pull/10833

update mime type to html
Attachment #772002 - Attachment is patch: false
Attachment #772002 - Attachment mime type: text/plain → text/html
Assignee: fbsc → leo.bugzilla.gaia
Im gonna move this to Gallery and re-assigning this properly.
Component: Gaia::SMS → Gaia::Gallery
Whiteboard: [TD-58436], [u=commsapps-user c=messaging p=0] → [TD-58436], [u=commsapps-user c=messaging p=1]
Comment on attachment 772002 [details]
Pointer to Pull Request: https://github.com/mozilla-b2g/gaia/pull/10833

David, I've seen this fix is related with Gallery, could you take a look? Thanks! Gracias :)
Attachment #772002 - Flags: review?(dflanagan)
Summary: [MMS] Unable to save the received cropped image → [Gallery][MMS] Unable to save the received cropped image
Comment on attachment 772002 [details]
Pointer to Pull Request: https://github.com/mozilla-b2g/gaia/pull/10833

Thanks for the patch, since David is on PTO I cleared the review for him.

Cleared the review for me, The code here is correct, but it the commit needs fixing before it can be merged, both files have an unncessary mode change (644->755), there is an indentation nit, and the commit message doesnt follow our guidelines (https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Gaia/Hacking#Submitting_a_patch has the details)

Add me as r? on the ammended commit, thanks
Attachment #772002 - Flags: review?(dflanagan)
Attachment #772002 - Flags: review?(dale)
(Reporter)

Comment 8

5 years ago
Review changes are squashed.Please reiew the attached patch.
(Reporter)

Updated

5 years ago
Attachment #772002 - Flags: review?(dale)
Comment on attachment 772002 [details]
Pointer to Pull Request: https://github.com/mozilla-b2g/gaia/pull/10833

Add feedback to Dominic. We had discussed this bug today. He had some concerns on this patch.
Attachment #772002 - Flags: feedback?(dkuo)
(Reporter)

Updated

5 years ago
Duplicate of this bug: 890155

Updated

5 years ago
Whiteboard: [TD-58436], [u=commsapps-user c=messaging p=1] → [TD-58436], [u=commsapps-user c=messaging p=1], TaipeiMMS

Comment 11

5 years ago
To save in the open activity of gallery, there should be two options called "allowSave" and "filename".

I think if one app(like sms) is going to open an image with save option, then "allowSave" should be set to true, and "filename" should be set to a certain name with extension. Although it will be great that gallery can handle the condition when the filename is missing an extension, but since sms already knows allowSave is set to true, it should also try to avoid an uncertain filename that probably cause a save failure.

Updated

5 years ago
Attachment #772002 - Flags: feedback?(dkuo)
Comment on attachment 772002 [details]
Pointer to Pull Request: https://github.com/mozilla-b2g/gaia/pull/10833

I wasnt able to diagnose which error is causing the mms to fail sending, however this is patching the wrong file, when an mms attaches an image it is sending a pick activity to the gallery which doesnt load open.html

Sending still failed for me with this patch applied, its possible for a different reason but either way the STR need clarified for me to review, as far as I know there is no path to save an image when composing an mms
Attachment #772002 - Flags: review?(dale) → review-
The gallery should probably auto-fix extensions / normalize / de-duplicate filenames on the saving option?

What happens if the file name is already taken in gallery?

What happens with attachments sent from iPhone (as they don't have any filenames associated usually and are just called something like "200")

"allowSave" should just turn on the save button, and the filename should be fixed by gallery IMO
(Reporter)

Comment 14

5 years ago
In reply to Dale Harvey ,
STR:
1. This issue is not related to sending a mms file(image attachment).
2. This is basically , the user who receives a MMS( cropped image, sender has cropped the image ) is not able to save the image file.

The patch uploaded only fixes "saving of a received cropped image file"  .

In reply to Corey Frang ,

First we are checking in the filename for presence of extension, if extension is not present we add the extension based on mimetype , refer the patch .

After that if filename is already taken is gallery , it uses getUnusedFilename() function (refer ...shared\js\device_storage_utils.js) to create a new filename as below,
.....
var newname = dir + base + '_' + (version + 1) + ext;
.....
Here it doesn't matter whether its a character based filename or numeric (the function getUnusedFilename() creates new file name appended with a version).


Hi Dominic,
I have a doubt here, so we should be adding the "allowsave" and " filename" as part of gallery open activity ?
Iam not clear about "allowsave" and "filename". Can you please elaborate a bit more.

Thanks,
(Reporter)

Updated

5 years ago
Flags: needinfo?(dkuo)

Comment 15

5 years ago
I think actually there are two issues here:

1. In the open activity of gallery, when we try to save an image attachment without an extension, it might be some cropped image or raw data image, and since DeviceStorage.addNamed() does not allow to save a file without an extension, we cannot save it directly unless we add one extension for it.

2. In sms, if the sent/received attachment has no filename, it will just give a very simple name called "untitled" without any description/hint or extension. And if we fixed issue 1 with some method like MimeMapper, the users might see the saved files with filenames like "untitled_1.jpg", "untitled_2.amr",... which users are unable to recognize where or which app they used to save these files, also there is no specific folder for saving these attachments, so when users are trying to copy the saved attachments from sms by PC, they don't know where and which files they should look for.

In fact, 3GPP TS 26.140 - "Multimedia Messaging Service (MMS); Media formats and codecs" has defined the minimum set of supported formats for MMS:
http://www.3gpp.org/ftp/Specs/html-info/26140.htm

Base on the above spec, it's possible that we can use the mimetype to decide the extension because we should know the supported formats for MMS, and if it's a raw media data that has no extension, we can fix it then issue 1 won't be a problem. Also, to have better UX, it will be nice that sms use some meaningful filenames or add prefix to save the attachment in a specific folder, instead of using the "untitled" name for every media attachments.

So I would like to propose to fix issue 2 more then fix issue 1 because it can prevent more potential bugs in the other scenarios, or fix the two issues together will be better.
Flags: needinfo?(dkuo)

Comment 16

5 years ago
(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #13)
> What happens if the file name is already taken in gallery?

Gallery used shared/js/device_storage_utils.js to handle this but it assumes the filename contains extension.

> What happens with attachments sent from iPhone (as they don't have any
> filenames associated usually and are just called something like "200")

If the mimetype is not something like "application/octet-stream", it should be able to match one open activity to view the attachment. I remember you mentioned it's not common in bug 884722 comment 8?

(In reply to Leo from comment #14)
> Hi Dominic,
> I have a doubt here, so we should be adding the "allowsave" and " filename"
> as part of gallery open activity ?
> Iam not clear about "allowsave" and "filename". Can you please elaborate a
> bit more.

The "allowsave" and "filename" are the options for the open activity of gallery and they are already used, you can see how sms invoke MozActivity in attachment.js.
(Reporter)

Comment 17

5 years ago
Created attachment 779658 [details] [diff] [review]
gallery_crop.patch

Hi Dominic, 

I have a created patch, this patch fixes the issue.

Please review it , and add comments here.

Thanks,
Attachment #779658 - Flags: review?(dkuo)
Flags: needinfo?(dkuo)

Comment 18

5 years ago
Comment on attachment 779658 [details] [diff] [review]
gallery_crop.patch

Leo, I think your patch only fix the issue when gallery is trying to save a file without extension, but for music and video, when saving audio/video like that, the same issue whill occur, so we better fix them together.

I guess we can modify shared/js/device_storage_utils.js to fix the missing extension, instead of changing gallery/music/video apps, more, other apps that use device_storage_utils.js can also avoid this issue.
Attachment #779658 - Flags: review?(dkuo)

Updated

5 years ago
Depends on: 897434
Flags: needinfo?(dkuo)
(Reporter)

Comment 19

5 years ago
Created attachment 780885 [details] [diff] [review]
galleryext.patch

Hi Dkuo,

I have uploaded a patch as per your suggestions, please review it and let me know your comments .

Thanks,
Attachment #780885 - Flags: review?(dkuo)
Flags: needinfo?(dkuo)

Comment 20

5 years ago
Comment on attachment 780885 [details] [diff] [review]
galleryext.patch

Leo,

I think there are two major issue in this patch. First, I think the pickedFile is from deviceStorage, so it should have an extension or deviceStorage won't return it to gallery, and actually we don't need MimeMapper to guess the extension, you can just use the original extension from the pickedFile. Second, I think this patch only fixes this issue in gallery, I am afraid there might be some audio or video will have the some problem, so I would like to see some patch implements the way I mentioned in comment 18.
Attachment #780885 - Flags: review?(dkuo)

Comment 21

5 years ago
Since I am the person who proposed the fix in comment 15, so I am going to take this bug and implement the fix.
Assignee: leo.bugzilla.gaia → dkuo
Flags: needinfo?(dkuo)
moving to leo?
not been looked at 7/30. Please nominate if looking at bug.
blocking-b2g: leo+ → leo?

Comment 23

5 years ago
Update: I am in the Media team meet-up week so don't have a chance to work on it, any one feel free to steal it if you want to.
Hi,

Is this a corner case or is this a general case?

Please confirm.
Flags: needinfo?(dkuo)
Triage - partner will check if this is still blocking and feedback blocking decision on their end.


Preeti, this is a general case that can be systematically reproduced with the STR provided.

(In reply to Preeti Raghunath(:Preeti) from comment #24)
> Hi,
> 
> Is this a corner case or is this a general case?
> 
> Please confirm.

Updated

5 years ago
blocking-b2g: leo? → leo+
Hi Dominic,

Are you looking at this bug?

Mike,

Do you have an engineer looking at this bug from your team?
Flags: needinfo?(mlee)

Comment 27

5 years ago
David,
Please Preeti's comment #26 re: assignee.

Preeti,
David runs the Comms Apps team that handles MMS.
Flags: needinfo?(mlee) → needinfo?(dscravaglieri)

Comment 28

5 years ago
Currently I am not working on this because it had became leo? then leo+ so I have pulled out my resources for now, feel free to take it if anyone would like to work on it, or I can come back to this one after I finish my current work.
Flags: needinfo?(dkuo)

Comment 29

5 years ago
Re-assign to nobody first because I am not working on this.
Assignee: dkuo → nobody
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 895726
I found this bug is a duplicate of bug 895726. These two bugs are related to the correct behavior of blob saving of gallery app.
clear ni?
Flags: needinfo?(dscravaglieri)
You need to log in before you can comment on or make changes to this bug.