sakura icon indicating copy to clipboard operation
sakura copied to clipboard

指定した検索条件を履歴に残したい (PatchUnicode-1046 各履歴のお気に入り)

Open dep5 opened this issue 3 years ago • 57 comments

PR の目的

何度も検索すると検索条件が検索履歴から消えてしまいます。 そうならないように、また使いたい検索条件を保護するために

Mocaさん作の PatchUnicode-1046 各履歴のお気に入り  を適用します

カテゴリ

  • 機能追加
  • 仕様変更

PR の背景

検索や置換の履歴がいっぱいになると必要な検索語句も履歴からいずれ消えてしまいます。 履歴に残すために、お気に入り機能を使います

PR のメリット

  • 使いたい検索条件が履歴からあふれて消えてしまう
  • 正規表現を試行錯誤していると、期待通りに動く検索条件がどれかわからなくなる
  • 非常に長い検索条件で末尾が確認できなくてわからなくなる。

お気に入りに入れることでそれを検索履歴に残し続けます。 検索、置換、GREPファイル、GREPフォルダ、コマンド、カレントディレクトリ 以上のタブで同様のことが可能です。

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

Grepダイアログには「除外ファイル」、「除外フォルダ」もありますが これには対応していません。 オリジナルで対応しているものだけです。

お気に入りに指定できるのは25個までです。 コマンド、カレントディレクトリは27個までです。

お気に入りに指定した分だけ履歴として使える個数は減るので注意してください。

仕様・動作説明

ファイル・フォルダにはお気に入りに入れることで履歴に残すことができましたが、 この機能を検索・置換・GREPファイル・GREPフォルダ・コマンド・カレントディレクトリも対象にします。 ファイル・フォルダ以外でもお気に入り機能が使えます

PR の影響範囲

履歴の最大数と同じだけお気に入りにすると、問題があるという指摘に対応するために ダイアログでチェックを入れられるのは検索条件は25個までにしています。

テスト内容

テスト1

手順

関連 issue, PR

参考資料

https://eternalwindows.jp/control/listview/listview07.html

適用前 before 適用後 after チェックは合計25個まで

dep5 avatar Apr 22 '21 13:04 dep5

:white_check_mark: Build sakura 1.0.3696 completed (commit https://github.com/sakura-editor/sakura/commit/0d85756bbd by @dep5)

AppVeyorBot avatar Apr 22 '21 14:04 AppVeyorBot

試していないで書き込みますが、外部コマンド実行か何かで、履歴の1番目をアクティブな変数として使用しているコードがあったような気がします。 その状態で履歴をすべてお気に入りにすると、新しく追加できなくなり、現在アクティブのものが追加されずに、履歴のトップが使われてしまう、という問題がある「かも」しれないです。

usagisita avatar Apr 22 '21 16:04 usagisita

ファイル履歴で使える「お気に入り」の機能を、 「検索キーワード」と「置換文字列」にも適用したい ということでしょうか? (Grep系の4項目が対象?Clearってどこから出てきたの?)

現状の説明では、何をどうしたいのかがほとんど読み取れないので、そこを教えてほしいです。

既存パッチ取り込みとしてやるから焦点がボケるんでは?と思いました。

sanomari avatar Apr 22 '21 23:04 sanomari

:white_check_mark: Build sakura 1.0.3700 completed (commit https://github.com/sakura-editor/sakura/commit/3bbac4eafc by @dep5)

AppVeyorBot avatar Apr 23 '21 13:04 AppVeyorBot

:white_check_mark: Build sakura 1.0.3701 completed (commit https://github.com/sakura-editor/sakura/commit/97c52304aa by @dep5)

AppVeyorBot avatar Apr 23 '21 13:04 AppVeyorBot

#1639 でアドバイスをいただいた、誤ったAuthorに関連付けされていたのを直しました。

dep5 avatar Apr 23 '21 13:04 dep5

sanomariさん 繰り返し検索をして履歴があふれて必要な検索キーワードが消えてしまったり、 しばらくたってからあの検索キーワードなんだっけ、 となって困ったことはないでしょうか。 わたしは以前検索を繰り返すマクロなどを使っていてたびたび失敗してきましたので 困っていたところこのパッチを見つけて使ってきました。

それからは本体側のコード変更を追いかけていただけなので、現在必要かどうかわかりません。 Grep除外ファイル・フォルダについては途中まで書いてみましたが、動かなかったので反映していません。

使用例です。 正規表現を使っているなら、コメントで名前を付けるとわかりやすいです fav dialog

dep5 avatar Apr 23 '21 14:04 dep5

usagisitaさん 履歴のトップが使われてしまう、という問題がある「かも」というのは 入力した文字が無視されるかも、ということでしょうか。 sakura_core/dlg/CDlgExec.cppのことでしょうか。

外部コマンド実行・ファイル名を指定して実行で あふれるまで履歴を埋めてすべてお気に入りにして、新たにコマンドを入力して実行してみましたが、 入力したコマンドが実行されました。 しかし、確かに履歴に新しく追加できなくなりました。 こういう問題もあるんですね。

dep5 avatar Apr 23 '21 15:04 dep5

dep5さん、困っていることは(少なくとも自分は)わかりました。 やりたいことも「Grepの履歴をお気に入りに登録しておきたい」ということだと自分は理解しています。

こんなことを書ける立場じゃないとは思いましたが、 #1622 や #1639 でも感じられた、次の点を指摘しておきます。

  1. PRの背景に挙げられているのは変更による「メリット」だと思います。
  2. PRのメリットに挙げられているのは「背景(=変更したい理由)」だと思います。
    • https://github.com/sakura-editor/sakura/pull/1640#issuecomment-825691307 も「背景」の詳細説明ですね。
  3. パッチの変更内容を説明する必要があると思います。
  4. 古いパッチを無変更で取り込めるとは思えません。パッチに対して加えた変更、もしくは取り除いた変更などがあれば説明してください。
  5. この変更で困っていることが解消したことを、どうすれば確認できるか説明してください。
  6. この変更で新たに別の問題が発生するかもしれませんので、変更の影響を受ける機能などがあれば列挙してください。

https://github.com/sakura-editor/sakura/pull/1640#issuecomment-825721142 こういうこともあるので、単純な取り込みはできないはずです。

ghost avatar Apr 23 '21 15:04 ghost

オリジナルからの変更内容は以下の通りです

v0.2 バージョン番号を最新に変えた v0.3 #1255 で追加された入力欄の下矢印で出てくるリストから Deleteキーで直接履歴を消す機能でもお気に入りが効くようにした

dep5 avatar Apr 24 '21 12:04 dep5

パッチを適用したビルドを数年使用してきて便利なので投稿してみただけですので あまり詳しいことはわかりませんが少し書き直してみました。

dep5 avatar Apr 24 '21 12:04 dep5

パッチを適用したビルドを数年使用してきて便利なので投稿してみただけですので あまり詳しいことはわかりませんが少し書き直してみました。

ようやく分かってきました。そういうこと(c++開発者ではない)っすね。 プログラムは本来、英語が読めればなんとなく読み解けるものですよね。 c/c++に詳しい人ばかり相手にしていてそのことを忘れていました。

それなら、誰かが引き継いで機能取込までもっていく方向ではどうでしょうか?

現状が既に実績のあるマージコードであってもベースが色々変わってるので、追加の修正やら確認がいると思っています。

berryzplus avatar Apr 25 '21 05:04 berryzplus

パッチを適用したビルドを数年使用してきて便利なので投稿してみただけですので あまり詳しいことはわかりませんが少し書き直してみました。

ようやく分かってきました。そういうこと(c++開発者ではない)っすね。 c/c++に詳しい人ばかり相手にしていてそのことを忘れていました。

この点気が付けなかったです。申し訳ありませんでした。

ghost avatar Apr 25 '21 05:04 ghost

dep5さん、誤った対応をしてしまい申し訳ありませんでした。

Grepの除外ファイル・フォルダには対応できていません

軽く調べたところ、これらの項目は「履歴とお気に入りの管理ダイアログ」でユーザーが管理することはできませんが、履歴自体は内部で持っています。 これらの項目の中からお気に入りに設定できるようにするには、ダイアログ項目の追加が必要になりそうです。

ghost avatar Apr 25 '21 06:04 ghost

kazasakuさん わかりやすい書き方教えてもらってよかったです。ありがとうございました。

この機能を追加してもよいとなったら、あとはできたらみなさんにおまかせしたいです。

dep5 avatar Apr 25 '21 12:04 dep5

履歴の性質が異なるので、単純に「お気に入り」を有効にする対応ではマズそうに思います。

項目 追加タイミング 削除タイミング
ファイル履歴 ファイルを開いたとき(開いた後に登録しようとする) 件数超過で古いものから削除、お気に入りダイアログで削除(一度お気に入り登録したものはお気に入りダイアログ以外で削除できなくなる)
検索履歴 検索するとき(検索する前に登録し、登録した検索条件で検索する) 件数超過で古いものから削除、お気に入りダイアログで削除、検索コンボで削除

ファイル履歴は、新しく追加できなくなっても困らないです。新しい履歴が登録できないだけなので。 検索履歴は、新しく追加できなくなると困ります。新しい検索条件で検索できなくなるからです。 人によっては困らないかも知れませんが、普通は困るような気がします。

取込しようとしているパッチでこのあたりの仕様を考慮できているようには見えないので、 仕様変更提案は「一旦却下」が妥当かと思います。

berryzplus avatar Apr 26 '21 23:04 berryzplus

sakura.iniを編集して、MAXの履歴をすべてお気に入りに入れた状態でテストしてみました。 コマンド・カレントディレクトリについては最大32個・そのほかは30個設定できるようです。 Grep,Grep置換,検索,置換,外部コマンド実行について、 それぞれのダイアログを表示して入力して操作する分には通常通りに動きます。 Grep,Grep置換,検索,置換については同じタブやウインドウなら、もう一度ダイアログを表示すると、 同じ条件で検索・置換などができました。 外部コマンド実行については入力して実行自体は可能ですが 再度ダイアログを表示すると条件が2つともリセットされるようです。

しかし、ツールバーに追加した検索ボックスを使った場合は 入力した文字が無視されて履歴で検索されてしまって検索ができなくなりますね。 これがusagitaさんの指摘だと思います。

履歴に追加されなくても実行できるなら、それでいいかなと思っていましたが これは困りますね・・・

dep5 avatar May 01 '21 12:05 dep5

「履歴とお気に入りの管理」でしかお気に入りは設定できないのだから 削除もこのダイアログでしかできないようにするのがいいかなと思いましたが コンボボックスから簡単に削除できる機能があったほうがいいのでrevertしました。

dep5 avatar May 01 '21 13:05 dep5

:white_check_mark: Build sakura 1.0.3720 completed (commit https://github.com/sakura-editor/sakura/commit/9fb1b4f07c by @dep5)

AppVeyorBot avatar May 01 '21 13:05 AppVeyorBot

取込しようとしているパッチでこのあたりの仕様を考慮できているようには見えないので、 仕様変更提案は「一旦却下」が妥当かと思います。

これ、要するに保持可能な履歴の全件をお気に入りにできたらマズい、というだけの話なので、 全件をお気に入りにすることが不可能なように対策入れたらよい気がしています。

対策を入れるべき箇所

  • 設定読み込み処理(=CShareData_IOの該当関数) 不正な設定値を無視するコードを入れる
  • UI(=お気に入りダイアログ) 不正な設定値を保存できないようにする

対応が難しい、ないし、「めんどくさい」であれば、やはり「お蔵入り」で確定が妥当です。 何が妥当?、何が適切?の論議はおいておいて、とりあえず入れてしまってある既存実装は他にもあるので、 「入れてしまえ!」で良い人がいればそれもアリだと思います。

berryzplus avatar May 03 '21 10:05 berryzplus

保持可能な履歴の全件をお気に入りにできたらマズい

そういえば、お気に入り件数の上限って設定されていませんね。 故に「記憶可能な履歴件数 = 設定可能なお気に入り件数」になっている気がします。 MAX_FAVORITE_ITEM_COUNTを追加して上限値を分離すればいいんですかね。

ghost avatar May 03 '21 10:05 ghost

Code Smell

std::arrayならstd::array::fillで初期化できそうなので、初期化用のstatic関数を追加せずに済むと思いました。

追記:初期化用のstatic関数は不要だという指摘が来ましたね。

ghost avatar May 03 '21 11:05 ghost

:white_check_mark: Build sakura 1.0.3826 completed (commit https://github.com/sakura-editor/sakura/commit/8c5934f6e9 by @dep5)

AppVeyorBot avatar Jun 07 '21 13:06 AppVeyorBot

このプルリクエストは履歴というフォーマットを使って履歴とお気に入りで 排他的に利用する機能だと理解しているのですが、いかがでしょうか?

ユーザーが理解して使っている限りは履歴をあきらめて全件お気に入りにするのも ユーザー次第なのかなと思います。

kazasakuさん お気に入りの最大数を設定してみました。 履歴の最大は30か32なので決め打ちするなら値は一つでもいいかなと思います。 30のお気に入りを設定すると検索できなくなる検索ボックスのことを考えると最大は29ですが、 berryzplusさんのおっしゃるように本来は履歴として使うべきなのだから そのことも考えて15から25くらいがいいでしょうか?

共通設定-全般-履歴から履歴MAXを設定できるので「ファイル」「フォルダ」に関しては除外しています

履歴管理のダイアログでしかお気に入りは増やせず、 ここ以外ではお気に入りを減らすことしかできないので 履歴管理のダイアログを閉じる際に注意を促すのがよいかな、と思ってそうしています。 上に書いた通り、注意を促すだけです。

29を設定できないように注意を促す値と禁止する値があってもよいかなとは考えています

最大お気に入り数はテスト用に3にしたままになっています ご意見いただければ、その数字に変えようと思います。 この方向でよければ、値などはほかのファイルに移動するつもりです

dep5 avatar Jun 07 '21 13:06 dep5

共通設定-全般-履歴から履歴MAXを設定できるので「ファイル」「フォルダ」に関しては除外しています

それはメニューに表示可能な数を指定するものであって、履歴データの上限を指定するものではありません。 ダイアログ側で「非表示」となっているエントリが、表示可能範囲外だが管理可能なエントリになります。 逆に言えば、お気に入り件数が表示可能な数を超えると、超えた分はメニューに表示されないので、この表示件数を実質的な上限として取り扱っても良いのではないかと思います。 ただ、ファイル・フォルダの履歴に上限を設定するなら、お気に入り件数が上限よりも多い環境にインストールされたときの扱いも考えないといけません。

この表示可能数に関する処理はCRecent(履歴データ管理の実働部)が取り扱っていますから、お気に入りの上限件数もこのクラスで扱った方が良さそうです。 (僕の方で先にCRecentを作り直して単体テストを仕込む作業をやるつもりでいます。)

あと繰り返しになりますが、Grep除外条件はどうしたいですか? 僕はいつもBash上でfind <場所> <対象とする条件> | xargs grep -E <探したい文字列> | lessと書くことが多くてサクラエディタのGrep機能を使っていませんが、findの条件にはたいてい特定ディレクトリの除外を指定しています。

ghost avatar Jun 07 '21 18:06 ghost

:white_check_mark: Build sakura 1.0.3830 completed (commit https://github.com/sakura-editor/sakura/commit/178b6e609d by @dep5)

AppVeyorBot avatar Jun 09 '21 13:06 AppVeyorBot

CodeSmellsを書きなおしてみましたが、 bool m_aSearchKeysFav[MAX_SEARCHKEY]; std::array<bool, MAX_SEARCHKEY> m_aSearchKeysFav ビルド失敗したので対応はやめておきます

appendを使うのがいいらしいと読んだのでAppendStringFが使えるCNativeWがよさそうと思ったのですが、 目的外使用だったんですね。std::wstringで書いてみましたが、どうですか?

履歴MAXは表示のみだったのですね。それに関しては考え違いでした。 https://github.com/sakura-editor/sakura/pull/1640#issuecomment-827212635 berryzplusさんのおっしゃるように やはり「ファイル」「フォルダ」は性質が違うし、 お気に入りの上限を変更するならユーザーさんのことを考えると増やすことしかできないと思うので 元から制限のなかった「ファイル」「フォルダ」は私は対象外にしたいです。

Grep除外条件についてですが、わたしも #1639 で追加した機能で使うことが多く、 除外関連はデフォルトのままでしか使ってないのでよくわかりません。 元のパッチをまねて途中まで対応しましたが、ダイアログの設定でうまくいきませんでした。 文字数的にタブが大きくなるし、タブのスクロールや共通設定のように多段タブなども使う必要があるかもしれません。 このPullRequestがマージされるかどうかわからないですし、 わたしの技量的にも編集量は少なめに済ませたいと思っていますので、Grep除外条件への対応はほかの方にお願いしたいと思います。

dep5 avatar Jun 09 '21 13:06 dep5

CodeSmellsを書きなおしてみましたが、 bool m_aSearchKeysFav[MAX_SEARCHKEY]; std::array<bool, MAX_SEARCHKEY> m_aSearchKeysFav ビルド失敗したので対応はやめておきます

参考までに。ビルドと簡単な動作確認は済んでいます。 https://github.com/kazasaku/sakura/commit/3c27512bd480459f30bc04be9fe630c215c8d788

除外関連はデフォルトのままでしか使ってないのでよくわかりません。

後から誰かがリクエストしてきたら考えます、ということで。

ghost avatar Jun 09 '21 22:06 ghost

1行書き換えればOKというわけにはいかないんですね。 fillはここに書くのか、と勉強になります。ぜひプルリクエストお願いします。

dep5 avatar Jun 10 '21 11:06 dep5