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

config 配下のファイルを UTF-8 (BOM付) に変換。一部コメント末尾文字は ASCII に変換。 #249

Merged
merged 2 commits into from
Jul 14, 2018

Conversation

kobake
Copy link
Member

@kobake kobake commented Jul 12, 2018

文字コード変換

該当フォルダ内の文字コードをすべて UTF-8 (BOM付) に変換しました。

cd sakura_core/config
nkf --overwrite --oc=UTF-8-BOM *.cpp
nkf --overwrite --oc=UTF-8-BOM *.h

追加対応

ヘッダ内コメントの末尾文字がマルチバイトだと問題がることがあるので、末尾文字を ASCII 文字に差し替えています。

確認方法

WinMerge で変更前と変更後を比較すると、文字コードおよびコメント末尾文字以外の変更が無いことが確認できます。

関連 Issues

ソースコードのUnicode化 #112

@berryzplus
Copy link
Contributor

デバッグ版アプリのタイトルバー文字列の変更に当たると思います。

仕様変更と認識しますが、合ってますか?

@kobake
Copy link
Member Author

kobake commented Jul 12, 2018

仕様変更と認識しますが、合ってますか?

デベロッパ的には仕様変更になります。
ただ、一般リリース時の挙動や表示は変わらない認識です。

@kobake kobake changed the title config 配下のファイルを UTF-8 (BOM付) に変換。問題の出る文字列は表現差し替え。 [WIP] config 配下のファイルを UTF-8 (BOM付) に変換。問題の出る文字列は表現差し替え。 Jul 12, 2018
@m-tmatma
Copy link
Member

この PR は基本的に文字コード変換に対する PR なので、仕様変更を含むなら別 PR にするのがいいですが、その手間を省略するのであれば、仕様変更を行うことがわかる別チケットを作って、 #249 で対応したことを記載すればいいと思います。

@kobake
Copy link
Member Author

kobake commented Jul 12, 2018

うーん、小さな変更であれば対応を混ぜ込んでしまうのは自分はアリと思っている(他の人のPRでそういう部分があっても支障無ければ approve もしちゃう)のですが、混ざるとけっこう困ること多かったりしますか?

@m-tmatma
Copy link
Member

後でどんな修正をしたのかを追跡するのが
難しくなります。

@kobake
Copy link
Member Author

kobake commented Jul 12, 2018

自分は「匙加減」の問題だと思っています。変更影響が大きければ当然分けます。
このPRについてはPR名で伝わる情報で十分かなーとか。あと Blame すれば分析は難しくないですし・・・。さすがにコミットまで統合されていると困りますけど、コミットが分かれていれば問題なくないですか?

@kobake kobake changed the title [WIP] config 配下のファイルを UTF-8 (BOM付) に変換。問題の出る文字列は表現差し替え。 config 配下のファイルを UTF-8 (BOM付) に変換。問題の出る文字列は表現差し替え。 Jul 12, 2018
@berryzplus
Copy link
Contributor

仕様変更については意図が確認できれば良いです。

追加で、問題が出るの真意を確認しときたいです。

ぼくの認識ではL"(デバッグ版)"は国際的に見ても問題ない文字列です。

考慮漏らしてる可能性は高いので、どんな問題が起きるか知りたいです。

@kobake
Copy link
Member Author

kobake commented Jul 12, 2018

ぼくの認識ではL"(デバッグ版)"は国際的に見ても問題ない文字列です。

考慮漏らしてる可能性は高いので、どんな問題が起きるか知りたいです。

L"ほげほげ" が期待するエンコーディングは const wchar_t* の UTF-16 形式であると認識しています。
実行して試したわけではないですが、UTF-8 エンコーディングの C++ ファイル内で L"ほげほげ" みたいな文字列を書いてしまうと、リテラル表現が UTF-8 になってしまい、しかし動作としては UTF-16 を期待する動きをしてしまうので文字化けすると思います。なのでそれを避ける簡単な方法として ASCII 範囲の文字列に変更しました。

こういう文字列をちゃんと残したい場合にはリソース側に文字列定義を逃がしてリソース経由で文字列構築することになると思います。

@berryzplus
Copy link
Contributor

ちょっと実験してみたいです。

確かワイド文字列リテラルは変換かけられてから埋め込まれるものだった気がします。sjisだとsjis基準でユニコード化された文字列がutf16LEで埋め込まれます。utf8も一種のマルチバイトなので似たような変換がかかって正しく埋め込まれるような気がします。

@ds14050
Copy link
Contributor

ds14050 commented Jul 12, 2018

コンパイラがリテラルを扱う方法は berryzplus さんの言う通りになる気がします(実験結果待ってます)。なので問題は異なるエンコーディング(2種類の「ANSI」エンコーディング)のファイルを #include した(された)ときに起こるのかな、と想像していました。

なんとなくリテラルに日本語が使えないというような認識を持たないために、どこに問題があるのか知識を共有したいです。

@berryzplus
Copy link
Contributor

試してみた結果ですが、問題起きませんでした。

ConsoleApplication2.zip

なぜこんなに時間がかかったかというと、
こないだOSの言語を English に変えたのを忘れていて、
日本語文字列がまったく表示されない現象に悩まされていたためです。
mainの冒頭に ::_wsetlocale を入れることで解決できましたとさ・・・

@berryzplus
Copy link
Contributor

rc.exeには、ファイルの途中でエンコーディング解釈を変える命令があるので、
通常のcppソースコードと異なる動きをすることがあります。
sakura_rc.rcで起きた問題はよく分ってなかったんですけど・・・。

@kobake
Copy link
Member Author

kobake commented Jul 13, 2018

すみません爆睡してました。検証ありがとうございます。
ちょっとリテラル系は慎重に挙動確認していきたいですね。あとで単体PRも立ててみます。

@kobake kobake changed the title config 配下のファイルを UTF-8 (BOM付) に変換。問題の出る文字列は表現差し替え。 [WIP] config 配下のファイルを UTF-8 (BOM付) に変換。問題の出る文字列は表現差し替え。 Jul 13, 2018
@kobake
Copy link
Member Author

kobake commented Jul 13, 2018

#250 で文字列表現を変更せずに、ヘッダのファイルエンコーディングを変更する対応を行いました。これが問題ないようであれば、本 PR での文字列表現差し替え対応は取り下げる対応を行います。

@kobake kobake changed the title [WIP] config 配下のファイルを UTF-8 (BOM付) に変換。問題の出る文字列は表現差し替え。 config 配下のファイルを UTF-8 (BOM付) に変換。一部コメント末尾文字は ASCII に変換。 Jul 13, 2018
@kobake
Copy link
Member Author

kobake commented Jul 13, 2018

#250 の対応が完了しました。
日本語文字列定数がヘッダに含まれていても挙動に問題ないことが確認できましたので、本 PR での文字列表現差し替え処理も取り下げた形で rebase しました。

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.

LGTMです。

@kobake kobake merged commit 92d6a3b into sakura-editor:master Jul 14, 2018
@kobake kobake deleted the config-utf8 branch July 14, 2018 20:13
@m-tmatma m-tmatma added this to the next release milestone Jul 25, 2018
@ds14050 ds14050 added the refactoring リファクタリング 【ChangeLog除外】 label Sep 18, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
config 配下のファイルを UTF-8 (BOM付) に変換。一部コメント末尾文字は ASCII に変換。
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants