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

uchardet.dll が存在したらそれを使って文字エンコーディングの検出が行われるように処理追加 #1726

Merged
merged 4 commits into from
Sep 28, 2021

Conversation

beru
Copy link
Contributor

@beru beru commented Sep 20, 2021

PR の目的

uchardet という文字エンコーディングの検出を行うライブラリに対応するのが目的です。

カテゴリ

  • 機能追加

PR の背景

この uchardet というライブラリは Mozilla で使われていたライブラリのようです。今のFirefoxはrustで書かれた chardetng を使っているようです。

VS Codeは jschardet を使っているみたいです。jschardet は Python製の chardet を元にしていて、それの更に元になっているのがC++で書かれた uchardet のようです。

PR のメリット

#1104 で対応がされたICU4Cと比べてuchardetの方がビルドが容易でバイナリサイズも小さいです。

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

特にないと思いますが、あえて言うとDLLの確認が増える分だけ微妙に遅くなるかもしれません。

仕様・動作説明

sakura_core/charset/icu4c/CharsetDetector.hsakura_core/charset/icu4c/CharsetDetector.cpp
の実装を利用しています。フォルダ名が icu4c なのであまり良くないかもしれません。

ICU4C の ucsdet_getName が返す文字列と uchardet の uchardet_get_charset が返す文字列に互換性があるかはきちんと確認していません。大丈夫だといいな。

PR の影響範囲

文字コード検出処理に関係しますが、デフォルトでは uchardet.dll ファイルがパスが通った場所に無ければ処理が有効にならないので、あまり影響は出ないと思います。

テスト内容

テスト1

手順

  • x64\Debug フォルダにx64 Debugビルドした uchardet.dll を配置
  • gb2312.txt を呼んで文字化けしないか確認

テスト2

手順

  • x64\Release フォルダにx64 Releaseビルドした uchardet.dll を配置
  • gb2312.txt を呼んで文字化けしないか確認

関連 issue, PR

#1104 #1725

参考資料

https://docs.microsoft.com/en-us/windows/win32/intl/code-page-identifiers

https://www.freedesktop.org/wiki/Software/uchardet/

https://xz5012.wordpress.com/2017/11/10/my-chinese-version-of-the-moon-over-the-mountain/

@beru beru added the enhancement ■機能追加 label Sep 20, 2021
@AppVeyorBot
Copy link

Build sakura 1.0.3908 failed (commit 0ef1762133 by @beru)

@AppVeyorBot
Copy link

Build sakura 1.0.3909 completed (commit 0ff761fba4 by @beru)

@AppVeyorBot
Copy link

Build sakura 1.0.3911 completed (commit 5a17b768f1 by @beru)

@@ -30,39 +30,21 @@ CharsetDetector::CharsetDetector() noexcept
, _csd(nullptr)
{
_icuin.InitDll();
_uchardet.InitDll();
Copy link
Contributor

Choose a reason for hiding this comment

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

icu4cのためのコードを変更することに違和感があります。

sakura_core/charset/icu4c/CharsetDetector.cpp
 👇コピーして新規作成、icu4cの要素をuchardetの要素で置き換える
sakura_core/charset/uchardet/CharsetDetector.cpp

というアプローチは難しいのでしょうか?

icu4cは確か、バージョンごとに関数名が異なる「変な仕様」で、ビルドするのが大変な構造になっていたような記憶があります。uchardetのバージョン構造がどうなっているか分からないですが、icu4cの仕様に引きずられてビルドが難しくなるのはマイナスだと思います。icu4cの実装をコピペしてuchardetに必要なモノだけを定義していったほうがすっきりした実装になると気がするので、そういう方式がおすすめです。(中身の詳細は、きっと誰も見ないですしw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コピペして分けるのも有りかと思います。しかし namespace 使わない場合、重複するクラス名を付けられないので、既存の CharsetDetector じゃない名前のクラスにする必要が出てきそうです。uchardetCharsetDetector とか?

どうして既存の CharsetDetector クラスの実装を再利用したかというと、その方がコードの変更量が少なくて済むからです。CharsetDetector クラスは CCodeMediator::CheckKanjiCode メソッドで使われていますが、もし別のクラスを追加する場合はここの記述も変えないといけないので。

あと CharsetDetector クラスの public な interface は ICU4C の interface に依存していないので、uchardet を使った文字エンコーディング判定処理もここでやってしまうのが適切だと判断しました。

まぁそれならそれで、sakura_core\charset\icu4c フォルダではなくて上のフォルダにファイルを移動する変更も合わせて行った方が良いかもしれないですね。あと Pimplイディオム を使って CharsetDetector.h ファイルからは ICU4C や uchardet のDLLのラッパーの存在を表面化させないようにした方が良い気がします。

ICU4C と uchardet が返す文字セット名が同じかどうかをまず調べる必要がありそうです(uchardet は iconv と同じ名前を返すらしいです、ICU4Cは良くわかりません)。あと今は連続する if 文で文字セット名⇒サクラエディタ内部コードの変換を行っていますが、ICU4C や uchardet は各国の様々な文字エンコーディングに対応しているのできちんとそれらをWindowsのコードページIDに変換するように記述を追加したいです。

あと if 文の文字列判定を繰り返すのは処理効率が良くないので std::unordered_map で表引きする実装にしたいです。まぁ数が少ないうちは(数十ぐらいだったら)そんなに気にしなくても良いかもしれませんが。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CharsetDetector のファイルは親フォルダに移動しました。
ICU4Cを使う場合の処理は従来通りに戻しました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

icu4cは確か、バージョンごとに関数名が異なる「変な仕様」で、ビルドするのが大変な構造になっていたような記憶があります。uchardetのバージョン構造がどうなっているか分からないですが、icu4cの仕様に引きずられてビルドが難しくなるのはマイナスだと思います。icu4cの実装をコピペしてuchardetに必要なモノだけを定義していったほうがすっきりした実装になると気がするので、そういう方式がおすすめです。(中身の詳細は、きっと誰も見ないですしw

それについては sakura_core\extmodule\CIcu4cI18n.cpp でカバーされているので、uchardet を使う場合には影響はないです。uchardet の DLL を扱う対応は別のファイル sakura_core\extmodule\CUchardet.cpp で行われています。

CharsetDetector クラスは元々ICU4Cしか使っていなかったのですが今回uchardetも使うように処理を追加しました。このクラスに変更を入れずに別のクラスを使うようにした方が良い場合はそうします。

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

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

23.5% 23.5% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.3920 completed (commit 03f33d6d7a by @beru)

@berryzplus
Copy link
Contributor

uchardetの紹介ページに、
c++で書かれたライブラリをC言語に移植したものである
と説明されてるのが気になりました。

c++のオリジナルがあるならそちらを使ったほうがプログラムしやすいんではないかと。
(UNIXのC++だから、そのままではvisual studioではビルドできない可能性もありますが。)

あと、今更気付きましたが、uchardet.dllの入手方法が書いてないっす。

たぶんソースコードを落としてcmakeでビルドしたんだと思うんですが、これで「手順通りテスト」するのはかなり高度な気がしました。

とはいえ、これが入れば #1725 は解決するので、入れてしまった方がいいような気もします。。。

@beru
Copy link
Contributor Author

beru commented Sep 26, 2021

uchardetの紹介ページに、
c++で書かれたライブラリをC言語に移植したものである
と説明されてるのが気になりました。

そのような説明はされていないと思います。おそらく下記の英文を適当に読んだのではないかと思いますが、

uchardet started as a C language binding of the original C++ implementation of the universal charset detection library by Mozilla.

uchardetはMozillaの全世界の文字セット検出ライブラリのC++実装へのC言語バインディングから始まりました。

というような意味合いの文だと思います。
ソースコードはC++製です。https://github.com/freedesktop/uchardet/tree/master/src
外部I/FはC リンケージです。https://github.com/freedesktop/uchardet/blob/master/src/uchardet.h#L42

c++のオリジナルがあるならそちらを使ったほうがプログラムしやすいんではないかと。
(UNIXのC++だから、そのままではvisual studioではビルドできない可能性もありますが。)

自分もVC++だとビルド出来ないかもと思ったんですが試してみたら問題が無かったです。
Microsoft Visual Studio Community 2019 Version 16.11.2 を使っています。

あと、今更気付きましたが、uchardet.dllの入手方法が書いてないっす。

たぶんソースコードを落としてcmakeでビルドしたんだと思うんですが、これで「手順通りテスト」するのはかなり高度な気がしました。

git clone https://github.com/freedesktop/uchardet.git
cd uchardet
mkdir build
cd build
cmake ..
cmake --build . --target libuchardet --config Debug
cmake --build . --target libuchardet --config Release

こうすると uchardet/build/src/Debug フォルダの中にDebugビルドの uchardet.dll ファイルが作られます。
また uchardet/build/src/Release フォルダの中にReleaseビルドの uchardet.dll ファイルが作られます。

とはいえ、これが入れば #1725 は解決するので、入れてしまった方がいいような気もします。。。

uchardet が対応しているエンコーディングに GB2312 はないですがそのスーパーセットの GB18030 はあるので、それでなんとかなればいいなぁと思います。

@berryzplus
Copy link
Contributor

C++ライブラリのC言語バインディングってなんですか?
 👇おいらの理解
C++ライブラリをC言語から直接利用することはできないので、
C言語からでも扱えるようにインターフェースを再定義すること(≒つまり、C言語に移植するってこと)です。

バインディングは移植じゃねぇよ、もそれはそれで正しいと思います。

dllは、zipを貼りますか・・・。
入手性を高めるためにGHAを活用するのも1案ですが、やっていいのか分からないです。

とはいえ、これが入れば #1725 は解決するので、入れてしまった方がいいような気もします。。。

uchardet が対応しているエンコーディングに GB2312 はないですがそのスーパーセットの GB18030 はあるので、それでなんとかなればいいなぁと思います。

そればっかりは中国の人に聞いてみないと分からないですね。

@beru
Copy link
Contributor Author

beru commented Sep 27, 2021

dllは、zipを貼りますか・・・。
入手性を高めるためにGHAを活用するのも1案ですが、やっていいのか分からないです。

#1104 ではDLLファイルは提供していないのだし、これについてもやらなくてもいいんじゃないかと思ってます。「それとこれとは違う!」と言われたら「あ、はい」としか返せませんが…。

uchardet が対応しているエンコーディングに GB2312 はないですがそのスーパーセットの GB18030 はあるので、それでなんとかなればいいなぁと思います。

そればっかりは中国の人に聞いてみないと分からないですね。

そうですね。それにuchardetは世界各国で使われている色々なレガシーなエンコーディングに対応しているので、本当にきちんと動作するかは個人ではなかなか調べきれないでしょうね。

他に気になる点が色々ありますが、このPRで対応するべきか微妙です。

  • uchardet で判定した場合にエンコーディング名からコードページを表引きするようにしたが、ICU4C の場合のコードは元のままでポテンシャルを生かせていない。
    • ICU4Cに関するテストを行うのが手間なので意図的にこうしていますがちょっともったいないです。そもそもICU4C自体が規模が大きいライブラリなので文字エンコーディングの判定だけに使うのもなんだかという気がしますが…。
  • CharsetDetector の利用元のメソッド名が CheckKanjiCode という名前でコメントにも 日本語コードセット判別 と書かれているので実態に合っていない。
    • サクラエディタは元々日本語の文書向けに開発・テストされたものなので、コメントはその名残ですね。
  • コードページ指定でファイルを読むと、ツールバーステータスバーに CP数字 という形式でコードページが表示されるけれど人間には分かりづらい。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

動いてそうなのでいいんじゃないかと思います。

uchardetのwindows向けバイナリは、配布されていないように見えました。
動作確認のためにローカルビルドしたdllは以下になります。
uchardet_x86.zip
uchardet_x64.zip

文字コードセットについて、公的機関(≒IANA)が管理しているのは「名前」だったような気がします。文字コードセットの指定を「番号」ではなくて「名前」で指定できるようにしたいなぁ、と考えているのはちょっと別な話になりますけれども。

@beru
Copy link
Contributor Author

beru commented Sep 28, 2021

レビューありがとうございます。Mergeします。
もし後で問題が見つかったら別PRを作成します。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ■機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants