PatchUnicode-1051 現在のドキュメント内容をGrep

[dep5]

PR の目的

Mocaさん、novice123さん作の
PatchUnicode-1051 現在のドキュメント内容をGrep　を適用します

「現在編集中ファイルから検索」を、 現在開いているファイル名の保存ファイルからGrepだったものを、 現在のドキュメントの内容(無題等を含む)から直接Grepできるように変更したものです。 　ウィンドウからGrepを使うと、対象が"Windows:[タイトル]"と表示されて、 Grepのタグが":HWND:[01234567]"のハンドルが表示されます。 Grepの自分自身を指定することもできます。 (ヘッダで出力された検索キーワードが引っかかってしまう制限付き)

カテゴリ

• 機能追加
• 仕様変更

PR の背景

新規の文書・編集中の文書をGrepしたい場合、一時的にでもファイルとして保存する必要がありました。

PR のメリット

ファイルに保存していないテキストを、Grepできるようになります
検索結果のカウントなどもできます

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

Grepでファイルを編集中のGrepする対象が変わります。
GrepとGrep置換で挙動のずれが生じます

仕様・動作説明

Grep
新規のテキスト
（前）不可 （後）可
ファイルを編集中
（前）ファイルをGrep （後）テキストをGrep
ファイルに保存済み
（前）ファイルをGrep （後）ファイルをGrep？

Grep置換
機能しません

PR の影響範囲

テスト内容

テスト1

手順

関連 issue, PR

参考資料

[AppVeyorBot]

✅ Build sakura 1.0.3695 completed (commit 86aad795b5 by @dep5)

[beru]

もかさんは過去に mocaskr というユーザー名の github のアカウントで活動していたようです。

https://github.com/mocaskr/sakura

でグーグル検索すると結構ヒットします。今はもう引退されてしまったようですね。

[sanomari]

Secutiry HotSpotが出てるのはいけないんではないかと思いました。

4 Security Hotspots

パッチで提供された機能の取り込みなので、CodeSmellsの対応はめんどうならしなくてもいいでしょう。Security Hotspotはご時世的に対応必須です。

追加したい機能についてですが、仕様的におかしいと思います。
「要望がおかしい」ではなく「仕様的におかしい」です。

やりたいこと：
　編集中の未保存データ内を検索してGrepみたいな結果表示をさせたい。

仕様的におかしい点：
　やりたいことは「検索」の延長だと思います。（違う機能を拡張してません？）
　Grepは検索対象ファイル群を横断的に（グローバルに）検索する仕様です。
　（Grepを世間一般の仕様と乖離させたいという「仕様変更」は許容できますか？）

既存でも仕様がおかしいところは既にたくさんあるので、
このままいれちゃってよいようにも思います。

[berryzplus]

Security Hotspotsの対応が必須なのは同意見です。
CodeSmellsはめんどうなやつ（if文とか階層とかstd::stringとか)以外は直したいっす。

拡張するのはGrepじゃなくて検索にも同意ですが
流用したい結果表示機能を切り出すためにはCGrepAgentを触る必要があって、
結果表示のオプションを流用するためにはGrepダイアログを使うほうがいい気がしました。
（つまり、修正対象は大して変わらん気がします。）

よって、SonarCloud対策ができたら「とりあえず入れてみる」で良いと思います。

[ghost]

コミットに表示されている人がmocaさんではなく、フランスの開発者のようです。
authorを訂正したほうがいいと思いました。

[berryzplus]

コミットに表示されている人がmocaさんではなく、フランスの開発者のようです。
authorを訂正したほうがいいと思いました。

beruさんの書き込みと同じ指摘っぽいですね。

sourceforge時代の主力開発者 moca さんのアカウントは moca_skr なはずです。
既に削除されたアカウントを参照するのも若干アレなのですが、
フランス人（おそらく無関係の別人）が書いたことにしちゃうよりはマシかと思います。

代案は Moca<moca_skr ＠ users.sourceforge.jp> のように、
GitHubのシステムに誤認させないようにリアルな Author を設定することっす。

[dep5]

誤ったAuthorに関連付けされていたのを直しました。すいません。
アドバイスありがとうございます。

sanomariさん
複数のファイルを横断的に処理するのがGrepということですね。
現在当該チェックボックスでは一つのファイルだけをGrepしているので、
その延長で、ファイルにする前のテキストを対象にするのもありかなと思います。
しかし、欲しいものはファイル保存せずにGrepした結果の一覧やタグジャンプですので、
その機能があれば、場所や名称を変えてもよいと思います。

[dep5]

この変更でファイルを開いて編集中に「編集中のファイルから検索」でGrepした場合、
これまで保存済みのファイルが対象だったのが、編集中の内容が対象に変わります。
以前の挙動のほうが必要な方がいたらご意見聞かせてください。

[berryzplus]

むぅ、コンフリクトの内容を確認していたんですが、かなり難しい感じに衝突しているようです。
CViewCommander_TagJump.cpp (2箇所)

マージは慣れてるはずの自分が、断念するレベルっす・・・。

[dep5]

Secutiry HotSpotの修正は_countof()をつけるのでいいんでしょうか？
ビルドさえ成功すればいいと思って、使ってきたのでコード修正についてはわかりません。
できたらどなたか対応してもらえませんか。

[berryzplus]

Secutiry HotSpotの修正は_countof()をつけるのでいいんでしょうか？
ビルドさえ成功すればいいと思って、使ってきたのでコード修正についてはわかりません。
できたらどなたか対応してもらえませんか。

解析結果 https://sonarcloud.io/project/security_hotspots?id=sakura-editor_sakura&pullRequest=1639&resolved=false&types=SECURITY_HOTSPOT に書いてありますよ。

• wcscpy(dest, src)は、srcの中身がdestの確保サイズより大きい場合にオーバーフローします。
  wcsncpy_s(dest, _countof(dest), src, _TRUNCATE)のようにオーバーフローする部分を考慮した実装が推奨です。
• wcscat(dest, somethingToAdd)もオーバーfフローします。
  連結した文字列長が確保サイズを越えないことを保証したいだけなので_s版じゃなくてもよいです。
  同様に_s版関数を使うのか推奨です。
• wcslen(dest)はdestがnullptrのとき、アクセス違反で落ちます。
  destがNULLのときwcslen(dest)を呼ばないように修正すれば良いです。

当面でヤヴァいのはコンフリクトっす。

[suconbu]

この変更でファイルを開いて編集中に「編集中のファイルから検索」でGrepした場合、
これまで保存済みのファイルが対象だったのが、編集中の内容が対象に変わります。
以前の挙動のほうが必要な方がいたらご意見聞かせてください。

私は賛成です。
どこぞからコピーして無題ウィンドウに貼り付けたテキストを、いちいち保存せずにいつもの「Grep」ができるのは結構便利な気がします。※ grep というより sed な感覚ですね。

今までは「Grep」の中の一つの機能でしたけど、後には独立した機能 (フィルタ機能？) として「検索、置換、Grep、Grep置換」などの同列に加わる形でもいいのかもしれませんねえ。

[AppVeyorBot]

✅ Build sakura 1.0.3834 completed (commit 8a5d64b4a7 by @dep5)

[dep5]

v0.20でszJumpToFileをwstringで書き換えたかったのですが、
IsHWNDTagにNULLを渡す方法がわからず、タグジャンプを動かすことを優先にして
そのままにしていました。

IsHWNDTagでは3つの引数があるときは2番目の引数を使っていないようなので、
そのように書き換えると、wstringにすることができました。

[beru]

@dep5 さん、更新ありがとうございます。commit履歴が見やすくなるようにrebaseしてもらう事は可能ですか？

バージョン番号はコミットメッセージに含めているんですね。

動作確認等これから行っていきます。

[AppVeyorBot]

✅ Build sakura 1.0.3837 completed (commit 452602f795 by @dep5)

[beru]

@dep5 さん、このPRの変更でGrep画面で「編集中のファイルテキストから検索」にチェックを入れた場合に無効化されるコントロールが増えたように見えます。内部処理的の細かい変更はコードレビューで確認する事だと思いますが、ユーザー視点でどのように挙動が変わるのかについての解説をPRの説明でスクリーンショット付きで書いてくれると助かります。ただ面倒な場合は別にいいです…。

[beru]

「編集中のテキストから検索」にチェックを入れた場合に「検索場所」と「対象ファイル」のコンボボックスに表示されるテキストが (現在のドキュメント) に変化するようになりましたね。その状態で「上階層へ」ボタンや「現フォルダ」ボタンを押すと「検索場所」のコンボボックスに表示されるテキストが変わってしまいます。この挙動は元からそうなんですがなんか変ですね。「編集中のテキストから検索」にチェックを入れた場合には「上階層へ」ボタンと「現フォルダ」ボタンも無効化した方が良いのかもと思いました。どうでしょうか？

[AppVeyorBot]

✅ Build sakura 1.0.3838 completed (commit 452602f795 by @dep5)

[dep5]

beruさん
「上階層へ」「現フォルダ」ボタンがこんな動きをするとは初めて知りました。
ありがとうございます。さっそくグレーアウトしました。
ついでにサブフォルダのチェックが外れてしまうのもグレーアウトだけにしましたが、
何か問題あるかもしれません。

「commit履歴が見やすくなるようにrebase」というのは現在のmasterをベースに最初からということですか？それとも一部でよいですか？
数字を振ってないものはほんとはrebaseしたかったのでやりたいです。
このcommitはひとまとめでよいというアドバイスありましたら教えて下さい。

[AppVeyorBot]

✅ Build sakura 1.0.3841 completed (commit 8f6b324244 by @dep5)

[dep5]

適用後

適用前

現在のパスが表示されていた2項目が(現在のドキュメント)に
除外フォルダ・ファイルが空白に

今回
「上階層へ」「現フォルダ」
「サブフォルダも検索」の選択状態を保つように

そのうえで以上の項目がグレーアウトします

またGrep置換では「編集中のテキストから検索」が
最初からグレーアウトして操作できなくしてあります。

[dep5]

beruさん
https://github.com/dep5/sakura/compare/grepthis-r
最新masterで作り直してコミットもいくつかまとめてみましたがどうでしょうか？
新規にプルリクエストしてもよい気がしてきましたが、rebaseした方がよいですか？

[beru]

https://github.com/dep5/sakura/compare/grepthis-r
最新masterで作り直してコミットもいくつかまとめてみましたがどうでしょうか？
新規にプルリクエストしてもよい気がしてきましたが、rebaseした方がよいですか？

新規にPRを作成したかったらそれでも良いと思いますが、その場合このPRと内容が被るのでこのPRはCloseする事になりそうですね。念のためにお伝えしますが git rebase した後はコミットのSHA-1が変わるのでremoteに通常の git push は出来ないので、git push --force する必要が有ります。conflict が発生している場合の rebase は慣れないうちは少しややこしいので、面倒であれば rebase ではなくて新規にPR作成でも全然問題無いと思いますよ。

[beru]

@dep5 さん、grepthis-r ブランチ（54b534504e73c8d577a2c6853c1e6006ad995018）を動作確認してみました。

「編集中のテキストから検索」のチェックボックスを付けると下記のコントロールの表示内容が設定されて無効化でグレーアウトになります。そしてチェックを外すと元の状態に戻ります。

• 検索場所
• 対象ファイル
• 除外ファイル
• 除外フォルダ

チェックを付けたてから外すと元々設定されていたテキストに戻す動作は良いと思います。

気になるのは実際に検索を実行していないのにチェックを付けた時点のコントロールのテキストが記憶されてしまう事です。記憶されるというのは、ESCキーを押してGrepダイアログを閉じてから再度 Ctrl + G キーを押してGrepダイアログを開いた場合に前回設定されていたテキストが復帰される事を指します。

元の挙動はどうだったか master ブランチ (c1568de) に切り替えて挙動を確認してみたところ「編集中のファイルから検索」のチェックを付けると「検索場所」と「対象ファイル」のコントロールが無効化されますがその際に「検索場所」には変更前のテキストを設定して「対象ファイル」のテキストには現在開いているファイルのファイル名が設定される挙動でした。チェックを外すと「検索場所」と「対象ファイル」のコントロールが有効化されますが「対象ファイル」のテキストは元々設定されていた *.* には戻らずに現在開いているファイルのファイル名が設定されたままです。これはこれで良くない動作ですね。

[beru]

@dep5 さん #1639 (comment) でスクリーンショットを貼ってくれてありがとうございます。「適用前」と「適用後」の画像が反対ですかね？

[dep5]

スクリーンショットの文字を修正しました。
ほかに投稿したパッチをまとめて当てた際にエラーが出るようになったので、対応する間しばらくこのPRでこのまま続けさせてください。
Escで抜けるとこんな動きをするんですね。
除外関連はさわらないので気付きませんでした。
ほかの方法がありそうですが、とにかく対応してみました。
どうでしょうか？

[sonarcloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
4 Security Hotspots
39 Code Smells

0.4% Coverage
0.0% Duplication

[AppVeyorBot]

✅ Build sakura 1.0.3843 completed (commit 30ef3727d1 by @dep5)

[dep5]

PRからパッチをダウンロードして複数のパッチを当てた時に、
RC2104 undefined keyword or key name: sakura_rc.rc 90
と出るようになってビルドできなくなりました。
エラーの起きたキーワードも空白だし、該当行を見てもヒントになりません。

前回まではgit amでパッチを当てていたのをgit applyにしたためのようです。

コミット日時での前後情報がうまく処理されないのでしょうか？
UTF16のはずのsakura_rc.rcがUTF8になっていました。
これをUTF16で保存し直すとビルドできました。

パッチの問題ではなさそうなので、Security Hotspotsに対応して新規のPRにしました。
#1696

コメント・アドバイスいただいた方ありがとうございました

[beru]

スクリーンショットの文字を修正しました。

修正確認しました。自分は画像を交換する事しか思いつかなかったですが、スクリーンショットの見出しの文字を交換で対処する方法も有りですね。

ほかに投稿したパッチをまとめて当てた際にエラーが出るようになったので、対応する間しばらくこのPRでこのまま続けさせてください。
Escで抜けるとこんな動きをするんですね。
除外関連はさわらないので気付きませんでした。
ほかの方法がありそうですが、とにかく対応してみました。
どうでしょうか？

#1696 のブランチ dep5:grepthis-r d46ff92 で動作確認しましたが、#1639 (comment) に書いた現象は変わらずでした。

[beru]

PRからパッチをダウンロードして複数のパッチを当てた時に、
RC2104 undefined keyword or key name: sakura_rc.rc 90
と出るようになってビルドできなくなりました。
エラーの起きたキーワードも空白だし、該当行を見てもヒントになりません。

前回まではgit amでパッチを当てていたのをgit applyにしたためのようです。

PRからパッチをダウンロードって何でしょうか？自分はやった事が無い操作かも。。

コミット日時での前後情報がうまく処理されないのでしょうか？
UTF16のはずのsakura_rc.rcがUTF8になっていました。
これをUTF16で保存し直すとビルドできました。

使っているgitクライアントの名前とバージョンはなんでしょうか？

なお自分が使っているのは下記のものです。

TortoiseGit 2.12.0.0
git version 2.29.2.windows.2

パッチの問題ではなさそうなので、Security Hotspotsに対応して新規のPRにしました。
#1696

今後はそちらを確認する事にします。

[beru]

CDlgGrep::OnBnClicked において

if (::IsDlgButtonChecked(GetHwnd(), IDC_CHK_FROMTHISTEXT)) {

の条件判定は無い方が動作がより自然になると思いますがどうでしょうか？

[beru]

@dep5 さん、この if の判定は不要だと思います。

この判定がある事で下記の操作を行うと不自然な挙動になります。

操作内容	スクリーンショット
Ctrl + G キーを押してGrepダイアログを表示します。
「対象ファイル」のテキストを *.* から *.*あああ に変更します。
「編集中のテキストから検索」にチェックを入れます。
「編集中のテキストから検索」のチェックを外します。
Escapeキーを押してGrepダイアログを閉じます。
再度 Ctrl + G キーを押してGrepダイアログを表示すると「対象ファイル」のテキストが *.*あああ のままになっています。

自分が正しいと思う挙動についてですが、

• Grepダイアログを開いてから検索条件を変更する
  • 検索を実行してからGrepダイアログを閉じて再度Grepダイアログを開いた場合は、検索前にコントロールに設定していた検索条件が復帰される
  • 検索を実行せずにGrepダイアログを閉じて再度Grepダイアログを開いた場合は、検索条件を変更する前の検索条件がコントロールに設定される