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.

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

PatchUnicode-1051 現在のドキュメント内容をGrep(2) #1696

Merged
merged 15 commits into from Jul 4, 2021

Conversation

dep5
Copy link
Contributor

@dep5 dep5 commented Jun 20, 2021

PR の目的

#1639 のコミットを整理しました。

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置換
機能をオフにしてあります。

PR の影響範囲

テスト内容

テスト1

手順

関連 issue, PR

#1639

参考資料

@AppVeyorBot
Copy link

Build sakura 1.0.3844 completed (commit 80a1835c03 by @dep5)

@beru beru added enhancement ■機能追加 specification change ■仕様変更 🐛bug🦋 ■バグ修正(Something isn't working) labels Jun 20, 2021
@berryzplus
Copy link
Contributor

結局 Moca さんが無関係なフランス人を指している件を突っ込んどきます 😃

@beru
Copy link
Contributor

beru commented Jun 22, 2021

@dep5 さん、こちらのPRのブランチにも 1f088c1 を cherry-pick で追加お願いします。

@dep5
Copy link
Contributor Author

dep5 commented Jun 23, 2021

berryzplus さん
#1639 (comment)
でいただいた案でコミットしたつもりですので確認してみてください

beru さん
コミットの案を見てもらった後、ダイアログをキャンセルした場合の処理を追加してSecurity HotSpotにも対応して
このPRにしています。期待通りに動いてますか?

#1639 の続きですが
複数でなくても新規のブランチでも起きました。
テスト用にブランチを作成して
https://github.com/sakura-editor/sakura/pull/1696.patch
ブラウザでパッチの当てやすい場所へ保存して
git apply 1696.patch
sakura_rc.rc がUTF8になっています。
git am 1696.patch
だと問題ないです。

使用したバージョンはこれです
https://github.com/git-for-windows/git/releases/download/v2.32.0.windows.1/PortableGit-2.32.0-64-bit.7z.exe

https://github.com/git-for-windows/git/releases/tag/v2.29.2.windows.2
でも試しましたが、git apply だとUTF8になってしまいました。

わたしの使い方がおかしいかもしれないですね。

@beru
Copy link
Contributor

beru commented Jun 23, 2021

beru さん
コミットの案を見てもらった後、ダイアログをキャンセルした場合の処理を追加してSecurity HotSpotにも対応して
このPRにしています。期待通りに動いてますか?

163bf04 のコミットに、前のPRのコミット 1f088c1 の内容は含まれていますね。自分の確認間違いでした。コミットメッセージだけを見て変更が移行されたかを確認していました。

さて動作確認してみましたが期待通りの動作ではありませんでした。現象としては #1639 (comment) に書いたものと同様です。

#1639 の続きですが
複数でなくても新規のブランチでも起きました。
テスト用にブランチを作成して
https://github.com/sakura-editor/sakura/pull/1696.patch
ブラウザでパッチの当てやすい場所へ保存して
git apply 1696.patch
sakura_rc.rc がUTF8になっています。
git am 1696.patch
だと問題ないです。

使用したバージョンはこれです
https://github.com/git-for-windows/git/releases/download/v2.32.0.windows.1/PortableGit-2.32.0-64-bit.7z.exe

https://github.com/git-for-windows/git/releases/tag/v2.29.2.windows.2
でも試しましたが、git apply だとUTF8になってしまいました。

わたしの使い方がおかしいかもしれないですね。

そもそも何故 patch ファイル経由で操作してるんでしょうか?git client の機能で別のブランチのコミットを取り込んだり出来るのに…。

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

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

CDlgGrep::OnBnClicked において

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

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

@@ -557,6 +558,20 @@ BOOL CDlgGrep::OnBnClicked( int wID )
return TRUE;
case IDCANCEL:
// ::EndDialog( hwndDlg, FALSE );
if (::IsDlgButtonChecked(GetHwnd(), IDC_CHK_FROMTHISTEXT)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

そもそもどうしてこういう挙動になるのかという話ですが、CEditWnd クラスのメンバーとして CDlgGrep 型のインスタンス m_cDlgGrep が存在しています。なのでGrepダイアログを閉じても再度Grepダイアログを開いた時にそのインスタンスが使いまわされます。そのため、CDlgGrep のメンバー変数 m_szFile, m_szFolder 等に設定されている内容はダイアログを閉じる前の状態が保持されます。その為に過去の値が再度Grepダイアログを開いた際に、CDlgGrep::OnInitDialogCDialog::OnInitDialogCDlgGrep::SetData の順に処理が呼び出されて、CDlgGrep::SetData メソッドにおいて、CDlgGrep のメンバー変数 m_szFile, m_szFolder 等の値が DlgItem_SetText 関数でコントロールに設定されます。

@beru
Copy link
Contributor

beru commented Jun 23, 2021

#1639 の続きですが
複数でなくても新規のブランチでも起きました。
テスト用にブランチを作成して
https://github.com/sakura-editor/sakura/pull/1696.patch
ブラウザでパッチの当てやすい場所へ保存して
git apply 1696.patch
sakura_rc.rc がUTF8になっています。
git am 1696.patch
だと問題ないです。

使用したバージョンはこれです
https://github.com/git-for-windows/git/releases/download/v2.32.0.windows.1/PortableGit-2.32.0-64-bit.7z.exe

https://github.com/git-for-windows/git/releases/tag/v2.29.2.windows.2
でも試しましたが、git apply だとUTF8になってしまいました。

わたしの使い方がおかしいかもしれないですね。

@dep5 さん、.gitattributes ファイルに拡張子が .rc のファイルの文字エンコーディングが utf-16le-bom になるように設定されているので、通常 git pull 等でブランチを取り込めば sakura_core/sakura_rc.rc ファイルの文字エンコーディングが UTF8 になる事はないと思います。

git pullgit merge, git rebase で coflict が発生した場合にそれを解消する必要がありますが、その場合3者間の差分を見る必要があるので 3-way merge に対応している diff の表示と編集を行えるソフトがある方が便利です。

gitのサブコマンドは色々とあって慣れていないとややこしいです。ちなみに自分は TortoiseGit と WinMerge というソフトに慣れていて、大体それを使って rebase 等の操作をしています。patchファイルは殆ど使いません。

なおVS Codeにも標準でGitの機能があります。自分はそれにあまり慣れてなくて複雑な操作は行えませんが、簡単な事をする分には結構お手軽ですよ。

@AppVeyorBot
Copy link

Build sakura 1.0.3847 completed (commit f93e048a59 by @dep5)

@AppVeyorBot
Copy link

Build sakura 1.0.3848 completed (commit 5f35047edc by @dep5)

@dep5
Copy link
Contributor Author

dep5 commented Jun 24, 2021

beruさん
「編集中のテキストから検索」にチェックを入れた時点で文字列が保存されることも確認していたのですが、
チェックを入れた時点でダイアログを閉じてテストしていたので気付きませんでした。
「編集中のテキストから検索」を使わない人には再設定はいらないなと思って、ifで囲んだのですが、
条件が違いましたね。今回は条件を変更してみました。

#1639 (comment) で全部説明してもらっているのにすいません。

わたしもその挙動に賛成です。
「実行する(検索)」「実行しない(キャンセル・×・Esc)」のほかにも
Grepを実行した文書でもう一度Grepダイアログを開いた場合と
別の文書でGrepダイアログを開いた場合で挙動が異なっていますね。

ソフト起動後初めて開いた文書では「編集中のテキストから検索」のチェックが外れています。
特別な機能なので基本はOFFということで理解しています。
・・・ということも #1696 (comment) で説明してくれてますね。
そういう仕組みで動いているんですね。

git commitすると元に戻すのが大変ですが
git applyだと git checkout .で戻って楽なので使っていました。
でもパッチ内容によるようですね。
WinMerge便利ですよね。今回のPR作成の時にも使っています。
VS Codeも使っていますが重いので、場合によってはブラウザを終了しないといけないです。

#1696 (comment)
タイトルを修正するつもりがビルドチェックを止めてしまったみたいです。
どうしたらいいでしょうか。

nGrepTreeResult = -1;
break;
if( hWndTarget ){
for( HWND hwnd = hWndTarget; NULL != hwnd; hwnd = NULL ){
Copy link
Contributor

Choose a reason for hiding this comment

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

ここって1回しか回らないように見えますがどうしてforループになっているんでしょうか?

@beru
Copy link
Contributor

beru commented Jun 24, 2021

「編集中のテキストから検索」を使わない人には再設定はいらないなと思って、ifで囲んだのですが、
条件が違いましたね。今回は条件を変更してみました。

確認しました。メンバ変数 m_bSelectOnceThisText を増やしてそれで判定するようになっていますね。キャンセル時に無条件で処理しては駄目で何かしらの判定が必要な理由は自分には分かりませんが、問題の挙動は解消されたように見えます。

「実行する(検索)」「実行しない(キャンセル・×・Esc)」のほかにも
Grepを実行した文書でもう一度Grepダイアログを開いた場合と
別の文書でGrepダイアログを開いた場合で挙動が異なっていますね。

おぉ、そうだったんですね。ケースが色々なので確認が大変ですね。。

git commitすると元に戻すのが大変ですが
git applyだと git checkout .で戻って楽なので使っていました。
でもパッチ内容によるようですね。

自分は作業ブランチにはどんどんコミットは気軽に入れてます。amend で上書き出来るし後から複数のコミットを1コミットに統合したりは気軽に行えるので。変更前の状態に戻したい時は別のブランチに切り替えています。

WinMerge便利ですよね。今回のPR作成の時にも使っています。
VS Codeも使っていますが重いので、場合によってはブラウザを終了しないといけないです。

WinMergeは既にお使いでしたか、失礼しました。なおあれのテキストエディト部分は Crystal Edit (link) のクラスが使われています。サクラエディタに比べるとちょっと機能不足ですが、それでも素のエディトよりはずっと良いですね。

#1696 (comment)
タイトルを修正するつもりがビルドチェックを止めてしまったみたいです。
どうしたらいいでしょうか。

うーん、分からないです。ログインしてページを見てましたが Retry 的なボタンは無いですね。
まぁ Code Smell で挙げられている点のうち dep5 さんが気になる点があれば対応するのが良いと思います。

@beru
Copy link
Contributor

beru commented Jun 24, 2021

気になる点として空の文書で Ctrl + G キーを押してGrepダイアログを開いた後に「編集中のテキストから検索」にチェックを入れて、適当に「条件」に あああ と入力してから検索を行うと、下記のような検索結果になります。


□検索条件  "あああ"
検索対象       Window:[ 【Grep】"あああ"]
フォルダ       
除外ファイル   
除外フォルダ   
    (サブフォルダも検索)
    (英大文字小文字を区別しない)
    (正規表現:bregonig.dll Ver.4.20 with Onigmo 6.2.0)
    (文字コードセットの自動判別)
    (一致した行を出力)


:HWND:[0000000000070794](Grep)(2,9)  [UTF-8]: □検索条件  "あああ"
:HWND:[0000000000070794](Grep)(3,28)  [UTF-8]: 検索対象       Window:[ 【Grep】"あああ"]
2 個が検索されました。

空の文書ではなく適当に あいうえお というテキストを文書に入力してからGrepダイアログで同じ操作を行うと下記の結果になります。


□検索条件  "ああああ"
検索対象       Window:[(無題)1  [UTF-8]]
フォルダ       
除外ファイル   
除外フォルダ   
    (サブフォルダも検索)
    (英大文字小文字を区別しない)
    (正規表現:bregonig.dll Ver.4.20 with Onigmo 6.2.0)
    (文字コードセットの自動判別)
    (一致した行を出力)


0 個が検索されました。

という事でまっさらの空の文書で「編集中のテキストから検索」にチェックを入れて検索を行うと、何故かこれから開くGrep結果の文書から検索してしまう動作になっているんでしょうか?

@dep5
Copy link
Contributor Author

dep5 commented Jul 2, 2021

beruさん
置き換え元がforループだったのを踏襲したのでしょうかね?
他にも理由があるのかもしれませんが・・・
書き直すべきなら書き直してみます。

空の文書での挙動ですが、「編集中のテキストから検索」をチェックしないで通常のGrepを行うと
新規文書で未編集の場合はGrepウィンドウとして再利用されて、そのまま結果が表示されます。
編集した状態なら新規にGrepウィンドウが開かれてそこに結果が表示されます。
そして、「編集中のテキストから検索」をチェックすると空の文書がGrepウィンドウになってしまって
『Grepの自分自身を指定することもできます。 (ヘッダで出力された検索キーワードが引っかかってしまう制限付き) 』https://sourceforge.net/p/sakura-editor/patchunicode/1051/
ということのようです。

空の文書に対して「編集中のテキストから検索」を行ってテストしていたので、おかしいと思っていませんでしたが、
Grepできなくしたほうがいいですか?

@beru
Copy link
Contributor

beru commented Jul 2, 2021

@dep5 さん

置き換え元がforループだったのを踏襲したのでしょうかね?
他にも理由があるのかもしれませんが・・・
書き直すべきなら書き直してみます。

こちらはMocaさんのパッチの記述が元からこうなっているんですね。
どうしてこういう書き方にしたのかはともかく挙動としては問題ないと思います。
なので書き直しは今は不要だと思います。

空の文書での挙動ですが、「編集中のテキストから検索」をチェックしないで通常のGrepを行うと
新規文書で未編集の場合はGrepウィンドウとして再利用されて、そのまま結果が表示されます。
編集した状態なら新規にGrepウィンドウが開かれてそこに結果が表示されます。
そして、「編集中のテキストから検索」をチェックすると空の文書がGrepウィンドウになってしまって
『Grepの自分自身を指定することもできます。 (ヘッダで出力された検索キーワードが引っかかってしまう制限付き) 』https://sourceforge.net/p/sakura-editor/patchunicode/1051/
ということのようです。

確かに元から通常のGrepを空の文書で行うとその文書がGrepウィンドウとして再利用されてそのウィンドウに結果が表示される動作ですね。

その実装をベースにしているので「編集中のテキストから検索」にチェックを入れて空の文書でGrepを行うと、同様にその文書がGrepウィンドウとして再利用されてそのウィンドウに結果が表示される動作になるという事ですね。

副作用としてヘッダで出力された検索条件や検索対象からもGrepしてしまうと。。

空の文書に対して「編集中のテキストから検索」を行ってテストしていたので、おかしいと思っていませんでしたが、
Grepできなくしたほうがいいですか?

自分はこの挙動は不自然だなと感じましたが、元のパッチの動作という事でこのPRではこのままで良いと思います。

もしこの動作はおかしいよという意見がどんどん出てきた場合は後で別のPRで修正を入れたほうが良さそうですね。

しかし差分の量がとても多いのでコードの挙動が色々なケースで問題が無いかについては追い切れなそうです。dep5さんはよくまとめ上げたなと思います。すごいですね。。

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

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

動作確認してみて問題無いと思います。

@beru
Copy link
Contributor

beru commented Jul 2, 2021

他の方から数日中に Request changes が入らなければ Merge したいと思います。

@berryzplus
Copy link
Contributor

中身見てる余裕がないのでsonarcloudの再実行だけしときます。

Please retry analysis of this Pull-Request directly on SonarCloud.

CodeFactorの警告は、いつも通り見当違いなのでスルーでよいと思っています。
旧パッチの適用なので、SonarCloudの指摘もBugs以外はスルーでよいと思います。

@sonarcloud
Copy link

sonarcloud bot commented Jul 3, 2021

@berryzplus
Copy link
Contributor

Code Smell A 40 Code Smells

「旧パッチ適用だから」、おっけーっす 😄

@beru
Copy link
Contributor

beru commented Jul 4, 2021

@berryzplus さん、sonarcloudの再実行と確認ありがとうございます。

Mergeします。もし何か問題が見つかったら別のPRで対処しましょう。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working) enhancement ■機能追加 specification change ■仕様変更
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants