アクティブタブの上部にトップバンドを描画する #1684

suconbu · 2021-05-28T15:12:00Z

PR の目的

アクティブなタブの上部に目印 (トップバンド) を表示することで、多数のタブの中からでもすぐに発見できるようにします。

変更前	変更後

カテゴリ

仕様変更

PR の背景

アクティブタブは他のタブよりも少し明るく/少し大きく表示されてはいますが、多数のタブが表示されている状況ではよく目を凝らさないと発見できません。長時間の作業で疲れた目には応えます。

※ ODSN のフォーラムにも関連しそうな要望がありました。
https://osdn.net/projects/sakura-editor/forums/34071/44160/

PR のメリット

目的に記載の通りです。

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

描画物が増えるので操作レスポンスが悪化する可能性があります。

仕様・動作説明

トップバンドの描画処理は 2006 年頃から存在していたようなのですが、条件判定により「クラシック表示以外」の場合には描画がされないようになっています。

sakura/sakura_core/window/CTabWnd.cpp

Lines 1607 to 1609 in 5135813

    
           // Windowsクラシックスタイルの場合はアクティブタブの上部にトップバンドを描画する	// 2006.03.27 ryoji 
        
           if( !m_bVisualStyle ) 
        
           {

WindowsXP の標準タブコントロールではアクティブタブに自動的に色が付くので、クラシック表示以外ではトップバンドの描画は不要だったと考えられますが、WindowsVista 以降の標準タブコントロールはこの色付けはなくなり、さらにフラットな意匠になっているためトップバンドなしではアクティブタブを見つけづらくなっています。

本 PR ではこの条件を見直してトップバンドが常時描画されるように変更します。

PR の影響範囲

タブバーの表示内容・性能への影響があります。

テスト内容

トップバンドが正しく表示されることと、性能劣化がないことを確認する予定です。

タブバーの設定を色々した時にも正しい位置に表示されること。(アイコン有無、等幅/可変、閉じるボタン有無、...)
ディスプレイ設定の表示スケールを 200% に変更した時に隙間や重なりが起きないこと。
CTabWnd::OnPaint の処理時間を変更前後で比較すること。

参考資料

AppVeyorBot · 2021-05-28T16:20:05Z

✅ Build sakura 1.0.3804 completed (commit 7c1f08ebbe by @suconbu)

suconbu · 2021-05-28T16:25:07Z

……ないよりはだいぶ良い気がします😅

suconbu · 2021-05-29T02:01:07Z

トップバンドの色をタイトルバーとお揃いにしてみました。

青系	橙系

タイトルバーの色 (アクセントカラー) の取得に使用する DwmGetColorizationColor がサポートされるのが Windows Vista 以降となっていましたが、サクラエディタの Windows XP サポートはすでに終了しているため問題ないと思います。
https://docs.microsoft.com/en-us/windows/win32/api/dwmapi/nf-dwmapi-dwmgetcolorizationcolor

Minimum supported client	Windows Vista [desktop apps only]

AppVeyorBot · 2021-05-29T02:38:03Z

✅ Build sakura 1.0.3805 completed (commit d84d6b879a by @suconbu)

AppVeyorBot · 2021-05-29T03:36:46Z

✅ Build sakura 1.0.3806 completed (commit 8422e4b052 by @suconbu)

AppVeyorBot · 2021-05-29T04:37:23Z

✅ Build sakura 1.0.3807 completed (commit 5d8301bff0 by @suconbu)

* トップバンドの横幅がタブに対してすこし狭く見えるのを調整する * 表示スケールが100%より大きい時にトップバンドの上に隙間が空かないようにする * 左側はみだし判定が冗長なので廃止する

AppVeyorBot · 2021-05-29T09:39:57Z

✅ Build sakura 1.0.3808 completed (commit 369349c232 by @suconbu)

berryzplus · 2021-05-29T13:02:47Z

MinGWビルドでエラーが出ています。
https://dev.azure.com/sakuraeditor/sakura/_build/results?buildId=3034&view=logs&j=b39c645f-42c7-549c-3e61-e6ffdeb72915&t=ea0a0af4-dbfa-5a7a-b1b8-f35f253695ca&l=514

たぶん、Makefileにインポートライブラリを追加してないことが原因です。
https://docs.microsoft.com/en-us/windows/win32/api/dwmapi/nf-dwmapi-dwmgetcolorizationcolor

この辺にDwmapi.libを追加してやれば解消するはず。

sakura/sakura_core/Makefile

Lines 92 to 93 in 5135813

    
           -lcomctl32 \ 
        
           -limm32 \

berryzplus · 2021-05-29T13:03:57Z

あ、👆はunittestsのMakefileにも必要だと思います。

suconbu · 2021-05-29T13:20:17Z

sakura_core/window/CTabWnd.cpp

+	@param[in]	lprcClient	タブウィンドウのクライアント領域
+	@param[in]	nTabIndex	対象タブのインデックス
+*/
+void CTabWnd::DrawTopBand( const CGraphics& gr, const RECT* prcClient, int nTabIndex ) const


これは知らなくて今回 SonarCloud の指摘で気付いたのですが、(他の関数にならい) const LPRECT hoge とした場合は hoge の参照先 (hoge->left など) は書き込み可能となってしまうようでした。

RECT rect = {}; const RECT* pRect = &rect; const LPRECT lpRect = &rect; pRect = &rect; // OK pRect->left = 1; // コンパイルエラー lpRect = &rect; // コンパイルエラー lpRect->left = 1; // OK (予想外)

const RECT *
RECT const *
の違いですかね・・・

const RECT&
とすれば、おかしなことは起きないですが、参照渡しを嫌がる人は結構いるらしいです。

出力引数が参照渡しとなっていると、呼び出し側のコードを見た時に出力ということが分かりづらい、出力不要 (NULL) を表現できない、というのはあると思いますけど、今回のような必須の入力引数であれば参照の方が良さそうです。
第一引数と同じく RECT も参照渡しに変更しておこうと思います。

AppVeyorBot · 2021-05-29T13:55:28Z

✅ Build sakura 1.0.3811 completed (commit 7aa40f3ef0 by @suconbu)

suconbu · 2021-05-29T14:16:09Z

MinGWビルドでエラーが出ています。
https://dev.azure.com/sakuraeditor/sakura/_build/results?buildId=3034&view=logs&j=b39c645f-42c7-549c-3e61-e6ffdeb72915&t=ea0a0af4-dbfa-5a7a-b1b8-f35f253695ca&l=514

たぶん、Makefileにインポートライブラリを追加してないことが原因です。
https://docs.microsoft.com/en-us/windows/win32/api/dwmapi/nf-dwmapi-dwmgetcolorizationcolor

この辺にDwmapi.libを追加してやれば解消するはず。

sakura/sakura_core/Makefile

Lines 92 to 93 in 5135813

-lcomctl32 \

-limm32 \

あ、👆はunittestsのMakefileにも必要だと思います。

ありがとうございます。
Makefile にライブラリ指定を追加して MinGW ビルドを通せました。

suconbu · 2021-05-29T14:36:04Z

CTabWnd::DrawTopBand の定義を置く位置がちょっとおかしいので直させて下さい。

sonarcloud · 2021-05-29T15:25:57Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

83.3% Coverage
0.0% Duplication

suconbu · 2021-05-29T15:30:51Z

描画物が増えるので操作レスポンスが悪化する可能性があります。

トップバンドの描画処理 (DrawTopBand) 追加により CTabWnd::OnPaint の処理時間が 0.15 ms ほど増加しましたが、操作性への影響はないものと判断します。

変更前：

name	time (ms)	diff (ms)	message
CTabWnd::OnPaint	0.000	0.000	== Enter ==
CTabWnd::OnPaint	0.244	0.244	== Exit Scope ==

変更後：

name	time (ms)	diff (ms)	message
CTabWnd::OnPaint	0.000	0.000	== Enter ==
CTabWnd::OnPaint	0.241	0.241	DrawTopBand 開始
CTabWnd::OnPaint	0.395	0.155	DrawTopBand 終了
CTabWnd::OnPaint	0.416	0.021	== Exit Scope ==

DrawTopBand の内訳を見ると、GetSystemAccentColor (アクセントカラー取得) が半分程度 (0.135 ms 中の 0.065 ms) を占めていました。
GetSystemAccentColor の側にキャッシュ機構を実装すれば時間短縮できそうですが、キャッシュしてしまうとユーザーがアクセントカラーを変更したことの検知が必要になってしまいます。
本 PR ではこれが呼び出される頻度は比較的少なめですので、このままとしたいと思います。

変更後 (DrawTopBand 内訳)：

name	time (ms)	diff (ms)	message
_ CTabWnd::DrawTopBand	0.000	0.000	== Enter ==
_ CTabWnd::DrawTopBand	0.036	0.036	::TabCtrl_GetItemRect 終了
_ CTabWnd::DrawTopBand	0.040	0.004	::ScreenToClient 終了
_ CTabWnd::DrawTopBand	0.046	0.006	rcTopBand 設定終了
_ CTabWnd::DrawTopBand	0.111	0.065	::GetSystemAccentColor 終了
_ CTabWnd::DrawTopBand	0.135	0.024	== Exit Scope

suconbu · 2021-05-29T15:34:59Z

[2 Code Smells]

今回変更していない箇所の指摘でしたのでそのままとさせて下さい。

AppVeyorBot · 2021-05-29T15:55:24Z

✅ Build sakura 1.0.3813 completed (commit 0747cf7977 by @suconbu)

AppVeyorBot · 2021-05-29T17:01:15Z

✅ Build sakura 1.0.3814 completed (commit 0a600a9b18 by @suconbu)

AppVeyorBot · 2021-05-29T18:10:29Z

✅ Build sakura 1.0.3815 completed (commit 366aac0fdb by @suconbu)

beru · 2021-05-30T00:13:58Z

sakura_core/util/window.cpp

+	bool bResult = FALSE;
+	if( SUCCEEDED( ::DwmGetColorizationColor( &dwArgb, &bOpaque ) ) ){
+		if( pColorOut != nullptr ){
+			*pColorOut = RGB( (dwArgb >> 16) & 0xFFU, (dwArgb >> 8) & 0xFFU, dwArgb & 0xFFU );


// 0xAARRGGBB to 0x00BBGGRR(COLORREF) *pColorOut = ((dwArgb & 0xFF0000U) >> 16) | (dwArgb & 0xFF00U) | ((dwArgb & 0xFFU) << 16);

このように書くと少し命令数が少なくなる事を確認しました。
処理時間の確認はしていませんが、DwmGetColorizationColor の呼び出しの時間が支配的なので変化無いと思います。

やりたいことがソレ(BGRA→RGBの変換)なら、そう書けばいいように思いました。

数式をインラインで書くと、コピペして変数名を修正するときにミスることあります。
C言語のマクロ関数ならば、変数名が変わっても大丈夫です。
C++のconstexpr関数ならば、普通の関数みたいに処理を書いてインライン化できます。

//! 0xAARRGGBB to 0x00BBGGRR constexpr COLORREF ConvertBgraToRgb(DWORD bgra) { COLORREF rgb = bgra & 0x0000FF00; rgb |= (bgra & 0x000000FF) << 16; rgb |= (bgra & 0x00FF0000) >> 16; return rgb; //※beruさんが書いたのと同じ意味のコードのつもりっす。 }

この試みに関して思ったことが２つあります。
・いまやらないといかんの？（≒指摘事項なの？）
・どこに書くの？（≒関数が属する適切なヘッダファイルはどれ？）

総合的に考えて、PR指摘としてはスルーでいいんじゃないかな・・・。

Requested change ではないのでもちろん対応は不要です。

// 0xAARRGGBB to 0x00BBGGRR(COLORREF) *pColorOut = ((dwArgb & 0xFF0000U) >> 16) | (dwArgb & 0xFF00U) | ((dwArgb & 0xFFU) << 16);

代入文100万回実行時の合計処理時間で比較してみました所、RGB マクロの 1.5 ms (1.5ns/回) 程度から 1.2 ms (1.2ns/回) 程度へと 20% も高速化していました。

berryzplus · 2021-05-30T04:49:24Z

描画物が増えるので操作レスポンスが悪化する可能性があります。

トップバンドの描画処理 (DrawTopBand) 追加により CTabWnd::OnPaint の処理時間が 0.15 ms ほど増加しましたが、操作性への影響はないものと判断します。

処理時間 0.15 ms なら、単体では影響なしと言ってよいと思います。

現代のゲーマースペックPCは、240fps出せるものがあるようですが、
240fps（＝1秒間に240フレーム描画）だとしても描画間隔は 5ms となります。
間隔が 5ms ということは 1.5ms かかる処理を 30回行っても余裕があるということです。

実は、アクティブタブの決定処理はかなり非効率であると考えられるので、
描画自体が十分に高速であったとしても描画に影響が出る可能性があります。
※この件は未解決課題「vista以降のAeroスナップに対応してない」で保留中。

測定した処理時間を見る限りでは、なんかあってもこのPRのせいじゃねぇ！と判断するのは妥当であるように思います。

berryzplus

LGTMです。

目視動作確認はできないためapproveにはしませんが、
コードは問題なさそうに見えます。

berryzplus · 2021-05-30T04:59:20Z

sakura_core/util/window.cpp

+	BOOL bOpaque = FALSE;
+	bool bResult = FALSE;
+	if( SUCCEEDED( ::DwmGetColorizationColor( &dwArgb, &bOpaque ) ) ){
+		if( pColorOut != nullptr ){


指摘じゃないっすけども、out引数に出力可能でない場合にシステムコールするのは無駄なので、NULLチェックはもう少し早いほうがいいと思います。

あと、CErrorInfoの仕組みを活用すれば HRESULT 値から「例外」を生成できるので、戻り値は bool じゃなくても良い（＝COLORREFを返したら良い）ような気もします。

たぶん、周りの作りに合わせて書いていくと「アクセントカラーの取得」が失敗したときに適切に異常終了させるようにコードを書くよりも、失敗した時に使う値を決めてベタ書きしておいて、呼出元で失敗を考慮する必要がないように設計していくのが良さそうに思います。

繰り返しますが指摘じゃないっす。地味にレベル高い要求してる自覚はありますので 😃

指摘じゃないっすけども、out引数に出力可能でない場合にシステムコールするのは無駄なので、NULLチェックはもう少し早いほうがいいと思います。

ここは呼び元でもし出力値が不要な (取得成否のみが欲しい) 場合には nullptr を指定してもらう、という意図であえてこのようにしてしまいました。
アクセントカラーの取得成否だけが必要、というケースはほぼないとは思いますけど😅

取得できなかった時にどんな色にしないかは、多分ですが呼び出し元の処理により様々だと思いますで、今回のように正常/異常を区別できるようにする、あるいは GetProfileInt の nDefault のように取得失敗時の既定値を呼び出し元から受け取る、というのが良いのかもしれません。

beru

問題無いと思います。

suconbu · 2021-05-30T15:16:34Z

レビューありがとうございました。マージします。

suconbu · 2021-05-30T15:45:36Z

本 PR の対応にあたって現状のタブバーの処理を調査する中で、
タブバーにサイズ変更グリップが出現する場合があったり、タブバー右端の「×」ボタンが、標準のボタンコントロールではなく自力描画、かつ予想外の方法で描画されていたりと、興味深い発見がありました。

arigayas · 2021-05-30T15:47:48Z

多段タブバーが有効時にズレないようにする対応を楽しみに待ってます😆

アクティブタブの上部にトップバンドを描画する

81e9888

suconbu force-pushed the feature/add_topband_on_activetab branch 2 times, most recently from 19fe079 to 97bfeae Compare May 29, 2021 02:20

suconbu added 3 commits May 29, 2021 16:50

トップバンドの色にアクセントカラーを使う

b67d091

CodeSmell対応

07e6167

suconbu force-pushed the feature/add_topband_on_activetab branch from 97bfeae to 07e6167 Compare May 29, 2021 08:41

suconbu marked this pull request as ready for review May 29, 2021 11:35

意図せず消してしまった行末空白を元に戻す

d90a9ca

suconbu commented May 29, 2021

View reviewed changes

MinGWビルドエラー対応

de374ff

CTabWnd::DrawTopBandの引数を参照渡しにする/定義位置を移動

54a79bd

suconbu force-pushed the feature/add_topband_on_activetab branch from 478f458 to 54a79bd Compare May 29, 2021 14:57

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

beru reviewed May 30, 2021

View reviewed changes

berryzplus reviewed May 30, 2021

View reviewed changes

beru approved these changes May 30, 2021

View reviewed changes

suconbu merged commit 9152b83 into sakura-editor:master May 30, 2021

suconbu mentioned this pull request Jun 8, 2021

タブバー描画処理のリファクタリング #1695

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

アクティブタブの上部にトップバンドを描画する #1684

アクティブタブの上部にトップバンドを描画する #1684

suconbu commented May 28, 2021 •

edited

AppVeyorBot commented May 28, 2021

suconbu commented May 28, 2021

suconbu commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

berryzplus commented May 29, 2021

berryzplus commented May 29, 2021

suconbu May 29, 2021 •

edited

berryzplus May 29, 2021

suconbu May 29, 2021

AppVeyorBot commented May 29, 2021

suconbu commented May 29, 2021

suconbu commented May 29, 2021

sonarcloud bot commented May 29, 2021

suconbu commented May 29, 2021

suconbu commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

beru May 30, 2021

berryzplus May 30, 2021

beru May 30, 2021

suconbu May 30, 2021

berryzplus commented May 30, 2021

berryzplus left a comment

berryzplus May 30, 2021

suconbu May 30, 2021

beru left a comment

suconbu commented May 30, 2021

suconbu commented May 30, 2021

arigayas commented May 30, 2021

	// Windowsクラシックスタイルの場合はアクティブタブの上部にトップバンドを描画する // 2006.03.27 ryoji
	if( !m_bVisualStyle )
	{

アクティブタブの上部にトップバンドを描画する #1684

アクティブタブの上部にトップバンドを描画する #1684

Conversation

suconbu commented May 28, 2021 • edited

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

AppVeyorBot commented May 28, 2021

suconbu commented May 28, 2021

suconbu commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

berryzplus commented May 29, 2021

berryzplus commented May 29, 2021

suconbu May 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented May 29, 2021

suconbu commented May 29, 2021

suconbu commented May 29, 2021

sonarcloud bot commented May 29, 2021

suconbu commented May 29, 2021

suconbu commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

AppVeyorBot commented May 29, 2021

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

berryzplus commented May 30, 2021

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru left a comment

Choose a reason for hiding this comment

suconbu commented May 30, 2021

suconbu commented May 30, 2021

arigayas commented May 30, 2021

suconbu commented May 28, 2021 •

edited

suconbu May 29, 2021 •

edited