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

[gbmusic] Fix #3737 and a handful of other fixes #3740

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

thinkpoop
Copy link
Contributor

@thinkpoop thinkpoop commented Feb 8, 2025

App Link: https://thinkpoop.github.io/BangleApps/?id=gbmusic

  1. Fix for [gbmusic] scrolling track titles cause track numbers to not clear #3737

I'll try to save way to recreate this, in case anyone wants to work on figuring out the root cause.

  1. Fix saved.state being a string instead of an object

boot.js around line 31 saves the e.state instead of e.
app.js function state(e) expects an object though, so it was saving stat=e.state, and stat was ending up undefined.

  1. Send "playpause" if stat is not "play".

This way, if stat is unknown/undefined, gadgetbridge can figure out what to do. gbmusic could always send playpause and that would work too.

  1. Ignore touches if they're in the widget area.

Opening message notifications was pausing music on me. I'm not sure if I should add the bottom widget bar too? (I'm assuming it'd be pos.y <= Bangle.appRect.y2?)

  1. Fix lint warnings (and use globalThis instead of global in the boot.js).

@thinkpoop
Copy link
Contributor Author

This PR to fix existing issues before I submit a couple possible enhancements (config setting for pause-timeout, and adding thumbs up/down commands).

Let me know if you'd prefer I combine any of these into this PR to cut down on updates.

@thinkpoop thinkpoop marked this pull request as ready for review February 9, 2025 02:18
@thyttan
Copy link
Collaborator

thyttan commented Feb 12, 2025

@rigrig if you'd like to comment since you wrote most of this app.

@rigrig
Copy link
Contributor

rigrig commented Feb 12, 2025

Thanks for pinging me.

I haven't used/looked at this app for quite some time though. It's pretty old, so I'm not surprised it bitrotted. (Does it even play nice with e.g. the Message UI app? Otherwise it might be time to mark it as deprecated.)

One general comment: please delete unused code instead of commenting it out: it clutters up the file, and we have git to keep track of history.

@thinkpoop
Copy link
Contributor Author

Deleted the commented code.

I can't remember why I ended up on gbmusic.. a mix of testing random apps, and maybe it was easier to modify with my own tweaks?

It's been my go-to for a few months though.

The pause/inactive timeouts being so long have been my only remaining complaint -- I was going to add a settings options to tweak them. But it's been buggy so I'm holding off on that.

function togglePlay() {
sendCommand(stat==="play" ? "pause" : "play");
sendCommand(stat==="play" ? "pause" : "playpause");
Copy link
Collaborator

@thyttan thyttan Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to use "playpause" than "play", I believe that's the reason for the change? I guess it handles if the recorded state on watch is out of step with the actual state on the android device?

I think this goes to .../GBMusicControlReceiver.java in gadgetbridge, am I correct?

"playpause" should maybe be added to the list of possible values for the command parameter for the sendCommand function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that is correct. I prefer "playpause" but wasn't sure if there was a reason not to, so was minimizing the change.

Usually if it knows the state is play, it's correct. The issue mostly comes up when manually opening the app and it hasn't received a musicstate update yet, it gets stuck sending "play" when it's already playing.

And I've updated the jsdoc comment to list possible values.

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.

3 participants