[messages][messagegui] - fix music info handling

[kidneyhex]

Closing this: The fix requires design choices, so I'll just log an issue and let someone else decide. :P

[bobrippling]

I'll hold off reviewing / testing until this is out of draft, but with it being a core app I'll tag @gfwilliams in the meanwhile

[kidneyhex]

The current code is working, but I ran into an app where GadgetBridge isn't sending musicinfo even on play/pause (libro.fm), so the music entry/controls aren't showing up.

If we could just request the info/state, it would simplify things and reduce the need for writing to flash. (I think?)

I see someone else proposed this too (https://codeberg.org/Freeyourgadget/Gadgetbridge/issues/4886), and it wasn't outright rejected, so I might do that this weekend and see if it's accepted, then revise this to use it.

--- I don't have access to an apple device though, so that does complicate things a little.

[bobrippling]

Cool! For that last bit, you could ask in discussions, I think there's a few iPhone users about

[gfwilliams]

I'm really not convinced about requesting state (it feels like a hack to work around a bug somewhere) - I've put some more of my thoughts on this on https://codeberg.org/Freeyourgadget/Gadgetbridge/issues/4886#issuecomment-8622057

edit:

I ran into an app where GadgetBridge isn't sending musicinfo even on play/pause (libro.fm)

This is kind of what I mean - we should fix this in Gadgetbridge (or maybe it's a libro.fm issue?), and then it'll help everyone using it (even those without a Bangle.js).

[kidneyhex]

@gfwilliams The issue I ran into is how to store the music info/state. We might get the music info, but we lose it unless we store it somewhere.

For Libro.fm the issue is audiobook chapters means no new musicinfo for a long time. But even if we had the musicinfo, we lose it as soon as we change apps unless we keep writing to flash. But if you listen to short things like songs or youtube shorts, you change musicinfo ever 30s-3min, so it adds up to a lot of writes to flash.

I wouldn't poll for music info/state, just once on opening messagegui... but then I ran into the "No messages" popup needing to be closed or delayed to show the new music info. Little design choices started adding up beyond what I was comfortable with.

Edit: oooh, wait... Just write to flash on kill events maybe? Set up the kill event handler in the listener and some flag that it was already set up. 🤦

[kidneyhex]

I've updated this with logic to store the music in the require('messages').music, and then on kill event persist it into messages.music.json

Then messagegui uses require('messages').getMusic() to read the latest music info and there's a _lastPlayed flag to help expire old messages.

This still has a debug method in it to count number of times it writes to file--so not really ready for review/merge. Just here for info purposes for now.

[gfwilliams]

oooh, wait... Just write to flash on kill events maybe? Set up the kill event handler in the listener

Yes - to be honest I thought this is what happened already! The idea was always that android/app.js converted the music updates into a message type with id:"music" which are added with require("messages").pushMessage and should have got stored along with the other messages (where there's already code to handle saving to flash only when swapping apps).

But I've just looked into this, and as you say, that doesn't happen. I think it might be the return statement here? https://github.com/espruino/BangleApps/blob/master/apps/messagegui/lib.js#L41

So I'm not sure at what point we stopped storing those and why - it looks like it might have slipped in (perhaps even as a typo) as part of 2a506e7

So ideally, I'd really like to try and fix that rather than having code that deliberately extracts the music messages from the messages list and then writes it into a separate file.

[gfwilliams]

I just tried it, and it does feel like literally just removing that one line (else return;) fixes it?

You can send music updates from the Web IDE by pasting in:

GB({"t":"musicstate","state":"play","position":1,"shuffle":1,"repeat":1})
GB({"t":"musicinfo","artist":"My Artist","album":"Album","track":"A song with a reasonably long name","dur":240,"c":-1,"n":-1})

And then you can use require("messages").getMessages() to get the messages (which should include music), and it persists even after restarting with load()

[gfwilliams]

Just to add, the whole exports.music thing in messages/lib.js appears to be completely pointless. I can't actually see any code that's using it?

edit: Assuming that removing else return; works for you, I can push a change to remove it (as well as the .music) field.

[kidneyhex]

Just to add, the whole exports.music thing in messages/lib.js appears to be completely pointless. I can't actually see any code that's using it?

Yes, according to an old issue about it, exports.music was only still around to be what helps combine musicinfo/musicstate. I don't think it needs to be exposed, or even exist if that happens elsewhere.

edit: Assuming that removing else return; works for you, I can push a change to remove it (as well as the .music) field.

I assumed the idea was to not write to flash too often. Just removing that return makes it write a ton. But I maybe overly cautious about how much wear that'd cause?

But I'm okay with anything, my caution was just trying to decern what the original intent was and not break anything. 😄

[gfwilliams]

Just removing that return makes it write a ton

Are you sure? because I thought the messages app was supposed to be smart enough to only write to flash when apps were switched (but maybe I'm mis-remembering)

[kidneyhex]

I think the messagegui listener writes to flash for every message if you don't have auto-open enabled.

BangleApps/apps/messagegui/lib.js

Line 48 in 702db69

// save messages from RAM to flash if we decide not to launch app

It only does the write on kill in the app.js itself.

[gfwilliams]

Ok, thanks - I'm looking into this.

Can you see any reason why making messages do a delayed write for every message type wouldn't work? I'm setting it up so require("messages").getMessages() would still return the updated messages even if they weren't saved right away.

[kidneyhex]

Yep, I think that'd work fine.