search

Martchus / syncthingtray

Tray application and Dolphin/Plasma integration for Syncthing
https://martchus.github.io/syncthingtray/
Other
1.68k stars 44 forks source link

Non-latin filepaths don't work on Windows #107

Closed sledgehammer999 closed 3 years ago

sledgehammer999 commented 3 years ago

Relevant components

Environment and versions

Bug description When syncthing.exe is placed in a filepath with non-latin characters on Windows then syncthingtray fails to launch it because the system can't find the file.

I believe I've tracked down the issue in the source code. Unless I am mistaken this is the part that launches syncthing: https://github.com/Martchus/syncthingtray/blob/4c6315b4509b5247f69d3f60a7980932a1b942fc/connector/syncthingprocess.cpp#L377

The issue stems from boost::filesystem/boost::process not having a cross-platform way of handling string encodings. On Windows you either pass a char array(or std::string) encoded in the system's 8-bit locale or a wchar_t (or std::wstring) encoded in utf-16. The first uses the ANSI WINAPI and the second uses the UNICODE (WIDE) WINAPI.

In any case, you can do either QString::toLocal8Bit().constData() or QString::toStdWString()

The std::string from QString::toStdString() is encoded in utf8. The change must be done for both the prog variable and the args vector.

Martchus commented 3 years ago

I somehow assumed boost would do the conversion under the hood but apparently not. So basically the same annoyance as with std::fstream::open. I'll use QString::toStdWString() under Windows then.

sledgehammer999 commented 3 years ago

I'll use QString::toStdWString() under Windows then.

A valid approach but: using QString::toLocal8Bit().constData() will work on all platforms and will save you from various ifdefs in the code just for this.

Martchus commented 3 years ago

No, QString::toLocal8Bit().constData() will not work for all Unicode characters. It'll only work for characters which can be represented in the current code page (which is usually not UTF-8 by default under Windows). I suppose for full Unicode support one should really use the UTF-16 API. (Alternatively one could set the code page to UTF-8 and just pass UTF-8 but the UTF-8 code page might not be as well supported as the UTF-16 API.)

Martchus commented 3 years ago

I made it use UTF-16 on master. Using UTF-16 makes also more sense considering Qt is using it under the hood anyways. I'll test it tomorrow under Windows, here's a build if you like to test it: https://martchus.no-ip.biz/repo/experimental/win/syncthingtray-b828d7c27491b373faf58b5c239338f6529e335b.exe

Martchus commented 3 years ago

Looks like it works just fine under Windows itself as well, e.g. I've been using D:\tmp\한자\syncthing-windows-amd64-v1.18.1\syncthing.exe as path. (한자 are the first best Chinese characters I found, no idea what they mean.)

sledgehammer999 commented 3 years ago

I confirm that it works. Thank you.