レイアウト桁位置に対するロジック桁位置の進め方を改善する #1658

ghost · 2021-05-03T12:39:37Z

PR の目的

レイアウト情報の作成過程で、ロジック桁位置の進む量を文字に応じて変化させる。

カテゴリ

仕様変更

PR の背景

現状のレイアウト作成処理では、レイアウト桁位置に対応するロジック桁を次の数だけ進めるようになっています。

ワードラップ: 単語の桁数
行頭禁則: 禁則対象文字と追い出される文字を合わせた桁数
その他の禁則処理: 禁則対象文字の桁数
それ以外: 1文字分の桁数

ここでいう文字の桁数は、1文字が必ずロジック単位で1つであることを想定しているようです。
ロジック行上のデータはUTF-16で表現されるため、サロゲートペアはロジック単位で2つ進めなければなりません。
このため、レイアウト作成フローはサロゲートペアが分割されてしまうリスクを抱えています。

一方、行データのイテレータである CMemoryIterator クラスでは、
以前からロジック・レイアウト単位の増分をそれぞれ CNativeW クラスの静的メンバで計算しており、
サロゲートペアに遭遇しても問題がないようになっています。

PR のメリット

レイアウト作成時にサロゲートペアが前後に分割処理されることによって発生しうる不具合を防げます。

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

この PR の変更対象としたクラスには単体テストがなく、共有データ依存のため追加も難しいです。
このため一時的にカバレッジが低下します（全期間で見たカバレッジには影響ありません）。

仕様・動作説明

レイアウト作成処理において、現在処理している物理データ行上の文字の位置は、 SLayoutWork 構造体のメンバ変数に保持されています。
これまでこの変数にロジック単位で（文字ごとに）1ずつ加算していたところを、各文字毎にCNativeW::GetSizeOfChar()で取得した値を加算するように変更します。

なお、既存コードに追加した GetSizeOfChar のオーバーロードを活用できる箇所がいくつかありますが、遡及適用は行っていません。

PR の影響範囲

各行のレイアウト作成を行う際の準処理（CLayoutMgr::_MakeOneLine()）

テスト内容

以下の手順で変更前後の動作に変更がないことを確認してください。

手順

指定した内容でタイプ別設定を変更したのち、添付したマクロ実行します。
テスト用マクロ (test.zip)

タイプ別設定一覧ダイアログから、「テキスト」タイプの設定を次のように変更します。
（この設定変更を忘れないでください。）
- 英文ワードラップ：有効
- 句読点ぶら下げ：有効
- 改行ぶらさげ：有効
- 行頭禁則：有効
- 行末禁則：有効
添付のマクロを実行し、結果を確認します。
- マクロで挿入されるデータは 329 文字・レイアウト 57 行・ロジック 29 27 行です。

参考資料

AppVeyorBot · 2021-05-03T13:16:00Z

✅ Build sakura 1.0.3728 completed (commit 2ec9b6627d by @kazasaku)

kengoide

概要を読んだ限りでは不適切な仕様を改善する変更のように思えますが、実際に現在の実装でサロゲートペアが分割される例があるのかどうかが分かりませんでした。

分割される例があるかどうかの検証が必要だと思います。実害がないなら変更そのものが不要になってしまいますし、あるのなら改善を証明するテスト手法があったほうが良いように思います。

kengoide · 2021-05-04T04:44:02Z

sakura_core/mem/CNativeW.h

+	//! 指定した位置の文字がwchar_t何個分かを返す
+	static CLogicInt GetSizeOfChar( const wchar_t* pData, int nDataLen, int nIdx );
+	static CLogicInt GetSizeOfChar( const CStringRef& cStr, int nIdx )
+		{ return GetSizeOfChar( cStr.GetPtr(), cStr.GetLength(), nIdx ); }


追加したオーバーロードを検証するテストが欲しいです。

CStringRef を扱う GetKetaOfChar ~~/ GetColmOfChar~~ のオーバーロードにもテストが無いです。
これも追加しときます。

対応完了。

CStringRef を扱う GetKetaOfChar / GetColmOfChar のオーバーロードにもテストが無いです。

これはたぶん私のやらかしです。追加していただきありがとうございます。

kengoide · 2021-05-04T04:51:58Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp

 		CLayoutInt k = cLayoutMgr.GetLayoutXOfChar(cLineStr, nPos);
-		if(0 == k)k = CLayoutInt(1);


これ、削除して大丈夫ですか？ゼロ幅スペースなどの幅0pxの文字でも1px描画するのが今の仕様であるように見えます。消してしまうと仕様変更になりませんか？

現時点ではここに来るのは次の文字の時だけです（ IS_KEYWORD_CHAR の仕様による）。
#$0123456789@ABCDEFGHIJKLMNOPQRSTUVWXYZ\_abcdefghijklmnopqrstuvwxyz

自分には過去に行ったサロゲートペア対応時の削除漏れに見えました。
また、この箇所以外に0桁を1桁にみなす動作をしている箇所はすべてコメントアウトされているはずです。
（このPRではCode Smellになってしまうので除去しています。）

おっしゃる通り、IS_KEYWORD_CHAR の条件があるので不要ということでよさそうですね。了解しました。

ghost · 2021-05-04T05:44:51Z

不適切な仕様を改善する変更のように思えますが、実際に現在の実装でサロゲートペアが分割される例があるのかどうかが分かりません

実際に問題になったのは行頭禁則処理においてです。次行に追い出されるロジック単位文字数が足りませんでした。
#1543 では条件判定追加で済ませましたが、後から桁数の算出方法が非対称になっている（※）ことがそもそもの原因だと思いましたので、本PRを作成しました。
（※：レイアウト単位の増分は文字ごとに計算するのに対して、ロジック単位の増分は値が固定されている）

AppVeyorBot · 2021-05-04T09:05:32Z

✅ Build sakura 1.0.3730 completed (commit 8be14e313d by @kazasaku)

kengoide · 2021-05-04T10:10:05Z

tests/unittests/test-cnative.cpp

+	// テスト用の文字列参照を提供するユーティリティ
+	class GetStringRef {
+	public:
+		CStringRef SetString(const wchar_t* pcStr, size_t nLen) { return CStringRef(pcStr, nLen); }


GetSizeOfChar の呼び出し時に CStringRef を直接構築すると不都合があるのでしょうか？少し考えてみましたがよく分かりませんでした。

kengoide · 2021-05-04T10:14:54Z

tests/unittests/test-cnative.cpp

+	@brief CStringRef型文字列を使用したGetSizeOfCharの仕様確認
+	@remark 文字列中の指定位置にある文字の符号単位数を返す
+ */
+TEST(CNativeW, GetSizeOfChar_with_CStringRef) {


これは好みの問題かもしれませんが、テストケースごと増やすよりも以前からのテストケースに assertion を追加する形の方が簡潔で良いように思えます。同じコメントを繰り返す必要がなくなりますし、内容のほぼ同じ asssertion は隣接していた方が読む人のストレス軽減に役立ちそうです。

kengoide · 2021-05-04T10:17:07Z

sakura_core/mem/CNativeW.h

+	//! 指定した位置の文字がwchar_t何個分かを返す
+	static CLogicInt GetSizeOfChar( const wchar_t* pData, int nDataLen, int nIdx );
+	static CLogicInt GetSizeOfChar( const CStringRef& cStr, int nIdx )
+		{ return GetSizeOfChar( cStr.GetPtr(), cStr.GetLength(), nIdx ); }


CStringRef を扱う GetKetaOfChar / GetColmOfChar のオーバーロードにもテストが無いです。

これはたぶん私のやらかしです。追加していただきありがとうございます。

sonarcloud · 2021-05-04T11:36:06Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

23.5% Coverage
0.0% Duplication

AppVeyorBot · 2021-05-04T11:45:36Z

✅ Build sakura 1.0.3732 completed (commit 639d03ccf7 by @kazasaku)

kengoide · 2021-05-04T13:37:09Z

添付のマクロを動かしてみました。

PR適用前後で結果に変化はありませんでした。
「329 文字・レイアウト 57 行」でしたが、「ロジック 29 行」ではなく「27 行」になっているように見えます。（環境依存？ロジック行の数え方が間違っている？）

行番号を数えて29でしょうか？終了時のステータスバーの表示であれば「28 行」になっています。

ghost · 2021-05-04T13:51:42Z

…！

「329 文字・レイアウト 57 行」でしたが、「ロジック 29 行」ではなく「27 行」になっているように見えます。

27 行で正しいです。失礼しました。

kengoide · 2021-05-04T14:17:06Z

27 行で正しいです。

でしたら大丈夫ですね😃

実際に問題になったのは行頭禁則処理においてです。次行に追い出されるロジック単位文字数が足りませんでした。
#1543 では条件判定追加で済ませましたが、後から桁数の算出方法が非対称になっている（※）ことがそもそもの原因だと思いましたので、本PRを作成しました。

ある種の弥縫策を改めて本修正にしたという理解でよろしいでしょうか。そういうことでしたら前後で変化がないことを確認するテストで適切ですね。

kengoide

良さそうに見えます。

berryzplus · 2021-05-04T15:38:14Z

レビューありがとうございます。
このへんは難しくて、積極的にレビューに加われなかった感じですが、異論はないです。

サクラエディタの内部文字コードは、いわゆるUnicodeです。
Unicodeには複数のエンコーディング方式が存在していますが、Windows標準はUTF-16LEなので
サクラエディタの内部エンコーディングもUTF-16LEを基本としています。

形式	内容	備考
文字列	🚹 × 🚺
`uint32_t[]`	`\u1F6B9` `\xD7` `\u1F6BA`
`wchar_t[]`	`\xD83D` `\xDEB9` `\xD7` `\xD83D` `\xDEBA`	サロゲートペアのhi,lowは必ずこの順。
`std::byte[]`	`\x3D` `\xD8` `\xB9` `\xDE` `\xD7` `\x00` `\x3D` `\xD8` `\xBA` `\xDE`	リトルエンディアンなのでこう見える。

サクラエディタでは、Unicodeに変換できなかったバイトデータを \xDCxx にマップする（ことがある）という仕様があるので、末尾から前方に向けて一文字進めるときの処理が若干複雑になります。

ghost · 2021-05-05T06:39:22Z

レビューありがとうございます。
マージしてしまいます。
他に問題があれば対応いたしますのでお知らせください。

Kohki Akikaze added 3 commits May 3, 2021 20:20

CNativeW::GetSizeOfCharにCStringRef型の文字列を受け入れるオーバーロードを追加

9586733

レイアウト作成フローにおいて物理データ参照位置の増分を文字ごとに都度計算する

ffa332c

サロゲートペア対策としてコメントアウトしたコードを削除

fe56ced

beru added the specification change ■仕様変更 label May 4, 2021

kengoide requested changes May 4, 2021

View reviewed changes

kengoide reviewed May 4, 2021

View reviewed changes

CNativeW::GetSizeOfChar 及び CNativeW::GetKetaOfChar のオーバーロードに対するテストの追加

bf6a3af

kengoide approved these changes May 4, 2021

View reviewed changes

ghost merged commit 7206fa0 into sakura-editor:master May 5, 2021

ghost deleted the feature/change_layout_maker_use_getsizeofchar branch May 5, 2021 06:52

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

レイアウト桁位置に対するロジック桁位置の進め方を改善する #1658

レイアウト桁位置に対するロジック桁位置の進め方を改善する #1658

ghost commented May 3, 2021 •

edited by ghost

AppVeyorBot commented May 3, 2021

kengoide left a comment

kengoide May 4, 2021

ghost May 4, 2021 •

edited by ghost

ghost May 4, 2021

kengoide May 4, 2021

kengoide May 4, 2021

ghost May 4, 2021 •

edited by ghost

kengoide May 4, 2021

ghost commented May 4, 2021

AppVeyorBot commented May 4, 2021

kengoide May 4, 2021

kengoide May 4, 2021

kengoide May 4, 2021

sonarcloud bot commented May 4, 2021

AppVeyorBot commented May 4, 2021

kengoide commented May 4, 2021

ghost commented May 4, 2021

kengoide commented May 4, 2021

kengoide left a comment

berryzplus commented May 4, 2021

ghost commented May 5, 2021

		CLayoutInt k = cLayoutMgr.GetLayoutXOfChar(cLineStr, nPos);
		if(0 == k)k = CLayoutInt(1);

レイアウト桁位置に対するロジック桁位置の進め方を改善する #1658

レイアウト桁位置に対するロジック桁位置の進め方を改善する #1658

Conversation

ghost commented May 3, 2021 • edited by ghost

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

手順

関連 issue, PR

参考資料

AppVeyorBot commented May 3, 2021

kengoide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 4, 2021 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 4, 2021 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 4, 2021

AppVeyorBot commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 4, 2021

AppVeyorBot commented May 4, 2021

kengoide commented May 4, 2021

ghost commented May 4, 2021

kengoide commented May 4, 2021

kengoide left a comment

Choose a reason for hiding this comment

berryzplus commented May 4, 2021

ghost commented May 5, 2021

ghost commented May 3, 2021 •

edited by ghost

ghost May 4, 2021 •

edited by ghost

ghost May 4, 2021 •

edited by ghost