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

Use asearch #55

Merged
merged 11 commits into from
Jun 3, 2021
Merged

Use asearch #55

merged 11 commits into from
Jun 3, 2021

Conversation

takker99
Copy link
Contributor

あいまい検索library asearchでアイコンを絞り込むようにしてみました。

環境の都合上、E2Eテストだけ確認できていません。他のテストは通ることを確認しました

@mizdra
Copy link
Owner

mizdra commented Jun 1, 2021

PR ありがとうございます! レビューします 👀

const target = item.searchableText.toLowerCase();
return target.includes(query.toLowerCase());
});
const maxAambig = Math.min(Math.floor(query.length / 4) + 1, 3);
Copy link
Owner

Choose a reason for hiding this comment

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

メモ: あいまい度は0〜3しか指定できないので Math.min している
http://shokai.org/blog/archives/8471

Copy link
Owner

Choose a reason for hiding this comment

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

素朴な疑問: ちなみに Math.floor(query.length / 4) + 1 という式で あいまい度を算出しているのには、何か理由があるのでしょうか? どういう理由でクエリの長さを使ったり、それを4で割ったり、最後に1を足したりしているのかが、差分からだと読み取れず、気になったという次第です。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

素朴な疑問: ちなみに Math.floor(query.length / 4) + 1 という式で あいまい度を算出しているのには、何か理由があるのでしょうか? どういう理由でクエリの長さを使ったり、それを4で割ったり、最後に1を足したりしているのかが、差分からだと読み取れず、気になったという次第です。

クエリが長くなるほど打ち間違いが多くなりそうなので、クエリ長に比例して許容できるあいまい度を大きくしました。
具体的な数値に深い理由はありません。単にemoji selector 181行目の設定に倣っただけです。

Copy link
Owner

Choose a reason for hiding this comment

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

承知です〜。何か意図あるか気になっただけで、僕もどうしたいとか強い拘り今のところ無いので、一旦このままにしてリリースして様子みようと思います 👍

@mizdra
Copy link
Owner

mizdra commented Jun 1, 2021

SHOULD: ここのテストケース名も fuzzy search がされることが分かるような文言に変えて頂きたいです!

test('query に部分一致する items のみが返る', () => {

@mizdra
Copy link
Owner

mizdra commented Jun 1, 2021

https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/ に従って CI 走らせる権限を付与しようとしてみたのですが、何故かどこにも Approve and run ボタンが出てこなかったので、マージする直前に僕の手元でテスト実行してみようと思います (お手数おかけします)

image

Copy link
Owner

@mizdra mizdra left a comment

Choose a reason for hiding this comment

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

  • いくつか気になったところにコメント書きました
  • 検索便利にしたいなと思いつつ、あんまり高級なものを使うと bundle size が大きくなりそうで躊躇していたので助かりました! これくらいのサイズなら気軽に入れられそうです。
  • コメント書いたところお返事頂けたり、修正していただけたら approve できそうです
    • MAY の点に関しては対応してもしなくても大丈夫という温度感です

@takker99
Copy link
Contributor Author

takker99 commented Jun 1, 2021

SHOULD: ここのテストケース名も fuzzy search がされることが分かるような文言に変えて頂きたいです!

test('query に部分一致する items のみが返る', () => {

とりあえず「あいまい一致」にしてみました。言い回しが変であればご指摘お願いします

@mizdra
Copy link
Owner

mizdra commented Jun 1, 2021

メモ:

すみません、後出しで申し訳ないのですが、ちょっと気になるポイントがあったのでメモしてみます。

  • icon-suggestion は高速にアイコンを挿入できることを売りにしているので、できるだけ最小のクエリ入力で欲しいアイコンを絞り込めると嬉しい
    • もともと会議で議事録を取る時に、誰が発言したかをアイコンで表現しているようなケースで、とにかく議事録を取りやすいようにという目的で開発した
    • ref: https://www.mizdra.net/entry/2020/06/14/200119
    • ので、こうした使い方をしているユーザさんが不便にならないようにはしてあげたい
  • ところで Asearch だと user のようなクエリを入力した時に、userscript にマッチしない
    • このマッチングだとあいまい度で検索しないと引っかからない & Asearch は 3 が上限なので suggest されないことになりそう
    • あいまい検索できて便利だけど、長いページ名のアイコンを挿入しづらくなりそう
  • 個人的には結構不便になりそうなので、何とかしたいけど、どうすると良いかなー

@mizdra
Copy link
Owner

mizdra commented Jun 1, 2021

とりあえず「あいまい一致」にしてみました。言い回しが変であればご指摘お願いします

文言これで問題なさそうです!

@takker99
Copy link
Contributor Author

takker99 commented Jun 1, 2021

  • ところで Asearch だと user のようなクエリを入力した時に、userscript にマッチしない
    • このマッチングだとあいまい度で検索しないと引っかからない & Asearch は 3 が上限なので suggest されないことになりそう
  • あいまい検索できて便利だけど、長いページ名のアイコンを挿入しづらくなりそう

#55 (comment) にある通り、 で囲んでuserにすればuserscriptにmatchするのでその心配はありません

@mizdra
Copy link
Owner

mizdra commented Jun 3, 2021

#55 (comment) にある通り、 で囲んでuserにすればuserscriptにmatchするのでその心配はありません

ああなるほど! ようやく理解しました 🙇 であれば大丈夫そうですね。

- テストでは fuzzy search を無視できるよう、文字数を多くすることに
@mizdra mizdra merged commit bf8c9f8 into mizdra:main Jun 3, 2021
@mizdra
Copy link
Owner

mizdra commented Jun 3, 2021

CI 通るようにちょっと手直ししてマージしました。 PRありがとうございました!! @takker99

@mizdra
Copy link
Owner

mizdra commented Jun 3, 2021

@takker99 takker99 deleted the feature-asearch branch June 3, 2021 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants