-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 dummy preference descriptions to open AI config widget #15166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fulfills the purpose but I'm not so happy about the result as the "Edit in settings.json" is wildly misleading.
We might need to register a custom preference render to avoid producing it.
And if we're doing that, we could instead have a custom renderer for these sections and within there render the hint. This would allow us to avoid declaring a non-existing preference.
Generally speaking, supporting a description text for sections would be nice but might be out of scope for this issue.
I had the same thought. One easy workaround would be a special renderer for preferences with declared type |
ffc6c1c
to
9e34269
Compare
I've pushed a commit that adds a renderer for preferences whose only declared type is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn because it works, however in the end it's still a hack.
It's a hack because we have many unintended side effects:
- we register preferences although there are none
- we offer these preferences in the JSON Editor for autocomplete etc.
- we offer UI to copy the preferences name, id, etc. although they are useless
- they influence search in the preferences UI
Of course the overall impact is minimal, and I also don't see any collateral as I can't think of a real world use case for preferences of type null.
Overall it would be much nicer to enrich sections to allow optional descriptions. Then we could add this text to these sections and avoid registering additional preferences.
However I don't want to block the merge if we want to tackle this in a follow up then
packages/preferences/src/browser/views/components/preference-null-input.ts
Outdated
Show resolved
Hide resolved
6f2defa
to
8187689
Compare
@sdirix, since your comments
Regarding your comments:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite right, looks like leaving things in a skeletal state in #14048 created a number of issues in the preference system. I'll fill out the children that should have been added then. |
If we remove |
I'll do both: #13819 assumes that everything should be in the layout provider, and so broke sections that didn't manually specify all their children, because two-segment sections aren't rendered and intermediate labels aren't split correctly ( Manual specification has the advantage of better NLS support, but now I'm frustrated enough with the problems that I'll go and fix the automatic generation more thoroughly, as well, rather than plastering over it with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. Thanks! 👍
Coming back to the original topic of this PR: Using pseudo-preferences to achieve UI changes feels like we're working around the system rather than with it. This approach brings some minor but notable side effects that aren't ideal from a technical standpoint.
That said, the user experience benefits are significant enough that I'll approve this PR. I do think we should consider a cleaner solution in the future that accomplishes the same UI improvements without repurposing our preference system in ways it wasn't designed for.
74e8c9d
to
c213519
Compare
What it does
Fixes #15160 by adding preferences whose descriptions redirect users to the config widget to enable them to perform more advanced AI configuration.
Fixes #15165 by generating section names differently depending on presence or absence of layout item and by using the preference tree label provider's logic for label generation.
How to test
The third segment (
details
) is to work around #15165Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers