フォントサイズ変更時に不要な処理の呼び出しを行わないように判定追加 #1021

beru · 2019-08-29T17:23:13Z

PR の目的

Ctrlキーを押しながらマウスホイールを回転させる操作でお手軽にフォントサイズを一時的に変更する事が出来ますが、その操作を行った際の処理負荷を低減するのが目的です。

カテゴリ

速度向上

PR の背景

CViewCommander::Command_SETFONTSIZE から CEditDoc::OnChangeSetting を呼び出す場合は不必要な処理を実行しないように判定を追加しました。

展開済みメタ文字列のキャッシュを作成・更新する処理（CFileNameManager::TransformFileName_MakeCache）に比較的処理時間が掛かる事を確認しました。

この処理はフォントサイズ変更時には行う必要が無いので、フォントサイズ変更時には実施しないように判定を入れました。

PR のメリット

不必要な処理を実行しない事でPCへの負担が減り、操作に対する応答も良くなります。

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

ソースコードの記述が余計に増える事で見通しが悪くなります。コードの変更内容がベタなのであまり宜しくない感があります。

PR の影響範囲

フォントサイズ変更時の処理

…す場合は不必要な処理を実行しないように判定を追加

AppVeyorBot · 2019-08-29T17:44:40Z

✅ Build sakura 1.0.2203 completed (commit a451104392 by @beru)

berryzplus

趣旨については問題ないように思っていますが、
パッと見で関係なさそうなファイルに変更が入っていたので一旦止めます。

知ってる限りの情報はコメントで追記してみたので、もうちょい検討してみてください。
（たぶん、抜けがあると思うので不明点あれば突っ込んでください）

sakura_core/doc/layout/CLayoutMgr_DoLayout.cpp

sakura_core/doc/layout/CLayoutMgr.cpp

berryzplus · 2019-08-31T07:02:51Z

sakura_core/doc/CEditDoc.h

-	void OnChangeSetting(bool bDoLayout = true, bool bBlockingHook = true);		// ビューに設定変更を反映させる
+	void OnChangeSetting(bool bDoLayout = true,
+						 bool bBlockingHook = true,
+						 bool bFromSetFontSize = false);		// ビューに設定変更を反映させる


とくに問題ないと思います。

本質的には、名前キャッシュを一体何に使っているのかを見極めて、その結果でもって適切に対処するのがよい気がします。

個人的には名前キャッシュ自体を廃止すべきなんじゃないかと思っています。

展開済みメタ文字列のキャッシュを作成・更新 処理ですがフォントサイズ変更時に呼び出さないように対策するのは良いとして、じゃあそれ以外のケースでは設定変更時に常に呼び出すべきなのか？という疑問が湧きます。

共通設定 の ファイル名表示 の設定が変更された場合に対応する為に呼び出しを行うように記述されたんでしょうか？、もし目的がそれだとしたら本来はその設定が変更された場合にのみ呼び出すべきでしょうね。

上のほうの回答で書いたとおり、呼び出すタイミングが間違ってる説に1000点です。

1/3 はらたいらですか…。

ところでこの変更は 9acc291 で行われていますが、余計な処理をついでに行ってしまうとしても必要な処理を行わない事に比べたら全然良いんですよね。

色々な設定変更時に必要な処理だけを個別に記述しているとコード量が増えてしまいそうですし…。

思いつく手としては、設定変更時の処理は現状のまま集中して記述してその中でビットマスクで判定するようにするとかでしょうか…。設定変更時の処理を一ヵ所にまとめて記述したくない場合は addEventListener 的な仕組みを作ってイベントハンドラーを複数登録するとかかなぁ。。

しかしどちらにしろ、設定変更時に行う各処理に「どうしてそれを行うのか」という理由のコメントは残されていないので後世の人間が解読する際には想像力を働かせる必要が有ります。

berryzplus · 2019-08-31T07:10:44Z

sakura_core/doc/CEditDoc.cpp

-	m_pcEditWnd->UpdateCaption();
+	if(!bFromSetFontSize ){
+		// 親ウィンドウのタイトルを更新
+		m_pcEditWnd->UpdateCaption();


修正対象コード全体で、最終的にやりたいことはコレです。

タイトルバーに表示するフルパスを表示するためにこれをやってる。

長い場合はパスの途中を省略表示する。

「長いって何文字以上？」を判定するのにフォント情報を使います。

「どこを省略するの？」を判断するための専用ロジックがあります。

CEditDoc::OnChangeSetting は設定変更時に色々な変数を更新したり処理を実行していますね。関数が極端に長いというわけでもないですが、やっている事が雑多なのかコードを読んでいてもあんまりすっきりとしないです…。

親ウィンドウのタイトルを更新するのは単純に最後にやってる事のように見えますが、何か前の処理に依存しているのかなぁ…。

色々見てきて考えが変わったんですけど、
ぶっちゃけ、フォントサイズ変更時に行う処理として
「ウインドウタイトルの更新」って関係なくないですかね？

ここで言ってる「フォントサイズ変更」は、エディタ内の表示フォントの話で、
システムフォントで描画するタイトルバーには関係ないんです。

状況的に見て、他の設定更新系関数からコピペで作成したときに削り忘れただけ、という気配を感じています。
※つまり、「本質的には不要なんだけど判断できなかった」んじゃないかという予想。

berryzplus · 2019-08-31T07:16:28Z

sakura_core/doc/CEditDoc.cpp

-	m_cDocType.SetDocumentType( CDocTypeManager().GetDocumentTypeOfPath( m_cDocFile.GetFilePath() ), false );
+	if (!bFromSetFontSize) {
+		// 文書種別
+		m_cDocType.SetDocumentType( CDocTypeManager().GetDocumentTypeOfPath( m_cDocFile.GetFilePath() ), false );


これば文書アイコンを設定するための処理なので、フォント変更とは関係ないような気がします。

共通設定で「ドキュメントアイコンを使う」にしとくと、システムの登録アイコンが表示される、あの王です。

フォント変更時に再設定する必要は無いですね。なので if 文で囲ったほうが良いと思います。

呼出先を確認してみたところ、アイコン設定の他に、
背景画像の再読み込みとかしていたので 関係ないときは呼ばない (=このPRの対応) が良さそうに思いました。

よって、ここも問題なしっす。

berryzplus · 2019-08-31T07:26:22Z

sakura_core/doc/CEditDoc.cpp

-	CFileNameManager::getInstance()->TransformFileName_MakeCache();
+	if(!bFromSetFontSize ){
+		/* 展開済みメタ文字列のキャッシュを作成・更新 */
+		CFileNameManager::getInstance()->TransformFileName_MakeCache();


ここはたぶん問題なしです。

名前キャッシュは、パスの省略表示に使う代替名が何pxになるかを保持する機構なはずです。

例：
C:\Documents and Settings\berryzplus\マイドキュメント\temporary gabage files\Siriの研究.md
　↓省略表示
マイドキュメント\一時ファイル\Siriの研究.md

描画される位置がタイトリバーなのかタブなのかによってフォントが変わるので、この名前キャッシュが意味をなしているかどうかについては若干疑問ありです。

CFileNameManager::TransformFileName_MakeCache 以降の処理を眺めてみましたが、ピクセル数の保持とかはしていないように見えます。CFileNameManager クラスのメンバーの意味を解説するコメントが無いのが悪いと思いますが…。

CFileNameManager::m_szTransformFileNameFromExp は CFileNameManager::ExpandMetaToFolder の第2引数に渡される出力先文字列で変換後のファイルパスが入ります。そして CFileNameManager::GetTransformFileNameFast において CFileNameManager::GetFilePathFormat に渡されて簡易表示名に置換する処理の照合用に使われています。

呼出先関数を確認してみましたが、オイラが出した情報とまったく関係ない処理ですね。
なんでここでやる必要があるのか激しく疑問。

sakura/sakura_core/env/CFileNameManager.cpp

Line 102 in 21a9c0e

int CFileNameManager::TransformFileName_MakeCache( void ){

C言語の関数は読みづらくてかなわんとです・・・。
C++で同じ意味になるように整形してみました。

//! 展開済みメタ文字列のキャッシュを作成・更新する int CFileNameManager::TransformFileName_MakeCache( void ){ int nCount = 0; for( int i = 0; i < m_pShareData->m_Common.m_sFileName.m_nTransformFileNameArrNum; i++ ){ const auto &filenameFrom = m_pShareData->m_Common.m_sFileName.m_szTransformFileNameFrom[i]; if (!filenameFrom[0]) { continue; //展開元が空文字ならスキップ。 } auto &expanded = m_szTransformFileNameFromExp[nCount]; if (!ExpandMetaToFolder(filenameFrom, expanded, _countof(expanded))) { continue; //展開できなかったらスキップ。 } // m_szTransformFileNameToとm_szTransformFileNameFromExpの番号がずれることがあるので記録しておく m_nTransformFileNameOrgId[nCount++] = i; } m_nTransformFileNameCount = nCount; return nCount; }

この処理に時間がかかってるとすれば、ExpandMetaToFolderの実装効率がよくないと予想できます。

処理内容的には、それこそ 共通設定 -> ファイル名の変更時のみ に実施すればよい処理である気がします。

Visual Studio の Performance Profiler が出したレポートでは WindowsAPI の GetLongPathName の呼び出しに時間が掛かってました。

まぁ時間が掛かってるとはいっても、そこまで頻繁に呼ばれる処理では無く気にしようとしなければ気にならない程度かもしれません。。

sakura_core/doc/CEditDoc.cpp

AppVeyorBot · 2019-08-31T08:19:08Z

✅ Build sakura 1.0.2209 completed (commit 1fb986d811 by @)

berryzplus

というわけで、ぼくの疑問は解消しました。
残ってるのはスルーしてもいい問題と課題残しでそのうち対処したい問題のみなので review を更新します。

berryzplus · 2019-09-01T06:45:38Z

sakura_core/doc/CEditDoc.cpp

@@ -685,8 +688,10 @@ void CEditDoc::OnChangeSetting(
 		}
 	}

-	/* 共有データ構造体のアドレスを返す */
-	CFileNameManager::getInstance()->TransformFileName_MakeCache();
+	if(!bFromSetFontSize ){


指摘漏らしてました。
ソースコード全体で 空白の扱いが変 なので、
合わせるなら全部合わせるべきな気がします。
個人的には、どうでもいいです。

誤) if(!bFromSetFontSize ){
正) if( !bFromSetFontSize ){

berryzplus · 2019-09-01T06:52:34Z

sakura_core/doc/CEditDoc.cpp

-	CFileNameManager::getInstance()->TransformFileName_MakeCache();
+	if(!bFromSetFontSize ){
+		/* 展開済みメタ文字列のキャッシュを作成・更新 */
+		CFileNameManager::getInstance()->TransformFileName_MakeCache();


呼出先関数を確認してみましたが、オイラが出した情報とまったく関係ない処理ですね。
なんでここでやる必要があるのか激しく疑問。

sakura/sakura_core/env/CFileNameManager.cpp

Line 102 in 21a9c0e

int CFileNameManager::TransformFileName_MakeCache( void ){

C言語の関数は読みづらくてかなわんとです・・・。
C++で同じ意味になるように整形してみました。

//! 展開済みメタ文字列のキャッシュを作成・更新する int CFileNameManager::TransformFileName_MakeCache( void ){ int nCount = 0; for( int i = 0; i < m_pShareData->m_Common.m_sFileName.m_nTransformFileNameArrNum; i++ ){ const auto &filenameFrom = m_pShareData->m_Common.m_sFileName.m_szTransformFileNameFrom[i]; if (!filenameFrom[0]) { continue; //展開元が空文字ならスキップ。 } auto &expanded = m_szTransformFileNameFromExp[nCount]; if (!ExpandMetaToFolder(filenameFrom, expanded, _countof(expanded))) { continue; //展開できなかったらスキップ。 } // m_szTransformFileNameToとm_szTransformFileNameFromExpの番号がずれることがあるので記録しておく m_nTransformFileNameOrgId[nCount++] = i; } m_nTransformFileNameCount = nCount; return nCount; }

この処理に時間がかかってるとすれば、ExpandMetaToFolderの実装効率がよくないと予想できます。

処理内容的には、それこそ 共通設定 -> ファイル名の変更時のみ に実施すればよい処理である気がします。

berryzplus · 2019-09-01T06:54:22Z

sakura_core/doc/CEditDoc.cpp

@@ -714,8 +719,10 @@ void CEditDoc::OnChangeSetting(
 	}
 	const CKetaXInt nTabSpaceOld = m_cDocType.GetDocumentAttribute().m_nTabSpace;

-	// 文書種別
-	m_cDocType.SetDocumentType( CDocTypeManager().GetDocumentTypeOfPath( m_cDocFile.GetFilePath() ), false );
+	if (!bFromSetFontSize) {


指摘漏らしてました。
上のほうと同じです。

誤) if (!bFromSetFontSize) {
正) if( !bFromSetFontSize ){

当然ですが、スルー可 😃

berryzplus · 2019-09-01T06:57:20Z

sakura_core/doc/CEditDoc.cpp

-	m_cDocType.SetDocumentType( CDocTypeManager().GetDocumentTypeOfPath( m_cDocFile.GetFilePath() ), false );
+	if (!bFromSetFontSize) {
+		// 文書種別
+		m_cDocType.SetDocumentType( CDocTypeManager().GetDocumentTypeOfPath( m_cDocFile.GetFilePath() ), false );


呼出先を確認してみたところ、アイコン設定の他に、
背景画像の再読み込みとかしていたので 関係ないときは呼ばない (=このPRの対応) が良さそうに思いました。

よって、ここも問題なしっす。

berryzplus · 2019-09-01T06:58:51Z

sakura_core/doc/CEditDoc.cpp

@@ -832,8 +839,10 @@ void CEditDoc::OnChangeSetting(
 		SelectCharWidthCache( CWM_FONT_PRINT, CWM_CACHE_LOCAL );
 	}

-	// 親ウィンドウのタイトルを更新
-	m_pcEditWnd->UpdateCaption();
+	if(!bFromSetFontSize ){


指摘漏らしてました。

誤) if(!bFromSetFontSize ){
正) if( !bFromSetFontSize ){

approveにしてしまいますので、必要あると判断したら直したって下さい。

berryzplus · 2019-09-01T07:08:12Z

sakura_core/doc/CEditDoc.cpp

-	m_pcEditWnd->UpdateCaption();
+	if(!bFromSetFontSize ){
+		// 親ウィンドウのタイトルを更新
+		m_pcEditWnd->UpdateCaption();


色々見てきて考えが変わったんですけど、
ぶっちゃけ、フォントサイズ変更時に行う処理として
「ウインドウタイトルの更新」って関係なくないですかね？

ここで言ってる「フォントサイズ変更」は、エディタ内の表示フォントの話で、
システムフォントで描画するタイトルバーには関係ないんです。

状況的に見て、他の設定更新系関数からコピペで作成したときに削り忘れただけ、という気配を感じています。
※つまり、「本質的には不要なんだけど判断できなかった」んじゃないかという予想。

berryzplus · 2019-09-01T07:09:26Z

sakura_core/doc/CEditDoc.h

-	void OnChangeSetting(bool bDoLayout = true, bool bBlockingHook = true);		// ビューに設定変更を反映させる
+	void OnChangeSetting(bool bDoLayout = true,
+						 bool bBlockingHook = true,
+						 bool bFromSetFontSize = false);		// ビューに設定変更を反映させる


上のほうの回答で書いたとおり、呼び出すタイミングが間違ってる説に1000点です。

beru · 2019-09-01T08:00:59Z

空白の入れ方が元のソースコードと一致していない指摘には未対応ですが、大きな問題ではないと思うので Merge します。

beru · 2019-09-01T08:01:58Z

@berryzplus さん
レビューありがとうございました。
もし問題が見つかったら別PRで対応します。

フォントサイズ変更時に不要な処理の呼び出しを行わないように判定追加

CViewCommander::Command_SETFONTSIZE から CEditDoc::OnChangeSetting を呼び出…

39ed046

…す場合は不必要な処理を実行しないように判定を追加

beru added the 🚅 speed up 🚀 高速化 label Aug 29, 2019

berryzplus reviewed Aug 31, 2019

View reviewed changes

レビュー指摘対応

9e41dc4

berryzplus approved these changes Sep 1, 2019

View reviewed changes

beru merged commit 4b38732 into sakura-editor:master Sep 1, 2019

beru deleted the Command_SETFONTSIZE branch September 1, 2019 08:01

m-tmatma added this to the v2.4.0 milestone Dec 29, 2019

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020

Merge pull request sakura-editor#1021 from beru/Command_SETFONTSIZE

66d2e88

フォントサイズ変更時に不要な処理の呼び出しを行わないように判定追加

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

フォントサイズ変更時に不要な処理の呼び出しを行わないように判定追加 #1021

フォントサイズ変更時に不要な処理の呼び出しを行わないように判定追加 #1021

beru commented Aug 29, 2019

AppVeyorBot commented Aug 29, 2019

berryzplus left a comment

berryzplus Aug 31, 2019

beru Aug 31, 2019

berryzplus Sep 1, 2019

beru Sep 1, 2019 •

edited

Loading

berryzplus Aug 31, 2019

beru Aug 31, 2019

berryzplus Sep 1, 2019

berryzplus Aug 31, 2019

beru Aug 31, 2019

berryzplus Sep 1, 2019

berryzplus Aug 31, 2019

beru Aug 31, 2019

berryzplus Sep 1, 2019

beru Sep 1, 2019

AppVeyorBot commented Aug 31, 2019

berryzplus left a comment

berryzplus Sep 1, 2019

berryzplus Sep 1, 2019

berryzplus Sep 1, 2019

berryzplus Sep 1, 2019

berryzplus Sep 1, 2019

berryzplus Sep 1, 2019

berryzplus Sep 1, 2019

beru commented Sep 1, 2019 •

edited

Loading

beru commented Sep 1, 2019

フォントサイズ変更時に不要な処理の呼び出しを行わないように判定追加 #1021

フォントサイズ変更時に不要な処理の呼び出しを行わないように判定追加 #1021

Conversation

beru commented Aug 29, 2019

PR の目的

カテゴリ

PR の背景

PR のメリット

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

PR の影響範囲

AppVeyorBot commented Aug 29, 2019

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

Choose a reason for hiding this comment

beru Sep 1, 2019 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Aug 31, 2019

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

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

Choose a reason for hiding this comment

beru commented Sep 1, 2019 • edited Loading

beru commented Sep 1, 2019

beru Sep 1, 2019 •

edited

Loading

beru commented Sep 1, 2019 •

edited

Loading