Sakura: Grep結果の対象リスト表示の気になる点

Created on 11 Jan 2020  ·  10Comments  ·  Source: sakura-editor/sakura

1133 で報告した潜在的課題の詳細です。

PR のデメリット (トレードオフとかあれば)
•修正部分（パスなどのパターンリストを表示する処理）を分かりやすくすることによって、現状のコードに存在している 些細な問題点 が気になってしまう結果になるかもしれません。

PRの影響範囲
•改修箇所は、Grep検索およびGrep置換の「検索対象」、「フォルダ」、「除外ファイル」、「除外フォルダ」のパスリスト表示に関連する部分です。

デメリット説明をよく見て欲しいんですが、
修正部分は、パスなどのパターンリストを表示する処理である
と読み取れる記述になっています。

コードが読める人は修正内容と合わせて見比べてみてください。

コード上で、パスリストを出力しているのは「フォルダ」のみです。

他3項目「検索対象」、「除外ファイル」、「除外フォルダ」は、引数で渡された文字列をそのまま出力しています。

PRの説明と違うじゃねーか！（怒

と思うかも知れません。

実はこれが、デメリットに指摘した 些細な問題点 だったりします。

「検索対象」、「除外ファイル」、「除外フォルダ」に出力するものは何でしょうか？

  • 検索対象には、検索対象ファイルを特定するパターンのリストを出力します。
  • 除外ファイルには、Grep検索時のファイル探索から除外するパターンのリストを出力します。
  • 除外フォルダには、Grep検索時のフォルダ探索から除外するパターンのリストを出力します。

現状のコードは、単に渡された引数をそのまま出してしまっています。
つまり、コードが設計（仕様）を正しく反映していません。

どうしたらいいんだっけ？

はこれからボチボチ考えていく感じでいいのかな、と思っとります。

picture berryzplus
😕1

All 10 comments

「検索対象」、「除外ファイル」、「除外フォルダ」に出力するものは何でしょうか？

  • 検索対象には、検索対象ファイルを特定するパターンのリストを出力します。
  • 除外ファイルには、Grep検索時のファイル探索から除外するパターンのリストを出力します。
  • 除外フォルダには、Grep検索時のフォルダ探索から除外するパターンのリストを出力します。

現状のコードは、単に渡された引数をそのまま出してしまっています。
つまり、コードが設計（仕様）を正しく反映していません。

どうしたらいいんだっけ？

はこれからボチボチ考えていく感じでいいのかな、と思っとります。

1133 で削除される 実処理のないif分岐を削除 の部分には下記のコメントがありますが、

/* 文字列区切り記号エスケープ方法  0=[\"][\'] 1=[""][''] */

そのコメントの後方にある 0=[\"][\'] 1=[""][''] というのは内容が古いのかも知れないと思いました。ソースコードの色々な場所でこの記述がありますが、enum EStringLiteralType 型ではなくて int 型の変数にこのコメントが付いている事が多いです。多分同じものなんだと思いますが統一されなかったんでしょうか？それとも似て非なるものなのか…。

enum EStringLiteralType に関してはファイルタイプ別設定の カラー タブの 文字列エスケープ グループボックス内のドロップダウンリスト設定に対応していると思います。

CGrepAgent.cpp ファイル中の EscapeStringLiteral 関数は色指定が有効な場合に、

  • STRING_LITERAL_CPP(C/C++言語風), STRING_LITERAL_CSHARP(C#風), STRING_LITERAL_PYTHON(Python風) のどれかの場合は

    • \ を \\ にエスケープ

    • ' を \' にエスケープ

    • " を \" にエスケープ

  • STRING_LITERAL_PLSQL(PL/SQL風) の場合は

    • ' を '' にエスケープ

    • " を "" にエスケープ

します。STRING_LITERAL_HTML(HTML/XML風) の場合のエスケープ処理が記述されていませんが気にしない事にします。

実装が何を正しく反映していないのかは仕様や設計が記載された文書が無いので想像するしか無さそうです。昔コードを書いた人達はおそらくみな虎眼先生のように少々曖昧になっていると思われますのでお伺いするのには大変勇気が……恐れ多いです。

単に渡された引数をそのまま出してしまうのだと、どういう問題があるんでしょうかね？エディタ上にエスケープ出力する意味や必要性がそもそも自分はよくわかってません。

検索条件 は EscapeStringLiteral 関数を使ってエスケープされたのが出力されます。
検索対象, フォルダ, 除外ファイル, 除外フォルダ についてはEscapeStringLiteral 関数を使ったエスケープ出力は行われません。

beru picture beru on 11 Jan 2020

単に渡された引数をそのまま出してしまうのだと、どういう問題があるんでしょうかね？エディタ上にエスケープ出力する意味や必要性がそもそも自分はよくわかってません。

引数で渡されるもの＝パターン指定を ';' 区切りで文字列化したもの（リスト）×３
出力すべきもの＝引数で渡された３つの文字列を解析して生成される5つの std::vector<LPCWSTR> を';' 区切りで文字列化した値（引数の指定内容とは異なる可能性がある）

この辺の追加処理の結果を整形して出すべきじゃないかと思います。
https://github.com/sakura-editor/sakura/blob/45c8c9ee42462dc5f82a0aabddc7bc592e53fa4b/sakura_core/CGrepAgent.cpp#L379-L383

リスト要素には ';' を含む可能性があるので、何らかのエスケープが必要と考えられます。

扱うものがWindowsのパスなので、文字列エスケープは都合がよくないと思っています。
※Windowsはフォルダ区切りが '\' なので「文字列」に含めると変な感じになるから。

例：

  • "C:\Windows\System32\notepad.exe"

    • '\Wはエスケープとして認識されません。</li> <li>'\S はエスケープとして認識されません。

    • パスの途中に改行を含めることはできません。

仮に EscapeStringLiteral を使った場合、 '\' が '\\' になってしまい、見辛いと思います。
ゆえに、エスケープは必要だけど「文字列エスケープじゃない」ような気がしています。

berryzplus picture berryzplus on 12 Jan 2020
😕1

引数で渡されるもの＝パターン指定を ';' 区切りで文字列化したもの（リスト）×３
出力すべきもの＝引数で渡された３つの文字列を解析して生成される5つの std::vector を';' 区切りで文字列化した値（引数の指定内容とは異なる可能性がある）

この すべき というのには何か材料というか理由はあるんでしょうか？

これこれこうだからこうするべきなんだよ、と言われたら ふむふむ… と考えられますが、
～すべきだから～すべきなんだよ と言われると ｴｪｩｧ… という状態に陥ります。

beru picture beru on 12 Jan 2020

仕様寄りの話なので、どうだったら使いやすい（と思う）かを考えたらいいように思います。

現状

Grep置換の結果表示例

□検索条件  "(?<=Copyright \(C\) 2018-)2019(?= Sakura Editor Organization)"
置換後         "2020"
検索対象       *.*;#.git;#.svn;#.vs;#v15;!*.msi;!*.exe;!*.obj;!*.pdb;!*.ilk;!*.res;!*.pch;!*.iobj;!*.ipdb;!*.png
フォルダ       C:\work\sakura
除外ファイル   *.msi;*.exe;*.obj;*.pdb;*.ilk;*.res;*.pch;*.iobj;*.ipdb;*.png
除外フォルダ   .git;.svn;.vs;v15
    (サブフォルダも検索)
    (英大文字小文字を区別する)
    (正規表現:bregonig.dll Ver.4.12 with Onigmo 6.1.2)
    (文字コードセットの自動判別)
    (一致した行を出力)

提案を実装するとどうなるか

Grep置換の結果表示例

□検索条件  "(?<=Copyright \(C\) 2018-)2019(?= Sakura Editor Organization)"
置換後         "2020"
検索対象       *.*
フォルダ       C:\work\sakura
除外ファイル   *.msi;*.exe;*.obj;*.pdb;*.ilk;*.res;*.pch;*.iobj;*.ipdb;*.png
除外フォルダ   .git;.svn;.vs;v15
    (サブフォルダも検索)
    (英大文字小文字を区別する)
    (正規表現:bregonig.dll Ver.4.12 with Onigmo 6.1.2)
    (文字コードセットの自動判別)
    (一致した行を出力)

その他

上記置換内容の PR を、今年は一体誰が出すんでしょう・・・

berryzplus picture berryzplus on 12 Jan 2020
😕1

この すべき というのには何か材料というか理由はあるんでしょうか？

これこれこうだからこうするべきなんだよ、と言われたら ふむふむ… と考えられますが、
～すべきだから～すべきなんだよ と言われると ｴｪｩｧ… という状態に陥ります。

どうあったら使いやすいか？と考えた結果、そうすべきだと判断しました。

berryzplus picture berryzplus on 12 Jan 2020
😕1

「出力すべきもの」を「出てくれたら嬉しいもの」と言い換えたらニュアンス伝わりますか？

berryzplus picture berryzplus on 12 Jan 2020
😕1

https://github.com/sakura-editor/sakura/issues/1134#issuecomment-573392076

どうあったら使いやすいか？と考えた結果、そうすべきだと判断しました。
「出力すべきもの」を「出てくれたら嬉しいもの」と言い換えたらニュアンス伝わりますか？

うーん、ちゃんと明確な文章で表現をしていなくないですか？:confused:

例えば、下記のような文章で示されたら（意図と一致しているのか不明ですが）

  • 変更内容 : Grep 条件の検索対象から除外ファイルの指定を取り除く
  • 変更理由 : 除外ファイルの記述は別の行に表示しているので表示内容が重複している。検索対象に除外ファイルの出力も行うと内容が読み取りにくくなるので取り除きたい。

理解出来るかなと思います。また一定のルールに沿った出力にする事で自然と（例えば）上記の出力が得られる、という事なのであればその一定のルールが示されないと話が繋がらない気がします。

beru picture beru on 12 Jan 2020

どうあったら使いやすいか？と考えた結果、そうすべきだと判断しました。
「出力すべきもの」を「出てくれたら嬉しいもの」と言い換えたらニュアンス伝わりますか？

うーん、ちゃんと明確な文章で表現をしていなくないですか？😕

してませんね。

「出力すべきの根拠はなんですか？」と訊かれたので
ぼくなりに「普通に考えてそうだ」に至った経緯を書きました。

例えば、下記のような文章で示されたら（意図と一致しているのか不明ですが）
•変更内容 : Grep 条件の検索対象から除外ファイルの指定を取り除く
•変更理由 : 除外ファイルの記述は別の行に表示しているので表示内容が重複している。検索対象に除外ファイルの出力も行うと内容が読み取りにくくなるので取り除きたい。

理解出来るかなと思います。また一定のルールに沿った出力にする事で自然と（例えば）上記の出力が得られる、という事なのであればその一定のルールが示されないと話が繋がらない気がします。

(それ、文章じゃなくね？....)

ぶざけるのはいいかげんにするとして真面目に回答します。

検索対象に関して変更内容の認識はあってそうです。

DoGrep関数は、渡された引数を加工して独自のリストを構築します。
検索時に実際に使われる検索対象、除外ファイル、除外フォルダは、
関数内で作られた独自リストの要素アクセスをラップするアクセサとして渡されます。
※詳しくは DoGrepTree の呼出引数がどのように生成されるかを見てみてください。

Grep検索のヘッダーは、検索条件を示す目的の出力だと思います。
出力は、実際に使われる条件にできるだけ近いものであってほしいです。
これは価値観による主張ですが、実際の条件と違うなら出さなくていいと思います。

引数で渡されたリストと解析後リストの違いは以下になります。

項目 | 関数内解析で変化する内容
-- | --
検索対象 | 除外ファイル、除外フォルダの指定が取り除かれる
除外ファイル | 検索対象の指定分 (先頭に!) が追加される。リストが2つに分かれる。
除外フォルダ | 検索対象の指定分 (先頭に#) が追加される。リストが2つに分かれる。

なので、変更内容は以下3つになります。

■変更内容①：検索対象から除外ファイルと除外フォルダを取り除く
■変更内容②：除外ファイルに検索対象内の除外ファイル指定をマージする
■変更内容③：除外フォルダに検索対象内の除外フォルダ指定をマージする

■変更理由：Grep検索の検索条件は、関数内で組み替えられて条件が変わるので、
実際に使われる検索条件となるべく近い情報を出力したい。

・・・ん？実際に使われる検索条件を出せてないのってバグじゃね？って突っ込まれるのかな？

berryzplus picture berryzplus on 12 Jan 2020

ちょっと自分が議題の内容を最初は捉えられてなかった事があって、コメントのやり取りの流れがあらぬ方向に走ってしまった気がします。

なお、コードのI/Fと実際の出力形式にどこまで相似性を持たせられるかどうかというのは気になるところかもしれませんが、まずそれは置いておいてどういう出力にしたいのか、という事を明確に表現した方がIssueとしての内容が分かりやすいんじゃないかと思います。実装の詳細を見ていない人も検討に参加出来る余地が生まれますし。

簡単にまとめてしまうと、Grep結果出力のヘッダ的な部分の形式をこれこれこうしたい、って事でしょうか？ちょっと理解しきれてないかもしれませんが、Grep条件入力画面の入力項目と同じ値でGrep結果出力のヘッダを表示したい、とかなのかな？

誤解等を招くような大きい支障が無い限りは大概の変更はOKなんじゃないかと思います。例えば、Grep条件入力画面が昔より巨大化してしまいましたが、人によってはそれが好みかも知れないし、ある程度大きいモニタを使っていれば問題無い気がします。あと長い間使っていれば慣れるかもと。

もし方式を選ぶ必要があるほどの結構な問題だとしたら設定を画面で選択する等の手は考えられますが、別に些細な事ならそのうち気にならなくなりそうな気も…。

beru picture beru on 15 Jan 2020

1210 で渡されたパラメータではなく、解析済みの条件リストを出力するように変更しました。

「気になる点」が解消されたのでこのissueは閉じておきます。
（そして、コメント中に書いた close #1134; は機能しませんでした(^^;）

berryzplus picture berryzplus on 15 Mar 2020
Was this page helpful?
0 / 5 - 0 ratings

Related issues

m-tmatma picture m-tmatma  ·  34Comments
m-tmatma picture m-tmatma  ·  33Comments
m-tmatma picture m-tmatma  ·  59Comments
kobake picture kobake  ·  32Comments
arigayas picture arigayas  ·  24Comments