Closed Bug 874313 Opened 11 years ago Closed 11 years ago

Settings > Media storage > Default media location can select SD card even if user removes sd-card

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
1.1 QE4 (15jul)

People

(Reporter: jeremykimleo, Assigned: arthurcc)

References

Details

(Keywords: late-l10n)

Attachments

(3 files)

STR > 
After Remove sd-card by user, 
Go To Settings -> Media storage -> Default media location 

User can change to "SD card" as default. but, there already removes sd-card by user.

Expected behavior >
"Default media location" should be disabled. and it only displays Internal as default. 

Revision Information
Gecko's Revision : ada3b9647e60742aacee32918596c0ae022df1cb
Gaia's Revision  : 0d5f55fef5b46b8a53b87daed9abe3b9a37ce322
Assignee: nobody → arthur.chen
blocking-b2g: --- → leo?
So if the external sdcard is selected as the default, but its not available, then device storage will pick the internal sdcard to store new files to.

If the external sdcard becomes available in the future, then device storage will switch back to using it.

I'll leave the decision of how the UX should look/behave to the UX people.
Evelyn, Rudy, could you help review the related parts?
Evelyn, this is a relatively large patch, please let me know if any problem when review it, thanks!
Attachment #756313 - Flags: review?(rlu)
Attachment #756313 - Flags: review?(ehung)
Casey, could you help provide some inputs here? Thanks!
Flags: needinfo?(kyee)
Comment on attachment 756313 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10122

Cancel the review request at first.
Attachment #756313 - Flags: review?(rlu)
Attachment #756313 - Flags: review?(ehung)
Flags: needinfo?(kyee) → needinfo?(rmacdonald)
(In reply to Dave Hylands [:dhylands] from comment #1)
> So if the external sdcard is selected as the default, but its not available,
> then device storage will pick the internal sdcard to store new files to.
> 
> If the external sdcard becomes available in the future, then device storage
> will switch back to using it.
> 
> I'll leave the decision of how the UX should look/behave to the UX people.

Hi everyone...

This approach sounds reasonable, however, this is not currently how it works on my unagi phone. 

If the media card is in and I'm in Media Storage and attempt to select a default media location, the value selector button is displayed but does not work. 

If the memory card is removed, the link to Settings > Media Storage is disabled. Further, a full screen dialog is displayed in Gallery, Videos or Camera that notifies me that I need to insert a memory card.

So a few issues / questions...

First, are we supporting the use of phone memory for media files? Or is it only for specific devices? 

If we are not supporting the use of phone memory, then we need to remove the default media location selection option from Settings > Media Storage.

If we are supporting the user of phone memory and the SD card, will the media apps scan both drives for content? And, will media apps open if an SD card is not inserted?

I suspect one of the reasons we don't support putting files on phone memory is that we don't currently have any means of transferring the files back and forth... whether that be a file/document manager or a way to move files around from within media apps. I'll confirm this with Casey until he's back from PTO on Monday but, if anyone any insight on the above questions in the meantime, feel free to chime in.

- Rob
Flags: needinfo?(rmacdonald) → needinfo?(kyee)
(In reply to Rob MacDonald [:robmac] from comment #5)
...snip...
> First, are we supporting the use of phone memory for media files? Or is it
> only for specific devices? 

For the unagi, the only place we store media files is on the physical sdcard.
For the leo, it has both an internal (non-removable) storage location in addition to the physical sdcard.

> If we are not supporting the use of phone memory, then we need to remove the
> default media location selection option from Settings > Media Storage.

That will be phone specific.

> If we are supporting the user of phone memory and the SD card, will the
> media apps scan both drives for content? And, will media apps open if an SD
> card is not inserted?

For the leo, yes, both internal and external sdcard are scanned. And you can use just the internal sdcard with no physical sdcard.

> I suspect one of the reasons we don't support putting files on phone memory
> is that we don't currently have any means of transferring the files back and
> forth... whether that be a file/document manager or a way to move files
> around from within media apps. I'll confirm this with Casey until he's back
> from PTO on Monday but, if anyone any insight on the above questions in the
> meantime, feel free to chime in.

The real reason we don't support putting files on phone memory on the unagi like phones, is that there isn't any (some devices have more and some less, but for the initial release, the phones just don't have enough internal memory to use for storing media)
blocking-b2g: leo? → leo+
Priority: -- → P1
Target Milestone: --- → 1.1 QE2 (6jun)
ni?Rob again with comment 6 answering his questions.
Casey or Rob, can you comment from UX perspective to unblock Arthur on this?

Thanks.
Flags: needinfo?(rmacdonald)
Please clarify what additional UX assistance is needed to unblock Arthur. I'm not sure what is still needed after Rob's comment 5. Thanks!
Flags: needinfo?(kyee)
I talked to Casey today and there is a spec for this, located in drop box...

https://mozilla.box.com/s/gzdk6x2dyny4r3xwvtzc

There are two items missing from the spec based on the thread above.

First, for phones where we only support a single storage drive, the entire default media location selection option from Settings > Media Storage should be removed. 

Second, and also for phones where we only support a single storage drive, the "Share using USB" toggle should be removed from the individual drive settings. (See page 7, item C, in the spec.) Instead the single "Enable USB storage" at the top of the view (see page 7) toggles the setting for the one drive.

We'll update the spec with these changes, but, in the meantime, feel free to flag Casey or myself with your questions.

- Rob
Flags: needinfo?(rmacdonald)
(In reply to Dave Hylands [:dhylands] (Travellling until June 7) from comment #1)
> So if the external sdcard is selected as the default, but its not available,
> then device storage will pick the internal sdcard to store new files to.
> 
> If the external sdcard becomes available in the future, then device storage
> will switch back to using it.
> 
> I'll leave the decision of how the UX should look/behave to the UX people.

Also, talking to Casey we were thinking we should keep things simple for now and not swap the default locations when the selected drive is full. We thought it's better if the user is informed that the drive is full and, based on that, they can then decide whether they want to switch default locations or clear out data.

Rob
Rob, thanks for the comment. However, it still remains unclear how to handle the case when users remove the SD card. The following summary is based from previous comments, please help confirm or correct it.

For leo devices:
- If the current selected storage removed by users, inform users about the status. Users are not allowed to save media to the storage or read media from the storage. 
- If the current selected storage is full, inform users about the status. Users are not allowed to save media to the storage.
- Internal storage and SD card should always be listed as options no matter they are full or unavailable (removed by users).
- The default location setting should never be altered by the system in all conditions.
Flags: needinfo?(rmacdonald)
(In reply to Arthur Chen [:arthurcc] from comment #11)
> Rob, thanks for the comment. However, it still remains unclear how to handle
> the case when users remove the SD card. The following summary is based from
> previous comments, please help confirm or correct it.
> 
> For leo devices:
> - If the current selected storage removed by users, inform users about the
> status. Users are not allowed to save media to the storage or read media
> from the storage. 

Correct.

> - If the current selected storage is full, inform users about the status.
> Users are not allowed to save media to the storage.

Correct.

> - Internal storage and SD card should always be listed as options no matter
> they are full or unavailable (removed by users).

Correct.

> - The default location setting should never be altered by the system in all
> conditions.

Correct.

Thanks, Arthur, you managed to summarize my unnecessarily lengthy thought process quite nicely. :) We'll need to document this for the leo case, as well as for the case where there is only a single drive. 

Flagging Stephany for resourcing / scheduling.
Flags: needinfo?(rmacdonald) → needinfo?(swilkes)
Just to wrap this one up: Our team will add this information to specs, so that the decisions made in this thread are documented and shared more broadly. In the meantime, development should move forward based on the conversation with Rob and Arthur in these comments.
Flags: needinfo?(swilkes)
Based on comment 12, we simply add a warning displayed when users try to switch to an unavailable storage. Stephany, I'll start to implement it when the string is ready.

Dave, could you help change the gecko part accordingly (not fallback to available storages)? Thanks!
Flags: needinfo?(dhylands)
Sure - although this seems contrary to what I've been hearing that was wanted (I heard that they wanted it to fallback if the file doesn't fit, for example).
Flags: needinfo?(dhylands)
Depends on: 881749
Arthur, so I sat down and thought about this, and wrote my thoughts down in bug 881749, to cover off what actually needs to change.

If we remove the fallback, this will have much further impact than just the settings app.

It will also require a new API to be added to device storage, and will imapact at least all of the following:
- taking a screen snapshot
- taking a still photo
- capturing a video
- saving an email attachment
- transferring a file via bluetooth
- possibly the mediadb

I just wanted you to be perfectly aware that this isn't just a simple change.

I put needinfo on just to make sure that you're aware that all of the above will also need some changes if I remove the fallback.
Flags: needinfo?(arthur.chen)
I'd also like to have Jonas and/or Doug Turner also weigh in on this.
Flags: needinfo?(jonas)
Flags: needinfo?(doug.turner)
Dave, you are right. I know some of the apps handle cases such as users un-plug SD card while scanning (gallery, music). However, I think they don't handle exact this case. We will need to check all related apps after the API changed. Given the current schedule, we should reconsider if we are going to implement the proposal.

BTW, what kind of new API needs to be added to device storage?
Flags: needinfo?(arthur.chen)
So the current available function for composite storage works as follows:

1 - If any of the real storage areas are available, then return available
2 - If none are available, and any are shared, then return shared
3 - otherwise return unavailable

With the failover the way it is currently, available covers both available-for-reading and available-for-writing.

If we remove the failover, the we need to change something. Apps don't have access to the "default location to save" setting, so we need to add a variant of available which means "the default storage area is available for writing". We need a way to query the initial value and a way to be notified of changes (either due to the default storage area changing (i.e. user changes the setting), or the state of the default area changed (i.e. the user inserts the sdcard)).
The original design of device storage tends to make it transparent to Apps, which means Apps should not aware the exact storage they are using. If we remove the failover, we only need to make sure that the App will be notified when the state of the default storage changes and will respond to it correctly. Please correct me if I misunderstood it.
(In reply to Arthur Chen [:arthurcc] from comment #20)
> The original design of device storage tends to make it transparent to Apps,
> which means Apps should not aware the exact storage they are using. If we
> remove the failover, we only need to make sure that the App will be notified
> when the state of the default storage changes and will respond to it
> correctly. Please correct me if I misunderstood it.

Exactly. It's just that the existing API, doesn't provide any mechanism to make this happen.
Dave, is it possible to return a dummy storage when one device storage becomes unavailable? When apps try to access the dummy storage it always fails. By doing this we can leverage the current error handling mechanism.
Flags: needinfo?(dhylands)
I don't quite understand by what you mean to "return a dummy storage area".

It's relatively straight forward to add more storage areas. I don't understand how this helps.

What would each of the APIs do in this dummy storage system?

AddNamed, Delete, Get, Enumerate, Available, FreeSpace, UsedSpace
Flags: needinfo?(dhylands)
Given that most apps handle the error cases when calling to the APIs of the storage, what I meant was when users try to get the default storage that is not available, we can still return a storage object that cannot be operated. All APIs of the storage object except for Available should execute the "onerror" callbacks. For Available it should execute the "onsuccess" callback with the state "unavailable".
Target Milestone: 1.1 QE2 (6jun) → 1.1 QE3 (24jun)
Whiteboard: TaipeiWW
I *think* we'd solve the problems here if we make the shared/available/size state of the composite storage area reflect the state of the default storage. I.e. if the default area is not available, that's what the composite storage area will report. And if the default area is full, then that's what the composite area will report.

This will mean that existing code that was really written to accommodate a single storage area will behave "as expected" when the default storage area is removed or is full.

I agree with UX here. Silently saving files to a different location than the one the user has selected seems like a bad idea. I'd prefer to keep things simple for now and see how we can improve things based on experience.

We do actually already have all the needed platform support to enable applications to query the state of the different storage areas however it wants. And even to do automatic fallback from one storage area to another as they grow full.

This is all possible if application use the explicit storage areas rather than the composite one.

Longer term I think we need to migrate apps off of using the composite storage area completely. Instead we should expose a .default property on storage areas so that applications can know which one the user has configured as the default one.

I simply don't think it's possible to try to be so clever in the platform APIs that we pretend there is only a single storage areas. Applications just have to learn to deal with the fact that there actually are multiple storage areas with different states and different available sizes.
Flags: needinfo?(jonas)
Flags: needinfo?(doug.turner)
(In reply to Jonas Sicking (:sicking) from comment #25)
> I *think* we'd solve the problems here if we make the shared/available/size
> state of the composite storage area reflect the state of the default
> storage. I.e. if the default area is not available, that's what the
> composite storage area will report. And if the default area is full, then
> that's what the composite area will report.

I like it. It fits and makes everything consistent.

> Longer term I think we need to migrate apps off of using the composite
> storage area completely. Instead we should expose a .default property on
> storage areas so that applications can know which one the user has
> configured as the default one.

The .default attribute has already been added.

I think that I'll file a new bug to further refine this.
Blocks: 885753
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Take since Arthur is supporting another event this week. I can help on moving the bug forward.
Assignee: arthur.chen → ehung
So back to this issue here, per comment 12 and comment 26 and bug 885753, Settings will do the change as what comment 12 said. The change needs bug 885753 being fixed, to return the exact user specified default location, and fallback to available storage, so the setting and exact behavior will be consistent.
(In reply to Evelyn Hung [:evelyn] from comment #28)
> So back to this issue here, per comment 12 and comment 26 and bug 885753,
> Settings will do the change as what comment 12 said. The change needs bug
> 885753 being fixed, to return the exact user specified default location, and
> fallback to available storage, so the setting and exact behavior will be
"never" fallback to available storage ...
> consistent.
(In reply to Rob MacDonald [:robmac] from comment #12)
> (In reply to Arthur Chen [:arthurcc] from comment #11)
> > Rob, thanks for the comment. However, it still remains unclear how to handle
> > the case when users remove the SD card. The following summary is based from
> > previous comments, please help confirm or correct it.
> > 
> > For leo devices:
> > - If the current selected storage removed by users, inform users about the
> > status. Users are not allowed to save media to the storage or read media
> > from the storage. 
> 
> Correct.
Can you suggest a way to *inform* users that he/she is removing the default location? is it a dialog pop-up or something? Can the user dismiss(ignore) the alert? Do we need a link on the alert to direct the user to Settings app?

Regard to "Users are not allowed to save media to the storage or read media from the storage", media apps will behave correct if bug 885753 fixed.

> 
> > - If the current selected storage is full, inform users about the status.
> > Users are not allowed to save media to the storage.
> 
> Correct.
> 
similar to above.
Flags: needinfo?(rmacdonald)
If string changes are being requested, please add the l10n tag.
(In reply to Stephany Wilkes from comment #31)
> If string changes are being requested, please add the l10n tag.

Looks like it's not clear that changes are needed until Rob answers Evelyn's questions.
(In reply to Evelyn Hung [:evelyn] from comment #30)

> > > For leo devices:
> > > - If the current selected storage removed by users, inform users about the
> > > status. Users are not allowed to save media to the storage or read media
> > > from the storage. 

> Can you suggest a way to *inform* users that he/she is removing the default
> location? is it a dialog pop-up or something? Can the user dismiss(ignore)
> the alert? Do we need a link on the alert to direct the user to Settings app?

I was originally thinking we would show the existing dialogs when you launch each app and the SD card is not available... assuming here that the SD card is selected. However, there are two potential enhancements to this that we could consider.

The first would be add a link from the existing dialog to settings and change the wording for each app, as you have suggested. For each error message, we'd modify the string to add, "or select an alternate default media location in settings." In addition, at the bottom of the error for each app, we would add a button that reads "Manage media storage" and links directly to the "Media storage settings" view in settings. So, for Gallery for example, it would read something like this (forgive the lack of screen shot)

     No memory card found
     -----------------------
     Insert a memory card 
     to use this app or 
     select an alternate 
     default media location 
     in settings.

     [Media storage settings]


Camera would read

     No memory card found
     -----------------------
     Insert a memory card 
     to take pictures or 
     select an alternate 
     default media location 
     in settings.

     [Media storage settings]

Music...

     No memory card found
     -----------------------
     Insert a memory card 
     to listen to songs or 
     select an alternate 
     default media location 
     in settings.

     [Media storage settings]

Video...

     No memory card found
     -----------------------
     Insert a memory card 
     to watch videos or 
     select an alternate 
     default media location 
     in settings.

     [Media storage settings]

Let me know your thoughts.

Second, when the SD card is physically removed, and if it is the default media storage location, we could alert the user with a confirmation dialog that states.

     Default media storage removed
     -----------------------------------
     Insert a memory card to view 
     and record media, or select an 
     alternate default media location 
     in settings.

     [Ignore]
     [Media storage settings]



> > > - If the current selected storage is full, inform users about the status.
> > > Users are not allowed to save media to the storage.
> > 
> > Correct.
> > 
> similar to above.

There are existing specifications for this that I believe Casey has worked on. Casey, can you post these to the bug and verify my proposal above?

- Rob
Flags: needinfo?(rmacdonald) → needinfo?(kyee)
The proposal looks good.   I will make changes as per Comment 9 and add the additional UX cases outlined in Comment 33.
Flags: needinfo?(kyee)
Casey, do you have a ETA of the spec? We need it for detail check. Thank you.
Arthur, can you start the implementation with the information in comment 33? Thanks!
Assignee: ehung → arthur.chen
Flags: needinfo?(kyee)
No longer blocks: 885753
Depends on: 885753
Hi Dale, David, and Dominic,
This patch modified the messages of the case of unavailable media storage. A button linking to the settings panel was also added to the warning. Could you help review the patch? Thanks!
Attachment #771823 - Flags: review?(dkuo)
Attachment #771823 - Flags: review?(dflanagan)
Attachment #771823 - Flags: review?(dale)
Attached image screenshot
Rob, could you help confirm the screenshot is as what you have in mind? Thanks!
Flags: needinfo?(rmacdonald)
Comment on attachment 771823 [details]
link to https://github.com/mozilla-b2g/gaia/pull/10818

Switching out the CSS here is slightly worrying, however the code looks good and seems to be working.
Attachment #771823 - Flags: review?(dale) → review+
Whiteboard: TaipeiWW
(In reply to Arthur Chen [:arthurcc] from comment #37)

> Rob, could you help confirm the screenshot is as what you have in mind?
> Thanks!

This looks good to me. Thanks!
Flags: needinfo?(rmacdonald)
Please make sure this lands before the end of the week to get proper testing ahead of the upcoming deadline.
Target Milestone: 1.1 QE3 (26jun) → 1.1 QE4 (15jul)
Update: I am reviewing my part(Music) and since David is PTO, I will also review the rest parts.
Comment on attachment 771823 [details]
link to https://github.com/mozilla-b2g/gaia/pull/10818

Arthur, this patch looks good. Before you land this patch, there are some minor issues that I found, and the way I suggested you can probably avoid the change in the hidden class, please see the details on github comments, thanks.
Attachment #771823 - Flags: review?(dkuo)
Attachment #771823 - Flags: review?(dflanagan)
Attachment #771823 - Flags: review+
clear the ni because Arthur has moved forward.
Flags: needinfo?(kyee)
Will test the patch when bug 885753 lands.
Comment on attachment 771823 [details]
link to https://github.com/mozilla-b2g/gaia/pull/10818

r-, this changes strings without changing their IDs.

Reviewers, please pay attention to that in your reviews.

Also, is the least-l10n-impact patch to address the issue that block leo?

This bug is *really* late in the game by now.
Attachment #771823 - Flags: review-
Dave - Triage is trying to figure out if we should continue to block on this or not. Some questions on this.

1. Let's say a user selects the SD card with no SD card present. What happens? Does the user get into a bad state? Or do they get an error?

2. Would restarting the Settings app act as a workaround to this problem?

3. Any additional thoughts on if we should or shouldn't block on this?
Flags: needinfo?(dhylands)
I think that what should happen is that the user should get an error indicating that the media is missing.

For the non-leo devices, this is the only storage are that they can select.

The user should either reinsert an sdcard or if they have multiple storage areas (like on the leo) go into setting and change the default storage area to be the internal storage area.

To me, I think that the use SHOULD be able to select the non-existant media, perhaps with a "is this really what you wanted to do" warning. Or if we do make it non-selectable, the user should still be able to see that it exists (i.e. perhaps its grayed out)

Otherwise, I can get the same effect by inserting an sdcard, selecting that as the default, and then removing the sdcard.

I guess this is really a UX decision. In any event the code still has to deal with the sdcard being missing, because the user can remove it at any time.
Flags: needinfo?(dhylands)
no comment in the bug for 7 days.   what are we doing to do?

we need to get english strings locked down, so localization work can start if we plan to take this bug.
This patch depends on Bug 885753 and which is not landed to v1-train yet. I was wondering if it can be landed in time.

Wayne, could you confirm if this is still a blocker for leo?
Flags: needinfo?(wchang)
Moving to leo? in the meantime
blocking-b2g: leo+ → leo?
Discussed with partner offline, they agree to not block on this and postpone for koi.
blocking-b2g: leo? → koi?
Flags: needinfo?(wchang)
Comment on attachment 771823 [details]
link to https://github.com/mozilla-b2g/gaia/pull/10818

Patch updated as comment 45. Axel, could you help review it? Thanks!
Attachment #771823 - Flags: review- → review?(l10n)
Comment on attachment 771823 [details]
link to https://github.com/mozilla-b2g/gaia/pull/10818

Axel will be AFK for most of the next 10 days, trying to cover for him ;-)

The patch looks good from a l10n point of view (no strings changed while maintaining the old IDs).

Two minor nits, feel free to take them into consideration or discard them.

nocard2-title, nocard2-text, nocard3-title, nocard3-text: why not go for completely new IDs and be consistent through all files (e.g. nomemorycard-title, nomemorycard-text)? Numbering IDs should be a last resort.

All apps use "No memory card found" for nocard*-title, Gallery uses "No memory card". Is that difference really wanted?
Attachment #771823 - Flags: review?(l10n) → review+
The don't like the numbering IDs, either. :( In cases we need to modify the the string slightly and it is just difficult to come up with a better ID. It would be great if we can simply mark the IDs that need to be updated and inform the localization team on bugzilla. Then I think we can get rid of this. Do you think it is feasible?
Flags: needinfo?(francesco.lodolo)
(In reply to Arthur Chen [:arthurcc] from comment #54)
> It would be great if we can simply mark the IDs that need to be updated and
> inform the localization team on bugzilla. Then I think we can get rid of
> this. Do you think it is feasible?

Unfortunately no, that's not possible. 

Changing the ID is the only safe way to ensure that a) localizers are aware of the change, b) the current localized string reflects the updated English content. 

That's not just for Gaia but for the whole Mozilla ecosystem, and that's something reviewers should always keep in mind when checking patches that involve strings :-)
https://developer.mozilla.org/en-US/docs/Making_String_Changes
Flags: needinfo?(francesco.lodolo)
Francesco, thank you for the clarification!

master: https://github.com/mozilla-b2g/gaia/commit/a8e784b60054d06fe1448304528110a9761bc05c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Per bug 909632 comment 3, flag hd? here.
blocking-b2g: koi? → hd?
The existing fix in this bug has string changes, we shouldn't take this on 1.1 or hd.
Flags: needinfo?(wchang)
Hi Wayne :
 Please help us to push to merge this case into V1.1 HD. It blocked the Helix Device .If you can not resolve it , please get the authorization from carrier.
We should not take this on v1.1hd given comment 59, and also that this is not on v1.1.
blocking-b2g: hd? → ---
Flags: needinfo?(wchang)
See Also: → 919834
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: