Skip to content

Terms based entities search#10991

Merged
zsarnett merged 7 commits intohome-assistant:devfrom
vanackej:feature/terms_based_entities_search
Apr 27, 2022
Merged

Terms based entities search#10991
zsarnett merged 7 commits intohome-assistant:devfrom
vanackej:feature/terms_based_entities_search

Conversation

@vanackej
Copy link
Copy Markdown
Contributor

@vanackej vanackej commented Dec 21, 2021

Proposed change

The entity picker component currently just look for searched entities using simple contains comparison. It is sometimes hard to find the entity you are looking for because you have to remember the exact order if the words used in the entity id or friendly name.

This PR introduce flexible terms based search for entities ids and name in the entity picker component.

For exemple, given the following entities :

  • sensor.bedroom_temperature
  • sensor.kitchen_temperature
  • sensor.kitchen_humidity

Currently, if you want to match the last one, you have to search for 'kitchen_h' at minimum in order to refine your search to the desired input.
With the proposed change, you just have to type 'k hu' for instance, in any order. Thats much more flexible does not require user to remember the exact order and full naming of its entities.

You can find many more usage samples in the included tests.

The code for matching regexp and search function is isolated from the entity picker component. I also applyied it to the worker in charge of filtering data tables. If it seems ok, I could easily extend this behavior to various other search boxes (devices/services/integrations/automations...)

Here is a screenshot of this search in action :
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

No extra configuration required

Additional information

None

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@spacegaier
Copy link
Copy Markdown
Member

How will this affect the performance? Some users have a huge amount of entities and therefore a very long list to be filtered.

@vanackej
Copy link
Copy Markdown
Contributor Author

vanackej commented Dec 25, 2021

Well, Regex matching is really fast, and I took care to compile the regex only once per search in order to minimize overhead. I use the same exact code in professional apps I develop and honestly it feels instantaneous.

I made a benchmark locally, and searching across 1000 entities took 10ms. Edit Actually, my benchmark was with 4000 entities for 10ms search time :-)

@zsarnett
Copy link
Copy Markdown
Contributor

Should we use the Sequence matching instead that is implemented in the Quick Bar?

@vanackej
Copy link
Copy Markdown
Contributor Author

Should we use the Sequence matching instead that is implemented in the Quick Bar?

Hi @zsarnett,

The behavior is not really the same, what I propose here is more permissive (except it does no fuzzy matching) and is totally different in it's usage.
The main goal of what I propose here : type any part of what you are looking for in any order and refine search by adding new search terms if too many results. If you are looking for a switch in the basement, just type "swi base" and you are sure to find it. If you want all automations related to your alarm, just type "alarm auto" and there you are. Quick bar does not work this way.

The quick bar uses the FuzzyFilterSort, seems sensitive to words order and does not play well with multiple terms separated with spaces from my experience. I wanted exact partial matching for all the search terms, in any order. Maybe I don't get it, but I almost never find what I want when using the quick bar :-( As soon as I don't enter search terms in the right order, quick bar does not find what I am looking for.

If you look at the example screenshot I provided in the description, fuzzy match does not return any result. Same for the unit tests I provided, fuzzy search does not match most of the time.

@zsarnett
Copy link
Copy Markdown
Contributor

In one of the examples you mention, sensor.kitchen_humdity

The Quickbar would get this with your k hu text example...

I am just not sure we should have two different ways to search for entities. The experience should be the same through out the application

image

@vanackej
Copy link
Copy Markdown
Contributor Author

Try reversing you input (3 p), I think it does not work anymore.

image

image

That seems inconsistant to me.

I understand we don't want to introduce too many different search mecanisms. I Just wanted to introduce a simple improvement for all basic search inputs, because currently the simple "contains" approach is really frustrating.

@vanackej
Copy link
Copy Markdown
Contributor Author

@zsarnett BTW : Congrats for your new position at Nabu Casa ! Exciting news :-)

@balloob
Copy link
Copy Markdown
Member

balloob commented Jan 24, 2022

The quick bar and entity picker should use the same search logic.

@vanackej
Copy link
Copy Markdown
Contributor Author

The quick bar and entity picker should use the same search logic.

Agreed, but which one ? Have anyone tried to use the one I propose ?
It is different from the existing one, but to me (maybe I'm wrong, looking for feedbacks) usability is better. If you have a look at the test cases I provided with this PR, you will see most of them won't work with fuzzyScore (the one used by quick bar)
Maybe a mix of both ? What is missing in the implementation for search I propose to be able to get included in replacement of the existing one ? Fuzzy matching ? Scoring ? If yes, there are really good libraries out there that provide such search options to integrate (but I don't know what is the policy regarding external libraries addition)

Let me know, I'm willing to adapt the proposal to your feedbacks. Using the current entity picker is a pain, just want to improve it in any way.

@zsarnett
Copy link
Copy Markdown
Contributor

Definitely open to better logic for the quick bar and entity search.

@zsarnett
Copy link
Copy Markdown
Contributor

Would love to have the Quickbar logic implemented into the entities search. Is that still something you want to tackle?

@vanackej
Copy link
Copy Markdown
Contributor Author

vanackej commented Mar 17, 2022

Hi,

So you basically just want to apply the current quick bar matching function to the entity search ? No room for improvements here ?

I honestly don't understand how quick bar matching function is supposed to work, from a user perspective but also from a developer perspective after reading it's code and associated unit tests

const shouldMatchEntity = [
createExpectation("automation.ticker", 131),
createExpectation("automation.ticke", 121),
createExpectation("automation.", 82),
createExpectation("au", 10),
createExpectation("automationticker", 85),
createExpectation("tion.tick", 8),
createExpectation("ticker", -4),
createExpectation("automation.r", 73),
createExpectation("tick", -8),
createExpectation("aumatick", 9),
createExpectation("aion.tck", 4),
createExpectation("ioticker", -4),
createExpectation("atmto.ikr", -34),
createExpectation("uoaintce", -39),
createExpectation("au.tce", -3),
createExpectation("tomaontkr", -19),
createExpectation("s", 1),
createExpectation("stocks", 42),
createExpectation("sks", -5),
];
=> What are those magic values as expectations ?

But anyway, yes, based on this PR, changing the matching function is a one-liner (just have to change the implementation for termsSearchFunction)

@zsarnett
Copy link
Copy Markdown
Contributor

Definitely open to better logic for the quick bar and entity search.

#10991 (comment)

We are open to better logic but the quick bar logic and the entity logic should be the same logic

@vanackej
Copy link
Copy Markdown
Contributor Author

OK. I will first adapt this PR to use the same logic for both and mutualize as much code as possible, then we'll see what can be done to get an improved search logic.

@vanackej vanackej force-pushed the feature/terms_based_entities_search branch from 1f4128f to cfdd4ff Compare March 31, 2022 11:07
@vanackej
Copy link
Copy Markdown
Contributor Author

Hi @zsarnett

I just pushed an update including some intermediary work on this, in order to be able to test and evaluate different matching logic.

I refactored quick bar and entity picker in order to use the same search function. It can be switched by just reassigning the defaultFuzzyFilterSort exported constant here: https://github.com/home-assistant/frontend/pull/10991/files#diff-692eb3ca3265b7ffde8a1d7e15fc059f21c80d88ef1105fd046e0a0c6ad3da4cR118

There are currently 3 implementations available :

  • fuzzyFilterSort: the original that was used by the quick bar
  • termsFilterSort: the one I originally implemented in this PR, based on simple regex matching for each query string token
  • ratioFilterSort: another one based on token set ratio algorithm, provided by fuzzball library

The good part here is that other matching algorithms can be easily tested.

Let me know what you think about this work in progress and how you think we can move on from here.

@zsarnett zsarnett added the Major Feature These are features that may need call outs in release notes label Apr 15, 2022
@zsarnett zsarnett self-assigned this Apr 20, 2022
@zsarnett
Copy link
Copy Markdown
Contributor

Let's go ahead and remove the old logic and the Fuzzball logic for the new Regex Logic.

I think the REGEX is definitely more difficult to maintain though...

Fuzzball is a large package so I would rather not use that.

@vanackej
Copy link
Copy Markdown
Contributor Author

The thing that is missing in the regex approach is that there is no scoring. I hadn't that in mind when I implemented it first because I was just trying to replace entities filtering, not the quick bar.
I'll have a try to get a combination of both, remove the dependency to fuzzball, I let you know.

@vanackej
Copy link
Copy Markdown
Contributor Author

I looked at the history of the current fuzzy matching feature : #7367
It is based on a copy/paste of VSCode search feature.
Are you ok to switch this copy/paste for an external lightweight library : https://www.npmjs.com/package/fuzzysort
It implements basically the same functionnality, is very efficient, and avoid maintaining this logic here (maintaining copy/pasted code that no one really understand is not a good strategy in my opinion).

I have a version of the search functionnality that combine the old behavior, using the fuzzysort library, and with a few tweaks it also supports all the test cases I added for my initial Regex proposition.
@zsarnett If you are ok with it, I can clean this up and push it for review.

@zsarnett
Copy link
Copy Markdown
Contributor

Yea I think that would be great. Not having to maintain that logic would be nice.

@vanackej
Copy link
Copy Markdown
Contributor Author

I just finished cleaning everything, seems ok for me.
Behavior is almost like original sequence matching, with a major difference : search terms are split on space and scores for each search terms are added (score are negative, 0 is a perfect match so adding scores = refining results) . It allows further query refinement using independant search terms, terms order does not influence the results.
I also activated allowTypo option, in order to match when some chars are out of order.

@vanackej
Copy link
Copy Markdown
Contributor Author

Another interesting feature for future use : fuzzysort provide the location of the matches => It would be possible to use it for end users

@zsarnett
Copy link
Copy Markdown
Contributor

Nice. Ill take a look soon!

Comment on lines +339 to +342
const sortableEntityStates = this._states.map((entityState) => ({
strings: [entityState.entity_id, computeStateName(entityState)],
entityState: entityState,
}));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems expensive to do each time. Can we store this? Possibly memoize?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you fix this in a new PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll have a look at this

Copy link
Copy Markdown
Contributor

@zsarnett zsarnett left a comment

Choose a reason for hiding this comment

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

Looks good!

@zsarnett zsarnett merged commit 8da73d4 into home-assistant:dev Apr 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed Major Feature These are features that may need call outs in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants