messages: pass all messages through "message" event #2205

rigrig · 2022-10-28T17:29:05Z

Instead of directly calling from the lib into the app/widget, always use "message" events.
Also adds some optimization so we only read messages from storage once per event.

This also gets rid of the update method, see https://forum.espruino.com/comments/16740860/

Marking this as draft because it's friday night, and I don't have time to properly test it, but other than that it is finished ;-)

gfwilliams · 2022-11-15T10:58:45Z

apps/messages/widget.js

@@ -51,5 +53,6 @@ WIDGETS["messages"]={area:"tl", width:0, draw:function(recall) {
 /* We might have returned here if we were in the Messages app for a
 message but then the watch was never viewed. */
 if (global.MESSAGES===undefined)
-  WIDGETS["messages"].update(require("messages").getMessages());
+  WIDGETS["messages"].onMsg('',{});
+  Bangle.on("message", WIDGETS["messages"].onMsg);


If MESSAGES is defined and we're in the app, don't we still want to register our Bangle.on("message" handler?

I think I'm not really sure what's going on here actually...
Do we even want to load the widget while in the messages app?

That's a good question, maybe not... I just pushed a change to the messages lib that makes it 'fast load' the app if it can (which is really neat because it makes it much more responsive). But, that means that if the messages widget was already loaded, it'll stay loaded - so would have to cope with being around for the messages app anyway.

gfwilliams · 2022-11-15T10:59:44Z

Marking this as draft because it's friday night, and I don't have time to properly test it, but other than that it is finished ;-)

How are you feeling about this now? Did you have a chance to test it properly?

Apart from that event handler registering I just posted, this looks good to me

rigrig · 2022-11-15T11:56:35Z

Did you have a chance to test it properly?

Whoops, I totally forgot, sorry. I'll have a look at it tonight.

rigrig · 2022-11-15T20:08:57Z

Ran into a bug where msgs wasn't set, no idea when I broke that, but fixed now.
Also rebased, and added a __FILE__ check to hide the widget while the app is open

rigrig · 2022-11-16T09:54:46Z

Converted back to draft: this morning I didn't get any music notifications, so I'll have to look into that first :-(

gfwilliams · 2022-11-16T10:22:36Z

Thanks! This is looking good though

rigrig · 2022-11-16T18:59:17Z

Fixed it: there was a race when two events arrived in a row(e.g. musicinfo+musicstate), causing the first one to clean up MESSAGES while the second was still using it. Changed it so that a) that doesn't happen anymore, and b) it won't break even if it does happen.

Even to the default app/widget

Basically always keep MESSAGES in memory until event handlers have had a chance to run, so they don't all cause Storage.read()s when they call require("messages").getMessages()

Happens for music: we get both a musicinfo and a musicstate event, and when the first one finishes processing, it cleans up MESSAGES before the next event is done. Also just use exports.getMessages() to make sure it won't break.

rigrig · 2022-11-16T19:13:43Z

One thing I just thought about: it now saves all messages before emitting the event.
Should it maybe not do that? Something like if any listener sets event.handled, only store the message if it also sets event.save?
That could be a bit tricky though, as other listeners (like the widget) will assume the message now exists...

gfwilliams · 2022-11-17T09:06:27Z

One thing I just thought about: it now saves all messages before emitting the event. Should it maybe not do that?

Argh, yes - that's a pain. That could be problematic then.

I thought the idea was that stuff like the music app could intercept the message before it got to be stored, so it didn't end up in the list of messages.

What about updating the message list, and then instead of having it as a global MESSAGES we just pass it as an argument to the event (it might be better to make it a one-argument event that passes an object, so it's more readable/flexible?).

Then we only write the message to storage if the message isn't handled, but things like the widget still get access to the full (new) list of messages?

rigrig · 2022-11-17T09:46:20Z

I thought the idea was that stuff like the music app could intercept the message before it got to be stored, so it didn't end up in the list of messages.

It was, but apparently I lost track of that somewhere.
(And it's a bit tricky, because we do need to store the first one before launching the app)

What about updating the message list, and (...) pass it as an argument to the event

I guess that makes sense, as we're loading MESSAGES anyway.
But ideally we wouldn't need to load them all to handle incoming events: usually a listener is going to launch the app or ignore the event anyway. (Widgets could load them once at startup, and then keep track of event srcs internally, instead of re-filtering them all every time.)

make it a one-argument event that passes an object, so it's more readable/flexible?

I could do that if you want, although I'm not really a fan of the idea. I think it makes sense for arguments with optional fields, but if we're always passing (type,event,all) arguments, I don't see the benefit of wrapping those in an object?

How about for now I'll adjust this PR so

it passes the full message list in events
if an event is handled, it won't be saved unless store is also set
music events are not stored by default, unless we are going to launch the app

And a follow-up PR to try and separate the "default" code from the library.

gfwilliams · 2022-11-17T14:39:36Z

if we're always passing (type,event,all) arguments, I don't see the benefit of wrapping those in an object?

It's just for readability really - but yes, let's keep it with args for now.

How about for now I'll adjust this PR so

That sounds great. but...

music events are not stored by default, unless we are going to launch the app

You mean the messages app? Because at the moment the messages app needs to store them...

rigrig · 2022-11-17T15:49:43Z

at the moment the messages app needs to store them...

Urgh, juggling too many message apps in my head.
I guess for now no special-casing for music events. (But aiming for the app to eventually mark them as handled and keep them only in memory until kill.)

rigrig · 2022-11-21T13:14:09Z

Closing in favour of #2297
(Seemed cleaner to start a new branch)

rigrig mentioned this pull request Oct 28, 2022

Save messages in append-StorageFile #1498

Closed

gfwilliams reviewed Nov 15, 2022

View reviewed changes

rigrig force-pushed the message-events branch from 5c64341 to 4ceef51 Compare November 15, 2022 20:02

rigrig marked this pull request as ready for review November 15, 2022 20:03

rigrig marked this pull request as draft November 16, 2022 09:53

rigrig marked this pull request as ready for review November 16, 2022 18:56

rigrig added 4 commits November 16, 2022 20:05

messages: pass all messages through "message" event

2528b5f

Even to the default app/widget

messages: optimize MESSAGES usage

8af8a98

Basically always keep MESSAGES in memory until event handlers have had a chance to run, so they don't all cause Storage.read()s when they call require("messages").getMessages()

messages: some fixes, hide widget while app is open

f4adebc

rigrig force-pushed the message-events branch from 5735335 to e8bf4be Compare November 16, 2022 19:08

rigrig marked this pull request as draft November 17, 2022 09:22

rigrig marked this pull request as ready for review November 17, 2022 09:31

rigrig closed this Nov 21, 2022

rigrig deleted the message-events branch November 28, 2022 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

messages: pass all messages through "message" event #2205

messages: pass all messages through "message" event #2205

rigrig commented Oct 28, 2022

gfwilliams Nov 15, 2022

rigrig Nov 15, 2022

gfwilliams Nov 15, 2022

gfwilliams commented Nov 15, 2022

rigrig commented Nov 15, 2022

rigrig commented Nov 15, 2022

rigrig commented Nov 16, 2022

gfwilliams commented Nov 16, 2022

rigrig commented Nov 16, 2022 •

edited

rigrig commented Nov 16, 2022

gfwilliams commented Nov 17, 2022

rigrig commented Nov 17, 2022

gfwilliams commented Nov 17, 2022

rigrig commented Nov 17, 2022

rigrig commented Nov 21, 2022

messages: pass all messages through "message" event #2205

messages: pass all messages through "message" event #2205

Conversation

rigrig commented Oct 28, 2022

gfwilliams Nov 15, 2022

Choose a reason for hiding this comment

rigrig Nov 15, 2022

Choose a reason for hiding this comment

gfwilliams Nov 15, 2022

Choose a reason for hiding this comment

gfwilliams commented Nov 15, 2022

rigrig commented Nov 15, 2022

rigrig commented Nov 15, 2022

rigrig commented Nov 16, 2022

gfwilliams commented Nov 16, 2022

rigrig commented Nov 16, 2022 • edited

rigrig commented Nov 16, 2022

gfwilliams commented Nov 17, 2022

rigrig commented Nov 17, 2022

gfwilliams commented Nov 17, 2022

rigrig commented Nov 17, 2022

rigrig commented Nov 21, 2022

rigrig commented Nov 16, 2022 •

edited