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

utils: Make fs::path::string() always return utf-8 string on Windows #13877

Merged
merged 1 commit into from Sep 25, 2018

Conversation

Projects
None yet
6 participants
@ken2812221
Copy link
Member

commented Aug 4, 2018

Imbue fs::path with std::codecvt_utf8_utf16 at SetupEnvironment(), so that default string encoding will be utf-8 inside fs::path.

@ken2812221 ken2812221 referenced this pull request Aug 4, 2018

Closed

Filename and command line encoding issue on Windows #13869

11 of 12 tasks complete

@ken2812221 ken2812221 force-pushed the ken2812221:fs-path-utf8 branch Aug 4, 2018

@ken2812221 ken2812221 changed the title utils: Make fs::path::string() always return utf-8 string utils: Make fs::path::string() always return utf-8 string on Windows Aug 4, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #14123 (gui: Add GUIUtil::bringToFront by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@ken2812221 ken2812221 force-pushed the ken2812221:fs-path-utf8 branch Aug 4, 2018

@ken2812221 ken2812221 changed the title utils: Make fs::path::string() always return utf-8 string on Windows wip, utils: Make fs::path::string() always return utf-8 string on Windows Aug 5, 2018

@ken2812221 ken2812221 changed the title wip, utils: Make fs::path::string() always return utf-8 string on Windows utils: Make fs::path::string() always return utf-8 string on Windows Aug 5, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:fs-path-utf8 branch 5 times, most recently Aug 5, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:fs-path-utf8 branch Aug 24, 2018

src/util.cpp Outdated
@@ -58,6 +58,7 @@
#ifndef NOMINMAX
#define NOMINMAX
#endif
#include <codecvt>

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Aug 31, 2018

Member

maybe include only if WIN32?

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 31, 2018

Author Member

OK. But I'll wait for gitian build done.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 6, 2018

Author Member

This is already be defined only if WIN32. Forgot about this

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 22, 2018

Member

Previously applied patch detected.

Needs rebase?

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 22, 2018

Author Member

Rebased

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

Done quick code review (on other commits as well), seems ok, I will test more deeply later this week. Very excited about this.

@@ -62,8 +62,6 @@
#include <QFontDatabase>
#endif

static fs::detail::utf8_codecvt_facet utf8;

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 6, 2018

Author Member

Drop this because this convert between UTF-8 and UCS-2, not UTF-16. Use std::codecvt_utf8_utf16 instead.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 6, 2018

Contributor

Drop this because this convert between UTF-8 and UCS-2, not UTF-16. Use std::codecvt_utf8_utf16 instead.

This is documented at https://www.boost.org/doc/libs/1_68_0/boost/detail/utf8_codecvt_facet.hpp, in case anybody else is curious.

@ryanofsky
Copy link
Contributor

left a comment

utACK 26d6fe4a10c2c8cd16303835dd7cd5289f2d3b67

@@ -62,8 +62,6 @@
#include <QFontDatabase>
#endif

static fs::detail::utf8_codecvt_facet utf8;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 6, 2018

Contributor

Drop this because this convert between UTF-8 and UCS-2, not UTF-16. Use std::codecvt_utf8_utf16 instead.

This is documented at https://www.boost.org/doc/libs/1_68_0/boost/detail/utf8_codecvt_facet.hpp, in case anybody else is curious.

Show resolved Hide resolved src/qt/guiutil.cpp
@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Done quick code review (on other commits as well), seems ok, I will test more deeply later this week. Very excited about this.

@NicolasDorier, are you still planning on testing this? If not, I think it would be good to get this merged since #13878 depends on it.

Note to reviewers: even though fix here is pretty esoteric, it should be hopefully should be clear that it doesn't effect anything other than fs::path encodings used on windows.

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Sep 21, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

Gitian builds for commit 920c090 (master):

Gitian builds for commit af0794a4f1e0a892780150c6069154f8327cb1e2 (master and this pull):

@ken2812221 ken2812221 force-pushed the ken2812221:fs-path-utf8 branch to 2c3eade Sep 22, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 22, 2018

utACK 2c3eade (Only checked that this does't affect linux)

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

utACK 2c3eade

@MarcoFalke MarcoFalke merged commit 2c3eade into bitcoin:master Sep 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 25, 2018

Merge #13877: utils: Make fs::path::string() always return utf-8 stri…
…ng on Windows

2c3eade Make fs::path::string() always return utf-8 string (Chun Kuan Lee)

Pull request description:

  Imbue `fs::path` with `std::codecvt_utf8_utf16` at `SetupEnvironment()`, so that default string encoding will be utf-8 inside `fs::path`.

Tree-SHA512: 0cb59464d777278decbf24771fc5ff0cb2caa7bc2fe8ee5cd36c97a2324873a3caad131f08f050393b488316ee7f4ab0b28b7fa4699e41839f8e51b9867d5118

@ken2812221 ken2812221 deleted the ken2812221:fs-path-utf8 branch Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.