プロポーショナル版で禁則処理を使えるようにする #1543

ghost · 2021-02-17T04:15:07Z

PR の目的

禁則処理が使えない問題を解決します。

カテゴリ

不具合修正

PR の背景

過去に行われたプロポーショナルフォント対応の際、内部で取り扱うレイアウト上の桁数が半角単位からピクセル単位に変更されました。
禁則処理用の関数に対して、これに対応する変更がなされなかったため、条件分岐が正しく判定されなくなっていました。
（※似たような機能で「改行ぶら下げ」がありますが、これは正常に機能しているようです。）

PR のメリット

自明だと思います。

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

仕様・動作説明

patchunicode#1034として登録されているパッチをもとに文字幅の換算処理を追加して、以前と同様に「行末付近でのみ判定を行う」ようにしました。
句読点ぶら下げの時は行末の半角2桁、行頭・行末禁則の時は行末の半角4桁の範囲内に達したときに限り禁則処理の実施判定が行われます。
（この範囲に現在位置にある文字と次位の文字が収まらず、折り返しが発生する時に禁則処理が実施されます。）

またIsKinsokuPosHead()及びIsKinsokuPosTail()は、これまで行の残り桁数と各文字の桁数の組み合わせで判定を行っていましたが、文字幅がピクセル単位の数値となった現在、この方法では正しく判定ができません。
そのため、IsKinsokuPosKuto()の条件式と同様に文字幅（注：ここでいう文字幅は現在の仕様では文字間隔を含みます）が行の残り幅より大きいかどうかで判定を行うようにし、単位の違いに関わらず判定できるようにしました。

そのほか、次の変更を含みます。

関係する関数に記述されたコメントで内容が現状にそぐわない箇所を更新しました。
禁則の判定関数にconst修飾子の有り/無しが混在していたので有りに統一しました。
~~IsKinsokuPosKuto関数をヘッダファイルからソースファイル（CLayoutMgr_New.cpp）に移動しました。~~
処理位置の判定に使われるIsKinsokuPosHead/IsKinsokuPosTail/IsKinsokuPosKutoをstatic関数に変更し、CLayoutMgrクラスのメンバから外しました。
未使用の判定関数があったので、パッチによってコメントアウトされるコードとともに除去しました。
- 除去した_ExistKinsokuHead()はIsKinsokuHead()と、_ExistKinsokuKuto() はIsKinsokuKuto()と等価です。
行頭禁則対象の直前の文字がサロゲートペアである場合に行頭禁則が動作しないことがある問題を修正しました。

PR の影響範囲

句読点ぶら下げ・行頭禁則・行末禁則のいずれかが有効になっている環境の次のビュー

編集ビュー
印刷プレビュー

テスト内容

各禁則処理の動作を以前のバージョンと比べて確認しました。
- 正常動作する最後のバージョンはv2.2.0.1、動作しなくなったのはv2.3.0.0以降です。
- ~~比較画像を後で用意しておきます。~~

テスト1：編集ビューにおける禁則処理

タイプ別設定に「テキスト」を一時適用する
- 「テキスト」であれば、行頭・行末禁則の対象文字に既定値が設定されます。
句読点ぶら下げ・行頭禁則・行末禁則を有効にする
- 確認を容易にするため、折り返し方法を「指定桁で折り返し」に設定し、適度な桁数を設定するとよいです。
禁則対象文字(下記参照)が行頭・行末にくるようなテキストを入力し、動作を確認する

テスト2：印刷プレビューにおける禁則処理

印刷ページ設定で句読点ぶら下げ・行頭禁則・行末禁則を有効にする
禁則対象文字(下記参照)が行頭・行末にくるようなテキストを入力する
印刷プレビューを表示して動作を確認する

補足：禁則対象文字の既定値（タイプ別設定がテキストの時）

句読点ぶら下げ（8種類）
、。，．､｡,.
行頭禁則（51種類）
!%),.:;?]}\xa2°’”‰′″℃、。々〉》」』】〕゛゜ゝゞ・ヽヾ！％），．：；？］｝｡｣､･ﾞﾟ￠
- \xa2は半角セント記号です。
行末禁則（22種類）
$([\\{\xa3\xa5‘“〈《「『【〔＄（［｛｢￡￥
- \xa3は半角ポンド記号、\xa5は半角円記号です。

参考資料

patchunicode#1034

AppVeyorBot · 2021-02-17T04:44:01Z

✅ Build sakura 1.0.3454 completed (commit 4a1e2e2af7 by @kazasaku)

kengoide · 2021-02-17T09:04:20Z

sakura_core/doc/layout/CLayoutMgr_New.cpp

+	@param[in] nCharChars 現在の位置にある文字の幅と間隔
+	@return 処理が必要な位置である場合にtrue
+*/
+bool CLayoutMgr::IsKinsokuPosKuto( CLayoutInt nRest, CLayoutInt nCharChars ) const


もとからあった関数ですけれど、短いですね…。引数を比較しているだけなのでメンバ関数である必要もないようです。

ワードラップ（＝欧文の禁則処理）で使う関数に_GetKeywordLength()というstatic関数がありましたので、これに倣ってstatic化しておきます。

kengoide · 2021-02-17T09:07:03Z

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp

-	if( (GetMaxLineLayout() - pWork->nPosX < 2) && (pWork->eKinsokuType == KINSOKU_TYPE_NONE) )
+	// 現在位置が行末付近で禁則処理の実行中でないこと
+	if( ( GetMaxLineLayout() - pWork->nPosX < 2 * GetWidthPerKeta() )
+	 && ( pWork->eKinsokuType == KINSOKU_TYPE_NONE ) )


内側の括弧、なくても意味は同じですね。

除去してほしいという要求ではありません。元からですし…。

kengoide · 2021-02-17T09:11:12Z

sakura_core/doc/layout/CLayoutMgr_New.cpp

-			return true;
-		}
-		break;
+	if( nRest < nCharKetas ){


この分岐はなくても大丈夫のような気がします。nCharKetas2 が負になり得るのであれば話が変わってきますが…。

コメントありがとうございます。

たしかに要らないかも知れませんね。
残り1桁分しかない行に2桁分の文字を入れようとしたときに成立しそうですが、nCharKetas2が負数にならない限りはもう一つのif文で対応できそうです。
文字間隔の設定値が負数だったり、WCODE::CalcPxWidthByFont()の戻り値が負数だったりすれば、ここに負数が入ってくるのではないかと。
確認しておきます。

これを削るとこの関数もreturn文だけの関数になってしまいますね。

berryzplus · 2021-02-17T13:07:05Z

質問

このPRって「内部で扱う水平方向の内部単位をpxに統一したい」であっています？

疑問

サクラエディタは元々、古代のコンソールのように半角6px、全角12pxの等幅フォントを表示する前提で書かれています。これはそもそもの前提として、文字コードに対応するフォントグリフ（≒字形）を検索するには時間がかかる（に違いない）を起点にして考案された描画スタイルと考えられます。検索に時間がかかるならキャッシュしてしまえ、という考え方自体は正しいのですが、本当に時間がかかるの？については疑問を持っています。
プロポーショナルフォントに対応したことにより、半角・全角のフォント幅は一様ではなくなりました。半角iの幅は超狭いですし、全角Wの幅は漢字よりも広くなります。文字コードに対応するフォントグリフの検索に本当に時間がかかるのであれば、従来通りキャッシュを行う手法は有効と考えられますが、個人的にはグリフの検索に時間がかかるの仮定がどうもしっくりきません。
プロポーショナルフォントに含まれている情報はグリフ（≒字形）だけではありません。サクラエディタのプロポーショナルフォント実装は文字間隔をユーザーが指定する固定の px 分だけ空けるようになっていますが、本来プロポーショナルフォントが持つ情報の中には、特定の文字の連続を描画するのに適切な間隙の量もあります。（もちろん、その情報がないフォントファイルもある。）おそらく、本当にプロポーショナルフォントに対応したいのであれば、従来サクラエディタが行っている、1文字ずつ描画するをやめて、プロポーショナルフォント向けにビューを組みなおすべきなんだと思います。（注：既に特定の状況下でひとかたまりの文字列をまとめて描画する処理を取り込み済みですが、それとは関係ありません。）

もちろん、目先の不具合っぽいものに対応しようぜ！って趣旨であれば、どうあるべきかは横に置いておいてマージ前提で話をすすめていくことに異論はありません。

ghost · 2021-02-17T14:42:24Z

「内部で扱う水平方向の内部単位をpxに統一したい」

そういう意図はないです。
ただ、この不具合がユーザーに指摘され(2016-01-23)てから5年が経ちましたので。

berryzplus · 2021-02-18T15:18:23Z

一旦了解しました。

では、どなたか動作確認していただいてコメントつけてもらえれば、自分はapproveで良いと思います。

kengoide · 2021-02-19T12:17:20Z

動かしてみました。編集ビュー・印刷プレビューともに正しく動作しているものと思います。

ghost · 2021-02-20T01:05:12Z

編集ビューのみではありますが、変更前後の比較画像を置いておきます。
左がv2.4.1、右がこのPR（ f6babac ）です。

AppVeyorBot · 2021-02-20T01:25:17Z

✅ Build sakura 1.0.3460 completed (commit fe4796c935 by @kazasaku)

suconbu · 2021-02-20T02:53:53Z

恥ずかしながらサクラエディタにこんなワープロソフトばりの機能があることをこれまで意識してませんでした💦

私も動作を見させてもらったところ、折返し位置の手前に絵文字がある状態でその後に行頭禁則文字を置いた時に、カーソル位置や文字挿入がおかしくなることがあったため動画を撮ってみました。
なんとなく修正前からの潜在問題のような気がしますが。。

※ #1543 (comment) の Win32 にて確認しました。

ghost · 2021-02-20T09:03:38Z

suconbu様、お疲れ様です。この機能自体は2002年の春に実装されています（ANSI版の開発掲示板による）。

折返し位置の手前に絵文字がある状態でその後に行頭禁則文字を置いた時に、カーソル位置や文字挿入がおかしくなる

とりあえず、禁則対象文字の手前がサロゲートペアの場合、CNativeW::GetHabaOfCharが、下位サロゲートに対して0を返すため、IsKinsokuPosHeadは「処理位置ではない」と判断したことが判明しました。
例示された画像で行頭禁則が効いていないように見えるのは、絵文字がU+1F436でサロゲートペアになっているためと考えています。

引き続き調査を続けます。

sanomari · 2021-02-20T14:18:20Z

0.0% Coverage

個人的には、修正したコードのカバレッジが完全にゼロなのが気になっています。

マクロから指定した文字列を挿入する
マクロから折り返しモードを変更する
マクロから折り返し桁数を変更する
マクロから「元に戻す」を実行して文字列挿入をなかったことにする

印刷プレビューの折り返し処理コードを走らせるのは無理にしても、実行させてみない限りは追加した分岐が必要なことを証明できないように思います。

プロポーショナル版で幅の計算方式がかわったのに禁則処理が対応していないバグの修正です。

未使用の関数2個(CLayoutMgr::_ExistKinsoku~)も併せて除去した

カーソル位置という表現は正しくなかった。

サロゲートペアは上位側のみ読み取るようにして、折り返し行を描画したときにペアが壊れるのを防ぐ。

ghost · 2021-02-21T03:44:24Z

仮ですがサロゲートペア対策を入れてみました。
サクラエディタの禁則処理は、対象文字の前後にある文字とともに次の行に送る、追い出し処理によるものなのですが、
サロゲートペアの時に下位側も読み取っていたため、処理が行われる時に下位側だけが次の行に送られていました。
このため、表示上は折り返し前の行にあるものの、内部では二つの行に分かれて保持されていました。

AppVeyorBot · 2021-02-21T04:08:17Z

✅ Build sakura 1.0.3463 completed (commit 21751d3ed7 by @kazasaku)

berryzplus

動作確認 #1543 (comment) もしていだだいたので、基本「行き」でよいと思っています。

4 Code Smells

対応できない種類のCodeSmellsは放置でよいと思っていますが、今回は対応できそうなので修正をお願いしたいです。
[[nodiscard]] は付けとくべきかな？と思います。
メンバを変更しない関数のconstも付けとくべきかな？と思います。

カバレッジ＝ゼロに関してはどうなんですかね？
一応、「やったほうがいいけど必須ではない」の認識です。

1. static関数に対してnodiscard属性を追加 2. レイアウト処理の補助関数で関数ポインタを引数に取らないものにconst修飾子を追加

suconbu · 2021-02-21T07:05:19Z

仮ですがサロゲートペア対策を入れてみました。

調査と対策ありがとうございました。
#1543 (comment) の問題が解消されていることを確認しました。

下位サロゲートを読み飛ばす時は境界チェックもやり直す。

ghost · 2021-02-21T07:38:08Z

suconbuさん、ご確認ありがとうございました。

余談ですが、MSゴシックでも絵文字の文字幅が全角・半角のどちらでもないということに気が付きました。

sonarqubecloud · 2021-02-21T07:47:38Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

AppVeyorBot · 2021-02-21T07:59:14Z

✅ Build sakura 1.0.3466 completed (commit 001679d5e6 by @kazasaku)

suconbu · 2021-02-21T11:21:29Z

余談ですが、MSゴシックでも絵文字の文字幅が全角・半角のどちらでもないということに気が付きました。

MS ゴシックには絵文字は収録されていないはずですので、フォントフォールバックで絵文字の所だけ Segoe UI Symbol あたりが使われていそうですね。
🗻 (U+1F5FB) など全角よりも広くなる文字もあるようです。

CodeSmells解消したので。

kengoide

指摘するべき問題はなさそうです。

何らかの自動テストは確かにあるべきだと思います。追加タイミングについてはお任せします。

berryzplus

追加タイミングについてはお任せします。

自分もそれでよいと思います。

ghost · 2021-02-22T15:05:39Z

皆様、ご確認ありがとうございました。
テストは別途用意しておきます。

ghost added 💩degradation🧻🚽 デグレ (前に動いていた機能が動かなくなった) 🐛bug🦋 ■バグ修正(Something isn't working) and removed 💩degradation🧻🚽 デグレ (前に動いていた機能が動かなくなった) labels Feb 17, 2021

kengoide reviewed Feb 17, 2021

View reviewed changes

ghost marked this pull request as ready for review February 20, 2021 01:09

Moca and others added 10 commits February 21, 2021 12:33

プロポーショナル版で禁則処理がおかしい問題の修正 (patchunicode#1034)

6995a8e

プロポーショナル版で幅の計算方式がかわったのに禁則処理が対応していないバグの修正です。

現在位置が行末付近であるかを確認させる

4088c49

コメントの追加と修正

a6d11fc

禁則処理に使う関数にconst修飾子を付与

5d2449e

IsKinsokuPosKuto関数の定義をヘッダファイルから分離

9d45e72

不要となるコードの除去

08aa3bc

未使用の関数2個(CLayoutMgr::_ExistKinsoku~)も併せて除去した

コメント中の語句を訂正

cc6fe97

カーソル位置という表現は正しくなかった。

処理位置を判定をメンバ関数から非メンバのstatic関数で行うようにする

eca18da

判定条件を1つにまとめる

e9aa765

行頭禁則処理におけるサロゲートペア対策

bfaddf2

サロゲートペアは上位側のみ読み取るようにして、折り返し行を描画したときにペアが壊れるのを防ぐ。

berryzplus previously requested changes Feb 21, 2021

View reviewed changes

レビュー指摘対応

0cb59f2

1. static関数に対してnodiscard属性を追加 2. レイアウト処理の補助関数で関数ポインタを引数に取らないものにconst修飾子を追加

サロゲートペア対策（追加）

4118854

下位サロゲートを読み飛ばす時は境界チェックもやり直す。

kengoide approved these changes Feb 21, 2021

View reviewed changes

berryzplus approved these changes Feb 22, 2021

View reviewed changes

ghost merged commit 2deebe0 into sakura-editor:master Feb 22, 2021

ghost deleted the feature/fix_line_breaking_process branch February 23, 2021 10:08

ghost mentioned this pull request Apr 12, 2021

(sf1034)プロポーショナル版で禁則処理がおかしい問題の修正 #1410

Closed

ghost mentioned this pull request Apr 25, 2021

レイアウト座標を半角単位桁数に戻したい #1645

Closed

4 tasks

ghost mentioned this pull request May 3, 2021

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

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

プロポーショナル版で禁則処理を使えるようにする #1543

プロポーショナル版で禁則処理を使えるようにする #1543

ghost commented Feb 17, 2021 •

edited by ghost

Loading

AppVeyorBot commented Feb 17, 2021

kengoide Feb 17, 2021

ghost Feb 20, 2021

kengoide Feb 17, 2021

kengoide Feb 17, 2021

ghost Feb 17, 2021 •

edited by ghost

Loading

berryzplus commented Feb 17, 2021

ghost commented Feb 17, 2021 •

edited by ghost

Loading

berryzplus commented Feb 18, 2021

kengoide commented Feb 19, 2021 •

edited

Loading

ghost commented Feb 20, 2021

AppVeyorBot commented Feb 20, 2021

suconbu commented Feb 20, 2021

ghost commented Feb 20, 2021 •

edited by ghost

Loading

sanomari commented Feb 20, 2021

ghost commented Feb 21, 2021 •

edited by ghost

Loading

AppVeyorBot commented Feb 21, 2021

berryzplus left a comment

suconbu commented Feb 21, 2021

ghost commented Feb 21, 2021 •

edited by ghost

Loading

sonarqubecloud bot commented Feb 21, 2021

AppVeyorBot commented Feb 21, 2021

suconbu commented Feb 21, 2021 •

edited

Loading

kengoide left a comment

berryzplus left a comment

ghost commented Feb 22, 2021 •

edited by ghost

Loading

プロポーショナル版で禁則処理を使えるようにする #1543

プロポーショナル版で禁則処理を使えるようにする #1543

Conversation

ghost commented Feb 17, 2021 • edited by ghost Loading

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

テスト1：編集ビューにおける禁則処理

テスト2：印刷プレビューにおける禁則処理

補足：禁則対象文字の既定値（タイプ別設定がテキストの時）

関連 issue, PR

参考資料

AppVeyorBot commented Feb 17, 2021

kengoide Feb 17, 2021

Choose a reason for hiding this comment

ghost Feb 20, 2021

Choose a reason for hiding this comment

kengoide Feb 17, 2021

Choose a reason for hiding this comment

kengoide Feb 17, 2021

Choose a reason for hiding this comment

ghost Feb 17, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

berryzplus commented Feb 17, 2021

質問

疑問

ghost commented Feb 17, 2021 • edited by ghost Loading

berryzplus commented Feb 18, 2021

kengoide commented Feb 19, 2021 • edited Loading

ghost commented Feb 20, 2021

AppVeyorBot commented Feb 20, 2021

suconbu commented Feb 20, 2021

ghost commented Feb 20, 2021 • edited by ghost Loading

sanomari commented Feb 20, 2021

ghost commented Feb 21, 2021 • edited by ghost Loading

AppVeyorBot commented Feb 21, 2021

berryzplus left a comment

Choose a reason for hiding this comment

suconbu commented Feb 21, 2021

ghost commented Feb 21, 2021 • edited by ghost Loading

sonarqubecloud bot commented Feb 21, 2021

AppVeyorBot commented Feb 21, 2021

suconbu commented Feb 21, 2021 • edited Loading

kengoide left a comment

Choose a reason for hiding this comment

berryzplus left a comment

Choose a reason for hiding this comment

ghost commented Feb 22, 2021 • edited by ghost Loading

ghost commented Feb 17, 2021 •

edited by ghost

Loading

ghost Feb 17, 2021 •

edited by ghost

Loading

ghost commented Feb 17, 2021 •

edited by ghost

Loading

kengoide commented Feb 19, 2021 •

edited

Loading

ghost commented Feb 20, 2021 •

edited by ghost

Loading

ghost commented Feb 21, 2021 •

edited by ghost

Loading

ghost commented Feb 21, 2021 •

edited by ghost

Loading

suconbu commented Feb 21, 2021 •

edited

Loading

ghost commented Feb 22, 2021 •

edited by ghost

Loading