Add MusicIP host setting and Dynamic Path Conversion - #1629
Conversation
917e9ab to
bb3a172
Compare
michaelherger
left a comment
michaelherger
left a comment
There was a problem hiding this comment.
I just noticed you created this PR for 9.1. But please rebase it on top of 9.2. You're re-inventing some stuff you already changed in 9.2 a few weeks ago.
| $MMShost = $prefs->get('host') || 'localhost'; | ||
|
|
||
| main::INFOLOG && $log->info("Testing for API on localhost:$MMSport"); | ||
| main::INFOLOG && $log->info("Testing for API on $MMShost:$MMSport"); | ||
|
|
||
| my $initialized = get( "http://localhost:$MMSport/api/version", 5 ); | ||
| my $initialized = get( "http://$MMShost:$MMSport/api/version", 5 ); |
There was a problem hiding this comment.
use Slim::Plugin::MusicMagic::Common::getBaseUrl() instead.
d74a2eb to
2ec5207
Compare
Signed-off-by: HB64 <HB64@users.noreply.github.com>
2ec5207 to
c1ea366
Compare
|
Good catch, thanks — rebased onto public/9.2 and dropped the host-related changes entirely, since getBaseUrl() already covers that there. What's left is just the Dynamic Path Conversion setting (the actual point of this PR) plus a small bug I ran into while retesting: musicmagic.html's pref_host field was missing the prefs.exists('pref_host') guard that every other field on that page has, so it was leaking onto the per-player settings page as a non-functional orphaned input. Fixed that too. Base branch and PR description are updated to match. Should be a much smaller, more focused diff now. |
|
One more question while I have you: would you be open to a similar PR against public/9.1? That branch doesn't have getBaseUrl(), so it would need both the host setting and the DPC change together (same as this PR did before the rebase). I ask because 9.1.x is what most non-bleeding-edge users are actually running in production, and it could use this just as much as 9.2. Happy to open it as a separate PR if that's welcome. |
No. 9.1 is bug fixes only (mostly anyway). But this is a rather big change, probably more than this plugin has seen in ten years! 9.2 usually is stable enough for the users with special needs to give it a try. |
michaelherger
left a comment
michaelherger
left a comment
There was a problem hiding this comment.
Looking pretty good! I believe the conversion itself deserves some more love to make it safer on various platforms. Otherwise mostly cosmetics. Thanks!
|
|
||
| [% WRAPPER settingGroup title="SETUP_MMSHOST" desc="SETUP_MMSHOST_DESC" %] | ||
| <input type="text" class="stdedit" name="pref_host" id="host" value="[% prefs.pref_host || 'localhost' | html %]" size="40"> | ||
| [% IF prefs.exists('pref_host') %] |
There was a problem hiding this comment.
Why would you make this optional? It's hard to define a host when the input is not available?
| SV Värdnamnet eller IP-adressen för MusicIP-servern. Ändra detta om MusicIP körs på en annan värd än Lyrion Music Server (t.ex. i en separat Docker-container eller VM). Standard är localhost. Obs: filsökvägarna till din musik måste vara identiska på båda värdarna. Docker-användare bör säkerställa detta genom att använda matchande volymmonterade sökvägar i båda containrarna. | ||
| ZH_CN MusicIP服务器的主机名或IP地址。如果MusicIP运行在与Lyrion Music Server不同的主机上（例如在单独的Docker容器或虚拟机中），请更改此设置。默认为localhost。注意：音乐文件路径在两台主机上必须完全相同。Docker用户应通过在两个容器中使用相同的卷挂载路径来确保这一点。 | ||
|
|
||
| MUSICIP_DPC |
There was a problem hiding this comment.
Would you mind using the full, human readable DYNAMIC_PATH_CONVERSION everywhere? String tokens, variable names, prefs (all uppercase for the string token only, of course. I bet you know what I mean 😁). "DPC" is really only for those in the know... Had come down here to figure it out!
Maybe "path conversion" is good enough anyway.
| [% "MUSICIP_DPC_SOURCE" | string %] <input type="text" class="stdedit" name="pref_dpc_source" id="dpc_source" value="[% prefs.pref_dpc_source | html %]" size="40" placeholder="Z:\music"> | ||
| <br> | ||
| [% "MUSICIP_DPC_DEST" | string %] <input type="text" class="stdedit" name="pref_dpc_dest" id="dpc_dest" value="[% prefs.pref_dpc_dest | html %]" size="40" placeholder="/music"> |
There was a problem hiding this comment.
Nitpick: I'd rather see the header as a setting group title, not in-line.
| if ($path =~ s|^\Q$source\E|$dest|i) { | ||
| $path =~ s|\\|/|g; | ||
| } |
There was a problem hiding this comment.
This would only work Windows -> Linux, right? But not the other way around? Maybe you could leverage a module like File::Spec::catdir() to re-join split path parts in a platform aware way??
| if (!-e $songInfo{'file'}) { | ||
| my $converted = Slim::Plugin::MusicMagic::Common::translatePath($songInfo{'file'}); | ||
| $songInfo{'file'} = $converted if -e $converted; | ||
| } |
There was a problem hiding this comment.
This is a potentially "expensive" check, as it might two disk reads for each path. Maybe you should optimise this to only run the check if the path didn't look specific to the underlying OS. Eg. you'd never have a leading c:\ on a Linux system.
Would you ever have to deal with relative paths? Or are they always absolute? Or could they be UNCs (\\server\share)?
# only run check if feature is enabled and path doesn't look native to OS
if ($prefs->get('dpc_enabled') && (main::ISWINDOWS ? $songInfo{'file'} =~ m{^/} : $songInfo{'file'} !~ m{^/}) {
my $converted = Slim::Plugin::MusicMagic::Common::translatePath($songInfo{'file'});
# only check existence if the path was translated
$songInfo{'file'} = $converted if $converted ne $songInfo{'file'} -e $converted;}(didn't test this)
As you would still return an inexistent path there's no real need to check the existence here, is there? In that case you could further simplify the code to replace $songInfo{file} always.
| if (!-e $songs[$j] && !-e Slim::Utils::Unicode::utf8encode_locale($songs[$j])) { | ||
|
|
||
| my $converted = Slim::Plugin::MusicMagic::Common::translatePath($songs[$j]); | ||
|
|
||
| $songs[$j] = $converted if -e $converted; | ||
| } |
There was a problem hiding this comment.
Create a shared helper in Common, as this is basically the same code as above in Importer.
2 participants
MusicIP-reported song paths are used as-is, which breaks down once MusicIP and Lyrion Music Server don't share the exact same filesystem view — for example MusicIP running in a separate Docker container/VM, or under Wine reporting Windows-style paths (the case from #1624, which this PR supersedes with a general solution). The settings page description for the existing host setting already points at this scenario, but currently only suggests matching volume mounts as a workaround; that isn't always possible.
Dynamic Path Conversion (DPC)
A configurable source/destination path prefix pair, applied wherever MusicIP-reported paths are consumed (Importer.pm::processSong, Plugin.pm::getMix) — the same two call sites the Wine-specific fix in #1624 targeted. Disabled by default and a no-op unless both source and destination are configured, so it never affects a standard installation. Works the same way the SugarCube LMS plugin's own DPC setting does.
Fix: host field leaking onto the per-player settings page
While testing, found that musicmagic.html's pref_host field was the only field on the page not wrapped in [% IF prefs.exists('pref_host') %] — every other field, including port, has that guard. Since ClientSettings.pm correctly excludes host from the per-player prefs list, this meant the field rendered anyway (as an orphaned, non-functional input) whenever the page was reused for the per-player view via mipclient.html. Added the missing guard, matching every other field on the page.
Testing
Verified on a public/9.2 LMS instance:
Mood Mixer / MusicIP Mix work correctly with DPC configured, and fail without it, confirming the path translation is actually exercised
Host field no longer appears on the per-player settings page after the guard fix