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

lp 16878282: Fix playlist import #2200

Merged
merged 4 commits into from
Jul 6, 2019
Merged

lp 16878282: Fix playlist import #2200

merged 4 commits into from
Jul 6, 2019

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Jul 4, 2019

  • Accept text files that start with UTF-8 BOM
  • Fix parsing of lines, i.e. comments start with '#' vs. file path may contain '#'

@uklotzde uklotzde added this to the 2.2.2 milestone Jul 4, 2019
@uklotzde uklotzde changed the base branch from master to 2.2 July 4, 2019 10:54
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.

Do we need to skip the utf8 bom?
Can skip in this case the other utf8 recognize call?

src/library/parser.cpp Outdated Show resolved Hide resolved
@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 4, 2019

I have force-pushed the reformatted commits.

@daschuer
Copy link
Member

daschuer commented Jul 5, 2019

There is a build issue:

[CXX] src\library\parser.cpp
parser.cpp
src\library\parser.cpp(71): error C2601: 'Parser::isUtf8': local function definitions are illegal
src\library\parser.cpp(40): note: this line contains a '{' which has not yet been matched
src\library\parser.cpp(149): error C2601: 'Parser::playlistEntrytoLocalFile': local function definitions are illegal
src\library\parser.cpp(40): note: this line contains a '{' which has not yet been matched
src\library\parser.cpp(40): fatal error C1075: the left brace '{' was unmatched at the end of the file
scons: *** [win64_build\library\parser.obj] Error 2
scons: building terminated because of errors.

@daschuer
Copy link
Member

daschuer commented Jul 5, 2019

Mm, we can be sure that the # is encoded in all cases. Why not use the trimmed line above and then check for a preceding #?

@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 5, 2019

Mm, we can be sure that the # is encoded in all cases. Why not use the trimmed line above and then check for a preceding #?

I don't understand what you want to tell me. That's what I did before -> startsWith(). After your comments I reverted back to the previous version where all lines containing a '#' are ignored.

@daschuer
Copy link
Member

daschuer commented Jul 5, 2019

Your original solution was already Ok.
Sorry for the noise.

@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 5, 2019

Ok, don't mind :)

@daschuer
Copy link
Member

daschuer commented Jul 5, 2019

.. but building still fails.

@daschuer
Copy link
Member

daschuer commented Jul 6, 2019

Thank you! LGTM

@daschuer daschuer merged commit 3ae5a7b into mixxxdj:2.2 Jul 6, 2019
@uklotzde uklotzde deleted the 2.2_fix_playlist_import branch July 6, 2019 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants