feat: enable macOS media key support via AppKit run loop#726
Conversation
hasezoey
left a comment
There was a problem hiding this comment.
Thanks for investigating what the issue is and trying to fix it.
As i cannot test it myself as i dont have a macos device, i would like some feedback from actual macos users.
Aside from that, please remove the changes that are beside the "fix macos media control" changes (ie your Updated TUI to be like I wanted commit).
Additionally, please remove the 5-second threshold on Previous: if >5s into the current track, change as that is also beside the point and would make it inconsistent with the rest of termusic.
All of the changes removed can be another PR if you want.
|
Sorry I'm new to this PR world so I didn't think that my commits after the PR was created will be in the PR. I will drop these commits. Thanks. |
|
I verified that it works well with the media buttons, but I haven't checked the Now Playing integration yet. I'll test that and report back |
030ac1a to
83ca8e7
Compare
|
I changed what you wanted and tested the now playing integration. It It works perfectly fine. You can skip forward, go to the previous song, and play and pause normally. Waiting to be merged... |
hasezoey
left a comment
There was a problem hiding this comment.
Thanks for resolving the changes, just some minor style nitpicks left (along with some extra testing).
Also please fix the lint (see failing CI).
Personally, i dont like things that require to run a specific thread, but if that is required to make this work, i have nothing against adding it (except are the things that are created on one thread and require to run on that thread (ex. a MutexGuard)).
hasezoey
left a comment
There was a problem hiding this comment.
Some testing has now been done in #640 to confirm this PR is working.
So here is a recap of the things i would like still to be done before merging:
- consider switching to
objc2orobjrasobjcis not updated anymore - reduce the changes in
server.rs
And in #640 it was noted that artwork display seemingly does not work, consider investigating this please.
|
I've tested, and sometimes the artwork doesn't show up and sometimes it does. I will test it thoroughly tomorrow. |
Do you have any indicator as to what tracks work and what dont? But based on that some do work, it is not the issue that macos does not want it encoded as base64 inline-data instead of a file, right? |
…llows Changes per hasezoey's review on PR tramhao#726: - server/src/server.rs: use run_with_run_loop from macos module - playback/src/mpris.rs: add run_with_run_loop() to macos module - playback/src/mpris.rs: objc 0.2 -> objc2 0.6 (AnyClass::get) - playback/src/lib.rs: remove crate-wide allow(unexpected_cfgs) - playback/Cargo.toml: objc = "0.2" -> objc2 = "0.6"
eca00d9 to
588b488
Compare
Root cause of intermittent artwork on macOS:
|
7e56152 to
7be7108
Compare
fixed by writing the cover to tmp file, every time you change the song it replaces the image and the now plaing shows that images, work every time |
|
I am back a night later and see like 20 emails for this pr alone. |
|
Sorry for the inconvenience, but i think so, thank you very much |
hasezoey
left a comment
There was a problem hiding this comment.
Looks better now that objc2 is used, though there are still some style nitpicks, mainly documentation related.
|
Btw, as a side-effect of using a file for the coverart in mpris again, is that artwork will likely work with GNOME again. @Porkepix Could you try this PR on your linux machine and see if it still would cause the issue described in #731? This would at least rule-out / narrow down the issue GNOME is having. |
As i do not have a Linux machine I cannot check. I will wait for a report from a GNOME user. |
This was only meant as a side-note and not relevant to the PR being merged. |
Is this PR supposed to have any effect on the matter to specifically try this one? I only build masters on a regular basis on my other Linux computer (better and more recent CPU, and 4x more RAM; on the one where I got the crash I tend to avoid due to how heavy it weighs regarding RAM usage). |
Ah ok.
Yes, because from what i remember on GNOME the artwork was also not working, which is the environment you reported to be using in #731 and because only some songs seemingly had the issue and some did not, so this would help narrow down (or rule-out) what parts cause GNOME to lock-up. |
|
So, building from this branch:
I can fairly claim issue comes from the code around ueberzug, and this with both I didn't checked if it was fixing covers not showing for GNOME for other songs though, is it something needed too? |
Thanks for narrowing the issue.
Yes that would be great, but not necessary. |
4ad769f to
35153c0
Compare
hasezoey
left a comment
There was a problem hiding this comment.
Looks almost ready, still some minor style nitpicks.
Once done, please reduce the commits down to 2 (one for the macos change and one for the coverart change)
Note that the commit message style is meant to include the crate that is modified in the scope field. (see CONTRIBUING, also see recent past commits for reference)
| Ok(()) | Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => break, | ||
| Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {} |
There was a problem hiding this comment.
Import std::sync::mpsc::RecvTimeoutError
There was a problem hiding this comment.
Fixed — imported RecvTimeoutError and used short form in the match arms.
There was a problem hiding this comment.
This does not look applied
| let mut path = std::env::temp_dir(); | ||
| path.push("termusic-cover"); | ||
| if let Some(mime) = v.mime_type() | ||
| && let Some(ext) = mime.ext() | ||
| { | ||
| path.set_extension(ext); | ||
| } | ||
| std::fs::write(&path, v.data()) |
There was a problem hiding this comment.
I just remembered we are also doing a similar thing for the ueberzug case already and there always set the JPG extension.
This is just a side-note and does not require any change in this PR.
There was a problem hiding this comment.
Upon further investigation (which i overlooked), we do always set the JPG extension, but also always convert the image to JPG.
35153c0 to
36f559e
Compare
hasezoey
left a comment
There was a problem hiding this comment.
Looks great, minor style nitpicks (which can be fixed after this PR), though i will re-test tomorrow and then merge.
| Ok(()) | Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => break, | ||
| Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {} |
There was a problem hiding this comment.
This does not look applied
| # soundtouch= { git = 'https://github.com/Drewol/soundtouch-rs.git' } | ||
|
|
||
| [target.'cfg(target_os = "macos")'.dependencies] | ||
| objc2 = "0.6" |
There was a problem hiding this comment.
Any dependency version should be defined (at least currently) in the workspace's Cargo.toml
| | :-------------------: | :-------------: | | ||
| | `termusic-server.log` | The server logs | | ||
| | `termusic-tui.log` | The TUI logs | | ||
| | `termusic-cover` | MPRIS cover | |
There was a problem hiding this comment.
| | `termusic-cover` | MPRIS cover | | |
| | `termusic-cover.EXT` | MPRIS cover | |
|
so should i make the changes or no? |
Summary
Fixes macOS media keys (F-row play/pause/next/previous) not reaching termusic — instead they opened Apple Music or were ignored.
Root cause
souvlaki registers
MPRemoteCommandCenterhandlers for media key events,but macOS delivers those events exclusively through AppKit's main run
loop. Rust CLI binaries don't initialize an
NSApplicationrun loop bydefault, so the registered handlers never received callbacks.
Additionally,
MPRemoteCommandCenterdispatches callbacks via GCD's maindispatch queue, which executes blocks on the process's main thread. The
existing
#[tokio::main]setup monopolized the main thread, preventingGCD from dispatching the callbacks.
Changes
— macOS module
init_macos_main_thread(): InitializesNSApplicationwithActivationPolicyAccessory(no Dock icon, but can receive mediaevents).
pump_run_loop(): Pumps the mainCFRunLoopin 50ms intervals,processing GCD blocks from
MPRemoteCommandCenter.restart it; if ≤5s, skip to the previous track (standard behaviour
matching Spotify / Apple Music).
— Main thread restructure (macOS only)
the main thread to pump
CFRunLoopfor AppKit event dispatch.tokio::runtime::Runtime::new().block_on(actual_main())instead of
#[tokio::main]).objc = 0.2gated behind[target.'cfg(target_os = \"macos\")'.dependencies].Code style
#[allow(unsafe_code)]on the macos module, matching the existingpattern used by the Windows module in the same file.
#[allow(unexpected_cfgs)]at the playback crate root for the olderobjccrate's internalcfg(cargo-clippy)usage.Testing
cargo build --features mpv --releaseon macOS.termusic --backend mpv.(if >5s in), second press skips to the previous track.
Closes #...