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

Status: RESOLVED WONTFIX

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

Description

[Julien Wajsberg [:julienw]]

See https://etherpad.mozilla.org/devicestorage for the spec links.

Comment 1

[Julien Wajsberg [:julienw]]

Hey Fred, I assume you'll handle this bug ?

Comment 2

[[e-mail]]

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

Comment 3

[[e-mail]]

[Blocking Requested - why for this release]: late feature

Comment 4

[Matej Novak [:matej]]

"Low Device Storage" sounds a little unclear and awkward. Here are a few other options:

Insufficient Storage Space

Not Enough Storage

More Space Required

Comment 5

[Scott Wu [:scottwu]]

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.

Comment 6

[[e-mail]]

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?

Comment 7

[katieC]

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

Comment 8

[[e-mail]]

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

Comment 9

[Scott Wu [:scottwu]]

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!

Comment 10

[Michael Henretty [:mikehenrty][: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.

Comment 11

[Julien Wajsberg [:julienw]]

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.

Comment 12

[Matej Novak [:matej]]

(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

Comment 13

[katieC]

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.

Comment 14

[Scott Wu [:scottwu]]

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

Comment 15

[Julien Wajsberg [:julienw]]

Not a 2.5+ feature anymore.

Comment 16

[Julien Wajsberg [:julienw]]

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

Comment 17

[Scott Wu [:scottwu]]

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.

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] scottwu:1208893-low-storage-mode-warning > mozilla-b2g:master

Comment 19

[GitHub Autolander Bot]

Attachment: [gaia] scottwu:1208893-low-storage-mode-warning > mozilla-b2g:master

Comment 20

[Scott Wu [:scottwu]]

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.

Comment 21

[[e-mail]]

The code looks good to me.

William could you help check on device to see if it work as expect?

Comment 22

[William Hsu [: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.

Comment 23

[William Hsu [:whsu]]

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.

Comment 24

[William Hsu [:whsu]]

Attachment: FlameKK_Patch32426_Result-01.png

Comment 25

[[e-mail]]

Thanks William for test!

We'll double check through your test steps.

Comment 26

[[e-mail]]

Comment on attachment 8673460 [details] [review]
[gaia] scottwu:1208893-low-storage-mode-warning > mozilla-b2g:master

will review again once above issue is fixed

Comment 27

[Scott Wu [:scottwu]]

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.

Comment 28

[BMO Automation]

Firefox OS is not being worked on