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

文字コード自動判別にwin10組み込みのICU4Cを使う #1784

Closed

Conversation

berryzplus
Copy link
Contributor

PR の目的

タイトル通りです。
#1104 のPRを改善して、ICU4Cの巨大なDLLを用意しなくても済むように改良します。

カテゴリ

  • 仕様変更

PR の背景

#1783 の焼き直しです。

#1783 は、変更を最小限に抑えるために、
手元で作っていた仮対応からMInGW対応を考慮を除外していました。
(結果、ビルドが通らなかったので閉じてしまいましたが・・・)

妥協できるところまでの精査が終わったのでPRとする次第です。

PR のメリット

PR のデメリット (トレードオフとかあれば)

仕様・動作説明

変更前: ICU4C66があれば自動判別にICU4Cを使う。
変更後: Windows 10 1903以降なら自動判別にICU4Cを使う。

Windows 10のアップデートは、
端末のスペックが不十分である場合を除いて、
ユーザーが固定しない限りは「勝手にWindows 11に更新される」ようになっています。

修正により #1726 の機能が実質的に死にますが、
その点が「いいか悪いか」は一旦わきに置いておきたいです。

PR の影響範囲

文字コード自動判別に影響する変更です。

テスト内容

テスト1

手順

関連 issue, PR

参考資料

正規のWindows10 SDKを使った場合、省略すると利用可能な最新バージョンを使う設定になる。
MinGWのSDKではバージョン定義が正しく行われないので、
統合されたICU4Cを利用するために必要な定義を行う。

NTDDI_WIN10_RS3が定義されるのはWindows 10 SDK v10.0.17763.0以降。
MinGWにはicu.hがないので、正規のWindows 10 SDKから該当ファイル(自動生成)をコピーする。
コピー処理がテキトーなので改善のかなり余地がある。
(コピー元がv10.0.18362.0固定とか。)
@AppVeyorBot
Copy link

Build sakura 1.0.4015 failed (commit 300a2d9ae7 by @berryzplus)

@berryzplus berryzplus marked this pull request as draft January 30, 2022 10:57
@berryzplus
Copy link
Contributor Author

テストが失敗している・・・

@sonarcloud
Copy link

sonarcloud bot commented Jan 30, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

58.3% 58.3% Coverage
0.0% 0.0% Duplication

@beru beru added the specification change ■仕様変更 label Jan 30, 2022
@@ -55,7 +55,7 @@
<ItemDefinitionGroup Label="sakura.common">
<ClCompile>
<AdditionalIncludeDirectories>..\sakura_core;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<PreprocessorDefinitions>_WIN32_WINNT=_WIN32_WINNT_WIN7;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<PreprocessorDefinitions>_WIN32_WINNT=_WIN32_WINNT_WIN10;NTDDI_VERSION=NTDDI_WIN10_RS3;%(PreprocessorDefinitions)</PreprocessorDefinitions>
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows 8.1を使っているユーザーがビルドが出来ないという苦情が来ないか心配…は特にしていません。

NTDDI_VERSION についてのページ : https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN

Windows 10 Home and Pro の各リリースについて書かれているページ : https://docs.microsoft.com/en-us/lifecycle/products/windows-10-home-and-pro

古いバージョンでの確認はいつかはされなくなる運命にありますね。

Copy link
Contributor

Choose a reason for hiding this comment

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

@beru
Copy link
Contributor

beru commented Jan 30, 2022

修正により #1726 の機能が実質的に死にますが、
その点が「いいか悪いか」は一旦わきに置いておきたいです。

これは別に構わないと思います。uchardet対応を生かしたいと思った人がまた別のMRで共存する方法を考えれば良いかなと。

自分が思いつくのは設定でどちらを使うかを切り替えるとかですが、今の作りだと設定項目を増やして設定画面UI側でも新規項目に対応させるように改造するのが大変なので、もっと楽に設定項目を増やせる方式のUIに切り替えた後にした方が良いと思います。いつかはやりたいと思って放置している点の一つですね。

他に気になる点としては、CharsetDetector::Detect の実装で _icuin の場合は _uchardet と違って表引きしていないので対応する文字セットが限られている点です。でもuchardet対応自体が隠し機能みたいなものだし、後でまた別のPRとかで対応すればよいと思います。

@berryzplus
Copy link
Contributor Author

自分が思いつくのは設定でどちらを使うかを切り替えるとかですが、今の作りだと設定項目を増やして設定画面UI側でも新規項目に対応させるように改造するのが大変なので、もっと楽に設定項目を増やせる方式のUIに切り替えた後にした方が良いと思います。いつかはやりたいと思って放置している点の一つですね。

設定による振る舞いの切替はやりたくないっす。

  • 設定の読み込み処理に不安がある。
  • 設定の書き込み処理に不安がある。
  • 設定の変更管理方法に不安がある。
  • 設定の同期方法に不安がある。
  • 設定を保持する型の型名が誤っている(DLLじゃないのにDLLと付いている)

他に気になる点としては、CharsetDetector::Detect の実装で _icuin の場合は _uchardet と違って表引きしていないので対応する文字セットが限られている点です。

ここは改善できる部分だと思ってます。
文字セット変換器の生成に「名前」を指定できるようにしたらいいのかな、と。
現状は「独自の文字セット番号」か「Windowsのコードページ(番号)」を指定してます。
影響範囲広いし、めんどうなので対応進めていませんけれども。

@beru
Copy link
Contributor

beru commented Jan 31, 2022

設定による振る舞いの切替はやりたくないっす。

  • 設定の読み込み処理に不安がある。
  • 設定の書き込み処理に不安がある。
  • 設定の変更管理方法に不安がある。
  • 設定の同期方法に不安がある。
  • 設定を保持する型の型名が誤っている(DLLじゃないのにDLLと付いている)

これは特定の設定の事なのか、それとも設定全般の事なのか、、後者だとしたら全体的に不安定だという事になりますがまぁミッションクリティカルな用途に使える堅牢性がある実装でない事は確かですね。

ここは改善できる部分だと思ってます。 文字セット変換器の生成に「名前」を指定できるようにしたらいいのかな、と。 現状は「独自の文字セット番号」か「Windowsのコードページ(番号)」を指定してます。 影響範囲広いし、めんどうなので対応進めていませんけれども。

ICU4Cもuchardetも指定できる文字コードセットの名前って互換性が無かったですっけ?未調査ですが…。

@beru
Copy link
Contributor

beru commented Feb 2, 2022

修正により #1726 の機能が実質的に死にますが、
その点が「いいか悪いか」は一旦わきに置いておきたいです。

思いついた解決策として、CharsetDetector::Detect メソッドで _icuin より先に _uchardet を使った記述を持ってくるように移動すれば良いような気がします。uchardetのDLLはデフォルトでは無いし殆どの環境で置かれていないので影響は少なく、DLLファイルを置いた人だけ使えます。ICU4CのDLLはWindows10の途中から同梱されるようになったという事なので殆どの動作環境で有効化されて使われると思います。

@berryzplus
Copy link
Contributor Author

ICU4Cの機能は自動判別だけじゃないので、
「それ以外の機能」も使えるよう提案するのが良さそうに思えてきたので閉じます。

@berryzplus berryzplus closed this Feb 6, 2022
@berryzplus berryzplus deleted the feature/use_icu_of_win10 branch February 6, 2022 19:07
@berryzplus
Copy link
Contributor Author

メモ(NGになったテストの詳細):

これ。

ConvTestParamType{ F_CODECNV_AUTO2SJIS, L"化けラッタ!!", L"化けラッタ!!" },

  1. L"化けラッタ!!" を SJIS に変換。
    バイナリデータ"化けラッタ!!"(SJIS) ができる。
  2. バイナリデータの文字コードを自動判別する。
    ここで判別される文字コードが UTF-16BE(誤判定) になっている。
  3. 判別された文字コードでUNICODEに戻す。

uchardetを使った場合には正しく判定できるので、
ICU4Cの単発文字コード判別は使い物にならない
という結論が出たと思っています。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specification change ■仕様変更
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants