BangleApps icon indicating copy to clipboard operation
BangleApps copied to clipboard

[messages] is 'music' export needed?

Open gfwilliams opened this issue 3 years ago • 8 comments

@rigrig I've just been looking through the messages library and I see we still have music export with the currently playing track info in.

But as far as I can see, this isn't used by anything now? Any playing music is treated as a new item in the messages list (which messagesgui uses), and since we're only saving it on exit now, it's not a big deal for us updating it.

Or am I missing something?

gfwilliams avatar Dec 15 '22 10:12 gfwilliams

It's used to combine GB({"t":"musicinfo" and GB({"t":"musicstate" messages: we get musicinfo for every track change, but musicinfo only when music starts/stops.
This way (almost) all {id:"music"} messages have both "state" and "track" fields present.

rigrig avatar Dec 15 '22 10:12 rigrig

Ok, thanks! Are you sure that's needed now though? Because it looks like we should really just store what's needed in the messages list?

... or maybe if it is needed, that code should go into the Android library as I guess it's not needed for iOS

gfwilliams avatar Dec 15 '22 10:12 gfwilliams

that code should go into the Android library

You're right, ios already pushes {id:"music"} directly, so I guess the whole musicstate/musicinfo split should just be handled inside android.

And looking at ios, I wonder whether it even includes "state" info? (It uses NRF.amsGetMusicInfo(), which isn't in the documentation...) If not, I suspect auto open might just be broken for iOS, because the UI bootcode waits for both state and title to be present.

rigrig avatar Dec 15 '22 11:12 rigrig

Yes, that's interesting - I don't know about iOS - maybe that doesn't work...

With the music var though, I'm still not sure it's needed? Without it, you get the two messages pretty close, so surely you'd get:

GB({"t":"musicstate","state":"play","position":0,"shuffle":1,"repeat":1})
// messages now contains {"id":"music","state":"play","position":0,"shuffle":1,"repeat":1}
GB({"t":"musicinfo","artist":"My Artist","album":"My Album","track":"Track One","dur":241,"c":2,"n":2})
// messages now contains {"id":"music","state":"play","position":0,"shuffle":1,"repeat":1,"artist":"My Artist","album":"My Album","track":"Track One","dur":241,"c":2,"n":2}

Or similar - I can't remember exactly what goes in, but you get the idea. Even if you jump to the messages app on the first message, you'd hope it would handle the changed message and then decide it should then display it?

gfwilliams avatar Dec 15 '22 11:12 gfwilliams

With the music var though, I'm still not sure it's needed?

Oh right, it already combines them because they are {t:"modify"}. You're right, we don't need to keep special track of music.
(I think it's a leftover from trying to only emit/handle music with both state and info, but UIs should just be able to handle "incomplete" messages)

I can't remember exactly what goes in

Me neither, that's why I added sample_messages.js a while ago 😉 (should probably move that to android)

rigrig avatar Dec 15 '22 13:12 rigrig

Ok, great. I'll leave this issue open then and will have a go at getting rid of that.

Me neither, that's why I added sample_messages.js

I didn't spot that! I have one (not committed!) here as well.

In the Espruino repo we have something where some actual tests are run on the factory apps in an emulator: https://github.com/espruino/Espruino/tree/master/tests/FactoryApps

How that's done isn't ideal, but I can't help thinking that we should probably have something in BangleApps - and specifically the whole messages system is so big it'd be really nice to have a clear set of tests so it was obvious if something broke.

gfwilliams avatar Dec 15 '22 13:12 gfwilliams

specifically the whole messages system is so big it'd be really nice to have a clear set of tests so it was obvious if something broke.

Yes, that would be nice.

rigrig avatar Dec 15 '22 14:12 rigrig

I've just added the basis of something at b89a368bd996b3cbfdbf728c9af8d9fa51329472 but not sure I'll get much further with it this year - hopefully at some point we'll have something usable though

gfwilliams avatar Dec 16 '22 10:12 gfwilliams