空行描画のオプションが誤っているのを修正する。 #1358
空行描画のオプションが誤っているのを修正する。 #1358
Conversation
|
✅ Build sakura 1.0.2967 completed (commit 9cddb11991 by @berryzplus) |
| // ダミー文字列の高さを取得する | ||
| nHeight = ::DrawText( hdc, szDummy, _countof(szDummy) - 1, &rc, DT_CALCRECT ); | ||
| nHeight = ::DrawText( hdc, szDummy, _countof(szDummy) - 1, &rc, DT_EXTERNALLEADING ); |
There was a problem hiding this comment.
DT_CALCRECT | DT_EXTERNALLEADING でサイズを計算するだけではなく、実際に描画しないとダメなのでしょうか。
そうだとするとコメントも修正すべきだと思います。
しかし、文字列がブランクの場合も描画が必要となると、253行目の if 文で分岐する必要はあるのでしょうか。DrawText に長さ 0 を渡すとダメなんでしたっけ。
188行目にも似た処理がありますが、そちらは修正不要でしょうか。
There was a problem hiding this comment.
問題の発生原因が「矩形を更新してるから」なので、そこだけ対処しています。
188行目にも似た処理がありますが、そちらは修正不要でしょうか。
188行目のコードは、CTipWndのサイズを確定させるコードです。こちらは「矩形を更新する」で正しいです。
しかし、文字列がブランクの場合も描画が必要となると、253行目の if 文で分岐する必要はあるのでしょうか。DrawText に長さ 0 を渡すとダメなんでしたっけ。
これは試していないです。
描画対象が0文字な場合、高さが返らないんじゃなかったかな？と。（テキトー
DT_CALCRECT | DT_EXTERNALLEADINGでサイズを計算するだけではなく、実際に描画しないとダメなのでしょうか。
そうだとするとコメントも修正すべきだと思います。
ややめんどくさいんですが、コメントは合っています。
DrawTextは描画した文字列の高さを返す仕様で、nHeightに高さを取得してるのは間違いないので。
ただ、フォントを変えずに空行の高さが変わる事態はありえないので、毎回再計算するのではなく、一度だけ計算して以降は計算済みの値を使いまわすロジックにするのがベターだと思います。
それしないと不具合直らないわけじゃないっす！
というのが言い訳です:smiley:
There was a problem hiding this comment.
原因に対する対処だけじゃダメでしょうか・・・？
There was a problem hiding this comment.
ちょっと勘違いしてました。DT_CALCRECT を指定していると描画はせずに、描画に必要なrectを計算しますが、DT_CALCRECT を指定しない場合は実際に描画された領域に合わせてrectを更新するわけではないのですね。
で、今回の不具合は、ダミー文字列に対して DT_CALCRECT を指定することで、rectが縮小されてしまい、それを次の描画領域として渡すことでそれ以降何も描画されなかったということですね。
188行目のコードは、CTipWndのサイズを確定させるコードです。こちらは「矩形を更新する」で正しいです。
ここは DT_CALCRECT ではなく、DT_CALCRECT | DT_EXTERNALLEADING とすべきではないかという指摘でした。（私の環境ではどちらもたまたま同じ値が返ってきているようですが。）
これは試していないです。
描画対象が0文字な場合、高さが返らないんじゃなかったかな？と。（テキトー
試してみましたが、問題なく動きました。なので if で分岐する必要はないと思います。
ダミー文字列などというものは出来れば使いたくないところです。
ただ、フォントを変えずに空行の高さが変わる事態はありえないので、毎回再計算するのではなく、一度だけ計算して以降は計算済みの値を使いまわすロジックにするのがベターだと思います。
処理が複雑になるだけの早すぎる最適化だとおもいます。
最適化のポイントは、第1は最適化しない、次が測定するですので。
There was a problem hiding this comment.
とはいえ、#647 以前からブランクかどうかで処理を分けていたのか。
There was a problem hiding this comment.
https://github.com/berryzplus/sakura/blob/6ae553a3d1eb2d100afc36996dd85e27a1c97e77/sakura_core/window/CTipWnd.cpp
以前のコードを見直してみましたが、ブランクかどうかで処理を分けてはいますが、フラグは全く同じになっていますね。
There was a problem hiding this comment.
ここは
DT_CALCRECTではなく、DT_CALCRECT | DT_EXTERNALLEADINGとすべきではないかという指摘でした。（私の環境ではどちらもたまたま同じ値が返ってきているようですが。）
その通りだと思います。
これは試していないです。
描画対象が0文字な場合、高さが返らないんじゃなかったかな？と。（テキトー試してみましたが、問題なく動きました。なので if で分岐する必要はないと思います。
ダミー文字列などというものは出来れば使いたくないところです。
では、ダミー文字列は廃止する方向で行きたいです。
ただ、フォントを変えずに空行の高さが変わる事態はありえないので、毎回再計算するのではなく、一度だけ計算して以降は計算済みの値を使いまわすロジックにするのがベターだと思います。
処理が複雑になるだけの早すぎる最適化だとおもいます。
最適化のポイントは、第1は最適化しない、次が測定するですので。
ダミー文字列を使わない方向にするなら、ここの考慮は要らなくなりそうです。
既に最適化されてしまっているコードをどうするか？ってところが、既存を触るときにめんどくさいポイントだと思います。
不具合に対処したいと良くないロジックを改善したいは別な軸の話だと思うので、できれば別PRで対応したいです。
There was a problem hiding this comment.
ああ、188行目は修正しときます。
There was a problem hiding this comment.
不具合に対処したいと良くないロジックを改善したいは別な軸の話だと思うので、できれば別PRで対応したいです。
okです。
ダミー文字列を使わないようにするかどうかは可能であれば実測して判断したいところです。
|
今回の件とは別件ですが、148行目で |
残念ながら、 その辺の対応を進める前にエディタ部分のレイアウト機構をなんとかしたい思惑があって、PerMonitorの件は後回しなのかなぁと思っています。対応するならこれ使わないといけないはず。 |
レビュー指摘対応。
|
✅ Build sakura 1.0.2968 completed (commit 853621db2d by @berryzplus) |
DPIが同じでも幅が異なるとおかしなことになると思います。PerMoniter DPIとは別の話です。 |
|
azpがfailしてるのは、chmで2回連続落ちてるのか…。リトライ回数増やすべきだろうか？ |
|
レビューありがとうございます。マージしちゃいます。 |
PR の目的
空行描画のオプションが誤っているのが原因で起きている不具合を修正します。
カテゴリ
PR の背景
#1353
OSDN転載: キーワードヘルプで複数行の説明テキストを入れると2行目以降が表示されない。キーワードヘルプを表示させたときに、１行目しか表示されていません。
PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
テスト内容
改修効果確認のテストを行います。
テスト1
手順
タイプ別設定を開き、キーワードヘルプタブをこんな感じに設定します。キーワードヘルプを自動表示するをクリックして自動表示を有効にします。setにマウスカーソルを当てます。説明テキストの2行目以降が表示されていたらOKです。
PR の影響範囲
複数行の説明テキストを含むキーワードヘルプファイルを使う場合の挙動に影響します。
関連 issue, PR
fixes #1353
OSDN転載: キーワードヘルプで複数行の説明テキストを入れると2行目以降が表示されない。#647
辞書Tipの描画改善、およびHighDPI対応参考資料