Fix(music): mpris - handle no active mpris player on launch & hide label when player list is empty. #481
Conversation
slowsage
changed the title
I'll double check this later, but it's very possible it's not implemented EDIT: Ignore the closure, fat fingered the wrong button. |
|
Change made. Thank you! |
|
Please hold off on committing it. There might be an issue. Want to test for a few days more. Thank you. I will comment here once confirmed. |
|
Ready for review. Added more checks as I found them. Possible issue: I removed the status playing check on new player added since it complicated the code. It seems to work fine without it since the active player gets set on the playing/progress event. But this might be a function of my app. Can add it back if you prefer. Apart from the above issue, it seems to handle all edge cases on my setup with at most a second of delay after a player is terminated. Please take a look and possibly even run this for a few days/weeks if you have time. Do not want to rush this since user environments vary and concurrency is involved. Thank you!!! |
There was a problem hiding this comment.
Only 1 tiny nit if you get a chance, but otherwise lgtm. Thanks for your work on this!
I'll give this a few tests myself over the weekend, and if all looks good I'll merge. If anybody following master does find any bugs we can get it sorted before next release.
|
Apologies. Just ran into one more edge case. I can get back after a month of testing. If you want to close this till then, please do so. |
|
No rush, take your time. You can convert this to a draft if you want, but either way just give me a ping when you're happy with it. |
|
This is fairly stable (crossing fingers). Biggest new addition: made the lock acquisition order consistent with your earlier shutdown of player (current_player, then players). Please take a look at the locking code changes.
It switches to player1 if you either
Either way, I think its minor and can be a separate pull request once this is stable and merged in. |
|
Changes LGTM, thanks. Agreed that 'downgrade' is very minor, so happy to let that slide for now. I'll give this a test over the next couple of days myself then merge. |
|
I have just given this a test through, and I've found a regression: there's a second delay between player some updates (such as play/pause) and Ironbar reflecting that, even if toggled through the Ironbar popup. There is also a pre-existing bug I've found where if you close a player it remains the 'active' one, then trying to play it throws an error. I'm not sure how easy this is to fix, but it might be worth including a fix here as it's in scope. If not, I'll log a separate bug for it. |
Verified. Missed this regression. Thank you for noticing this.
I cannot reproduce this. Which player? My guess is yet another error is being thrown in the |
I tested with Ryhthmbox:
Log |
|
I cannot reproduce the error now with the latest version of PR. My earlier commit that released the lock before sleeping should have fixed the issue in your logs. This commit should have ensured that in step 7, MPRIS will show firefox again before you had a chance to interact with the stale Rhythmbox player. Going by the log's line 222 in mpris.rs, it was before the commit that released the locks:
Only thing I could think of: Could not reproduce your exact error but I set the delay in the loop to 5 seconds instead of 1 while holding on to the lock (on purpose) to stress test holding the lock.This triggered another error which we now handle by shutting down the player. Another reason to shutdown the player could be when the |
|
I think you're right, I was a commit behind. Sorry about that! I have just tested again on the latest commit and everything seems solid. The commits at the moment are possibly a bit technical and separated - do you think you could squash them down to reflect each bug please, as each commit will form an entry in the changelog? Once that's done I think it's merge time :) |
…erviceUnknown DBus errors in mpris.
|
Cheers for all your work on this! |
Sometimes, I see the NoActivePlayer DBusError when no music players are running.
The first commit prevents the panic so the song shows when the player is started after ironbar was launched.
The second commit checks if all the players have been cleared out and sends None to hide the label in this case. With this change, the music label does not stay after the app is closed.
I tested by
1.start ironbar
2. then spotify,
3. then another player
4. and closed both
5. and restarted spotify.
The show/hide behavior is as expected: 1. none, 2. song1, 3. song2, 4. song2->song1->None, 5. song1
It's entirely possible I missed the actual mechanism to hide the label when no players active. Please let me know if there is a better way. Thank you!