除外ファイル・除外フォルダの指定をGrep置換でも使えるようにする #1210

berryzplus · 2020-02-29T07:47:40Z

PR の目的

除外ファイル・除外フォルダの指定をGrep置換でも使えるようにする

カテゴリ

機能追加

PR の背景

sakura-editor/management-forum#75 (comment)

rep 置換の除外指定の件は、修正する必要があると思います

　↓

sakura-editor/management-forum#78 (comment)

＞Grep の除外指定の件は、制約事項にリストされてたと思うのでスルーで良いと思っています。

了解。 @m-tmatma さんがよければ、このままリリースへ、やっぱり直したいと言われれば、Beta5行こうと思います。

タグジャンプの件もありますし、来月どうするか考えますかね。

　↓

反応なし。

~~まじめに対処を入れるとレビューが難しくなるので、対症療法で最低限の変更を入れる方法をとってみることにしました。~~【3/12削除】

～～【3/12追加ここから】～～
レビュー対応の結果、まじめに対処を入れた場合とほぼ同等の修正内容となりました。
レビューは難しいと思いますが、できれば対応を入れたいと考えています。
～～【3/12追加ここまで】～～

当座のポイントは、
Grep置換をコマンドライン経由で実行する場合に -GFILE に除外情報が渡らない、
に対処することです。

PR のメリット

わかりやすくするため「実装漏れに対処しました」の形式にしています。
難しいことは一切してないので、誰でもレビューできると思われます。
【3/12追加】Grep/Grep置換の実行の検索条件の表示内容が改善されます。

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

Grep/Grep置換の起動周りには、もともと莫大な課題事項が存在していますが、このPRでは課題事項に一切対処しません。
Grep結果リストに除外フォルダ・除外ファイルを正しく表示できていない件(Grep結果の対象リスト表示の気になる点 #1134) については対処を入れていません。【3/12削除】
実装漏れを引き起こした原因に対処を入れていません。

PR の影響範囲

Grep置換の実行で置換結果が別ウインドウに表示される場合にも除外フォルダ・除外ファイルが適用されるようになります。
【3/12追加】Grep/Grep置換の実行で、ファイル検索条件が「ファイル(対象ファイル名のパターン)」、「除外ファイル」、「除外フォルダ」に分かれて表示されるようになります。この分類は旧仕様で「ファイル」欄に入力した場合と新仕様の追加UIから入力した場合のどちらにも適用されます。

参考資料

原因：除外ファイル・フォルダを-GFILEにpackする処理が1箇所でしか行われていない。対策: 該当のpack処理にCDlgGrepのインスタンス経由でアクセスできるように変更、必要な関数群をアクセスできる位置に移動。

分かりやすさのためにカット＆ペーストしていた。変数を適切な名前に改め、不足していたコメントを追加する。

AppVeyorBot · 2020-02-29T08:08:10Z

✅ Build sakura 1.0.2643 completed (commit 50c7a23c92 by @berryzplus)

sakura_core/_main/CControlTray.cpp

sakura_core/cmd/CViewCommander_Grep.cpp

sakura_core/dlg/CDlgGrep.cpp

sakura_core/dlg/CDlgGrep.h

sakura_core/dlg/CDlgGrep.cpp

sakura_core/_main/CControlTray.cpp

sakura_core/dlg/CDlgGrep.cpp

beru

動作確認してみましたが、master (3a69d72) では grep で検索が出来ていたのが、PRブランチ (00b3f3c) だと出来なくなってしまってます。

検索条件

どうして検索が出来なくなったのかはこれからデバッグして調べてみます。

beru · 2020-03-01T10:46:01Z

動作しない原因が判明しました。

下記がPRブランチをデバッグ実行中に CViewCommander::Command_GREP メソッドから CGrepAgent::DoGrep メソッドを呼び出す箇所でウォッチウィンドウで拾った内容です。

+		cmWork1	L"a"	CNativeW
+		cmWork4	empty	CNativeW
+		cmWork2	L"*.cpp#.git;#.svn;#.vs;!*.msi;!*.exe;!*.obj;!*.pdb;!*.ilk;!*.res;!*.pch;!*.iobj;!*.ipdb;"	CNativeW
+		cmWork3	L"C:\\projects\\sakura"	CNativeW
+		cmWorkExcludeFile	L"*.msi;*.exe;*.obj;*.pdb;*.ilk;*.res;*.pch;*.iobj;*.ipdb;"	CNativeW
+		cmWorkExcludeFolder	L".git;.svn;.vs"	CNativeW

cmWork2 の内容ですが *.cpp の後にセミコロン無しで除外ファイルパターンが後続に続いています。セミコロンを入れたらちゃんと検索されるようになりました。

sakura_core/dlg/CDlgGrep.cpp

beru · 2020-03-01T11:35:42Z

問題の再現方法を忘れてしまったので #743 のコメントを読み直して確認してみましたが、Grep置換の結果出力が別のエディタウィンドウに出力される場合に再現します。別のエディタウィンドウに出力されるようにするのに手っ取り早い方法として、デバッグ起動後に何か適当なファイルを開いた状態からGrep置換を実行すれば良いことが分かりました。

なお、#1210 (review) で報告した問題ですが Grep条件入力画面のファイルの文字列の末尾がセミコロンで終わるように手動で入れる事でも動作する事が分かりました。ただまぁ手動で入れないとちゃんと動作しないようでは不具合なので修正してください。

berryzplus · 2020-03-01T13:49:36Z

#1210 (review)
これの不具合原因は、ぼくがコードの追加順を入れ替えたからです。

berryzplus · 2020-03-01T14:18:24Z

ちゃんと書いていませんでしたが、今回の修正で影響を受けるのは「Grep置換が別ウインドウで実行される場合」です。

細かい起動条件は Command_GREP_REPLACE に書いてありますが、次のいずれかを満たす条件でGrep置換の「置換」ボタンを押したときに発動します。

保存されているファイルから実行（テキトーなファイルを開いて実行したらこれになる
変数済みの無題ファイルから実行（Ctrl+N して1文字でも入れたらこれになる
アウトプットウインドウから実行（何につかうモードなのかイマイチよく分かってないです(^^;
Grep実行中のウインドウから実行（無理ですｗ

このパターンで起動したときに除外ファイル・除外フォルダが渡らない、という問題を解決したいPRでした。

新設したメソッドGetFileが何故か SUPER 不評だったのでリネームしました。
GetFile ⇒ GetPackedGFileString

これならメソッドのやることを説明できてる気がします。
GFile ってフレーズからサザンの名曲を連想してしまうリスクをはらんどりますが・・・。

ここが直っても課題は残ります。
このPRでは、とりあえずGrep置換の除外が効くようになるのが目標到達点です。

AppVeyorBot · 2020-03-01T14:21:48Z

✅ Build sakura 1.0.2644 completed (commit e04ffa4480 by @berryzplus)

beru

CDlgGrep::GetPackedGFileString() メソッドにおける、コマンドライン用に二重引用符をエスケープする記述について #1210 (comment) で疑問点をコメントしました。

それに対して解説お願いできますか？

berryzplus · 2020-03-08T04:12:08Z

それに対して解説お願いできますか？

ええっと・・・

ここって二重引用符をエスケープ後に、AppendExcludeFolderPatterns メソッドと AppendExcludeFilePatterns メソッドを読んでパターン文字列作った後に、エスケープを元に戻していますがこういう風にする必要性ってあるんでしょうか？なんか回りくどいように思えます。GetEscapePattern 関数で返すエスケープ文字列を二重引用符1文字分だけにするのだと駄目なんでしょうか？結局後で二重引用符の連続2文字を1文字に戻すのならいっしょなのかなと思いました。まぁ自分が必要性を読み取れていないのかもしれませんが…。

結論から言うと「エスケープ処理に意味はない」です。

これは元々 CControlTray::DoGrepCreateWindow に入れていた処理をカット＆ペーストしただけの処理群で、話をややこしくしないためにそうしました。ぼくが新たに書いた処理を入れるんではなく、元々あった処理の処理位置を移動しただけ、というテイです。

二重引用符(\") を二重(\"\") にする目的は、コマンドライン対策です。

GFileに指定した文字列 = C:\Temp;"C:\work folder";C:\Users
　　↓
コマンドライン = sakura.exe -GFILE="C:\Temp;""C:\work folder"";C:\Users"

元々の CControlTray::DoGrepCreateWindow はコマンドラインを作る処理なので、ここではエスケープが必要でした。

今回PRの処理位置はコマンドラインを作る処理ではないのでエスケープは不要です。持ってきた関数群全体が「エスケープを行ったあとの文字列を操作する前提」になっているので一度エスケープをかけてから元に戻す感じで「エスケープを元に戻す」の処理を追加しました。

エスケープ自体に何の意味もないことについては今回スルーしています。

berryzplus · 2020-03-08T04:44:47Z

さて、どうしたものか。

「エスケープに意味はないです。」と説明したうえで、
「明らかに無駄な処理」を残しておく意義がよくわからなくなってきました。

比較的少ない修正で済む話なので対処を入れてしまうべきか悩み中・・・。

理由・そもそも不要な処理だった。・エスケープ(=引用符付加)によってCGrepEnumKeys::SplitPatternが誤動作していた。その他・エスケープ(=引用符付加)が必要なケースが漏れていたので追加した。・エスケープ(=引用符付加)の仕方が誤っていたのを修正した。"#folder", "!file"とする必要があるが、#"folder", !"file"になっていた。・代入演算子の前に空白が2個あるのがキモかったので削った。

berryzplus · 2020-03-08T07:14:41Z

不要なエスケープ処理を削ってみました。
　⇒ 7b93023

結果的に3件のバグを修正できました。

受け側(=CGrepAgent::DoGrep)が、-GFILEの指定内容を正しく表示できない問題も対処しておこうかどうか悩み中・・・。

AppVeyorBot · 2020-03-08T07:17:02Z

✅ Build sakura 1.0.2648 completed (commit d09d43fbe0 by @berryzplus)

beru · 2020-03-08T08:12:12Z

結果的に3件のバグを修正できたっていうのは不要なエスケープ処理を削った事によってでですか？

3件の不具合ってどこかのコメントに列挙されてたかな…。

Issueの説明には難しいことはしていないので誰でもレビュー出来ると書かれているので、他のまともな人ならいちいち質問しないでも完全に理解出来るのかな…。

どういう問題がまずあってどうすれば再現するか
その問題にたいしてどういう対処を加えたか
対策の適用後の現状のまとめ

等をいちいち時間割いて書くべきだとは思わないので、きっと謎は謎のままにしておいた方が後から見る人の脳の体操になるという配慮ですね。

berryzplus · 2020-03-08T08:54:51Z

3件の不具合ってどこかのコメントに列挙されてたかな…。

ここに 7b93023

コマンドライン用のエスケープ処理を外す

理由
・そもそも不要な処理だった。
・エスケープ(=引用符付加)によってCGrepEnumKeys::SplitPatternが誤動作していた。

その他
・エスケープ(=引用符付加)が必要なケースが漏れていたので追加した。
・エスケープ(=引用符付加)の仕方が誤っていたのを修正した。"#folder", "!file"とする必要があるが、#"folder", !"file"になっていた。
・代入演算子の前に空白が2個あるのがキモかったので削った。

•どういう問題がまずあってどうすれば再現するか

問題
Grep置換で別ウインドウ表示となる場合に、除外フォルダ・除外ファイルが反映されない。
再現方法

テキトーなファイルを開く
Grep置換ダイアログを開く
除外ファイル・除外フォルダに何か入っているのを見る、入ってなければなんか入れる
置換ボタンクリック

•その問題にたいしてどういう対処を加えたか

Grepを別ウインドウで起動する処理にだけ含まれていた -GFILE の pack 処理を CDlgGrep に移動し、必要な箇所で利用できるように変更した。

必要な箇所

Grep（別ウインドウで実行）　←ここにだけ pack 処理があった
Grep置換（別ウインドウで実行）　← ここの処理漏れを解決したい
Grep（自ウインドウで実行）　←ここに関しては現状、対処は必然ではない
Grep置換（自ウインドウで実行）　←ここに関しては現状、対処は必然ではない

•対策の適用後の現状のまとめ

改善点

Grep置換（別ウインドウで起動）に除外ファイル・除外フォルダ）が渡るようになる。

残課題

CGrepAgent::DoGrep の -GFILE 表示が除外ファイル・除外フォルダ追加前実装のままなので、除外ファイル・除外フォルダを指定した場合にそのまま表示してしまう。
CGrepAgent::DoGrep が除外ファイルに表示する内容が、外部から受け取ったパラメータそのままで、-GFILE に pack された情報を含まないので誤っている。
CGrepAgent::DoGrep が除外フォルダに表示する内容が、以下同文。
CGrepAgent::DoGrep のメソッドパラメータが異常なほど多い（21個）。人間が一度に把握できる事象の数は 7±2 らしいので、人智を超えたパラメータ数であると言えそうです。

berryzplus · 2020-03-11T04:00:04Z

追加の対応によってPR本文で宣言している修正内容と一部ズレているので、後で本文更新します。

berryzplus · 2020-03-14T06:12:18Z

PR本文修正は終わっているのでレビューお願いします。

sakura_core/CGrepAgent.cpp

GetPackedGFileString導入により使わなくなった変数を除去する。（実は対応漏れ。）

2つの解析済み配列からリストを作る処理(2つ)をメソッド化。

berryzplus · 2020-03-14T22:32:31Z

sakura_core/_main/CControlTray.cpp

-		cFilePattern.AppendStringF(L"!%s%s%s;", escapeStr, pattern.c_str(), escapeStr);
-	}
-}
-
 void CControlTray::DoGrepCreateWindow(HINSTANCE hinst, HWND msgParent, CDlgGrep& cDlgGrep)


別ウインドウを起動してのGrep置換に除外ファイル指定が渡らない不具合（？）を引き起こした元凶は、ここにこの関数が「存在すること」だと思っています。

問題を引き起こした原因には対処しません、と言ってる通り、このPRでここに手を打つのは重い気がするので対処は別の機会にしたいと思っています。

AppVeyorBot · 2020-03-14T22:39:51Z

✅ Build sakura 1.0.2653 completed (commit 1259a84fa1 by @berryzplus)

m-tmatma · 2020-03-15T03:00:05Z

sakura_core/CGrepEnumKeys.h

+		const auto& fileKeys = m_vecExceptFileKeys;
+		excludeFiles.insert( excludeFiles.cend(), fileKeys.cbegin(), fileKeys.cend() );
+		const auto& absFileKeys = m_vecExceptAbsFileKeys;
+		excludeFiles.insert( excludeFiles.cend(), absFileKeys.cbegin(), absFileKeys.cend() );
+		return excludeFiles;


以下のようにするか (個人的には、こちらのほうがわかりやすいと思う)

const auto& fileKeys = m_vecExceptFileKeys; const auto& absFileKeys = m_vecExceptAbsFileKeys; excludeFiles.insert( excludeFiles.cend(), fileKeys.cbegin(), fileKeys.cend() ); excludeFiles.insert( excludeFiles.cend(), absFileKeys.cbegin(), absFileKeys.cend() );

以下のようにする

const auto& fileKeys = m_vecExceptFileKeys; excludeFiles.insert( excludeFiles.cend(), fileKeys.cbegin(), fileKeys.cend() ); const auto& absFileKeys = m_vecExceptAbsFileKeys; excludeFiles.insert( excludeFiles.cend(), absFileKeys.cbegin(), absFileKeys.cend() );

のようにしたほうがいいと思います。

パッと見で、コードを読みにくい。

ここって参照の一時変数をわざわざ使って書く必要性が、ここの記述だけ見てますが自分にはよくわからないです。

まぁ書いた人がこの書き方が良いならそれでも良いんじゃないとも思ったりしますね。見なかったことにすればいいし。

@m-tmatma さん＞
1の書き方だと、宣言と利用箇所が離れてしまうのが嫌です。
2と今のコードの差は空行1行に見えました。関数コメント含めてみたらきっと分かる・・・と思うので放置で行きたいです。

#1210 (comment)

ここって参照の一時変数をわざわざ使って書く必要性が、ここの記述だけ見てますが自分にはよくわからないです。

元データの変数名が長いのでエイリアスを切っています。
1行80桁にはこだわりませんが、単に見辛いと感じました。

excludeFiles.insert( excludeFiles.cend(), m_vecExceptFileKeys.cbegin(), m_vecExceptFileKeys.cend() ); excludeFiles.insert( excludeFiles.cend(), m_vecExceptAbsFileKeys.cbegin(), m_vecExceptAbsFileKeys.cend() );

その場合は途中で改行を入れれば見やすくなると思いますよ。

改行は改行で、どこに入れるかで揉める系なんですよね:cry:

excludeFiles.insert( excludeFiles.cend(), m_vecExceptFileKeys.cbegin(), m_vecExceptFileKeys.cend() ); excludeFiles.insert( excludeFiles.cend(), m_vecExceptAbsFileKeys.cbegin(), m_vecExceptAbsFileKeys.cend() );

個人的にconst参照でハマったことはないのでエイリアスのまま行きたいです。

2と今のコードの差は空行1行に見えました。関数コメント含めてみたらきっと分かる・・・と思うので放置で行きたいです。

そうです。
どこからどこまでが処理の区切りかが、ぱっと見たときにわかりずらいです。

現状のコードでもわからないことはないですが、
著者以外の人が見たときにコンマ何秒考えないといけないのですが、

空行を入れることで、まったく考えなくていいです。

m-tmatma · 2020-03-15T04:00:26Z

sakura_core/dlg/CDlgGrep.cpp

+	@param[in]     pattern チェックするパターン
+	@return        true  エスケープする必要がある
+	@return        false エスケープする必要がない
+	@author m-tmatma


https://github.com/sakura-editor/sakura/pull/758/files

で私が追加したときと比べて、108 行目～ 115 行目の部分が増えているので
author とするのは微妙な気がします。

オリジナルと比べて、変更されているのなら、それがわかるようにしてほしいし、
そもそも論として別に author はなくてもいいです。

ノリで付けた感じです。なんか追加で修正する件があったらこのPRで削っときます。

m-tmatma · 2020-03-15T04:01:21Z

sakura_core/dlg/CDlgGrep.cpp

+	{
+		return true;
+	}
+	if (pattern.find(L'\x20') != NotFound)


L' ' としていなのは見やすさのためですか?

その通りです。L' ' だと見落とす危険があると思ったので文字コードで書きました。
「常にこうする」みたいな意図はないっす。

m-tmatma · 2020-03-15T04:09:59Z

sakura_core/CGrepAgent.cpp

+		// 除外フォルダの解析済みリストを取得する
+		auto excludeFolders = cGrepEnumKeys.GetExcludeFolders();
+		std::wstring strPatterns = FormatPathList( excludeFolders );
+		cmemMessage.AppendString( strPatterns.c_str(), strPatterns.length() );


細かいですが、念のため確認

cmemMessage.AppendString( strPatterns.c_str(), strPatterns.length() );

は

cmemMessage.AppendString( strPatterns.c_str() );

とも書けると思いますが、内部で wcslen する処理を省くため?

そうです。std::wstring は内部に計算済みの有効文字列長を持っているのでそれを活用しました。第2引数を省略したらダメか？っていうとそうでもないと思っています。

cmemMessage の型を std::wstring に変えた方が良いのかもしれないですね。
このPRでやるべき事ではないと思いますが。

全体的な構成を見た感じだと、WriteLine(const wchar_t* szLine, size_t cchCount) 的なメソッドを作るんじゃないかなと思っています。

cmemMessageに溜め込んだ出力データは最終的にCGrepAgent::AddTailに渡っていて、出力先は「Grepモードのエディタ or コンソール」になります。現状は「溜め込んだ文字列がN文字以上になったらフラッシュ」ってイメージで実装されとりますが、出力が「行単位」と分かっているのだからバッファを std::list<std::wstring> にして「バッファにN行以上溜まっている」が分かる構成にしたら実行速度と結果出力のレスポンスを取りやすくできるんじゃないかと思っとります。

ついででやるには重すぎる改修内容だと思うので、結構先の話になるイメージです。

m-tmatma

スタイル上のことで修正してほしいところはありますが、
OK としておきます。

berryzplus · 2020-03-15T08:32:20Z

レビューありがとうございます。
一旦 #1210 (review) の件の解除待ちします。（解決した気になってました。

beru

動作確認してませんが多分大丈夫だと思います。

berryzplus · 2020-03-15T16:26:01Z

レビューありがとうございます。
いったんマージしてしまいます。
何か問題が見つかったら（・・・というか色々問題だらけではありますが）随時別PRで対処していきたいと思っております。

…xcluds_to_grep_replace 除外ファイル・除外フォルダの指定をGrep置換でも使えるようにする

berryzplus added 2 commits February 29, 2020 15:36

CDlgGrep::GetFileの可読性を上げる

00b3f3c

分かりやすさのためにカット＆ペーストしていた。変数を適切な名前に改め、不足していたコメントを追加する。