Apply feedback#1
Conversation
| return regex.test(string); | ||
| }; | ||
|
|
||
| function wildcardToRegexp(source) { |
There was a problem hiding this comment.
FlexConfirmMailの https://github.com/FlexConfirmMail/Outlook-Office-Addin/blob/main/src/web/wildcard-to-regexp.mjs から移植。このファイルはMPL 2.0で、本リポジトリのファイルもMPL2.0で公開予定なので問題ないはず。
| return false; | ||
| }, | ||
|
|
||
| getBrowserName(section) { |
|
|
||
| console.log(`* Lookup sections for ${urlToMatch}`); | ||
| for (const section of config.Sections) { | ||
| if (section.Name.toLowerCase() !== "edge") |
There was a problem hiding this comment.
設定ファイル（のパーサー）は基本的にThinBridgeのものを流用しており、設定ファイルには複数のセクションを想定している。また、将来的にChromeなどでも同様の処理が必要になった場合に、複数セクションが定義できた方が良い。そのため、セクションを一つにするのではなくて、複数のセクションの中のedgeセクションのみを参照するように変更。
There was a problem hiding this comment.
BROWSER定数があるので、それと比較する方がよいように思います。
There was a problem hiding this comment.
それはそうと、「POSTの再送信を禁止するWebページのURLパターンのリスト」をブラウザーごとに持つ必要性ってあるのでしょうか……？
ニーズ的にはむしろ「どのブラウザーを使っているときでも、列挙したパターンに該当するページはPOSTの再送信を禁止する」という振る舞いになってくれた方が嬉しいように思いますが、どうでしょう。
There was a problem hiding this comment.
BROWSER定数があるので、それと比較する方がよいように思います。
ThinBridgeにあったBROWSER_PATHのことで認識正しいでしょうか？
この設定値はRepostConfirmationCancelerでは消したので（ブラウザを起動することがないため）、存在しなくなっています。
There was a problem hiding this comment.
それはそうと、「POSTの再送信を禁止するWebページのURLパターンのリスト」をブラウザーごとに持つ必要性ってあるのでしょうか……？ ニーズ的にはむしろ「どのブラウザーを使っているときでも、列挙したパターンに該当するページはPOSTの再送信を禁止する」という振る舞いになってくれた方が嬉しいように思いますが、どうでしょう。
確かに、URLパターンのリストはグローバルな設定の方が嬉しそうな気がしますね。。。
There was a problem hiding this comment.
内部的には複数セクションを持てるようになっていて、ただし今バージョンではTARGETSという名前のセクションだけを使う、ということだと理解しました。
他にセクションを持つとしたら、ブラウザーごとに何か対応していくというよりも、「POSTの再送信を自動キャンセル」以外に「このダイアログも自動キャンセルしたい」みたいな要望に応えていく場合の方がありえそうかなと思いました。
この方向性で機能を増やしていくとしたら、今回の名前付けも、[TARGETS]ではなく、[NO_REPOST_TARGETS]のようにした方がよいかもですね。
There was a problem hiding this comment.
BROWSER定数があるので、それと比較する方がよいように思います。
ThinBridgeにあった
BROWSER_PATHのことで認識正しいでしょうか？ この設定値はRepostConfirmationCancelerでは消したので（ブラウザを起動することがないため）、存在しなくなっています。
これのことを指しておりました。
なるほど。理解しました。
（今回BROWSER変数ではないセクションにすることになったので、使わなくなりました。）
There was a problem hiding this comment.
内部的には複数セクションを持てるようになっていて、ただし今バージョンではTARGETSという名前のセクションだけを使う、ということだと理解しました。
はい、最終的にそのようにしました。
他にセクションを持つとしたら、ブラウザーごとに何か対応していくというよりも、「POSTの再送信を自動キャンセル」以外に「このダイアログも自動キャンセルしたい」みたいな要望に応えていく場合の方がありえそうかなと思いました。 この方向性で機能を増やしていくとしたら、今回の名前付けも、
[TARGETS]ではなく、[NO_REPOST_TARGETS]のようにした方がよいかもですね。
うーん、どうなんでしょう。
その方向性を想定する場合、拡張機能の名前自体がRepostConfirmationCancelerとなっているので、拡張機能の名前自体を変えた方が良い気も。。。
There was a problem hiding this comment.
アイコンなどもリロード抑止のようなアイコンになっているので、一旦「別のダイアログをキャンセルしたい」という方向性は想定せずに、[TARGETS]のままでどうでしょうか？
There was a problem hiding this comment.
はい、同意です。
仮に将来適切な名前に変えるとしても、後方互換性のためのエイリアスとして対応することは容易ですし。
その方向性を想定する場合、拡張機能の名前自体がRepostConfirmationCancelerとなっているので、拡張機能の名前自体を変えた方が良い気も。。。
昔、FirefoでXULアドオン時代に「任意のダイアログの任意のボタンを自動で押すアドオン」というものを法人向けに提供していたことがあったのですが、WebExtenions化で使用できなくなってそのままとなっています。その代替あるいは後継として使えそう、という思いがあって先の発言につながっていました。
とはいえ、今の段階で備える必要性はそこまで高くはなく、このソフトウェアを一般向けにリリースした後でそういう相談が寄せられるようになったら、その時改めて考えればいいだろうと思っています。
* Remove unused settings * Drop edge section * Add targets section to specify urls * Move WARNING_WHEN_CLOSE_DIALOG to GLOBAL section
|
すみません、同じPRでNative messaging host向けの追加の指摘事項も対応してしまいました。 |
|
すみません、正しく動いていないことが分かったので修正中です。 |
|
修正済み。 |
2 participants
Fix comment
Improve
wildcmpImprove to check only the edge section
Remove a unused function
Support only Excludes and Patterns
Remove unused settings
Drop edge section
Add targets section to specify urls
Move WARNING_WHEN_CLOSE_DIALOG to GLOBAL section