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

Improvements to message path autocomplete #7256

Merged
merged 5 commits into from Jan 17, 2024
Merged

Improvements to message path autocomplete #7256

merged 5 commits into from Jan 17, 2024

Conversation

jtbandes
Copy link
Member

@jtbandes jtbandes commented Dec 18, 2023 •
edited

User-Facing Changes
Improved autocomplete suggestions when typing message path syntax.

Description
The logic that generated the initial combined (topics+paths) list of suggestions was bespoke and did not account for array fields. This PR changes it to use the existing messagePathsForStructure function (with some additional info returned). It also updates messagePathsForStructure to extend the typicalFilterName logic to strings, and fix a bug where quotes in string filters (...{field=="value"}) were being deleted.

Follow-up to #1971 & #3190

Before After
I am not sure why the header sub-fields were not shown:
image		 image
image Filter and array indexing suggestions are now shown:
image

@jtbandes jtbandes marked this pull request as ready for review December 19, 2023 18:33
defunctzombie
defunctzombie reviewed Dec 20, 2023
View reviewed changes
packages/studio-base/src/components/MessagePathSyntax/MessagePathInput.tsx
*
* Or use actual ROS primitive types:
*
* <MessagePathInput types={["uint16", "float64"]} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this types feature used anywhere in the codebase?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's been renamed to validTypes, but yes, we actually pipe it through the extension API:

studio/packages/studio/src/index.ts

Line 558 in c404fae

validTypes?: string[];

defunctzombie
defunctzombie reviewed Dec 20, 2023
View reviewed changes
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.

What's the gist of the change here? what was happening wrong before and what has been fixed?

I see some logic that is suppose to filter out header.seq but I see that in your screenshot. Is that logic no longer useful and should be removed? Or is the screenshot exposing a bug?

@jtbandes
Copy link
Member Author

jtbandes commented Jan 8, 2024

btw I'm planning to come back and address these comments at some point soon, but I don't see this being a super high priority issue that needs to be closed out before the plot stuff

@jtbandes
Copy link
Member Author

I see some logic that is suppose to filter out header.seq but I see that in your screenshot. Is that logic no longer useful and should be removed? Or is the screenshot exposing a bug?

I believe that is a different code path that runs when you have already started typing a partially valid path and drilled down to a topic. You can see header seq is not included here:

image image

That said, I feel like the difference between the two code paths is kind of weird so I'm happy to remove the logic that filters out seq.

@jtbandes
Copy link
Member Author

What's the gist of the change here? what was happening wrong before and what has been fixed?

Updated PR description:

The logic that generated the initial combined (topics+paths) list of suggestions was bespoke and did not account for array fields. This PR changes it to use the existing messagePathsForStructure function (with some additional info returned). It also updates messagePathsForStructure to extend the typicalFilterName logic to strings, and fix a bug where quotes in string filters (...{field=="value"}) were being deleted.

@jtbandes jtbandes merged commit ea38b3e into main Jan 17, 2024
13 of 14 checks passed
@jtbandes jtbandes deleted the jacob/fg-6059-message-path-autocomplete-is-not-suggesting-array-indexes branch January 17, 2024 02:39
@jtbandes jtbandes mentioned this pull request Jan 17, 2024
Merged
jtbandes added a commit that referenced this pull request Jan 17, 2024
@jtbandes
Follow-up from MessagePath autocomplete #7256 (#7334)
82ad124 
**User-Facing Changes**
None

**Description**
I meant to commit these changes to #7256 before merging, but apparently
I forgot to commit them...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@defunctzombie defunctzombie defunctzombie approved these changes

@achim-k achim-k Awaiting requested review from achim-k

@snosenzo snosenzo Awaiting requested review from snosenzo

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jtbandes @defunctzombie