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.

Sign up for GitHub

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

Jump to bottom

空行描画のオプションが誤っているのを修正する。 #1358

Merged
merged 2 commits into from
Aug 2, 2020
Merged

空行描画のオプションが誤っているのを修正する。 #1358

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c999d0b
空行描画のオプションが誤っているのを修正。
berryzplus Jul 31, 2020
4981d6f
空行の高さを計測するDrawTextのオプションが足りないのを追加
berryzplus Aug 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sakura_core/window/CTipWnd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ void CTipWnd::DrawTipText(
);
}else{
// ダミー文字列の高さを取得する
nHeight = ::DrawText( hdc, szDummy, _countof(szDummy) - 1, &rc, DT_CALCRECT );
nHeight = ::DrawText( hdc, szDummy, _countof(szDummy) - 1, &rc, DT_EXTERNALLEADING );
Comment on lines 259 to +260
Copy link
Member

Choose a reason for hiding this comment

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

DT_CALCRECT | DT_EXTERNALLEADING でサイズを計算するだけではなく、実際に描画しないとダメなのでしょうか。
そうだとするとコメントも修正すべきだと思います。
しかし、文字列がブランクの場合も描画が必要となると、253行目の if 文で分岐する必要はあるのでしょうか。DrawText に長さ 0 を渡すとダメなんでしたっけ。

188行目にも似た処理がありますが、そちらは修正不要でしょうか。

Copy link
Contributor Author

@berryzplus berryzplus Aug 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

問題の発生原因が「矩形を更新してるから」なので、そこだけ対処しています。

188行目にも似た処理がありますが、そちらは修正不要でしょうか。

188行目のコードは、CTipWndのサイズを確定させるコードです。こちらは「矩形を更新する」で正しいです。

しかし、文字列がブランクの場合も描画が必要となると、253行目の if 文で分岐する必要はあるのでしょうか。DrawText に長さ 0 を渡すとダメなんでしたっけ。

これは試していないです。
描画対象が0文字な場合、高さが返らないんじゃなかったかな？と。（テキトー

DT_CALCRECT | DT_EXTERNALLEADING でサイズを計算するだけではなく、実際に描画しないとダメなのでしょうか。
そうだとするとコメントも修正すべきだと思います。

ややめんどくさいんですが、コメントは合っています。
DrawTextは描画した文字列の高さを返す仕様で、nHeightに高さを取得してるのは間違いないので。

ただ、フォントを変えずに空行の高さが変わる事態はありえないので、毎回再計算するのではなく、一度だけ計算して以降は計算済みの値を使いまわすロジックにするのがベターだと思います。

それしないと不具合直らないわけじゃないっす！
というのが言い訳です:smiley:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

原因に対する対処だけじゃダメでしょうか・・・？

Copy link
Member

Choose a reason for hiding this comment

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

ちょっと勘違いしてました。DT_CALCRECT を指定していると描画はせずに、描画に必要なrectを計算しますが、DT_CALCRECT を指定しない場合は実際に描画された領域に合わせてrectを更新するわけではないのですね。
で、今回の不具合は、ダミー文字列に対して DT_CALCRECT を指定することで、rectが縮小されてしまい、それを次の描画領域として渡すことでそれ以降何も描画されなかったということですね。

188行目のコードは、CTipWndのサイズを確定させるコードです。こちらは「矩形を更新する」で正しいです。

ここは DT_CALCRECT ではなく、DT_CALCRECT | DT_EXTERNALLEADING とすべきではないかという指摘でした。（私の環境ではどちらもたまたま同じ値が返ってきているようですが。）

これは試していないです。
描画対象が0文字な場合、高さが返らないんじゃなかったかな？と。（テキトー

試してみましたが、問題なく動きました。なので if で分岐する必要はないと思います。
ダミー文字列などというものは出来れば使いたくないところです。

ただ、フォントを変えずに空行の高さが変わる事態はありえないので、毎回再計算するのではなく、一度だけ計算して以降は計算済みの値を使いまわすロジックにするのがベターだと思います。

処理が複雑になるだけの早すぎる最適化だとおもいます。
最適化のポイントは、第1は最適化しない、次が測定するですので。

Copy link
Member

Choose a reason for hiding this comment

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

とはいえ、#647 以前からブランクかどうかで処理を分けていたのか。

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/berryzplus/sakura/blob/6ae553a3d1eb2d100afc36996dd85e27a1c97e77/sakura_core/window/CTipWnd.cpp
以前のコードを見直してみましたが、ブランクかどうかで処理を分けてはいますが、フラグは全く同じになっていますね。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここは DT_CALCRECT ではなく、DT_CALCRECT | DT_EXTERNALLEADING とすべきではないかという指摘でした。（私の環境ではどちらもたまたま同じ値が返ってきているようですが。）

その通りだと思います。

これは試していないです。
描画対象が0文字な場合、高さが返らないんじゃなかったかな？と。（テキトー

試してみましたが、問題なく動きました。なので if で分岐する必要はないと思います。
ダミー文字列などというものは出来れば使いたくないところです。

では、ダミー文字列は廃止する方向で行きたいです。

ただ、フォントを変えずに空行の高さが変わる事態はありえないので、毎回再計算するのではなく、一度だけ計算して以降は計算済みの値を使いまわすロジックにするのがベターだと思います。

処理が複雑になるだけの早すぎる最適化だとおもいます。
最適化のポイントは、第1は最適化しない、次が測定するですので。

ダミー文字列を使わない方向にするなら、ここの考慮は要らなくなりそうです。
既に最適化されてしまっているコードをどうするか？ってところが、既存を触るときにめんどくさいポイントだと思います。

不具合に対処したいと良くないロジックを改善したいは別な軸の話だと思うので、できれば別PRで対応したいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ああ、188行目は修正しときます。

Copy link
Member

Choose a reason for hiding this comment

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

不具合に対処したいと良くないロジックを改善したいは別な軸の話だと思うので、できれば別PRで対応したいです。

okです。
ダミー文字列を使わないようにするかどうかは可能であれば実測して判断したいところです。

}

// 描画領域の上端を1行分ずらす
Expand Down