csync: Use Qt for encoding conversion instead of iconv #5875
Conversation
|
This looks good, in particular the dependency removal and being able to use Qt in csync. I worry about testing a bit - if this introduces issues we'll only notice by seeing bad filenames on users' machines. We should run the encoding tests on all platforms (I assume you covered linux?), and maybe check how thorough they are. |
We use iconv to normalize the string on mac. But the iconv version on mac does not support full unicode. So we will use Qt to normalize the string
It's a C++ file so we will be able to use Qt from it
csync/src/std/c_utf8.cc
Outdated
| #else | ||
| dst = c_strdup(wstr); | ||
| QTextDecoder dec(QTextCodec::codecForLocale()); | ||
| QString s = dec.toUnicode(wstr, qstrlen(wstr)); |
There was a problem hiding this comment.
const QString s = .... ?
Tested on Linux and works, but might be an issue on MacOSX
There was a problem hiding this comment.
's' can't be const for two reason: First, on mac, we update it with s = s.normalized(QString::NormalizationForm_C);
Second, we std::move it before calling toUtf8. This saves an allocation as there is an overload of toUtf8 which does the conversion in-place. (but does not work if s was const)
|
Regarding the check_encoding.c, it is true that the #ifdef HAVE_ICONV will not be compiled and could be cleaned up. I just kept it because i did not want to change the test. It should anyway pass with or without iconv as the behavior should be the same. (except for the codepoints that were not understood by macos's iconv) The CI is failiong for unrelated reason (it ran out of disk) Any objection for merging this? |
|
@jturcotte quickly tested this patch on windows and did not notice regressions. Since there are no objections, i'm merging now. |
|
Some troubles with APFS see here: #5650 |
Replaces #5715
Should fix issues:
[OC 10.0][OS X] libsync is able to download files with 4 byte-unicode names but upload etc #5661 On mac, iconv did not support all of unicode and some files with emoji in the filename could not be uploaded
[Linux] Support non-utf8 locale for filename #5719 , [Feature] Filename encoding with gbk standard should be supported #5676 On linux, we will now support non utf-8 locale