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

fix(search): prioritize start of word and exact match more #953

Merged
merged 4 commits into from
Nov 21, 2024

Conversation

nikku
Copy link
Member

@nikku nikku commented Nov 21, 2024

Test via npx @bpmn-io/sr bpmn-io/bpmn-js -l bpmn-io/diagram-js#search-start

Proposed Changes

This prioritizes start of term and exact matches more aggressively.

This yields more substantial results in large popup (append) menus:

capture SO2PNd_optimized

This yields better search results when querying larger data pools, where exact matches would otherwise de-prioritized:

capture 5sTQ3Y_optimized

Related to camunda/camunda-modeler#4122, camunda/camunda-modeler#3439.

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@barmac
Copy link
Member

barmac commented Nov 21, 2024

I added the sr script to the top comment, and am looking into this right now.

Comment on lines 180 to 186
? 131.9
: 7.87
)
: (
token.wordStart
? 2.19
: 1
Copy link
Member

Choose a reason for hiding this comment

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

What magic is this? :) Why such values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Standard magic values, call them temperature 🥵

Copy link
Member

Choose a reason for hiding this comment

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

Gives me AI engineer vibes :D

Copy link
Member Author

Choose a reason for hiding this comment

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

You gotta tune it some times. Though we have test coverage, which is good. 😉

@barmac
Copy link
Member

barmac commented Nov 21, 2024

While searching for the start of the word works better with this PR, the exact match with spaces is moved down:

This PR:

image

Current:

image

@barmac
Copy link
Member

barmac commented Nov 21, 2024

I can reproduce camunda/camunda-modeler#4711 (comment) in this PR too.

@@ -253,6 +253,32 @@ describe('features/search', function() {
}));


it('should prioritize longest match', inject(function(search) {
Copy link
Member

Choose a reason for hiding this comment

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

Good one!

@nikku
Copy link
Member Author

nikku commented Nov 21, 2024

@barmac Thanks for your tests! I think we get to something reasonable, one test at a time. Check my latest updates on this branch out or the following capture that shows this is fixed: We now score tokens by length, too, to prioritize longest matches. Also we de-prioritize non-matches:

capture i7bkrt_optimized

  This gives longer matches precedence.

chore(search): penalize unmatched tokens

  This ensures we score "most relevant" results higher.
@barmac
Copy link
Member

barmac commented Nov 21, 2024

Great improvement

@nikku nikku merged commit f117bfe into develop Nov 21, 2024
10 checks passed
@nikku nikku deleted the search-start branch November 21, 2024 14:22
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 21, 2024
github-merge-queue bot pushed a commit to camunda/camunda that referenced this pull request Dec 10, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [bpmn-js](https://redirect.github.com/bpmn-io/bpmn-js) | [`17.11.1` ->
`18.1.1`](https://renovatebot.com/diffs/npm/bpmn-js/17.11.1/18.1.1) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/bpmn-js/18.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/bpmn-js/18.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/bpmn-js/17.11.1/18.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/bpmn-js/17.11.1/18.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>bpmn-io/bpmn-js (bpmn-js)</summary>

###
[`v18.1.1`](https://redirect.github.com/bpmn-io/bpmn-js/blob/HEAD/CHANGELOG.md#1811)

[Compare
Source](https://redirect.github.com/bpmn-io/bpmn-js/compare/v18.1.0...v18.1.1)

- `FIX`: adjust search to prioritize start of word and exact matches
([bpmn-io/diagram-js#953](https://redirect.github.com/bpmn-io/diagram-js/pull/953))
- `FIX`: ignore whitespace when searching
([bpmn-io/diagram-js#954](https://redirect.github.com/bpmn-io/diagram-js/pull/954))

###
[`v18.1.0`](https://redirect.github.com/bpmn-io/bpmn-js/blob/HEAD/CHANGELOG.md#1810)

[Compare
Source](https://redirect.github.com/bpmn-io/bpmn-js/compare/v18.0.0...v18.1.0)

- `FIX`: clear selection when opening search pad
([bpmn-io/diagram-js#947](https://redirect.github.com/bpmn-io/diagram-js/pull/947))
- `FIX`: correct dangling selection after search pad interaction
([bpmn-io/diagram-js#947](https://redirect.github.com/bpmn-io/diagram-js/pull/947))
-   `DEPS`: update to `[email protected]`

###
[`v18.0.0`](https://redirect.github.com/bpmn-io/bpmn-js/blob/HEAD/CHANGELOG.md#1800)

[Compare
Source](https://redirect.github.com/bpmn-io/bpmn-js/compare/v17.11.1...v18.0.0)

- `FEAT`: remove `outline` from `Viewer` modules
([#&#8203;2135](https://redirect.github.com/bpmn-io/bpmn-js/issues/2135))
- `FEAT`: make `Canvas` a focusable element
([bpmn-io/diagram-js#662](https://redirect.github.com/bpmn-io/diagram-js/pull/662))
- `FEAT`: implicit keyboard binding
([bpmn-io/diagram-js#662](https://redirect.github.com/bpmn-io/diagram-js/pull/662))
- `FEAT`: integrate with global `search`
([#&#8203;2235](https://redirect.github.com/bpmn-io/bpmn-js/pull/2235))
- `FEAT`: integrate `popup-menu` with `search`
([bpmn-io/diagram-js#932](https://redirect.github.com/bpmn-io/diagram-js/pull/932))
- `FEAT`: recognize modern `search` tokens in `search-pad`
([bpmn-io/diagram-js#932](https://redirect.github.com/bpmn-io/diagram-js/pull/932))
- `FIX`: correctly handle duplicate entries and whitespace in `search`
([bpmn-io/diagram-js#932](https://redirect.github.com/bpmn-io/diagram-js/pull/932))
- `FIX`: find `search` terms across all keys
([bpmn-io/diagram-js#932](https://redirect.github.com/bpmn-io/diagram-js/pull/932))
- `FIX`: `search` always returns tokens for matched items
([bpmn-io/diagram-js#932](https://redirect.github.com/bpmn-io/diagram-js/pull/932))
- `FIX`: prevent crash during label adjustment
([#&#8203;2239](https://redirect.github.com/bpmn-io/bpmn-js/issues/2239))
- `FIX`: keep existing loop characteristics when toggling through the
replace menu
([#&#8203;2251](https://redirect.github.com/bpmn-io/bpmn-js/pull/2251))
- `FIX`: prevent covering multi selection with black box in `Viewer`
([#&#8203;2135](https://redirect.github.com/bpmn-io/bpmn-js/issues/2135))
- `FIX`: generate types for main entry
([`986e2bb`](https://redirect.github.com/bpmn-io/bpmn-js/commit/986e2bb51ea301e6e0df56f3606a27424fb90179))
- `FIX`: correct handling of group name with whitespace only
([#&#8203;2231](https://redirect.github.com/bpmn-io/bpmn-js/issues/2231))
- `DEPS`: update to `bpmn-moddle@9`
([#&#8203;2114](https://redirect.github.com/bpmn-io/bpmn-js/pull/2114))
-   `DEPS`: update to `[email protected]`
-   `DEPS`: update to `[email protected]`

##### Breaking Changes

-   Require `Node >= 20`
- `Canvas` is now a focusable element and provides better support for
native browser behaviors. Focus can be controlled with new `focus` and
`restoreFocus` APIs.
- Keyboard is now implicitly bound to canvas SVG element. Calls to
`keyboard.bind` and `keyboard.bindTo` now result with a descriptive
console error and have no effect.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/camunda/camunda).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xOS4wIiwidXBkYXRlZEluVmVyIjoiMzkuMTkuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiYXV0b21lcmdlIl19-->
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.

2 participants