Closed Bug 1208893 Opened 5 years ago Closed 2 years ago

[Settings] Add a warning text in Low Storage mode to the storage panels

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: julienw, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

Hey Fred, I assume you'll handle this bug ?
Flags: needinfo?(gasolin)
Attached image visual spec
Thanks julien for creating the bug. I'll do it or find someone to do the favor.


@matej here're new UI strings on settings app storage page, could you help confirm the wording is correct?
Flags: needinfo?(gasolin) → needinfo?(matej)
[Blocking Requested - why for this release]: late feature
blocking-b2g: --- → 2.5?
feature-b2g: --- → 2.5+
"Low Device Storage" sounds a little unclear and awkward. Here are a few other options:

Insufficient Storage Space

Not Enough Storage

More Space Required
Flags: needinfo?(matej)
I get a feeling "More Space Required" implies that you could be getting more somehow. I think "Insufficient Storage Space" is the most accurate though more wordy. "Not Enough Storage" is plain and good for me too, if the first one is too wordy.
Assignee: nobody → scwwu
Thanks Matej! I'd prefer `Not Enough Storage` or "Insufficient Storage Space" as well.

Katie, could you help pick one and update the string to other place?
Flags: needinfo?(kcaldwell)
Thanks Matej for the review and suggestions! Agreed that the term 'device storage' is slightly awkward and technical.

I also agree with Scott, in comment 5, that "More Space Required" suggests to the user more space is somehow available to get or add. 

The warning should imply the severity of the condition, as immediate action IS required. 
UX Recommendation: 

"Critically Low Storage" (for 2.5 Low Storage threshold: less than 5mb)


................
Note: Ultimately, there will an earlier warning for the user (a 10MB threshold, before immediate action is required). This would allow a simpler title of: 
"Low Storage" (Low Storage threshold: less than 10mb)
"Critically Low Storage" (Low Storage threshold: less than 5mb)

Matej - copy review thoughts? :)
Flags: needinfo?(kcaldwell) → needinfo?(matej)
Scott here's the low-disk-space API https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/device_storage_watcher.js#L159

We could add that into app_storage module and expose it as an observable parameter
Thanks Fred & Katie for the input.

I wonder if it's necessary to implement this with the new lowDiskSpace API? In the settings panel we'd be getting the free space info anyway and could use that instead. From what I see the API is a bool property, and I'm not sure if true means free space < 10mb or < 5mb either.

Michael, I saw that you've implemented a system feature using the new API. What's your experience using it? Do you see benefits in using the new API in this case? Thanks!
Flags: needinfo?(mhenretty)
(In reply to Scott Wu [:scottwu] from comment #9)
> Michael, I saw that you've implemented a system feature using the new API.
> What's your experience using it? Do you see benefits in using the new API in
> this case? Thanks!

For bug 1201930 I actually didn't use the new API, but instead relied on change events from the device storage API (navigator.getDeviceStorage('apps').addEventListener('change', ....). If you're already listening for those events (which I assume you are in Settings), then the explicit `lowDiskSpace` query is less useful IMO.
Flags: needinfo?(mhenretty)
Actually, the query is still useful. If you only listen to the event, you don't know the condition you are in at app's startup. That's exactly why we needed the `lowDiskSpace` property...

The System app does not have this issue of course, because it's always running.

But we need both ! Checking the property at startup, and listening to the "change" event to be able to react if the condition change while the app is running.


Currently the bool property is "true" when we entered the low storage condition (< 5 MB is left) but we haven't left it yet (we need to go above 10 MB to exit the low storage condition). That's why checking the free space only is noy enough: for example when 7MB is left, you don't know where you are.

Eventually, < 10MB will trigger the "low storage" (vs < 5MB will still trigger the current "critical low storage"), and we'll use an enum for the "lowDiskSpace" property (and likely change its name as well).


Please ping me if you need more information about how this works.

That said, the feature is not 2.5+ anymore so either it should not land before the branching, or land behind a flag.
blocking-b2g: 2.5? → ---
(In reply to katieC from comment #7)
> Thanks Matej for the review and suggestions! Agreed that the term 'device
> storage' is slightly awkward and technical.
> 
> I also agree with Scott, in comment 5, that "More Space Required" suggests
> to the user more space is somehow available to get or add. 
> 
> The warning should imply the severity of the condition, as immediate action
> IS required. 
> UX Recommendation: 
> 
> "Critically Low Storage" (for 2.5 Low Storage threshold: less than 5mb)
> 
> 
> ................
> Note: Ultimately, there will an earlier warning for the user (a 10MB
> threshold, before immediate action is required). This would allow a simpler
> title of: 
> "Low Storage" (Low Storage threshold: less than 10mb)
> "Critically Low Storage" (Low Storage threshold: less than 5mb)
> 
> Matej - copy review thoughts? :)

+1
Flags: needinfo?(matej)
Great! I'll update the strings in the specs and let Amy know for the Visual Design spec too. 

Please change strings from 'Low Device Storage' to 'Critically Low Storage'
Impacts:
• Settings App
• System App
• Message App
• Dialer App

I'll track down related bugs and add this note.
(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #11)
> But we need both ! Checking the property at startup, and listening to the
> "change" event to be able to react if the condition change while the app is
> running.

In settings, we need to display total space & used space & free space, so we check them at panel startup, and recalculate whenever a change occurs.
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/modules/app_storage.js#L73

Since we have the free space information from .freespace() method (down to byte), both at startup and on change, I'm still not sure the reason to involve another DeviceStorage API, especially when it's a temporary solution.

Furthermore, since we know exactly how much free space is left, we can already tell if we are in 'low storage' or 'critical low storage' state, without having to wait for the upcoming enum implementation.

Not sure if I'm missing anything. Do you think there's any other implication doing it this way? (performance, etc).

Thanks a lot Julien!
Not a 2.5+ feature anymore.
feature-b2g: 2.5+ → ---
Re: comment 14, let me explain better in a more concrete way :)

When I say "you", I mean "the user".

1. You use your device.
2. The device has more than 10 MB left on his "apps" partition.
=> Everything is awesome.
3. Then you install an app that makes the user go below 10MB.
=> You enter the "low storage" state and we display a message, but the phone still works fine.
4.You install more apps, and you go below 5 MB.
=> Then you enter the "critically low storage" state, we display a message and more, and several functionalities are disabled.
5. Now you delete some apps and you go above 5MB, but stays below 10MB. 
=> You are still in the "critically low storage" state !!
6. You delete more apps and go above 10 MB.
=> You leave the "critically low storage" state.

(Note: The "not critically" low storage state was not supposed to be implemented in 2.5).

So as you see, when you're betwee, 5 and 10 MB, you don't know which state you're in. I didn't implement the initial behavior as an hysteresis, it's here for some versions already. But that shows you why using the property is important.


We may want to change it but we'll need to ask Fabrice why it's been made like this in the first place. I personally think the hysteresis is useful in case we're very close to the limit, we may cross it both ways too much. And anyway, if you use properly the property, it will be easy to change it in one place only :)


I hope it's clearer now ! It took me some time to understand it fully as well. You can refer to Gecko's implementation in:
* https://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkDiskSpaceWatcher.cpp
* https://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#3605
* https://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/DeviceStorageStatics.cpp#618
Thanks for the detailed explanation Julien :)

I'm in complete agreement with the use case and flow. I guess my question was on the implementation details. My point was that it could be accomplished in a different way, although I do see the benefit in using the new low storage API. I've got a working patch with just a few lines of new code, and will put it up for review very soon.
Attachment #8672970 - Attachment is obsolete: true
Comment on attachment 8673460 [details] [review]
[gaia] scottwu:1208893-low-storage-mode-warning > mozilla-b2g:master

Please take a look at the patch. I don't have a way to actually test the lowDiskSpace though. So far I've just simulated it by changing the value manually.
Attachment #8673460 - Flags: review?(gasolin)
The code looks good to me.

William could you help check on device to see if it work as expect?
Flags: needinfo?(whsu)
Hi, Norry,

Please assign a test engineer to verify local patch (Comment 20).
Also, please inform Lancy to check if these new L10n strings are added to 2.5 L10n test cases.

Thank you.
Flags: needinfo?(whsu) → needinfo?(fan.luo)
Hi, Fred and Scott,

I spent some spare time to do smoketest.

The patch doesn't work. No warning message displayed on Application storage page.
Could you help find root cause?

Here are my test steps.
Step 01. Pull code from pull request-"32426", and merge code on master branch
        $ git checkout master
        $ git pull
        $ git fetch origin pull/32426/head:1208893-low-storage-mode-warning
        $ git merge 1208893-low-storage-mode-warning

Step 02. Use command to generate dummy files.
        e.g., $ dd if=/dev/zero of=testfile_1GB bs=1000000000 count=1 

Step 03. Push files to “/data” to fill application storage
        $ adb push “FILENAME” /data/

Step 04. Check warning message on Application Storage page.

* Actual result: As attachment (FlameKK_Patch32426_Result-01.png), no warning message displayed on Application Storage page.
Flags: needinfo?(fan.luo)
Thanks William for test!

We'll double check through your test steps.
Comment on attachment 8673460 [details] [review]
[gaia] scottwu:1208893-low-storage-mode-warning > mozilla-b2g:master

will review again once above issue is fixed
Attachment #8673460 - Flags: review?(gasolin)
There seems to be a bug with the lowDiskSpace API. From System it is showing the correct status (True when free space < 5MB), but in Settings it's not (always False).

I've raised a question on Bug 1204618 last week, now waiting for a response.
Depends on: 1221086
Assignee: scwwu → nobody
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.