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

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

Merged
merged 2 commits into from
Sep 1, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39ed046
CViewCommander::Command_SETFONTSIZE から CEditDoc::OnChangeSetting を呼び出…
beru Aug 29, 2019
9e41dc4
レビュー指摘対応
Aug 31, 2019
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/cmd/CViewCommander_Settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ void CViewCommander::Command_SETFONTSIZE( int fontSize, int shift, int mode )
);
}else if( mode == 2 ){
// 自分だけ更新
GetDocument()->OnChangeSetting( true, false );
GetDocument()->OnChangeSetting( true, false, true );
}
}

Expand Down
23 changes: 16 additions & 7 deletions sakura_core/doc/CEditDoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,14 +644,17 @@ void CEditDoc::OnChangeType()

/*! ビューに設定変更を反映させる
@param [in] bDoLayout レイアウト情報の再作成
@param [in] bBlockingHook 処理中のユーザー操作を可能にする
@param [in] bFromSetFontSize フォントサイズ設定から呼び出されたかどうか

@date 2004.06.09 Moca レイアウト再構築中にProgress Barを表示する．
@date 2008.05.30 nasukoji テキストの折り返し方法の変更処理を追加
@date 2013.04.22 novice レイアウト情報の再作成を設定できるようにした
*/
void CEditDoc::OnChangeSetting(
bool bDoLayout,
bool bBlockingHook
bool bBlockingHook,
bool bFromSetFontSize
)
{
int i;
Expand Down Expand Up @@ -685,8 +688,10 @@ void CEditDoc::OnChangeSetting(
}
}

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

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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の実装効率がよくないと予想できます。

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

}

CLogicPointEx* posSaveAry = NULL;

Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

当然ですが、スルー可 😃

// 文書種別
m_cDocType.SetDocumentType( CDocTypeManager().GetDocumentTypeOfPath( m_cDocFile.GetFilePath() ), false );
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

}

const STypeConfig& ref = m_cDocType.GetDocumentAttribute();

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

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

Choose a reason for hiding this comment

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

指摘漏らしてました。

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

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

// 親ウィンドウのタイトルを更新
m_pcEditWnd->UpdateCaption();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  • タイトルバーに表示するフルパスを表示するためにこれをやってる。
    • 長い場合はパスの途中を省略表示する。
      • 「長いって何文字以上？」を判定するのにフォント情報を使います。
      • 「どこを省略するの？」を判断するための専用ロジックがあります。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

}
}

/*! ファイルを閉じるときのMRU登録 & 保存確認 ＆ 保存実行
Expand Down
4 changes: 3 additions & 1 deletion sakura_core/doc/CEditDoc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ class CEditDoc
//イベント
BOOL HandleCommand(EFunctionCode nCommand);
void OnChangeType();
void OnChangeSetting(bool bDoLayout = true, bool bBlockingHook = true); // ビューに設定変更を反映させる
void OnChangeSetting(bool bDoLayout = true,
bool bBlockingHook = true,
bool bFromSetFontSize = false); // ビューに設定変更を反映させる
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

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
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@beru beru Sep 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

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

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

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

BOOL OnFileClose(bool bGrepNoConfirm); /* ファイルを閉じるときのMRU登録 & 保存確認 ＆ 保存実行 */

void RunAutoMacro( int idx, LPCTSTR pszSaveFilePath = NULL ); // 2006.09.01 ryoji マクロ自動実行
Expand Down