1115500 - [DeviceStorage][Settings][System] Automatically change default storage location without launch settings app.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

If the settings of default storage is external SD card, and there is no SD card in the SD card slot, settings app will support to change default storage from external to internal automatically. The feature is ready in v2.1/master. But it's needed to launch settings::media_storage panel.

We have to fix the problem while the platform launch.

Reference: Found out the issue from https://bugzilla.mozilla.org/show_bug.cgi?id=1110068#c13

Comment 1

[Alphan Chen [:alchen]]

Attachment: (0205) Donot return an unavailable storage name in GetDefaultStorageName - gecko patch

Hi Dave,
The pupose of this patch is as below:
1. From UX spec(http://goo.gl/OwvQOh), we need to auto correct the default media location if we only have one storage.
2. The settings app have mechanism to do this correction. However, if user don't open the settings app, the pref won't be changed to a proper one.
3. This behavior can be done either in gecko system app.


In this patch, gecko will check the storage before return. If the storage is not available, we will return the first storage (which should be sdcard) and modify the pref in the same time.

Comment 2

[Dave Hylands [:dhylands]]

I proposed this a long time ago, and it was firmly rejected (having the default storage area automatically switch). This was from back when we only had a single storage area and was introducing support for multiple storage areas.

So I personally don't have any issues with this.

I seem to recall that the main objection came from sicking, so I've ni'd him to see if his opinion has changed (the argument at the time had something to do with not storing things in a place that user chose).

Comment 3

[Alphan Chen [:alchen]]

Attachment: (0212) Return the first storage if preferred storage is not presented in GetDefaultStorageName

Revised the patch.

1. Add "#ifdef MOZ_WIDGET_GONK" for new added changes.
2. Check if the prefer storage is "NoMedia",
      yes: return the first storage from "GetOrderedVolumeNames".
       no: return the original prefer storage.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't have strong opinions either way. If I was the person most strongly objecting before, I've forgotten the reasons why.

The most important part to me is that we create a good UX. I.e. we should first decide what UX we want, and then make sure that the API makes implementing that UX sensible.

It makes sense to me that we'd want to automatically want to write pictures to internal storage if the user first choose the SD card as storage, and then eject the SD card.

It would however also make sense to me to warn the user in the UX when that happens. I.e. it might make sense to expose in the API that the user-chosen default storage is not available and that a secondary choice is used.

But again, we should drive this based on UX requirements. The particular API is less important.

Comment 5

[Ben Tian (inactive)]

ni? UX Jenny to comment.

Jenny, Jonas suggests to fix based on UX requirement and proposes two options as following. Please let us know your preferred one or any other idea.

(In reply to Jonas Sicking (:sicking) from comment #4)
> The most important part to me is that we create a good UX. I.e. we should
> first decide what UX we want, and then make sure that the API makes
> implementing that UX sensible.

Option 1:
> It makes sense to me that we'd want to automatically want to write pictures
> to internal storage if the user first choose the SD card as storage, and
> then eject the SD card.

Option 2: 
> It would however also make sense to me to warn the user in the UX when that
> happens. I.e. it might make sense to expose in the API that the user-chosen
> default storage is not available and that a secondary choice is used.
> 
> But again, we should drive this based on UX requirements. The particular API
> is less important.

Comment 6

[Jenny Lee]

Hi Ben,

I think Option 1 is better, because at this point user will have no choice but to save media to internal storage (unless there are more than one SD card), so no need to take an extra step to tell user about that. If user later inserts the SD card back, she will need to reset the default media storage to SD card manually. Thanks!

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8563291 [details] [diff] [review]
(0212) Return the first storage if preferred storage is not presented in GetDefaultStorageName

Thanks for Jenny's input. So we'll keep working on option 1 that switches default storage automatically.

Dave, can you help review Alphan's patch?

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 8563291 [details] [diff] [review]
(0212) Return the first storage if preferred storage is not presented in GetDefaultStorageName

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

r=me with changes

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3634,5 @@
> +    nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(aStorageType,
> +                                                            prefStorageName);
> +    dsf->GetStorageStatus(status);
> +
> +    if (!status.EqualsLiteral("NoMedia")) {

This is insufficient. The volume could also be unavailable for other reasons (i.e. currently being shared with the PC).

I think it would be better to use:

if (!dst->IsAvailable())

I also don't think that this code needs to be inside an #ifdef GONK

@@ +3651,5 @@
>    GetOrderedVolumeNames(volNames);
>    if (volNames.Length() > 0) {
>      aStorageName = volNames[0];
> +    // overwrite the value of "device.storage.writable.name"
> +    mozilla::Preferences::SetString("device.storage.writable.name", aStorageName);

nit: I'd like to see device.storage.writable.name made into a #define or constant string.

Comment 9

[Alphan Chen [:alchen]]

Comment on attachment 8563291 [details] [diff] [review]
(0212) Return the first storage if preferred storage is not presented in GetDefaultStorageName

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3634,5 @@
> +    nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(aStorageType,
> +                                                            prefStorageName);
> +    dsf->GetStorageStatus(status);
> +
> +    if (!status.EqualsLiteral("NoMedia")) {

We should not change the pref value when the SD card is shared with PC.
App will show that storage cannot be used while phone is plug in.

I think we only need to take care the following case:
1. device.storage.writeable.name is "SD card".
2. There is no SD card in the phone.
At this time, we auto-switch this pref to internal storage.

@@ +3651,5 @@
>    GetOrderedVolumeNames(volNames);
>    if (volNames.Length() > 0) {
>      aStorageName = volNames[0];
> +    // overwrite the value of "device.storage.writable.name"
> +    mozilla::Preferences::SetString("device.storage.writable.name", aStorageName);

Will do that.

Comment 10

[Dave Hylands [:dhylands]]

> We should not change the pref value when the SD card is shared with PC.
> App will show that storage cannot be used while phone is plug in.
> 
> I think we only need to take care the following case:
> 1. device.storage.writeable.name is "SD card".

Just to clarify, the name 'sdcard' always means internal storage, if internal storage exists (our early phones had no internal storage). Any name other than 'sdcard' typically refers to a physical sdcard.

Comment 11

[Alphan Chen [:alchen]]

Attachment: Return the first storage if preferred storage is not presented in GetDefaultStorageName. r=dhylands

Here is the try server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e92a7161a8c2
It looks fine.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a79ba1917c7f

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a79ba1917c7f