Skip to content

Enable hide/show of type size/alignment - #14822

Merged
Colen Garoutte-Carson (Colengms) merged 9 commits into
mainfrom
dev/coleng/make_size_alignment_optional
Oct 9, 2026
Merged

Colen Garoutte-Carson (Colengms) merged 9 commits into
mainfrom
dev/coleng/make_size_alignment_optional

Conversation

@Colengms

Copy link
Copy Markdown
Contributor

The recently added type size and alignment details are useful, but they make every eligible C++ hover larger. This adds a default-on, resource-scoped setting for hiding those fields and a Command Palette action that users can bind to a key. The extension deliberately does not claim a default keyboard shortcut.

Fixes #14820

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Setting changes confined to a non-default workspace folder do not notify the language server.

2 open findings
What changed in this PR

Adds configurable visibility for type size and alignment in IntelliSense hovers, addressing #14820.

Changes:

  • Adds a default-on, resource-scoped setting and forwards it to the language server.
  • Adds a Command Palette toggle without a default keyboard shortcut.
  • Adds configuration and command tests.
File Description
Extension/​test/​scenarios/​MultirootDeadlockTest/​tests/​quickInfo.test.ts Tests setting values and command toggling.
Extension/​src/​LanguageServer/​settings.ts Exposes the setting and makes updates awaitable.
Extension/​src/​LanguageServer/​extension.ts Registers and implements the toggle command.
Extension/​src/​LanguageServer/​client.ts Includes the setting in server configuration.
Extension/​package.nls.json Adds setting and command text.
Extension/​package.json Declares the setting and command contributions.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread Extension/src/LanguageServer/extension.ts
Configuration changes scoped to a non-default workspace folder did not send the all-folder settings payload to the native service. Send that notification once through the default client and cover the hover behavior in the multi-root scenario.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Cached hover content can prevent visibility changes from appearing when reopening the same hover.

1 open finding
2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reset hover state when settings change

Extension/​src/​LanguageServer/​extension.ts:305

With Copilot hovers enabled and a chat model available, toggling this setting and reopening the hover at the same position can still show the old details. HoverProvider.ts:24–26 returns lastContent when CopilotHoverProvider.isNewHover() is false, bypassing the server even after its settings are updated. Reset the default client's Copilot hover state when this setting changes so the next hover requests fresh content.

🧠 Review effort: Balanced

Comment thread Extension/test/scenarios/MultirootDeadlockTest/tests/quickInfo.test.ts Outdated
The hover behavior coverage requires native support that is not available in the currently published binary. Skip binary-dependent scenarios until 1.35.4 is publicly available.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Check size and alignment independently so a partial-hide regression cannot satisfy the hover integration test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cached same-position hovers can retain the previous size/alignment visibility after toggling.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reset Copilot hover cache when hover settings change

Extension/​src/​LanguageServer/​extension.ts:305

With Copilot hovers enabled and a model available, hovering a type, toggling this setting, and hovering the same position again returns the previous size/alignment visibility. HoverProvider.ts:24–26 reuses cached content when CopilotHoverProvider reports the same document and position; sending settings does not invalidate that cache. After sending the updated settings, reset the default client's Copilot hover provider when this option changes so the next hover requests fresh content. Handle this in the settings-change dispatcher to cover both command toggles and direct settings edits.

🧠 Review effort: Balanced

Reset the Copilot hover identity when type layout visibility changes so reopening the same position requests updated native hover content.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Colengms

Copy link
Copy Markdown
Contributor Author

Addressed the latest Copilot review note. Changing type size and alignment visibility now invalidates the same-position Copilot hover cache, so the next hover reflects the new setting.

Written by Copilot

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The changed update return type introduces existing floating promises that violate the extension’s ESLint configuration.

1 open finding

🧠 Review effort: Balanced

Comment thread Extension/src/LanguageServer/settings.ts
Preserve the existing asynchronous configuration-provider update behavior after CppSettings.update began returning its completion promise.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The setting, command, multi-root propagation, cache invalidation, and behavioral coverage are complete and consistent.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@Colengms
Colen Garoutte-Carson (Colengms) marked this pull request as ready for review October 8, 2026 22:35
@Colengms
Colen Garoutte-Carson (Colengms) merged commit e38cfef into main Oct 9, 2026
2 of 5 checks passed
@Colengms
Colen Garoutte-Carson (Colengms) deleted the dev/coleng/make_size_alignment_optional branch October 9, 2026 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Make the new type size & alignment hover hints hideable

3 participants