qt: do not assume temporary file name can be encoded with Latin1
Request review @chub.
Merge request reports
Activity
added MRStatus::Reviewable label
- Resolved by Fatih Uzunoğlu
added MRStatus::InReview label and removed MRStatus::Reviewable label
- Resolved by Fatih Uzunoğlu
added Component::Interface: Qt label
changed milestone to %4.0
- Resolved by Fatih Uzunoğlu
- Resolved by Fatih Uzunoğlu
added MRStatus::NotCompliant label and removed MRStatus::InReview label
@chub Did you want to use
QString::toLocal8Bit()
(I now remembered the existence of this), andQByteArray::constData()
, and keep usingrename()
andremove()
instead ofstd::filesystem
?On Windows, I'm not sure if
rename()
functions properly with the used code page if the used code page is UTF-8.QString::toLocal8Bit()
is also noted to not have a defined behavior if the string can not be represented by local 8-bit encoding.As far as I know, the temporary file path may be Unicode, even though the local code page is not Unicode. In that case, using
QString::toLocal8Bit()
would result in undefined behavior.Even if the temporary file path is Unicode and the local code page is
UTF-8
, I'm not sure if MSVCRT supportsUTF-8
withchar*
.For Win32 API [1], it seems that functions accepting
char*
(instead ofwchar*
) can support UTF-8:Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes.
[1] https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page#-a-vs--w-apis
from what I'm understanding std::filesystem::path is supposed to do the conversion, though (former version of) gcc seems to be bugged, I reproduce this crash on my machine:
If we have to choose between converting to UTF-8 or UTF-16, UTF-16 makes more sense because
QString
is UTF-16. That's why I usedQString::toStdU16String()
and let the path get constructed fromstd::u16string
.from what I'm understanding std::filesystem::path is supposed to do the conversion
We use
std::u16string
, can you try constructing from that instead of wide character literal?
added 61 commits
-
29f99af7...456d7c92 - 59 commits from branch
videolan:master
- 29d81552 - qt: do not convert rvalue into pointer in ModelRecoveryAgent
- d24d282e - qt: define QT_NO_CAST_FROM_BYTEARRAY
-
29f99af7...456d7c92 - 59 commits from branch
added MRStatus::InReview label and removed MRStatus::NotCompliant label
added MRStatus::NotCompliant label and removed MRStatus::InReview label
requested review from @chub
added 241 commits
-
d24d282e...3416bca8 - 239 commits from branch
videolan:master
- f5fd5f01 - qt: do not convert rvalue into pointer in ModelRecoveryAgent
- 3b31423d - qt: define QT_NO_CAST_FROM_BYTEARRAY
-
d24d282e...3416bca8 - 239 commits from branch
added MRStatus::InReview label and removed MRStatus::NotCompliant label
mentioned in merge request !6335 (merged)
added MRStatus::NotCompliant label and removed MRStatus::InReview label
@robUx4 I will wait until minimum GNU/LLVM is bumped to 9.1/9, so that I can use
std::filesystem
without directing the linker to link filesystem explicitly.I don't get why we can not use a standard C++ feature just to rely on in-house abstraction. The incentive to fix a broken part should not arise from realizing something else is doing it properly. If some part is broken, it needs to be fixed regardless.
I'm not even sure how to handle the encoding properly with POSIX. libiconv does not seem to be mandatory for VLC.
We don't really have any reason not to use
std::filesystem
(specifically since this is standard) as long as it works everywhere (compat or system impl), just like we didn't have to keep our bit manipulation code as soon as C23 exposed a dedicated standard API for that.The main problem is that writing a polyfill for C++ template code that is multiplatform is much much more demanding than what we see in C.