Zeugma440 / atldotnet

Fully managed, portable and easy-to-use C# library to read and edit audio data and metadata (tags) from various audio formats, playlists and CUE sheets
MIT License
440 stars 60 forks source link

Proper relative paths support in playlists #232

Closed fsobolev closed 8 months ago

fsobolev commented 9 months ago

The problem

Currently it's very problematic to work with playlists containing relative paths. PlaylistIO.FilePaths always gives absolute paths, no matter what is written in a file. It's possible to write relative path to a playlist, but let's say I want to remove a track from playlist:

var paths = playlist.FilePaths;
paths.Remove(index);
playlist.FilePaths = paths;

This will overwrite all relative paths with absolute ones.

Would it be possible to get a list of paths that are exactly the same that are written in playlist file (so if it's relative, then get relative path) or maybe provide a way to add/remove data in a playlist without overwriting all the data?

Environment

Zeugma440 commented 9 months ago

Thanks for that real life use case feedback.

I'll see what I can do 👍

nlogozzo commented 9 months ago

It'd also be nice if you could add a Save function to the PlaylistIO API.

Currently if we want to add or remove paths from a playlist we have to do:

var paths = _playlist.FilePaths;
paths.Remove(0);
paths.Add("Test.mp3");
paths.Add("Test2.mp3");
_playlist.FilePaths = paths;

since it is the setter of FilePaths that causes the changes to be written to the actually playlist file on disk.

It'd be nice to have a more friendly API such as the following:

_playlist.FilePaths.Remove(0);
_playlist.FilePaths.Add("Test.mp3");
_playlist.FilePaths.Add("Test2.mp3");
_playlist.Save();

I mean currently, we can do this:

_playlist.FilePaths.Remove(0);
_playlist.FilePaths.Add("Test.mp3");
_playlist.FilePaths.Add("Test2.mp3");
_playlist.FilePaths = _playlist.FilePaths;

but, as I said before, since the FilePaths setter is what writes the changes, it feels cumbersome doing it this way.

Zeugma440 commented 9 months ago

That last part has nothing to do with the initial description. I'm creating a specific issue for that => #233

Zeugma440 commented 9 months ago

The new Settings.PlaylistUseAbsolutePath flag (default : false) should do the trick.

It will be available in next release.

nlogozzo commented 9 months ago

The new Settings.PlaylistUseAbsolutePath flag (default : false) should do the trick.

It will be available in next release.

Would we be able to make this a per Playlist setting? Like set UseAbsolutePath per PlaylistIO object? As we support both files with absolute and relative so it'd be good for us to set it per Playlist we open.

Zeugma440 commented 9 months ago

Okay, that means the setting is not named properly. Actually, it only has an effect when writing, so it should be named PlaylistWriteAbsolutePath.

ATL supports both modes (absolute and relative) when reading playlists, whatever value you set.

Is that OK to you?

nlogozzo commented 9 months ago

Yeah when named like that it's fine...we can set it every time before we write.

Could you maybe add a flag to PlaylistIO something like HasRelativePaths to know if the Playlist was read with relative paths or not?

Zeugma440 commented 9 months ago

Could you maybe add a flag to PlaylistIO something like HasRelativePaths to know if the Playlist was read with relative paths or not?

Is the point to keep the playlist format as close as how you found it in the first place?

nlogozzo commented 9 months ago

ATL should be the one remembering for each track inside the playlist how it was formatted in the first place to be able to keep that format when updating it.

Yes, this is perfect, this is how we want it to be.

nlogozzo commented 9 months ago

When you finalize this and #233 , let me know...I'll test the main branch in Tagger make sure it's all good for us and then you can release a new ver

Zeugma440 commented 9 months ago

I'm on it... takes a little more time than expected, as I eventually need to do the refactoring I tried to avoid in #233 🙄 😆

nlogozzo commented 9 months ago

Haha no rush :)

Zeugma440 commented 9 months ago

Main branch is good to go !

Please keep me informed about your tests.

nlogozzo commented 9 months ago

Main branch is good to go !

  • Updating FilePaths and Tracks doesn't automatically save the playlist file anymore; you now have to call Save() do so so
  • Settings.PlaylistWriteAbsolutePath is used to determine whether new playlist elements are saved using a relative or absolute path
  • Unmodified playlist elements paths are kept as is

Please keep me informed about your tests.

Thanks...will do some testing now :)

nlogozzo commented 9 months ago

All works perfectly! Thank you so much!

Zeugma440 commented 9 months ago

You're welcome! New features are available on today's v5.09

fsobolev commented 9 months ago

Settings.PlaylistWriteAbsolutePath is used to determine whether new playlist elements are saved using a relative or absolute path

Sorry, but after further testing I figured out that this setting affects all paths, not just newly added.

The code we have to add item to playlist:

ATL.Settings.PlaylistWriteAbsolutePath = !useRelativePath;
_playlist.FilePaths.Add(path);
_playlist.Save();

So let's say I have a playlist with 1 track with relative path:

#EXTM3U
#EXTINF:-1,Miracle Of Sound - Between Heaven And Hell
Miracle Of Sound - Between Heaven And Hell.mp3

Then I add one track using absolute path:

#EXTM3U
#EXTINF:-1,Miracle Of Sound - Between Heaven And Hell
/home/fs/.var/distrobox/tumbleweed/Music/Miracle Of Sound - Between Heaven And Hell.mp3
#EXTINF:-1,Aviators - Stay Dead
/home/fs/.var/distrobox/tumbleweed/Music/Aviators - Stay Dead.m4a

Then one more track using relative path:

#EXTM3U
#EXTINF:-1,Miracle Of Sound - Between Heaven And Hell
Miracle Of Sound - Between Heaven And Hell.mp3
#EXTINF:-1,Aviators - Stay Dead
Aviators - Stay Dead.m4a
#EXTINF:-1,Another Day In Paradise
Level 12/Another Day In Paradise.mp3
Zeugma440 commented 9 months ago

Jesus, we both failed our testing xD

Let's go for another round!

Zeugma440 commented 8 months ago

I've fixed a seemingly unrelated issue in the main branch. I still can't reproduce the issue you described.

Could you please share a standalone piece of code to reproduce it?

Zeugma440 commented 8 months ago

Are you sure you're not re-creating the playlist file from scratch instead of loading the existing file, updating the Playlist object and saving?

If you re-create it from scratch, having tracks all written with relative and/or absolute path is the expected behaviour.

nlogozzo commented 8 months ago

Are you sure you're not re-creating the playlist file from scratch instead of loading the existing file, updating the Playlist object and saving?

This is Tagger's code for managing the playlist: https://github.com/NickvisionApps/Tagger/blob/main/NickvisionTagger.Shared/Models/MusicLibrary.cs

You can see we create the _playlist object in the constructor and then continue to use it throughout the object...AddFileToPlaylist, RemoveFileFromPlaylist.

If you re-create it from scratch, having tracks all written with relative and/or absolute path is the expected behaviour.

The only time the playlist object will every get "re-created" is if a user opens a playlist file and then closes the app and opens it again and opens the playlist file again.

So maybe you can try this in your testing...

You'll see all existing paths become relative as well. This to us is not expected behaviour as the users is expecting the one path to be added as relative and the others untouched.

Zeugma440 commented 8 months ago

So maybe you can try this in your testing... Create a playlist file and add 3 absolute paths. Close the app Open the playlist file again and add 1 relative path.

That's precisely what I'm doing in my unit tests :

1- Resource preparation https://github.com/Zeugma440/atldotnet/blob/b746a5ad7dfcf92511dab2f724082f69d43bc222/ATL.unit-test/Playlist/PlaylistIOTest.cs#L96 2- Testing if paths are written as expected https://github.com/Zeugma440/atldotnet/blob/b746a5ad7dfcf92511dab2f724082f69d43bc222/ATL.unit-test/Playlist/M3UIO.cs#L151

Your app's code looks fine but doesn't tell me how the test is done, and which parts are called when you're testing.

I don't use an app for testing, I'm using unit tests that can be run from anywhere automatically. That's why I asked for a standalone piece of code that can reproduce the problem everytime it is run.

nlogozzo commented 8 months ago

Ok so here's a concrete code you can use in tests to simulate Tahger behavior:

//Create playlist file
var _playlist = PlaylistIOFactory.GetInstance().GetPlaylistIO(Path, ATL.Playlist.PlaylistFormat.LocationFormatting.FilePath, ATL.Playlist.PlaylistFormat.FileEncoding.UTF8_NO_BOM);
ATL.Settings.PlaylistWriteAbsolutePath = true; //full paths
_playlist.FilePaths.Add(path1);
_playlist.FilePaths.Add(path2);
_playlist.Save();
/*
 * Assume at this point the app was closed and we are now reopening it
 */
//Open playlist file
_playlist = PlaylistIOFactory.GetInstance().GetPlaylistIO(Path, ATL.Playlist.PlaylistFormat.LocationFormatting.FilePath, ATL.Playlist.PlaylistFormat.FileEncoding.UTF8_NO_BOM);
ATL.Settings.PlaylistWriteAbsolutePath = false; //now relative Path
_playlist.FilePaths.Add(path3);
_playlist.Save();

You'll see that now all paths became relative even tho we only added one.

Now I know that this is considered as you said "re-creating" the playlist object and is "expected behavior", but you have to see from the point of view of Tagger and apps that open and close files, it's not expected that all paths become relative now, only the one added should be relative and the original paths should be unaffected.

Zeugma440 commented 8 months ago

Thanks for the quick reply ;)

Unfortunately, what I get when running that code is a playlist with three entries :

Is there something I'm missing? What environment are you running this on? I'm on Win11, under .NET SDK 7.x

nlogozzo commented 8 months ago

Hmm...also Win11 and .NET 7. Happens on Linux too

Unfortunately I'm not on my PC now so I can't double check until later...

Maybe @fsobolev can provide smth...

fsobolev commented 8 months ago

Tried to create a minimal reproducer, new project with Program.cs:

using ATL.Playlist;
using System;
using System.IO;

var path1 = "/home/fs/Музыка/Level 12/Another Day In Paradise.mp3";
var path2 = "/home/fs/Музыка/Level 12/Fallen Leaves.mp3";
var path3 = "/home/fs/Музыка/Level 12/Skal (Metal Version).mp3";

var playlistPath = "/home/fs/Музыка/test.m3u";

var playlist = PlaylistIOFactory.GetInstance().GetPlaylistIO(playlistPath, PlaylistFormat.LocationFormatting.FilePath, PlaylistFormat.FileEncoding.UTF8_NO_BOM);
playlist.FilePaths.Clear();
playlist.Save();

// Add absolute path
ATL.Settings.PlaylistWriteAbsolutePath = true;
playlist.FilePaths.Add(path1);
playlist.Save();

Console.WriteLine("Step 1:");
Console.WriteLine(File.ReadAllText(playlistPath));

// Add relative path
playlist = PlaylistIOFactory.GetInstance().GetPlaylistIO(playlistPath, PlaylistFormat.LocationFormatting.FilePath, PlaylistFormat.FileEncoding.UTF8_NO_BOM);
ATL.Settings.PlaylistWriteAbsolutePath = false;
playlist.FilePaths.Add(path2);
playlist.Save();

Console.WriteLine("\nStep 2:");
Console.WriteLine(File.ReadAllText(playlistPath));

// Add absolute path
playlist = PlaylistIOFactory.GetInstance().GetPlaylistIO(playlistPath, PlaylistFormat.LocationFormatting.FilePath, PlaylistFormat.FileEncoding.UTF8_NO_BOM);
ATL.Settings.PlaylistWriteAbsolutePath = true;
playlist.FilePaths.Add(path3);
playlist.Save();

Console.WriteLine("\nStep 3:");
Console.WriteLine(File.ReadAllText(playlistPath));

And the results:

Step 1:
#EXTM3U
#EXTINF:-1,Another Day In Paradise
/home/fs/Музыка/Level 12/Another Day In Paradise.mp3

Step 2:
#EXTM3U
#EXTINF:-1,Another Day In Paradise
/home/fs/Музыка/Level 12/Another Day In Paradise.mp3
#EXTINF:-1,Fallen Leaves
Level 12/Fallen Leaves.mp3

Step 3:
#EXTM3U
#EXTINF:-1,Another Day In Paradise
/home/fs/Музыка/Level 12/Another Day In Paradise.mp3
#EXTINF:-1,Fallen Leaves
/home/fs/Музыка/Level 12/Fallen Leaves.mp3
#EXTINF:-1,Skal (Metal Version)
/home/fs/Музыка/Level 12/Skal (Metal Version).mp3

As you can see, step 3 fails, track 2 that was added with relative path now has an absolute one. Commenting playlist = ... lines on steps 2 and 3 to use the same playlist object results in all absolute paths, even on step 2. Linux, dotnet 7.0.401.

Zeugma440 commented 8 months ago

Thank you @fsobolev 🎉

Turns out the issue happens when switching from relative to absolute, and not the other way around. And we tested the case that works!

I'm gonna fix that. Stay tuned~

nlogozzo commented 8 months ago

Turns out the issue happens when switching from relative to absolute, and not the other way around. And we tested the case that works!

Ahhh sorry I assumed it happened both ways 😆

I'm gonna fix that.

Thank you so much!!

Zeugma440 commented 8 months ago

Ahhh sorry I assumed it happened both ways

Don't be sorry, I made the very same assumption when doing my own tests 😆


Unit tests have been extended to include that new case and are now green for all playlists formats ✅

This time we should be good!

nlogozzo commented 8 months ago

Yay! Looking forward to the release :)

Zeugma440 commented 8 months ago

Could you confirm it does work as expected on your side?

nlogozzo commented 8 months ago

Sure, it'll just probably be a couple of days until I can.

Unless @fsobolev gets to it sooner

nlogozzo commented 8 months ago

Ok I was able to get to it sooner than I thought!

Can confirm it works 🎉 image

Zeugma440 commented 8 months ago

Excellent! I'm waiting to see if #234 is a quick fix or not. Anyhow, you'll get a release before friday 😋

nlogozzo commented 8 months ago

Anyhow, you'll get a release before friday

You're the best, thanks!

Zeugma440 commented 8 months ago

Available on today's v5.10