Closed Bug 929860 Opened 6 years ago Closed 6 years ago

[Gaia] SD card formatting

Categories

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

x86
macOS
defect
Not set

Tracking

(b2g-v1.4 fixed)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: jcheng, Assigned: iliu)

References

Details

(Whiteboard: [1.3:p2])

Attachments

(3 files)

This is the Gaia bug to complete user story Bug 921105 - [Devices][User Story] SD card formatting
Assignee: nobody → iliu
Blocks: 921105
Whiteboard: [1.3:p2]
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Note: Since Gecko doesn't support mount/unmount API and status, we're not able to implement notification message while a user plug-in/unplug-in SD card(https://bugzilla.mozilla.org/show_bug.cgi?id=921105#c12). Let's track the follow up feature via Bug 943825. 

For the issue, we focus on SD card formatting only.
Attached file pr14120_format_SDcard
Hi Evelyn,

Could you please help to review the patch? And this patch is needed the API format() via Bug 841660. Thanks.
Attachment #8339218 - Flags: review?(ehung)
Comment on attachment 8339218 [details] [review]
pr14120_format_SDcard

Hi Evelyn,

I'm not sure you have bandwidth to review the pr here. I think you and Kaze is more familiar the media storage in settings app. So, I also request Kaze to be the reviewer too.
Attachment #8339218 - Flags: review?(kaze)
Comment on attachment 8339218 [details] [review]
pr14120_format_SDcard

The function implementation looks good, but we need to consider the device with internal and external(sdcard) storage. Please re-org your code so it's able to deal with this case. Thanks!
Attachment #8339218 - Flags: review?(kaze)
Attachment #8339218 - Flags: review?(ehung)
Attachment #8339218 - Flags: feedback+
(In reply to Evelyn Hung [:evelyn] from comment #4)
> Comment on attachment 8339218 [details] [review]
> pr14120_format_SDcard
> 
> The function implementation looks good, but we need to consider the device
> with internal and external(sdcard) storage. Please re-org your code so it's
> able to deal with this case. Thanks!

one more thing:
Alan told me the total size of a storage should be the sum of usedSpace() of all media types, plus freeSpace(). Please correct it. Thanks.
(In reply to Evelyn Hung [:evelyn] from comment #5)
> (In reply to Evelyn Hung [:evelyn] from comment #4)
> > Comment on attachment 8339218 [details] [review]
> > pr14120_format_SDcard
> > 
> > The function implementation looks good, but we need to consider the device
> > with internal and external(sdcard) storage. Please re-org your code so it's
> > able to deal with this case. Thanks!
> 
> one more thing:
> Alan told me the total size of a storage should be the sum of usedSpace() of
> all media types, plus freeSpace(). Please correct it. Thanks.
Evelyn and Alan,

I think you miss non-media types files in above definition. The total size means whole space in the SDCard.

You may have some misunderstood in total size. Please check "sizes['sdcard']" again(https://github.com/mozilla-b2g/gaia/pull/14120/files#diff-2e669326542ce32522201f56dcf9d0cdR186). The parameter is coming from the results object.

sizes['sdcard'] : is the size of 'sdcard' type storage.usedSpace(). It means that all of the space are allocated.(etc, music, video, picture, document, more all other files.. (including of non-media type))
sizes['free'] : is the size of 'sdcard' type storage.freeSpace(). It means that all of the space are not allocated.(free/available space)

That is to say the total size should be sizes['sdcard'] + sizes['free']. I spend much time to understand the naming in media_storage.js. :) Might the naming make people confused..
Comment on attachment 8339218 [details] [review]
pr14120_format_SDcard

Thanks for your reviewing effort. I have revised some part of your comment. And leave some comment on Github. Please help to review it again.
Attachment #8339218 - Flags: review?(ehung)
Comment on attachment 8339218 [details] [review]
pr14120_format_SDcard

So I found we have a big difference of understanding the UX spec. I think we should have a format button per storage, i.e. one for internal storage and one for sdcard. I also confirmed with Alan that the format API can be used on internal storage. So what's the UX want?
Attachment #8339218 - Flags: review?(ehung)
Please help on clarify my question in comment 8.
Flags: needinfo?(nhsieh)
Hi Evelyn, 
I think that is too complicated to let user knows that SD card has one or two partition, or internal storage has one or two partition. If we provide one thing that user should knows (For devices has internal storage): Your SD card or mobile phone storage, that is enough.  Try to think about it : One normal user buy a SD card insert into mobile phone, what information should be there ? They can't imagine there are more than one external storage in that screen.  So the basic design rule is keep it simple and easy. We only provide two storage concept to user, One is internal storage and another is SD card. 
So,
If user perform "Factory reset", system will erase internal storage(Include all partition)and also if user perform "Format SD card" that will erase SD card all data and partition. For some devices has two or more hardware SD card slot, please put unmount and foramt button for each block.  Thanks
Flags: needinfo?(nhsieh)
Hi devs,

As previous discussion in Devices team weekly sync-up today, we still have a button to format "internal" storage. i.e. We will have a format button in each storage. e.g. One storage section, one "format" button. And it's better to have an option to toggling format "internal" storage in Settings::Device information::More Information::Reset Phone(follow up work).

Hi Neo,

Could you please help to confirm and update spec. again? Thanks.
Flags: needinfo?(nhsieh)
Attached file SD_Card_131219.pdf
Flags: needinfo?(nhsieh)
Comment on attachment 8339218 [details] [review]
pr14120_format_SDcard

Hi Evelyn,

I have updated my patch according to latest spec. And revised all of comment in GitHub. Please help to review it again. The patch is also working fine in multiple storages. (eg. Helix) Thanks for your patient in the reviewing progress.
Attachment #8339218 - Flags: review?(ehung)
Comment on attachment 8339218 [details] [review]
pr14120_format_SDcard

Due to API is not complete yet, this patch only implements "format" scenario, no "mount/unmount" cases covered. so I'm fine with current implementation although the UX spec doesn't not make sense to me. Here are some questions we've discussed offline, I hope we can think more on this spec and open follow-up bugs to complete the whole function.

1. in case of internal storage can't be formatted, do we need to hide the format button or add a warning message while the user click "format"? (we may need extra API support to know the internal storage is not format-able)

2. I'm not sure if we can unmount an internal storage, and if it's doable, what does it mean to the user? I think *internal* storage means the user can't unplug it, I don't know if there is a case that the user just want to unmount a internal storage anyway even he/she has nothing else can operate on it.

there is a wordy question in the spec: I see we use 'sdcard' in all cases no matter it's internal or external, do we need to distinguish them?

loop UX(Neo) and API developer to answer my question above. Thanks for your help.
Attachment #8339218 - Flags: review?(ehung) → review+
Flags: needinfo?(nhsieh)
Flags: needinfo?(ahuang)
For point 2. I agree with Evelyn. User will no need to Unmount internal storage.  
The reason why I design that was because in SW process flow, Format internal storage still perform unmount/mount.  But Alan Huang told us that will automatically execute. So user no need to know that .
Flags: needinfo?(nhsieh) → needinfo?(ehung)
Attached file SD_Card_140207.pdf
Neo, thanks for the modified spec. :)
Ian, can you follow-up the rest of things we need to do here? Thank you.
Flags: needinfo?(ehung)
(In reply to Evelyn Hung [:evelyn] from comment #14)

I talked with Neo and Ian last week. I think we don't need to provide unmount/mount UI for internal storage to users. Format command could handle these :)
Flags: needinfo?(ahuang)
(In reply to Evelyn Hung [:evelyn] from comment #17)
> Ian, can you follow-up the rest of things we need to do here? Thank you.
Sure, my patch was considered latest spec. mention. According Alan's comment 18, the format command is ready to handle case 1, comment 14. And I have received and updated my patch with your suggestion. Thanks for your reviewing effort:)
Since the patch is merged, we can close the issue now.

Gaia/master:  af6410dd182560e56c0ee3be282cc79a29a69d8a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
format-sdcard-description=It will erase all SD card data. Such as music or photos.

As a non native English speaker, this sentence sounds pretty strange, at least the full stop in the middle (also the not so clear implied subject).

Something like "All data stored on the SD card, such as music or photos, will be erased."

@Matej: your opinion?
Flags: needinfo?(Mnovak)
It would be great if we could get it into the active voice. Would this work in this context:

This will erase all data stored on your SD card, such as music and photos.
Flags: needinfo?(Mnovak)
I don't think it would be a problem. Ian? I can file a follow-up bug to update the string (and ID at this point).
Flags: needinfo?(iliu)
Depends on: 971615
Hi Francesco,

Comment 22 is okay for me. Please file a follow-up bug. Then I'll revise it via our discussion. Add Omega who is the new SD card formatting owner to the track.
Flags: needinfo?(iliu)
Depends on: 973787
Flags: in-moztrap?(mozillamarcia.knous)
You need to log in before you can comment on or make changes to this bug.