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

Parserm3u export #4752

Merged
merged 6 commits into from
May 16, 2022
Merged

Parserm3u export #4752

merged 6 commits into from
May 16, 2022

Conversation

fatihemreyildiz
Copy link
Contributor

I was trying to Export and Import my playlists. Then I realized when I tried to export my playlist with m3u there were some missing tracks. That was because of non encodable characters in Turkish. m3u8 was totally fine tho.

According to our conversation on Zulipchat, with your suggestion I tried to use URL Locations for m3u.

After I tried URL Locations, there were no missing tracks with m3u export/import.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you very much!

I think the logic needs to be improved a bit. Now the URL encoding is always used, for absolute paths and the relative path version will still have track misses.
I also consider the URL decoding only a s a last resort.
I think the strings can be tested with
https://doc.qt.io/qt-5/qtextcodec.html#canEncode-1

@fatihemreyildiz
Copy link
Contributor Author

Hey again!

I tried to improve the logic. Now what does parser do is. Basically It checks if a track has missing characters [in name, in folder or in path], unless it is m3u8 extension. If there are no missing characters and playlist extension is m3u8, everything is working just like before this pr. But if there are any missing characters, it does URL encoding.

So, with that URL encoding is the last choice.

I tried to test the strings with "QTextCodec::canEncode" but it was always returning true. And I searched about it, I couldn't find a solution. Then I realized I can test strings with ByteArray, and it was working just like a charm. I hope It is not a bad approach

fileContents += baseDirectory.relativeFilePath(item) + QStringLiteral("\n");
QByteArray trackByteArray = QTextCodec::codecForName(kStandardM3uTextEncoding)
->fromUnicode(item);
if ((trackByteArray == item) || useUtf8) {
Copy link
Member

@daschuer daschuer May 12, 2022

Choose a reason for hiding this comment

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

This line requires a source code comment comment.

It uses QString::toUtf8() see: https://doc.qt.io/qt-5/qbytearray.html#operator-eq-eq

So you compare the "Windows-1250" encoding with the Utf8 encoding. I think they match for all ASCII characters, but not for Ä and friends.
You approach should work when using QTextCodec::toUnicode(trackByteArray);

It would be also nice if the check is bypassed when useUtf8 is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review.

I will work on it 👍

} else {
fileContents += item + QStringLiteral("\n");
if (useRelativePath) {
QUrl itemRelativeUrl = QUrl::fromLocalFile(baseDirectory.relativeFilePath(item));
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if this is working. For my understanding file URLs are always absolute.
I would just delete the useRelativePath branch and add a source code comment about the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't need any URL location for relative paths right?

Copy link
Member

Choose a reason for hiding this comment

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

I am not aware of relative paths in URLs
You may verify it with VLC player wether it excepts such paths, or reimporting with Mixxx. If it turns out that it works flawlessly we can consider to add a solution. In general I prefer to stick with standards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried to re import relative paths on Mixxx and it worked. But it is not working on VLC. It was giving an error for the tracks which has missing characters for all URL located tracks(error is called "VLC is unable to open the MRL").

VLC is also giving that error in normal way when there are missing characters aswell.

I think as you said it is better to stick with standards. So, I will leave there a source code comment such as //URL Location is not working for Relative Paths

@daschuer
Copy link
Member

Thank you very much. We are getting close.

@daschuer
Copy link
Member

Thank you! We are getting close.

In your current solution we produce an invalid file in case relativ path is selected in the preferences and there are special characters in the path without telling the user. That is still not the best.

What is the best workaround?

Relativ paths are used to move the folder with files and m3u around.

I can imagine to use an absolute path for such a single file. In this case the file is no longer found after moving, but OK. At least we produce a valid file without random '?'.

Alternative would ne to warn the user and reject exporting.

May be silently use absolute path fits to this PR and a feedback dialog can be considered in a follow up.

@ronso0
Copy link
Member

ronso0 commented May 13, 2022

Alternative would ne to warn the user and reject exporting.

👍

@fatihemreyildiz
Copy link
Contributor Author

So at last, for both cases we can add dialog windows, if it is not affecting the usability right?

First, when a user selected relative path option and tried to import tracks with "m3u" extension. We can say "Exported file has some missing characters. It can result missing tracks when imported. Try to export with m3u8 extension.". After that the user is warned and be aware of missing tracks when it happened.

When a user didn't select relative path and tried to import tracks with "m3u" extension. We can warn and reject the exporting or just warn them about the situation why some files are URL located.

@daschuer
Copy link
Member

daschuer commented May 13, 2022

I would like to keep this PR small. Can you implement the fallback to absolute URLs in case of special character relative paths?

Than the issue of '?' in file paths is fully solved.

Ideally that can be secured against future regressions by adding a test to src/test/playlisttest.cpp.

For the dialogs follow up, I think we should warn about what will happen and encourage users to user m3u8. Something like
"Some file paths have special characters. They will be encoded as absolute file:// URL. Consider to select the m3u8 format that allows special characters natively."
(A nativ speaker should verify)

@fatihemreyildiz
Copy link
Contributor Author

I couldn't get for which paths [absolute or both] I should Implement the fallback. So, I implemented it for both. In this way, there is no missing tracks when user is just exports and imports the playlists and also can play those playlists on VLC. I can take it back for relative paths.

As final, If there are special characters (both for relative and absolute), they will be absolute URLs as a fallback. Users are warned and encouraged to use m3u8 (For beginner users they will have knowledge if there are missing tracks). If there are no special characters nothing will happen.

The warning is:
Screenshot from 2022-05-14 13-16-48

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Looks good and works like a charm.
Thank you very much!

@daschuer daschuer merged commit 7a015cc into mixxxdj:main May 16, 2022
@daschuer daschuer added the changelog This PR should be included in the changelog label May 16, 2022
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants