Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add device storage info #64

Merged
merged 3 commits into from
Aug 12, 2024
Merged

Add device storage info #64

merged 3 commits into from
Aug 12, 2024

Conversation

atjn
Copy link
Contributor

@atjn atjn commented Aug 8, 2024

Fixes espruino/BangleApps#2195 by adding the following information:

image

The yellow one is "trash". You can hover/click on each item in the bar to see more information:

image

@gfwilliams
Copy link
Member

Thanks! My concern with this is that if you've got a bunch of stuff in Storage, getStats could potentially take quite a long time, which will slow down the initial connection but also the installation of every new app as you're now calling it (I think?) once for every app on removal of the old one and installation of the new one.

Did you do any tests to see how long it takes? I did try t=getTime();require("Storage").getStats();getTime()-t on a Bangle here and I see 7ms which is basically nothing, but I'd be interested to see whether it is a problem as I think it the past it did tend to be

@atjn
Copy link
Contributor Author

atjn commented Aug 9, 2024

This collection only runs once when the device is connected. Any changes or updates to apps do not trigger the collection again, so that already minimises the problem a lot.

As for how bad the initial connection time is, all I can say is that I ran it many times on the bangle2 that I tested this on, and I did not notice it being slower than usual. When I started testing, 90% of the storage was trash, so (I think?) that proves that it will always be fast no matter how full the storage is.

@gfwilliams
Copy link
Member

Ok, cool, this looks good then - but by renaming getInstalledApps to getDeviceInfo you'll have broken a bunch of stuff in BangleApps unless you change that too. My inclination would be just not to change the name - because even when we rename it in BangleApps there are a bunch of forks of the app loader that we might well screw up.

And IMO in most cases we're calling it to get the list of currently installed apps, so you could argue getInstalledApps is more descriptive than getDeviceInfo

@atjn
Copy link
Contributor Author

atjn commented Aug 12, 2024

I don't think it should be called getInstalledApps when it does more than get the app list. At least I was very confused when I first looked at the code, because I was trying to find the other functions that would get information like device ID, and I did not expect that to happen inside getInstalledApps.

But if it breaks compatibility with other systems, then I think that is more important :)

@gfwilliams
Copy link
Member

Thanks, yes, the naming isn't ideal. Maybe at some point we can put a fix in for it (or maybe have two functions, as many calls do just want to find out the installed apps and not other info), but it's nice to try and keep each PRs as small in range as possible. Merging now!

@gfwilliams gfwilliams merged commit 294690a into espruino:master Aug 12, 2024
1 check passed
@atjn atjn deleted the storage-info branch August 12, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[apploader] [feature request] - free/used memory
2 participants