Fix m3u list parsing with /r/n end of line #4547

daschuer · 2021-11-28T17:44:44Z

This fixes a regression form
a006552

This fixes a regression form mixxxdj@a006552

Swiftb0y · 2021-11-28T18:11:11Z

Wdyt about adding a test to make sure this regression does not happen again? Should be relatively straightforward.
I guess only main is affected right?

src/library/parserm3u.cpp

daschuer · 2021-11-29T07:50:35Z

Test is in place now.

However I have discovered a kind of regression due to:

c1dcdcc

@uklotzde you have moved the existence check from a central point into the m3u parser. This removed the existence check from all other playlist types. What is you recommendation to fix this issue?

uklotzde · 2021-11-29T10:13:40Z

Test is in place now.

However I have discovered a kind of regression due to:

c1dcdcc

@uklotzde you have moved the existence check from a central point into the m3u parser. This removed the existence check from all other playlist types. What is you recommendation to fix this issue?

I don't spot any regression? Invoking TrackFile::checkExists() when needed instead of moving an anonymous boolean value around is safer and less error prone.

daschuer · 2021-11-29T16:34:14Z

I just had a closer look. The issue is that ParserCsv::parse() has now no existence check. Not existing tracks are leading to a debug assertion violation:

�[91mcritical�[0m [�[97mMain�[0m] DEBUG ASSERT: "sourceSynchronizedAt.isValid()" in function bool mixxx::TrackRecord::replaceMetadataFromSource(mixxx::TrackMetadata&&, const QDateTime&) at /home/sperry/workspace/2.3/src/track/trackrecord.cpp:184

I have misread the code around ParserPls::parse() which also removes not existing tracks. Now I have found the check.

The anonymous boolean was only a temporary solution, to show the issue.

I think this requires some more refactoring to make the code testable and keep the code in a good shape at the same time. A creeping scope of this PR :-/ .. but OK, right?

src/test/playlisttest.cpp

…column headers

daschuer · 2021-11-29T22:33:24Z

This fixes now also the debug assert with non existing csv entries.
I have also added tests for pls and csv.
csv files can now also be used if no or a translated header line is found.

src/library/parser.cpp

daschuer · 2021-11-30T22:29:41Z

I have now introduced the named function: parseAllLocations() this is even better than the anonymous lambda.

Swiftb0y · 2021-11-30T23:16:42Z

well, the lambda was basically the C++ workaround for what would otherwise just be a simple block expression in rust, but extracting it into a function is good too.

Swiftb0y

This PR seems done, right?
@Holzhaus we still need your approval/dismissal since you requested changes.

Swiftb0y · 2021-12-02T17:27:42Z

src/library/parserpls.cpp


-ParserPls::ParserPls() : Parser() {
-}
+QString getFilePath(QTextStream* stream) {


why the ptr instead of a const-ref?

It modifies the stream (moves the seek position) so I think a ptr is semantically appropriate.

Ah, right, so the text stream must be mutable, but that still does not explain why were passing a pointer instead of a (mutable) reference. AFAIK they are semantically the same, so is the pointer just used because it was there in the first place?

Keep in mind that this is not a request for changes, I'm must asking because I'd like improve my C++ knowledge.

I will change "stream" to "pStream" ...

From the C++ rules we may pass here a non const reference:
QString getFilePath(QTextStream& stream); But it is a matter of code style.

Than we see:
stream.readLine();
and not
pStream->readLine();

The later makes clear, that we do not have a local copy and all work with the pointed object will have an outside effect.

In case of QString getFilePath(cosnt QTextStream& stream); it does not matter if it is a copy by value or a reference, It has never an outside effect, because of the constantnes

This a fundamental difference between C++ and Java or C#, where objects are always a references and where thy are no pointers.

This was also part of the Google style. However they have recently dropped the strict rule.

See also:
https://github.com/mixxxdj/mixxx/wiki/Coding-Guidelines#non-const-references

(I have updated the text linking the original google rule)

Thanks for the explanation. From a type-safety perspective, when passing a pointer, while passing nullptr would not make sense, it is still permitted. Wouldn't it make sense to use a mutable reference instead since those can't be null?
Moreover, in this particular case, we don't treat the incoming QTextStream as an output variable, but rather consume the textstream and expect it to not be used in the caller afterwards, correct? Because then the parameter rather seems like it should semantically be a QTextStream&&. I'm aware that QTextStream does not have a move or copy constructor. turns out this assumption is wrong.

Wouldn't it make sense to use a mutable reference instead since those can't be null?

That is a valid argument for the other code style. Both styles are valid, we prefere the reliable value vs. pointer syntax style. By the way: in C++ a mutable reference can be null. It is only by convention never null. Pointers and references are exactly the same if you look at the produced assembler code.

That is a valid argument for the other code style. Both styles are valid, we prefere the reliable value vs. pointer syntax style.

Thanks for the clarification. I guess I'd prefer the the other style but I can adopt the mixxx style as well.

By the way: in C++ a mutable reference can be null. It is only by convention never null. Pointers and references are exactly the same if you look at the produced assembler code.

I know, but I thought the compiler does not allow references to be set to null (at compile time, there aren't any runtime checks unless you use something like gsl::not_null).

src/test/playlisttest.cpp

Holzhaus · 2021-12-02T17:59:59Z

src/library/parsercsv.cpp

    //qDebug() << "ParserCsv: Starting to parse.";
    if (file.open(QIODevice::ReadOnly)) {
        QByteArray ba = file.readAll();

        QList<QList<QString> > tokens = tokenize(ba, ',');

        // detect Location column
-        int loc_coll = 0x7fffffff;
+        int loc_col = -1;


If you rename the variable anyway, let's name it locationColumnIndex or something like that.

well the current naming is consistent with the entire function, so does it make sense to change just this one? Is worth to widen the scope yet again to rename all variables in that function?

Holzhaus · 2021-12-02T18:01:38Z

src/library/parserpls.cpp


-ParserPls::ParserPls() : Parser() {
-}
+QString getFilePath(QTextStream* stream) {


It modifies the stream (moves the seek position) so I think a ptr is semantically appropriate.

Holzhaus · 2021-12-02T18:03:35Z

This PR seems done, right?
@Holzhaus we still need your approval/dismissal since you requested changes.

Yes, I just re-reviewed and chose comment instead of request changes to clarify I'm okay with merging.

Swiftb0y · 2021-12-02T19:16:14Z

This PR seems done, right?
@Holzhaus we still need your approval/dismissal since you requested changes.

Yes, I just re-reviewed and chose comment instead of request changes to clarify I'm okay with merging.

Thanks for clarification. For some reason, GitHub does not show that when you look into the reviewers section at the top right of the PR. Maybe you need to dismiss the review or overwrite it via an explicit approval?

src/library/parser.cpp

daschuer · 2021-12-05T21:49:02Z

Done.

Swiftb0y · 2021-12-06T15:58:31Z

src/library/parsercsv.h

-    virtual ~ParserCsv();
-    /**Overwriting function parse in class Parser**/
-    QList<QString> parse(const QString&);
+class ParserCsv : Parser {


What is the consequence of leaving out the inheritance visibility specifier? Previously it was inheriting from Parser publicly, now its defaulting to something (I don't know what without having to look it up).
From pure gut feeling, leaving the specifier out seems like bad style to me, though I have 0 idea in practice.

It defaults to private.
But to be honest, I have just forget to write it explicitly. I will fix it.

Thanks, that was my concern. Seems like no outside component depends on the inheritance but I think it makes sense for the relationship to be public regardless.

Swiftb0y · 2021-12-07T00:01:07Z

Any idea whats tripping up clang-format? Related to #4557?

uklotzde · 2021-12-07T00:54:31Z

Any idea whats tripping up clang-format? Related to #4557?

We now require Standard: c++17 and >> is valid syntax since C++11. Legacy code using > > gets fixed.

uklotzde · 2021-12-07T00:58:21Z

It is probably a good idea to replace all occurrences of > > with >> in a subsequent PR. It is a small and trivial change that does not need to be deferred by the no mass reformatting rule.

daschuer · 2021-12-07T06:45:24Z

It is probably a good idea to replace all occurrences of > > with >> in a subsequent PR.

I have no interest to do that. The > > does not hurt my eyes and there is a good chance of creating conflicts with existing PRs.

Holzhaus · 2021-12-08T13:35:33Z

src/test/playlisttest.cpp

@@ -56,3 +57,52 @@ TEST_F(PlaylistTest, Relative) {
    EXPECT_EQ(QString("base/folder/../../bar.mp3"),
            parser.playlistEntryToFilePath("../../bar.mp3", "base/folder"));
 }
+
+TEST_F(PlaylistTest, m3uEndOfLine) {


Everywhere else we use PascalCase for the test names, let's keep this pattern here:

Suggested change

TEST_F(PlaylistTest, m3uEndOfLine) {

TEST_F(PlaylistTest, M3UEndOfLine) {

The space is unnecessary and clang-format complains about it. See mixxxdj#4547 (comment). This patch was created using: $ find src $ -name "*.h" -o -name "*.cpp" $ -exec sed -i 's/> >/>>/' {} \;

Holzhaus · 2021-12-08T13:59:08Z

It is probably a good idea to replace all occurrences of > > with >> in a subsequent PR.

I have no interest to do that.

No problem, I took care of it here: #4559

daschuer · 2021-12-17T08:21:34Z

Conflict resolved, merge?

Fix m3u list parsing with /r/n end of line

c7ab0c5

This fixes a regression form mixxxdj@a006552

github-actions bot added the library label Nov 28, 2021

Holzhaus requested changes Nov 28, 2021

View reviewed changes

src/library/parserm3u.cpp Outdated Show resolved Hide resolved

daschuer added 3 commits November 28, 2021 22:41

Move end of line regex to the global namespace

52a86b8

Added Test PlaylistTest.m3uEndOfLine

0147ff4

skip m3u comments

6c2ef37

github-actions bot added the code quality label Nov 29, 2021

Swiftb0y reviewed Nov 29, 2021

View reviewed changes

src/test/playlisttest.cpp Outdated Show resolved Hide resolved

daschuer added 8 commits November 29, 2021 17:46

Rename sFilename to playlistFile

4340fc4

Remove unused functions from Parser

6663515

Pipe all parse calls through static Parser::parse()

1f71a5d

Move file existence check to the common Parser::parse() function

b586485

Fix parsing of line ending in csv and add aworkaround for translated …

613af64

…column headers

Improve m3uEndOfLine test

ba37ff7

Silence debug

8f3c7dc

Added plsEndOfLine and csvendOfLine tests

fbcc514

Swiftb0y reviewed Nov 29, 2021

View reviewed changes

src/library/parser.cpp Outdated Show resolved Hide resolved

src/library/parser.cpp Outdated Show resolved Hide resolved

daschuer added 2 commits November 30, 2021 22:50

Removed unsued QString

a4eaf99

extract function parseAllLocations()

a8bbaab

Swiftb0y reviewed Dec 2, 2021

View reviewed changes

Holzhaus reviewed Dec 2, 2021

View reviewed changes

Swiftb0y reviewed Dec 3, 2021

View reviewed changes

src/library/parser.cpp Show resolved Hide resolved

Swiftb0y mentioned this pull request Dec 3, 2021

playlist export: add missing file extension #4531

Merged

daschuer added 3 commits December 5, 2021 15:49

rename stream to pStream

9008a9c

make entries const

5d4d43d

prevere at() over operator[]

87330f5

improve variable naming

317fb65

Holzhaus approved these changes Dec 6, 2021

View reviewed changes

Swiftb0y reviewed Dec 6, 2021

View reviewed changes

Inherit Parser public

db19ac9

remove extra space in nested templates

3b5d35d

Holzhaus reviewed Dec 8, 2021

View reviewed changes

Holzhaus mentioned this pull request Dec 8, 2021

Remove unneeded double closing angle bracket space #4559

Merged

daschuer added 2 commits December 9, 2021 14:26

Start new test names with a capital

63b49b6

Merge remote-tracking branch 'upstream/main' into m3u_fix

88a0a34

Holzhaus merged commit 7be89a4 into mixxxdj:main Dec 17, 2021

daschuer deleted the m3u_fix branch May 4, 2022 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix m3u list parsing with /r/n end of line #4547

Fix m3u list parsing with /r/n end of line #4547

daschuer commented Nov 28, 2021

Swiftb0y commented Nov 28, 2021

daschuer commented Nov 29, 2021

uklotzde commented Nov 29, 2021

daschuer commented Nov 29, 2021

daschuer commented Nov 29, 2021

daschuer commented Nov 30, 2021

Swiftb0y commented Nov 30, 2021

Swiftb0y left a comment

Swiftb0y Dec 2, 2021

Holzhaus Dec 2, 2021

Swiftb0y Dec 2, 2021

Swiftb0y Dec 2, 2021

daschuer Dec 5, 2021 •

edited

Loading

Swiftb0y Dec 5, 2021 •

edited

Loading

daschuer Dec 6, 2021

Swiftb0y Dec 6, 2021

Holzhaus Dec 2, 2021

Swiftb0y Dec 2, 2021

Holzhaus Dec 2, 2021

Holzhaus commented Dec 2, 2021

Swiftb0y commented Dec 2, 2021

daschuer commented Dec 5, 2021

Swiftb0y Dec 6, 2021

daschuer Dec 6, 2021

Swiftb0y Dec 6, 2021

daschuer Dec 6, 2021

Swiftb0y commented Dec 7, 2021

uklotzde commented Dec 7, 2021 •

edited

Loading

uklotzde commented Dec 7, 2021

daschuer commented Dec 7, 2021

Holzhaus Dec 8, 2021

Holzhaus commented Dec 8, 2021

daschuer commented Dec 17, 2021

	TEST_F(PlaylistTest, m3uEndOfLine) {
	TEST_F(PlaylistTest, M3UEndOfLine) {

Fix m3u list parsing with /r/n end of line #4547

Fix m3u list parsing with /r/n end of line #4547

Conversation

daschuer commented Nov 28, 2021

Swiftb0y commented Nov 28, 2021

daschuer commented Nov 29, 2021

uklotzde commented Nov 29, 2021

daschuer commented Nov 29, 2021

daschuer commented Nov 29, 2021

daschuer commented Nov 30, 2021

Swiftb0y commented Nov 30, 2021

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

Swiftb0y Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Dec 2, 2021

Swiftb0y commented Dec 2, 2021

daschuer commented Dec 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Dec 7, 2021

uklotzde commented Dec 7, 2021 • edited Loading

uklotzde commented Dec 7, 2021

daschuer commented Dec 7, 2021

Choose a reason for hiding this comment

Holzhaus commented Dec 8, 2021

daschuer commented Dec 17, 2021

daschuer Dec 5, 2021 •

edited

Loading

Swiftb0y Dec 5, 2021 •

edited

Loading

uklotzde commented Dec 7, 2021 •

edited

Loading