Skip to content

色種別リスト オーナー描画の改善・続 - #1887

Merged
beru merged 3 commits into
sakura-editor:masterfrom
beru:HighDPI_CPropTypesColor__DrawColorListItem_2nd
Dec 30, 2022
Merged

色種別リスト オーナー描画の改善・続#1887
beru merged 3 commits into
sakura-editor:masterfrom
beru:HighDPI_CPropTypesColor__DrawColorListItem_2nd

Conversation

@beru

@beru beru commented Dec 29, 2022

Copy link
Copy Markdown
Contributor

PR対象

  • アプリ(サクラエディタ本体)

カテゴリ

  • 改善
  • 不具合修正

PR の背景

#1882 の変更で色種別リストのオーナー描画を変更しましたが、その変更内容が原因で表示スケールが200%以上の動作環境で下線表示が不自然になってしまいました。#1882 (comment)

それを改善するのが主な目的です。

仕様・動作説明

  • フォント作成時にダイアログのフォントではなくて色リストのフォントを使うように変更
  • フォント作成は CViewFont クラスに任せるように変更
  • 下線描画は自前で線を引くのではなくて、下線付きのフォントを使う

PR の影響範囲

タイプ別設定のカラーの色指定リストの表示

テスト内容

変更後の表示内容が改善されているかを目視で確認し、改善されていると判断しました。

  • 表示スケールが200%以上の場合に下線表示に隙間が無くなった
  • 太字のフォントが他と揃うようになった
表示スケール 変更前 変更後
100% 100%_before 100%_after
150% 150%_before 150%_after
200% 200%_before 200%_after
300% 300%_before 300%_after

関連 issue, PR

#1882

参考資料

フォント作成じにダイアログのフォントではなくて色リストのフォントを使うように変更
フォント作成は CViewFont クラスに任せるように変更
下線描画は自前で線を引くのではなくて、下線付きのフォントを使う
@beru
beru requested a review from berryzplus December 29, 2022 04:11
@beru beru self-assigned this Dec 29, 2022
sFontAttr.m_bBoldFont = pColorInfo->m_sFontAttr.m_bBoldFont;
sFontAttr.m_bUnderLine = pColorInfo->m_sFontAttr.m_bUnderLine;
gr.PushMyFont(m_pViewFont->ChooseFontHandle(0, sFontAttr));
::DrawText(gr, pColorInfo->m_szName, -1, &rc1, DT_SINGLELINE | DT_VCENTER);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#1882 (comment) で berryzplus さんに教えてもらった DrawText 関数を使うようにしました。

@AppVeyorBot

Copy link
Copy Markdown

Build sakura 1.0.4231 completed (commit 5db0a5c285 by @beru)

@AppVeyorBot

Copy link
Copy Markdown

Build sakura 1.0.4232 completed (commit cc2a80771a by @beru)

berryzplus
berryzplus previously approved these changes Dec 29, 2022

@berryzplus berryzplus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

表示200%で確認しました。

URL

変更前だと「隙間がある」というより下線が描画されてないように見える瞬間ありますね。
変更を適用したら下線が出るようになったと思います。

#include "CDlgSameColor.h"
#include "CDlgKeywordSelect.h"
#include "view/colors/EColorIndexType.h"
#include "view/CViewFont.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ヘッダ側でインクルードできないんですかね?

書く位置によって余分な修正をしないといけなくなるかもなので対応しなくて良いです。
やろうとしたけど無理だった
の事情があれば残しておいたほうが良い気がしました。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

普通にビルドできました。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ビルド時の依存の連鎖を抑えるためにヘッダファイルに書く #include の数はなるべく減らしたいです。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

テストコード書くときに text_XXX.cpp に #include を書かないといけなくなるのでヘッダに書くべきだと思いました。

ヘッダに書く #include の数を減らしたいだと、できるだけ独自定義クラスに依存しないコードを書く、になるので、ほぼほぼ 罰ゲーム になる気がします。class CPropTypesclass CViewFont に依存させる変更なので、ヘッダ側にインクルードを書くのが正しいように思います。

PRの趣旨とは関係ない部分なので、このままスルーしますが。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

テストコード書くときに text_XXX.cpp に #include を書かないといけなくなるのでヘッダに書くべきだと思いました。

CPropTypesColor のテストをする為に書くテストコードで CViewFont のヘッダファイルを利用する必要性が分からないです。テストコードを書く時点でもし必要になったらその時に対処を考えるのはどうでしょうか?それからでも遅くないんじゃないかと。

ヘッダに書く #include の数を減らしたいだと、できるだけ独自定義クラスに依存しないコードを書く、になるので、ほぼほぼ 罰ゲーム になる気がします。class CPropTypes を class CViewFont に依存させる変更なので、ヘッダ側にインクルードを書くのが正しいように思います。

実装の詳細は隠しましょう的な考え方も有りますし、PImplイディオム的に使いました。

CPropTypes を継承した CPropTypesColor の実装に関わる変更で、それらのクラスの外部I/Fには関わらない変更なのでヘッダ側に #include を書かなければいけないという事は無いと考えています。

いやまぁ、どうしてもヘッダファイルに #include を書いた方が良いとえんえんと主張されたら折れて変更しますが…。

Comment thread sakura_core/typeprop/CPropTypesColor.cpp Outdated
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot

Copy link
Copy Markdown

Build sakura 1.0.4235 completed (commit 01a4ffb542 by @beru)

@beru
beru merged commit bdc9d6f into sakura-editor:master Dec 30, 2022
@beru
beru deleted the HighDPI_CPropTypesColor__DrawColorListItem_2nd branch December 30, 2022 05:06
@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛bug🦋 ■バグ修正(Something isn't working)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants