929860 - [Gaia] SD card formatting

Status: RESOLVED FIXED

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

Description

[Joe Cheng [:jcheng] (please needinfo)]

This is the Gaia bug to complete user story Bug 921105 - [Devices][User Story] SD card formatting

Comment 1

[[e-mail], [e-mail]]

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.

Comment 2

[[e-mail], [e-mail]]

Attachment: 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.

Comment 3

[[e-mail], [e-mail]]

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.

Comment 4

[Evelyn Hung]

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!

Comment 5

[Evelyn Hung]

(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.

Comment 6

[[e-mail], [e-mail]]

(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 7

[[e-mail], [e-mail]]

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.

Comment 8

[Evelyn Hung]

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?

Comment 9

[Evelyn Hung]

Please help on clarify my question in comment 8.

Comment 10

[Neo Hsieh [:neo]]

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

Comment 11

[[e-mail], [e-mail]]

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.

Comment 12

[Neo Hsieh [:neo]]

Attachment: SD_Card_131219.pdf

Comment 13

[[e-mail], [e-mail]]

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.

Comment 14

[Evelyn Hung]

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.

Comment 15

[Neo Hsieh [:neo]]

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 .

Comment 16

[Neo Hsieh [:neo]]

Attachment: SD_Card_140207.pdf

Comment 17

[Evelyn Hung]

Neo, thanks for the modified spec. :)
Ian, can you follow-up the rest of things we need to do here? Thank you.

Comment 18

[Alan Huang]

(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 :)

Comment 19

[[e-mail], [e-mail]]

(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:)

Comment 20

[[e-mail], [e-mail]]

Since the patch is merged, we can close the issue now.

Gaia/master:  af6410dd182560e56c0ee3be282cc79a29a69d8a

Comment 21

[Francesco Lodolo [:flod]]

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?

Comment 22

[Matej Novak [:matej]]

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.

Comment 23

[Francesco Lodolo [:flod]]

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).

Comment 24

[[e-mail], [e-mail]]

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.