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

Improve message path autocomplete logic #3190

Merged
merged 4 commits into from
Apr 26, 2022
Merged

Conversation

foxymiles
Copy link
Contributor

User-Facing Changes
This improves message path autocomplete suggestions.

Description
Currently we offer unhelpful message path autocomplete suggestions across arrays of message objects when they contain fields that we mistakenly identify as numeric ids. For example trying to treat the child_frame_id field of a transform as a numeric id. The solution in this PR is to check the datatype of that *_id field and only offer autocomplete values of *_id==0 if *_id is a primitive integer type.

Fixes #2997

@foxymiles foxymiles marked this pull request as draft April 14, 2022 15:47
@foxymiles foxymiles changed the title Miles/message path syntax Improve message path autocomplete logic Apr 14, 2022
@foxymiles foxymiles marked this pull request as ready for review April 14, 2022 19:36
@foxymiles foxymiles requested a review from jtbandes April 14, 2022 19:36
Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

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

I'm a little confused about how this interacts with the code that does getExamplePrimitive:

items.push(`${name}==${getExamplePrimitive(item.primitiveType)}`);

I guess only one of these code paths is taken, based on whether a full recognized topic has been typed already? Anyway, it seems like we should at least try to use the same getExamplePrimitive function here if it makes sense... maybe there's something I am missing though.

@foxymiles
Copy link
Contributor Author

Yeah this entire code path only runs if isTypicalFilterName is true:

export function isTypicalFilterName(name: string): boolean {
return /^id$|_id$|I[dD]$/.test(name);
}

Basically it's trying to find something that looks like an integer id and complete on it, which doesn't make sense for floats, whereas getExamplePrimitive doesn't discriminate ints from floats.

@jtbandes
Copy link
Member

It could be a string id too though (like frame_id). It doesn't seem necessary to couple the isTypicalFilterName heuristic with which types are supported in this case. Though I'm not suggesting adding more logic, I'm suggesting reusing the existing logic to simplify things a bit. Does that seem possible?

@foxymiles
Copy link
Contributor Author

Yeah the point of the followup logic there is to not try to autocomplete on a string _id. I'm not sure if I follow what you're suggesting by reusing the existing logic. Could you expand on what you have in mind a little?

@jtbandes
Copy link
Member

I guess I meant instead of adding the structureItemIsIntegerPrimitive check, just call getExamplePrimitive which will handle integers as well as other types that the selected field might have. Why wouldn't we want to offer autocomplete on string ids?

@foxymiles
Copy link
Contributor Author

Because this code doesn't have any knowledge of the contents of messages we've seen yet so it doesn't know what kind of string ids it could suggest.

@foxymiles
Copy link
Contributor Author

One could also make a strong case for just getting rid of this logic that tries to do a special case autocomplete on an array of items with a field like *_id.

@jtbandes
Copy link
Member

I think suggesting an empty string =="" is more useful than showing nothing at all because it shows the user the correct syntax.

I don't feel too strongly about keeping this logic but I do think it will become harder to discover the filter syntax if it never shows up in the autocomplete.

@foxymiles
Copy link
Contributor Author

At least in the case of arrays, which this code is special cased for, maybe it's better to just suggest [0]?

@foxymiles
Copy link
Contributor Author

And I think suggesting =="" for a string field is non-ideal because it seems to me like we want all the autocomplete suggests to actually result in a message path with data if a user clicks or selects it, not something that requires further editing.

Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

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

Leaving it up to you whether to make any changes based on above discussion or keep the PR as-is.

@foxymiles foxymiles merged commit 89aee35 into main Apr 26, 2022
@foxymiles foxymiles deleted the miles/message-path-syntax branch April 26, 2022 11:58
jtbandes added a commit that referenced this pull request Jan 17, 2024
**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: <br><img
width="274" alt="image"
src="https://github.com/foxglove/studio/assets/14237/bb610276-8653-4338-b70a-44ae3e5d50a2">
| <img width="291" alt="image"
src="https://github.com/foxglove/studio/assets/14237/e3c1c551-ad48-4a17-9cba-00a1dc30b324">
<img width="219" alt="image"
src="https://github.com/foxglove/studio/assets/14237/be154462-f800-43ac-a1db-9b74d2ab2b44">
| Filter and array indexing suggestions are now shown:<br> <img
width="299" alt="image"
src="https://github.com/foxglove/studio/assets/14237/f724e7cb-1da2-47bd-bc2f-7399b9cc1de0">
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.

Message path autocomplete suggestions are not useful in some cases
2 participants