Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Update topic list search to include schema results when searching for topicname+'.' #7040

Conversation

jtbandes
Copy link
Member

@jtbandes jtbandes commented Oct 27, 2023

User-Facing Changes
Improved search matching algorithm in the Topics list.

Description

Originally I'd explicitly excluded field results from the search if the search string only matched topic. – I'm not totally sure why I did that; didn't have a test covering it, and it sounds like including the results in this case is more desirable.

Before After
image
image
image
image

Copy link
Contributor

@defunctzombie defunctzombie left a comment

Choose a reason for hiding this comment

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

Magical. Feels much nicer to use.

I think your comment on line 58 is now outdated?

// the path suffix. In this case we show only the topic row and not the message path rows.

@achim-k
Copy link
Contributor

achim-k commented Oct 30, 2023

Personally, I think that there are too many results now (might have been the case before already). The examples below show some matches which are somewhat far fetched. I would prefer if e.g. imu matches only with topics that contain the exact sequence

Example 1 Example 2
image image

@jtbandes
Copy link
Member Author

I would prefer if e.g. imu matches only with topics that contain the exact sequence

What if you typed estruntime? Would you want to see the search result for /spot/status/battery_states.battery_states[:].estimated_runtime? I don't see how to distinguish that from typing imu and matching the same search result.

@jtbandes
Copy link
Member Author

I think your comment on line 58 is now outdated?

// the path suffix. In this case we show only the topic row and not the message path rows.

After pondering this some more, I believe the change actually made the comment more accurate. The comment said "…exclude results if the query matched only the topic name and not the path suffix…" but previously were were also excluding results if the query matched the topic name + the beginning . which is part of the suffix. Now we are only excluding them if the match range does not extend into the suffix at all.

@jtbandes jtbandes merged commit 7e28811 into main Oct 30, 2023
14 checks passed
@jtbandes jtbandes deleted the jacob/fg-5286-bad-topic-search-results-for-imu-in-sample-dataset branch October 30, 2023 15:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants