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

Add custom QuickPick highlighting #83424

Open
rmanthorpe opened this issue Oct 28, 2019 · 26 comments
Open

Add custom QuickPick highlighting #83424

rmanthorpe opened this issue Oct 28, 2019 · 26 comments
Assignees
Labels
api feature-request Request for new features or functionality quick-pick Quick-pick widget issues
Milestone

Comments

@rmanthorpe
Copy link

It would be nice to have a bit more control over highlighting in QuickPick. I'm using QuickPick as a search field using regex and the existing fuzzy matching doesn't make sense in this context. I'd like to be able to provide my own highlights for the detail field and avoid highlighting elsewhere, although for completion I think it makes sense to allow custom highlighting on all three fields.

The API could be similar to the proposed highlighted TreeItemLabel in #61482 for backward compatibility. A more general HighlightedString could be a good fit across the board. The implementation can then skip the fuzzy matching if the label type is HighlightedString and apply the highlights in there.

I feel that this would complement #77297 nicely and make for a more versatile QuickPick API.

@chrmarti chrmarti added api feature-request Request for new features or functionality quick-pick Quick-pick widget issues labels Oct 28, 2019
@rmanthorpe
Copy link
Author

@chrmarti would you consider a PR for this if I implemented it?

@chrmarti
Copy link
Collaborator

I agree, allowing extensions to fully control filtering and sorting will require them to also control the highlighting.

Would you add matchOnLabel to QuickPick to allow for completely disabling the built-in highlighting? How would you add the highlights to QuickPickItem?

@rmanthorpe
Copy link
Author

Yes I'd definitely expose matchOnLabel publicly (#83425).

I'd define

export interface HighlightedString{
    label: string;
    highlights?: [number, number][];
}

and then in IQuickPickItem just swap label: string; for label: string | HighlightedString; for backward compatibility. Then in QuickInputList.filter skip the default filtering and highlighting if you find a HighlightedString with a highlights defined and instead populate the IMatchs with the highlights provided. You'd probably want to do the same thing for description and detail too.

@chrmarti
Copy link
Collaborator

Agreed, text instead of label might combine better with label, description and detail on QuickPickItem.

/cc @bpasero @jrieken for feedback on allowing extensions to fully control filtering and highlighting on QuickPicks.

@jrieken
Copy link
Member

jrieken commented Feb 20, 2020

I have doubts that different highlights/filter strategies are going to be confusing. Quick pick always "looks" the same and this changes how it "feels" without me being able to tell.

@rmanthorpe
Copy link
Author

Yes text makes the most sense. This same pattern is currently the proposed API for TreeItem with TreeItemlabel and I think it would make sense to adopt this HighlightedString or HighlightedText pattern more generally.

@jrieken I'm not sure I follow - in what way does this change how QuickPick feels? The visible result is the same - a list of items with possibly highlighted labels, description & detail depending on whether something matched according to some criteria. Consider a case where you were doing a regex match with the input - the fuzzy matching/filtering isn't going to work, but you the developer know where the match was so you can supply the highlights and disable other highlighting and filtering.

@jrieken
Copy link
Member

jrieken commented Feb 20, 2020

I am just worried that this will encourage competing match-strategies. Regexp matches "more" than fuzzy match which is OK, I am more worried about the "less" case in which extension can force prefix match onto me. Tho, I do understand that this can be done already, just without benefit of highlights...

@rmanthorpe
Copy link
Author

I think if you give people a little more flexibility to match and highlight their own way they're going to be less likely to abuse the system rather than more. If they can do things in a simple way there's no need to hack around the problem and create a mess.

@rmanthorpe
Copy link
Author

I'm not sure I understand what you mean about extensions forcing a prefix match onto you - could you expand on that?

@jrieken
Copy link
Member

jrieken commented Feb 20, 2020

could you expand on that?

With QuickPick#onDidChangeValue you can update the list of items after every keystroke and there are only recommendations about how that should work (how fuzzy that should be). Once the results are returned we use "our" fuzzy score (which is very relaxed) to compute highlights and (sometimes) ranking.

@rmanthorpe
Copy link
Author

rmanthorpe commented Feb 20, 2020

I see yes. In all the use cases I'm imagining you'd be providing your own highlighting precisely because you're doing your own matching as the fuzzy matching/highlighting isn't what you want. Indeed my suggestion is that if a HighlightedString with defined highlights is provided then the fuzzy matching is skipped entirely as the user has clearly indicated that they are the ones doing the matching.

Right now you can turn off matching on all but the label, and then set alwaysShow = true to get the desired behaviour without the benefit of highlighting as you say, but with the overhead of doing a fuzzy match which is the not used.

@rmanthorpe
Copy link
Author

This could be the solution to other problems too like #90521. People clearly want more control and this + matchOnLabel gives it to them without having to implement all manor of additional filtering/highlighting callbacks etc.

@rmanthorpe
Copy link
Author

@jrieken any more thoughts on this? I'd really love to reach a conclusion here.

@chrmarti
Copy link
Collaborator

Setting alwaysShow: true on the items and sortByLabel: false (#73904 proposed API) on the QuickPick allows you to control the filtering and sorting. You could then let the built-in highlighting decorate the entries. This might sometime show unexpected highlights, but I wonder in which cases these are too much off to call it working fine.

I'm not that much worried about consistency when extensions know better how to handle filtering, sorting and highlighting in their particular case, but when we can avoid complexity in the API (which this change would clearly add), we should certainly do so.

@rmanthorpe
Copy link
Author

That built in filtering fails as soon as you have anything other than basic text in input box. Regular expressions for example, or a file mask or anything else that would let you build a powerful search tool. I understand the desire for simplicity but this pattern has been implemented before with highlighting in TreeItems.

@chrmarti
Copy link
Collaborator

@rmanthorpe Do you have an example of where you would use this with regular expressions or file masks?

@rmanthorpe
Copy link
Author

Sure, as an example, something like ReSharper's "Go to Text" which allows a text search + some inline flags to change the scope of the search. So "foo /fm:c" searches for "foo" in *.c files. Or something which allows you make calls to ripgrep or git grep (optionally with flags), as if you were on the command line but have the results populate a quickpick so you can pick the result from there. For example "-I foo\d" could do a case insensitive git grep for foo + a numeral. Obviously none of those is going to match the filtering properly because they are more than simple search text.

@chrmarti
Copy link
Collaborator

@rmanthorpe Is there an example in your own extension you are going to implement once this feature is in?

@rmanthorpe
Copy link
Author

Yes this is one I've implemented, but I haven't published it yet as I wanted to sort out the highlighting problems first.

@chrmarti
Copy link
Collaborator

@rmanthorpe That sounds like a good case for adding custom highlights. Do you have a screenshot or example of where the built-in highlighting does not match a user's expectation given the input and the items shown?

@rmanthorpe
Copy link
Author

Here I'm just doing a plain text search so the usual highlighting is fine
image

Here I've added an extension filter for declaration files, and now no matching occurs
image

Here I've done a regex search and again, no matching
image

The issue is rarely that you get the wrong thing highlighted, more often than not you just get no highlighting at all because nothing the plain text.

@rmanthorpe
Copy link
Author

@chrmarti any more thoughts on this? I think you can see there are legitimate use cases.

@kbysiec
Copy link

kbysiec commented Oct 30, 2020

@chrmarti Any update on this?

@chrmarti
Copy link
Collaborator

chrmarti commented Nov 2, 2020

@kbysiec No. It is in our list of feature requests for the QuickPick API.

@rmanthorpe
Copy link
Author

Great so back to my original question:

@chrmarti would you consider a PR for this if I implemented it?

@TylerLeonhardt TylerLeonhardt added this to the Backlog milestone Oct 21, 2021
@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Oct 21, 2021

@rmanthorpe sorry for the delay but yes I would consider a PR. I would expect that we would align the highlighting type structure to be similar to that of the other places in the API where we talk about highlighting (which I believe is tree views)

note: I will consider it in the bounds of my own ability to balance my work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api feature-request Request for new features or functionality quick-pick Quick-pick widget issues
Projects
None yet
Development

No branches or pull requests

5 participants