Thanks! There's a lot there so I'll have to go through this later. It looks good but it's a surprising amount of extra code needed.

I think it's probably worth not rewriting it in the messages app (as I imagine that'll be opened for most notifications anyway), but maybe reallocate if the size would grow above 40k (which is the point where storagefile would create a new file)

it's a surprising amount of extra code needed.

Well, "needed"...

The two main bulks are

• exports.load(): it keeps a list of ids while reading messages, so removing/modifying only needs to look at that list, instead of filtering the whole array every time.
• exports.haveNew(): basically the same as load(), except it only keeps a list of new ids. This might be over-optimized, we could just replace it with exports.load().some(e=>e.new). (But I was happily coding away, so I figured I'd optimize a bit for the widget while I had the thing in my mind)

maybe reallocate if the size would grow above 40k

I thought about looking at the size, but the docs says "This requires Espruino to read the file from scratch, which is not a fast operation.". So I figured checking that every time we append a message would kind of defeat the purpose. Unless most of the delay comes from parsing JSON?

Also I wasn't sure if every event definitely has an id set, if so It would probably simplify some things.

I rebased it onto master, and got rid of separate exports.haveNew() code: we don't check for new messages often enough that it needs special optimized parsing.

Thanks! I will get on this, I'm just pretty busy at the moment

Sure, there's no rush, and I definitely want you take a good look at changes like this before merging.

Still TODO: look into the numbers, come up with a sensible value to compare to StorageFile.getLength to determine whether to compact the messages file (and maybe make it smart enough to check if compacting will help at all)

Rebased it onto master, and got rid of compact(): it already rewrites the entire file every time we close the app anyway, so I figured that keeps it small enough for now.

But we can probably do better:

1. Instead of saving an updated MESSAGES, just append changes to the file, and only rewrite it when it gets too large
2. Make it even smarter: keep track of changes, and only write messages that need updating when leaving the app. (Alternatively: compare the in-memory MESSAGES with the saved file.)

untested: updated to keep the StorageFile around until it grows to 40000bytes.

Looks great - thanks! I'll have a play with this branch on mine and see how it's working :)

If you've got some time now that's great, but note that I really meant the untested bit for my last changes: I think it should work, but haven't gotten round to actually testing it, at all.

I tested it a bit in the emulator and fixed a few bugs, seems to work OK now:

• I uploaded messages/lib.js as messages, loaded android/boot.js into RAM, and pasted gbridge/sample_messages.js into the REPL. After that I checked require('messages').load(), and it looked good.
• After pasting in enough copies of the sample messages for messages.jsonl to grow over 40000bytes, it erased the file and started a fresh one without losing any messages.
• Doing

    M = require('messages').load();
    /* modify M: delete/modify messages */
    require('messages').write(M);
  

  only added lines for the deleted/modified messages.

(Edit: I'm planning to merge this into the code running on my watch this weekend for some real-world testing, but that probably won't hit 40KB for a while ;-)

Ok, great! Maybe it's best to hold off on merging this just a little more then until you've been running with it on the watch itself for at least a few days.

Just thinking aloud here but the actual file size is ~40930 bytes I think? So if you do a check at 40000 and then you add another 930 bytes of messages, a new file would get created anyway. I think it's unlikely but I guess we could drop the check to 38000, in which case it would be hugely unlikely that any one message would push it into a second file.

I'd also be interested to see if you notice any slowness when exiting the messages app. While I like the idea of the .write function, I guess in a lot of cases (marking read/deleting/etc) it'd be pretty easy to just write the extra line directly into the messages file without having to parse the entire thing. It also avoids having to read the whole file on exit even if nothing has changed, and potentially opens the way for not actually ever having to load all messages into RAM (we could just parse the file and read the single message out as and when it was needed).

... but maybe that's one for later when we've got this merged ;)

if you do a check at 40000 and then you add another 930 bytes of messages, a new file would get created anyway.
we could drop the check to 38000

Good point, I'll adjust that.

in a lot of cases (marking read/deleting/etc) it'd be pretty easy to just write the extra line directly into the messages file

Sure, but in most cases you probably open the app, open a message (marking it as read), and then delete it, so you end up writing a copy of the message except with new=false, and then delete it anyway.
But we could do this:

• write deletions to the file right away
• keep track of changed messages, only write those on exit

That way we don't need to parse the file on exit.

Could do, but probably not worth the extra effort just yet. Let's set your this PR goes.

Taking of which I used this branch and put in on my bangle and something is very wrong - short buzz and no notifications at all. But I'll have to look into it on Monday

Turns out it wasn't that much effort, so I added it before reading your comment.
I guess somewhere along the line something broke during some merge/rebase :-(
I just rebased onto master, figured the easiest way forward was to just fix merge conflicts in the rebase as best as I can, and now try to stamp out the bugs. (Merging in half a dozen options every now and then probably wasn't great for the code)

I gave it a try on my Bangle.js 1, but didn't have time to properly debug stuff yet.
Apart from other issues, I just noticed my messages.jsonl now contains exactly this line:

{"t":"modify","track":"\u0000\u0012\u0015\u0001\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u000F\u0000\u0003\u00F0\u0000\u00FF\u0000?\u00F0\u000F\u00F8\u0003\u00F8\u0000\u00F8\u00008\u0000\b\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000 Body Machine | Priest","dur":-1,"c":-1,"n":-1,"id":"music","title":"Music","new":true,"state":"play"}

That's from Firefox playing media being forwarded to my phone, so the "Body Machine | Priest" bit is fine, but all the \u0000 and the fact that there is exactly one line worries me.

Actually, I think what you have is fine... {"t":"modify","track":"\u0000\u0012\u0015\u0001... means Gadgetbridge has found characters it can't display and has instead sent a bitmap inside the string (they start with \0) - so it's a 12x15 px 1bpp bitmap.

I've just been pasting in some test messages, eg GB({"t":"notify","id":1575479849,"src":"Hangouts","title":"A Name","body":"message contents"}) - and I saw that for me the message seems to be stored ok but it doesn't load the messages app or forward the message to the widget for some reason... So something still doesn't look quite right to me...

Doh, the library code had an internal function named load(), changed it and now it does open the app.
I'm (on my Bangle.js 1) running into a bunch of LOW_MEMORY,MEMORY though, not sure if that's just me testing with too large messages/too much stuff installed, or some bug.

Ahh, that'd do it! Although if you're not in a clock, do message icons still appear? That didn't work for me either.

Do you not get out of memory errors with the current latest messages app? I've noticed the newish message icon code appears to keep the entire messages list in RAM, which is pretty wasteful.

How many messages did you have? It doesn't look to be like your code should use that much extra memory.

Thanks - I'll file an issue for that as there should really be a way to just query the messages library/widget

Strange, I know I tested filling up the storage file before, and it worked fine. But today I finally filled up my watch "naturally",and it broke :-(

Fixed now:
Turns out you can't do this (simplified a bit):

    const file = require("Storage").open("messages.jsonl", "a");
    if (file.getLength()>38000) {
      file.erase();
      file.write(JSON.stringify(e)+"\n"); // throws "Can't write in this mode"

So I changed it to

    let file = require("Storage").open("messages.jsonl", "a");
    if (file.getLength()>38000) {
      file.erase();
      file = require("Storage").open("messages.jsonl", "a");
      file.write(JSON.stringify(e)+"\n");

and now it works again.

(I'm not surprised that writing to an erased file throws, just that it used to work before, maybe I broke it during some merge/rebase...)

Ahh, interesting, thanks! It's good it fails though - that could have led to storage corruption before.

I'm pretty sure you don't actually need an erase. You can just use w and it'll erase as part of creating the new file:

file = require("Storage").open("messages.jsonl", "w");

Rebased it onto master.
I notice this PR has accumulated quite a bit of formatting cruft, I'll see about cleaning that up.

Rebased onto master (i.e. message events).
(whoops, hit the "close" button instead of "comment")

Set it back to draft, as it will need a bit of work(+more testing) to apply onto #2205

Some good news: now that the messages library can be extended, I am using this code as daily driver for my smessages custom messages app, so the chance of me running into any lurking bugs is higher :-)

I am using this code as daily driver for my smessages custom messages app, so the chance of me running into any lurking bugs is higher :-)

You mean you're using append-storagefile?

Thanks! and sorry It's taken so long for me to test/merge this one in - it just feels like quite a big one :)

You mean you're using append-storagefile?

I have been for quite a while now, just not this exact code:
My own (still WIP) fork already uses the storage file. The difference is that now the message events PR landed, it uses require("message") (the version in this PR) instead of duplicating a bunch of code.

Thanks! and sorry It's taken so long for me to test/merge this one in - it just feels like quite a big one :)

I totally understand, it's a big change and we definitely don't want the messages app to break.
And waiting a bit doesn't really bother me: I can just load this code onto my watch anyway ;-)

Putting this on hold for a bit, at least until we have a stable require("messages") API.

I guess we do now have a more stable API - and I think we could in fact just create a separate messages_sf app now that would allow both the old and new to coexist?

However, do you think there's a need for this now? I've made a few changes to storage in the last 6 months that has made slowdowns from lots of file writes basically disappear completely - even completely uncompacted it should be basically as fast as right after a compact.

By contrast I'm a bit concerned that reading the messages line by line from a StorageFile may actually end up being slower than the current solution.

With your recent changes to the library it seems we have the option to not save messages too, and we already fast-load the messagesgui, so with a little more work I think we could probably avoid saving the messages to Storage in 90% of cases (eg. when getting a message when on the clock, dismissing it, and going back to the clock).

we could in fact just create a separate messages_sf app now that would allow both the old and new to coexist?

We could, but I don't think there would be much point to having two libraries providing the exact same functionality. And it would complicate things whenever anybody has a problem with messages.

do you think there's a need for this now? I've (...) made slowdowns from lots of file writes basically disappear completely
(...)
reading the messages line by line from a StorageFile may actually end up being slower

Well, that definitely sounds like there isn't any need for this anymore 🙂

I think we could probably avoid saving the messages to Storage in 90% of cases

Yes, now that we have fast loading, that definitely seems like the way to go.

(Also: we should optimize it to avoid writing a new file when nothing has changed, or at least just delete the file instead of writing [])

Thanks for all your work on this - I know it's taken ages and I'm sorry not to have merged it after all that.

(Also: we should optimize it to avoid writing a new file when nothing has changed, or at least just delete the file instead of writing [])

That's a great idea! I've filed a new issue at #2370 to keep track of these.

Storage.write (and writeJSON) should be smart enough not to overwrite a file if the contents are the same, so we don't have to worry about that - but in the very common case of having no messages, then getting a message and dismissing it, not writing [] could save us half the writes!