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

Adds gbridge call notification and refactor widget #155

Merged
merged 5 commits into from Apr 3, 2020

Conversation

detached
Copy link
Contributor

The current gbridge Android app already sends call notifications. This change just adds notifications for incoming calls.

I refactored the widget to be more readable, please have a critical look on my code as I am not very experienced with js.

@gfwilliams
Copy link
Member

Thanks! It looks good - it's nice having the global drawContent function to handle all different notifications.

Just because we're on a device without too many resources (and gadgetbridge is running in the background with other apps in the foreground), there are a few things that I'm not sure are a good idea...

Basically, on Bangle.js function code is stored and executed from Flash memory, so doesn't use any RAM. Stuff that does use RAM is any variables/objects (even const) and each function declaration (just not the code in it).

  • Stuff like musicState.STOP is defined and used once. It makes it harder to see what the code is actually checking against, but it's using extra RAM in order to store the whole musicState structure. I guess if we moved to minification then the minifier would undo that for us though.
  • I wonder about making the images external vs. inline, especially for small ones? I know it does make code a little more readable, but where it's only ~100 bytes of code I'm not sure if it's really that much of a win, as having to search for and load another file first slows things down

There's another PR (#130) for handling fullscreen notifications which abstracts stuff in a similar way, and I think perhaps this would be a good time to actually split Gadgetbridge up - having the main gadgetbridge handling just the decoding, with another (choosable) app handling how those notifications are displayed.

I'll leave this and the other PR open until I get a moment to merge both

@gfwilliams
Copy link
Member

Specifically: #162

@detached
Copy link
Contributor Author

Thanks for your time!
I wasn't aware that the variable declarations are affecting the RAM usage so much. I will look into this in the next days.

@detached
Copy link
Contributor Author

detached commented Apr 2, 2020

I inlined the variables again and restored most of the notification code. That should make it easier to integrate it with the other PRs.

@gfwilliams gfwilliams merged commit 9f3ecbc into espruino:master Apr 3, 2020
@gfwilliams
Copy link
Member

Thanks for this! Just merged.

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.

None yet

2 participants