Skip to content

Conversation

ken2812221
Copy link
Contributor

@ken2812221 ken2812221 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 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
Copy link
Contributor

DrahtBot commented Aug 4, 2018

Note to reviewers: This pull request conflicts with the following ones:

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 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
src/util.cpp Outdated
@@ -58,6 +58,7 @@
#ifndef NOMINMAX
#define NOMINMAX
#endif
#include <codecvt>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe include only if WIN32?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously applied patch detected.

Needs rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased

@NicolasDorier
Copy link
Contributor

NicolasDorier 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 26d6fe4a10c2c8cd16303835dd7cd5289f2d3b67

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

static fs::detail::utf8_codecvt_facet utf8;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

@maflcko maflcko added this to the 0.18.0 milestone Sep 21, 2018
@DrahtBot
Copy link
Contributor

Gitian builds for commit 920c090 (master):

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

@maflcko
Copy link
Member

maflcko commented Sep 22, 2018

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

@laanwj
Copy link
Member

laanwj commented Sep 23, 2018

utACK 2c3eade

@maflcko maflcko merged commit 2c3eade into bitcoin:master Sep 25, 2018
maflcko pushed a commit that referenced this pull request Sep 25, 2018
…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 fs-path-utf8 branch September 25, 2018 18:09
Warrows added a commit to Warrows/PIVX that referenced this pull request Oct 14, 2019
Backports bitcoin#13877

Imbue fs::path with std::codecvt_utf8_utf16 at SetupEnvironment(), so
that default string encoding will be utf-8 inside fs::path.
Warrows added a commit to Warrows/PIVX that referenced this pull request Nov 23, 2019
Backports bitcoin#13877

Imbue fs::path with std::codecvt_utf8_utf16 at SetupEnvironment(), so
that default string encoding will be utf-8 inside fs::path.
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 10, 2021
…-8 string 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

# Conflicts:
#	src/qt/guiutil.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 10, 2021
…-8 string 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

# Conflicts:
#	src/qt/guiutil.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 11, 2021
…-8 string 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

# Conflicts:
#	src/qt/guiutil.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 12, 2021
…-8 string 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

# Conflicts:
#	src/qt/guiutil.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 12, 2021
…-8 string 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

# Conflicts:
#	src/qt/guiutil.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 13, 2021
…-8 string 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

# Conflicts:
#	src/qt/guiutil.cpp
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 13, 2021
…-8 string 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

# Conflicts:
#	src/qt/guiutil.cpp
UdjinM6 added a commit to dashpay/dash that referenced this pull request Jul 13, 2021
Merge bitcoin#13877: utils: Make fs::path::string() always return utf-8 string on Windows
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 5, 2021
63e0be6 [Remove] By-pass logprint-scanner restriction. (furszy)
280ced3 utils: Fix broken Windows filelock (Chun Kuan Lee)
be89860 utils: Convert Windows args to utf-8 string (Chun Kuan Lee)
e8cfa6e Call unicode API on Windows (Chun Kuan Lee)
1a02a8a tests: Add test case for std::ios_base::ate (Chun Kuan Lee)
2e57cd4 Move boost/std fstream to fsbridge (furszy)
9d8bcd4 utils: Add fsbridge fstream function wrapper (Chun Kuan Lee)
d59d48d utils: Convert fs error messages from multibyte to utf-8 (ken2812221)
9ef58cc Logging: use "fmterr" variable name for errors instead of general "e" that can be used by any other function. (furszy)
dd94241 utils: Use _wfopen and _wreopen on Windows (Chun Kuan Lee)
3993641 add unicode compatible file_lock for Windows (Chun Kuan Lee)
48349f8 Provide relevant error message if datadir is not writable. (murrayn)

Pull request description:

  As the software is currently using the ANSI encoding on Windows, the user's language settings could affect the proper functioning of the node/wallet, to the point of not be able to open some non-ASCII name files and directories.

  This solves the Windows encoding issues, completing the entire bitcoin#13869 work path (and some other required backports). Enabling for example users that use non-english characters in directories and file names to be accepted.

  Backported PRs:
  * bitcoin#12422.
  * bitcoin#12630.
  * bitcoin#13862.
  * bitcoin#13866.
  * bitcoin#13877.
  * bitcoin#13878.
  * bitcoin#13883.
  * bitcoin#13884.
  * bitcoin#13886.
  * bitcoin#13888.
  * bitcoin#14192.
  * bitcoin#13734.
  * bitcoin#14426.

  This is built on top of other two PRs that i have open #2423 and #2369.
  Solves old issues #940 and #2163.

  TODO:
  * Backport `assert_start_raises_init_error` and `ErrorMatch` in TestNode` (bitcoin#12718)

ACKs for top commit:
  Fuzzbawls:
    ACK 63e0be6
  random-zebra:
    ACK 63e0be6 and merging...

Tree-SHA512: cb1f7c23abb5b7b3af50bba18652cc2cad93fd7c2fca9c16ffd3fee34c4c152a3b666dfa87fe6b44c430064dcdee4367144dcb4a41203c91b0173b805bdb3d7d
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants