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

Fix some encoding issues (mostly on Windows) #4401

Merged
merged 6 commits into from Jul 6, 2018
Merged

Conversation

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jun 4, 2018

This pull request fixes several encoding issue. Every issue except STK one are Windows-specific.

  • ZynAddSubFX setting loading/saving
  • VST DLL loading
  • VST setting loading/saving
  • Sample file decoding
  • MIDI import
  • STK rawwave path (incomplete on Windows due to upstream issue)

Issues remaining on Windows:

  • .pat file loading
  • .gig file loading
  • STK rawwave path (which toLocal8Bit fails to handle)

I'm planning to track remaining issues in #1191.

Fixes #3855, Fixes #2662, and almost #1191.

@PhysSong PhysSong force-pushed the PhysSong:encoding branch from e162527 to e675b2d Jun 4, 2018
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jun 10, 2018

Every issue except STK one are Windows-specific

I replicated issue #2662 on LinuxMint/Qt4 . It's indeed fixed by this PR.

Ready to merge?

@lukas-w lukas-w self-requested a review Jun 10, 2018
if (!file.open(QIODevice::ReadOnly))
{
return false;
}

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

This looks like you're closing and then reopening the file. Is it possible to just not close the file to begin with, and then replace QByteArray arr = file.readAll(); with QByteArray arr = file().readAll(); further down?

This comment has been minimized.

@PhysSong

PhysSong Jun 10, 2018
Author Member

I wrote like this because of the const-ness of file(). I'll add a method to work around.

static std::wstring toWString(std::string s)
{
/*
QString qstr = QString::fromUtf8(s.data(), int(s.size()));

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

I see this is commented out, so I'm guessing it doesn't work. Did you try

return qstr.toStdWString();

?

This comment has been minimized.

@PhysSong

PhysSong Jun 10, 2018
Author Member

Beacause it requires including Qt headers(on Windows). If that don't matter (we already do that), I can switch to QString is needed.

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

Ah, I see. @lukas-w has offered me advice on where it's okay to use Qt stuff in the past. I think things like QString, at least, are fine everywhere, but hopefully he'll chime in here to confirm that this is OK if he's not too busy.

This comment has been minimized.

@PhysSong

PhysSong Jun 10, 2018
Author Member

To be more clear, RemoteVstPlugin use Qt libraries for IPC on Windows. So I think that would be fine.

Edit: Linux build also needs the function, too. That's why I didn't use Qt function here.

This comment has been minimized.

@lukas-w

lukas-w Jun 11, 2018
Member

@PhysSong is right, this is not a good place to use Qt because that would add MinGW Qt as a dependency on Linux builds. We can however use C++11 functions to convert to UTF-16:

#include <locale>
#include <codecvt>
std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>> converter;
return converter.from_bytes(s);

This does the same thing, just in fewer lines. I think we already use C++11 for RemoteVstPlugin on stable-1.2, but I didn't check.

This comment has been minimized.

@PhysSong

PhysSong Jun 11, 2018
Author Member

because that would add MinGW Qt as a dependency on Linux builds.

It seems like we can use host version Qt, but I don't think it's a good idea because it adds 32bit Qt as a dependency on 64bit Linux.

We can however use C++11 functions to convert to UTF-16:

That doesn't work with our old MinGW because of missing header files(it should work with newest g++-mingw-w64). Those functions are deprecated in C++17, too.

#define F_OPEN(a, b) _wfopen(toWString(a).c_str(), L##b)
#else
#define F_OPEN(a, b) fopen((a).c_str(), b)
#endif

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

I see you added these F_OPEN defines, but they don't seem to be used anywhere (?)

This comment has been minimized.

@PhysSong

PhysSong Jun 10, 2018
Author Member

Forgot to commit that. Thank you!

This comment has been minimized.

@PhysSong

PhysSong Jun 11, 2018
Author Member

Addressed.

@@ -2083,7 +2106,7 @@ int main( int _argc, char * * _argv )
}

// constructor automatically will process messages until it receives
// a IdVstLoadPlugin message and processes it
// a IdVstLoadPlugin message and processes it

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

^ No need to change the indentation here. It was fine before :)

#else
int fd = dup(f.handle());
#endif
f.close();

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

I see you're using this to open files by their descriptor instead of the filename. Is it gzopen that's giving you problems, fopen, or both? Because if it's only gzopen that's broken, it might be possible to just always fopen, and then call gzdopen(fileno(file), options) if compression is enabled, rather than doing this trickery with duplicating the file handles and such.

This comment has been minimized.

@PhysSong

PhysSong Jun 10, 2018
Author Member

Both are broken, because C library only supports characters in local code page.

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

Oh, oops. I had typed out that comment and after digging deeper to better understand the problem I meant to remove it before posting the review >.<

Yeah, I agree with your conclusion here. It's not pretty, but I don't see any better solution.

#else
int fd = dup(f.handle());
#endif
f.close();

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

I wonder if you could offload this logic to a helper function,

static int fdopenFromQString(QString filename, QIODevice::OpenMode mode)

to avoid duplicating the body in multiple functins, and then leave a comment near that function explaining why the function is necessary (to most readers, this code will look silly and they may be tempted to simplify it back to the way it was before).

This comment has been minimized.

@PhysSong

PhysSong Jun 10, 2018
Author Member

Good suggestion!

QFile f(file);
f.open(QIODevice::ReadOnly);
QByteArray dat = f.readAll().constData();
is.str(string(dat.constData(), dat.size()));

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

The casual reader will see this and think "we're reading the entire file into a string, and then operating on it only as a stream? Why not directly open the file with ifstream?" and will be tempted to change this back to how it was previously. I think there should be some [short] comment explaining why we must open the file with QFile, possibly linking back to this github PR.

@@ -416,7 +412,10 @@ f_cnt_t SampleBuffer::decodeSampleSF( const char * _f,
f_cnt_t frames = 0;
bool sf_rr = false;

if( ( snd_file = sf_open( _f, SFM_READ, &sf_info ) ) != NULL )

QFile f(_f);

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 10, 2018
Member

I'm thinking these should all be commented with something along the lines of

// Open the file with QFile and pass the descriptor to sf_open_fd instead of directly calling sf_open
// for better filename encoding support. See https://github.com/LMMS/lmms/pull/4401

so that people know not to revert this change.

This comment has been minimized.

@PhysSong

PhysSong Jun 10, 2018
Author Member

Good point.

@PhysSong PhysSong force-pushed the PhysSong:encoding branch 4 times, most recently from 6cd2d26 to 1f09c05 Jun 11, 2018
@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Jun 11, 2018

@PhysSong Thanks for the improvements!

I'm marking this as "approved" now, since you've addressed any concerns I had. But it looks like Lukas wanted to review this too, so you should probably wait until that before merging 🎉

#else
.toUtf8().constData() );
#endif

This comment has been minimized.

@lukas-w

lukas-w Jun 11, 2018
Member

I think we can (should?) use toLocal8Bit on Unix as well. It should be equivalent to toUtf8 on most Linux and macOS systems. Correct me if I'm wrong.

This comment has been minimized.

@PhysSong

PhysSong Jun 11, 2018
Author Member

Yes, I think that would be simpler. AFAIK that wouldn't break anything, too.

static std::wstring toWString(std::string s)
{
/*
QString qstr = QString::fromUtf8(s.data(), int(s.size()));

This comment has been minimized.

@lukas-w

lukas-w Jun 11, 2018
Member

@PhysSong is right, this is not a good place to use Qt because that would add MinGW Qt as a dependency on Linux builds. We can however use C++11 functions to convert to UTF-16:

#include <locale>
#include <codecvt>
std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>> converter;
return converter.from_bytes(s);

This does the same thing, just in fewer lines. I think we already use C++11 for RemoteVstPlugin on stable-1.2, but I didn't check.

@@ -193,9 +216,11 @@ int QtXmlWrapper::dosavefile(const char *filename,
int compression,
const char *xmldata) const
{
int fd = fdopenFromUtf8(filename, QIODevice::WriteOnly);

This comment has been minimized.

@lukas-w

lukas-w Jun 11, 2018
Member

Why not use the same method here that's used in RemoteVstPlugin.cpp? We can move the toWString method and the F_OPEN defines to a common header and include that in both files.

This comment has been minimized.

@PhysSong

PhysSong Jun 11, 2018
Author Member

I think F_OPEN macro is an ugly workaround for fopen. I can do that if needed, but I don't like the macro approach.

This comment has been minimized.

@lukas-w

lukas-w Jun 11, 2018
Member

@PhysSong You can make it a function instead.

This comment has been minimized.

@PhysSong

PhysSong Jun 11, 2018
Author Member

Something tricky about that: "r" vs L"r" or something like that. That's why I chose macro approach.

This comment has been minimized.

@lukas-w

lukas-w Jun 11, 2018
Member

@PhysSong Sounds like you need to convert the mode string as well. So something like this:

std::FILE* openFile(std::string filename, std::string mode)
{
#ifdef LMMS_BUILD_WIN32
	return _wfopen(toWString(filename), toWString(mode));
#else
	return fopen(filename.data(), mode.data());
#endif
}

This comment has been minimized.

@lukas-w

lukas-w Jun 23, 2018
Member

You could, but I don't think there's a need for it, because they're just a couple lines each and operate on an UTF-16 QString instead of an UTF-8 std::string.

This comment has been minimized.

@PhysSong

PhysSong Jun 24, 2018
Author Member

I think either lmms_io.h or IoHelper.h will be fine. I'll address this point soon.

This comment has been minimized.

@PhysSong

PhysSong Jun 25, 2018
Author Member

Or how about putting them in RemotePlugin.h? I can't use-cases of UTF-8 string filename except in remote plugins.

Edit: It doesn't look like good because Zyn core needs it.

This comment has been minimized.

@PhysSong

PhysSong Jun 26, 2018
Author Member

Is it fine for our Zyn to include LMMS' headers at all? I think this will be fine in stable-1.2, but I'm not sure about master's submodule. It would be the last concern for me...

This comment has been minimized.

@PhysSong

PhysSong Jun 26, 2018
Author Member

I know it doesn't make sense to build https://github.com/LMMS/zynaddsubfx as a standalone, of course. However, I think it's possible to check if it's building with LMMS or not, though not really needed.

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jun 27, 2018

@lukas-w I addressed these issues and pushed to a separate branch. I'll push this change to the original branch when/if you says okay.

@PhysSong PhysSong force-pushed the PhysSong:encoding branch from 1f09c05 to d0b6173 Jun 27, 2018
@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jun 27, 2018

Oops, accidentally pushed them. I can roll back to original commits whenever needed.

@lukas-w
Copy link
Member

@lukas-w lukas-w commented Jun 27, 2018

@PhysSong Please roll back to the original commits, all the comment/review history on GitHub is lost otherwise. It's okay to have one extra commit on top of the original ones, there's no need to rewrite history.

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jun 27, 2018

all the comment/review history on GitHub is lost otherwise.

What do you mean? Review comments are still there unless you commented on specific commits, the are just collapsed.

Anyway, I agree that it's not necessary to rewrite history. I just wanted to keep things cleaner, but that was all. If you're still unhappy with this force-push, I can roll back.

@lukas-w
Copy link
Member

@lukas-w lukas-w commented Jun 27, 2018

What do you mean?

Sorry, I was being imprecise. The comments are still there, but the context is lost. Review comments are always on specific commits and you can't view that commits any more, so you can't see the changes the review refers to. Try clicking on the "View changes" button next to a review, this is what you'll get now:

image

If you're still unhappy with this force-push, I can roll back.

Yes, please do 👍

@PhysSong PhysSong force-pushed the PhysSong:encoding branch from d0b6173 to 34816e8 Jun 27, 2018
@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jun 27, 2018

I just noticed even a rebasing can cause such a thing. I just accidentally rebased commits because I didn't know that... I'll revert to the revision before rebasing if needed.

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jul 1, 2018

BTW @lukas-w, do you want any more changes in this pull request?

@lukas-w
Copy link
Member

@lukas-w lukas-w commented Jul 1, 2018

@PhysSong Apart from restoring the pre-force-push commits, nope.

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jul 4, 2018

Then do you mind if I merge this(probably with cleaner history)?

@lukas-w
Copy link
Member

@lukas-w lukas-w commented Jul 5, 2018

I don't mind, go ahead ❤️

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Jul 5, 2018

Just found a very rare encoding issues when using temporary directory whose name contains some non-ASCII characters on Linux. In that case VST plugin can't be loaded.
I can fix it, but it requires some dangerous changes(main->wmain and some more) and is easy to work around. So I'll open an issue instead and merge this soon.

@PhysSong PhysSong force-pushed the PhysSong:encoding branch from 34816e8 to 62d505b Jul 6, 2018
@PhysSong PhysSong merged commit 62d505b into LMMS:stable-1.2 Jul 6, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PhysSong PhysSong deleted the PhysSong:encoding branch Jul 6, 2018
int fd = open( _file.c_str(), O_WRONLY | O_BINARY );
if ( ::write( fd, chunk, len ) != len )
FILE* fp = F_OPEN_UTF8( _file, "wb" );
if ( fwrite( chunk, len, 1, fp ) != len )

This comment has been minimized.

@lukas-w

lukas-w Jul 6, 2018
Member

Only spotted this now: fwritereturns the count of elements successfully written, so in this case it will return 1 on success, not len. Therefore this condition is always leading to a false error message on success (except when coincidentally len == 1).

This comment has been minimized.

@PhysSong

PhysSong Jul 7, 2018
Author Member

Thank you! Changed to fwrite( chunk, 1, len, fp ) in 0f3b41f.

This comment has been minimized.

@PhysSong

PhysSong Jul 7, 2018
Author Member

Also synced into master via 170a46e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.