Skip to content

Fix #364 - Use image registries - #365

Open
travkin79 wants to merge 19 commits into
microsoft:mainfrom
travkin79:fix-use-image-registry
Open

Fix #364 - Use image registries#365
travkin79 wants to merge 19 commits into
microsoft:mainfrom
travkin79:fix-use-image-registry

Conversation

@travkin79

@travkin79 travkin79 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

This PR fixes issue #364.

  • Adds CopilotImages, a facade for the ui plug-in's images
  • Adds CopilotJobsImages, a facade for the jobs plug-in's images
  • Adds unit tests to ensure the resources can be loaded
  • Improves plug-in instantiation

Copilot AI review requested due to automatic review settings July 22, 2026 09:44
@travkin79 travkin79 mentioned this pull request Jul 22, 2026
Open
Copilot AI reviewed Jul 22, 2026
View reviewed changes

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.

Pull request overview

This PR addresses issue #364 by centralizing image loading in the ui and ui.jobs bundles via plugin ImageRegistry-backed facades (CopilotImages, CopilotJobsImages), replacing ad-hoc image creation/disposal at call sites and adding tests to catch missing icon resources.

Changes:

  • Introduces CopilotImages (UI) and CopilotJobsImages (Jobs) registries and migrates many call sites to registry/shared-image access.
  • Removes UiUtils.buildImage*FromPngPath(...) usage patterns and associated manual disposal logic across UI widgets/handlers/dialogs.
  • Adds reflection-based unit tests to validate that all IMG_* constants map to real bundle resources; adjusts plugin/test initialization accordingly.

Reviewed changes

Copilot reviewed 64 out of 64 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/utils/UiUtils.java Removes ad-hoc PNG image helper APIs to push callers toward registries.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/utils/MenuUtils.java Switches quota/blank icon descriptors to CopilotImages keys.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/utils/McpUtils.java Uses shared default MCP icon from CopilotImages.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/swt/SpinnerAnimator.java Uses registry spinner frames instead of per-frame image creation/disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/swt/ModelHoverContentProvider.java Replaces static cached/disposed icons with themed registry images.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/swt/DropdownPopup.java Replaces static cached/disposed icons with themed registry images.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/swt/DropdownButton.java Replaces static cached/disposed arrow icon with themed registry images.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/quickstart/FeaturePage.java Converts quickstart icons to registry-managed images and removes manual disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/WrappableIconLink.java Changes API to accept Image (non-owned) instead of path/shared-image variants.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/McpPreferencePage.java Uses CopilotImages for info/red-notice icons and shared images.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/GlobalAutoApproveSection.java Uses CopilotImages for shared warning icon.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/CustomModesPreferencePage.java Uses CopilotImages for information icon.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/ByokPreferencePage.java Uses CopilotImages for keep/delete icons and removes image disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/AutoApprovePreferencePage.java Uses CopilotImages for shared info icon.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/AddByokModelDialog.java Uses themed registry eye/eye-closed icons (no manual disposal).
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/AddApiKeyDialog.java Uses themed registry eye/eye-closed icons (no manual disposal).
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/nes/RulerColumn.java Uses CopilotImages for gutter-arrow descriptor source.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/nes/ActionMenu.java Uses CopilotImages for menu action icon descriptors.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/handlers/ToolConfigurationContextMenuHandler.java Uses CopilotImages for context menu icon descriptors.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/handlers/ShowStatusBarMenuHandler.java Uses CopilotImages descriptors (including spinner frames) instead of path literals.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/handlers/ShowMenuBarMenuHandler.java Uses CopilotImages icon descriptors instead of path literals.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/handlers/ShowContextMenuHandler.java Uses CopilotImages for context menu icon descriptor.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/dialogs/MissingTerminalDependenciesDialog.java Uses registry Copilot icon (no manual disposal).
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/dialogs/mcp/McpServerItem.java Uses themed registry images for MCP meta-row icons (no manual disposal).
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/dialogs/mcp/McpServerDetailDialog.java Uses registry/themed images and removes label-level disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/dialogs/mcp/McpRegistryDialog.java Uses registry images for dialog/buttons/banner icons and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/dialogs/mcp/McpApprovalDialog.java Uses registry/shared images for approve/deny icons (no manual disposal).
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/dialogs/jobs/BaseCopilotDialog.java Uses registry Copilot icon (no manual disposal).
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/CopilotUi.java Initializes image registry via CopilotImages and adjusts plugin singleton lifecycle.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/CopilotImages.java New UI image facade defining keys + registry/shared-image access helpers.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/WorkingSetBar.java Switches arrows/keep/undo/file icons to registry/shared images.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/WarnWidget.java Uses shared warning icon via CopilotImages.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/viewers/NoSubscriptionViewer.java Switches main icon to registry image and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/viewers/LoadingViewer.java Switches main icon to registry image and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/viewers/ChatHistoryViewer.java Switches navigation/edit icons to registry images; shared delete icon via facade.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/viewers/BeforeLoginWelcomeViewer.java Switches welcome/feature icons to registry images and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/viewers/AgentModeViewer.java Switches welcome/tools/attach-context icons to registry images and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/viewers/AfterLoginWelcomeViewer.java Switches welcome/attach-context icons to registry images and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/tools/RunInTerminalToolAdapter.java Uses registry Copilot icon descriptor for terminal integration.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/TodoListBar.java Switches status/arrow/clear icons to registry images and removes disposal code paths.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ThinkingBlock.java Uses registry cancel/arrow icons and removes disposal helper.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/StaticBanner.java Uses shared severity/close icons via CopilotImages.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/SourceViewerComposite.java Uses shared copy/paste icons via CopilotImages.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/AvatarService.java Uses registry default avatars and stops disposing them in service dispose().
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFile.java Uses registry close icon and shared warning icon via CopilotImages.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ModelPickerGroupsBuilder.java Uses themed warning icon via CopilotImages instead of static cache/dispose.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ErrorWidget.java Uses shared error icon via CopilotImages.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/dialogs/DynamicOauthDialog.java Uses registry Copilot icon and removes manual disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/CurrentReferencedFile.java Replaces static never-disposed eye icons with registry images.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/AgentToolCancelLabel.java Uses registry cancel icon and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/AgentStatusLabel.java Uses registry status icons and shared error icon; removes disposal of registry images.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/AgentMessageWidget.java Uses themed jobs PR icons via CopilotImages (no disposal).
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/AddContextButton.java Uses registry attach-context icon and removes disposal.
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java Uses registry/shared images for action bar icons and removes disposal blocks.
com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/jobs/CopilotJobsImagesTest.java New reflection-based test to validate jobs bundle icon resources exist.
com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/CopilotUiTests.java Updates UI plugin test startup approach (needs adjustment per review).
com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/CopilotImagesTest.java New reflection-based test to validate UI bundle icon resources exist.
com.microsoft.copilot.eclipse.ui.test/META-INF/MANIFEST.MF Adds dependency on com.microsoft.copilot.eclipse.ui.jobs for new tests.
com.microsoft.copilot.eclipse.ui.jobs/src/com/microsoft/copilot/eclipse/ui/jobs/views/JobsView.java Migrates jobs view icons to CopilotJobsImages and removes disposal.
com.microsoft.copilot.eclipse.ui.jobs/src/com/microsoft/copilot/eclipse/ui/jobs/utils/UiUtils.java Deletes ad-hoc jobs image helper in favor of registry facade.
com.microsoft.copilot.eclipse.ui.jobs/src/com/microsoft/copilot/eclipse/ui/jobs/CopilotJobsImages.java New jobs image facade defining keys + registry/shared-image access helpers.
com.microsoft.copilot.eclipse.ui.jobs/src/com/microsoft/copilot/eclipse/ui/jobs/CopilotJobs.java Initializes image registry via CopilotJobsImages and adds plugin singleton accessor.
com.microsoft.copilot.eclipse.ui.jobs/META-INF/MANIFEST.MF Exports jobs package (visibility consideration raised in review).
.github/copilot-instructions.md Documents the new “prefer image registries; don’t dispose registry images” guideline.

Comment thread com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/CopilotImages.java Outdated
Comment thread ...oft.copilot.eclipse.ui.jobs/src/com/microsoft/copilot/eclipse/ui/jobs/CopilotJobsImages.java Outdated
Comment thread com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/CopilotUiTests.java Outdated
Comment thread com.microsoft.copilot.eclipse.ui.jobs/META-INF/MANIFEST.MF Outdated
@travkin79
travkin79 force-pushed the fix-use-image-registry branch from ff58d34 to 86f6f66 Compare July 24, 2026 14:00
@jdneo

jdneo commented Jul 29, 2026

Copy link
Copy Markdown
Member

Thank you, nice refactoring to get rid of tons of dispose listeners.

Give me one more day to do some test on it, if no issues found, I will merge it :)

jdneo
jdneo reviewed Jul 29, 2026
View reviewed changes

@jdneo jdneo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Standards/spec review findings. The duplicate ownership finding from both axes is consolidated into one inline comment.

Comment thread com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/utils/McpUtils.java
Comment thread com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/swt/SpinnerAnimator.java
Comment thread com.microsoft.copilot.eclipse.ui.jobs.test/META-INF/MANIFEST.MF Outdated
Comment thread ...microsoft.copilot.eclipse.ui.jobs/src/com/microsoft/copilot/eclipse/ui/jobs/CopilotJobs.java
Comment thread com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/CopilotImages.java Outdated
Comment thread com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/CopilotImages.java Outdated
Comment thread com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/nes/RulerColumn.java Outdated
Comment thread ...soft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/SourceViewerComposite.java Outdated
jdneo
jdneo reviewed Jul 30, 2026
View reviewed changes

@jdneo jdneo left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two more comment:

  1. Please check McpRuntimeLogger.java as well, it can use the registry as well.
  2. We can remove UiUtils.resizeIcon(String path, int width, int height) since no caller now. (not highly related with this PR, but would be great to do the cleanup)

Comment thread ...microsoft.copilot.eclipse.ui.jobs/src/com/microsoft/copilot/eclipse/ui/jobs/CopilotJobs.java
Comment thread com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/CopilotImages.java Outdated
travkin79 added a commit to travkin79/copilot-for-eclipse that referenced this pull request Jul 30, 2026
@travkin79
Remove unused method
52308fe 
See review comment microsoft#365 (review)
@jdneo

jdneo commented Jul 30, 2026

Copy link
Copy Markdown
Member

Hi @travkin79 there is a conflict. Please resolve it and I can merge it asap since this is a huge change.

Thank you again for the contribution.

travkin79 and others added 15 commits July 30, 2026 10:58
@travkin79
feat: Add image registries (microsoft#364)
4081440 
- Add CopilotImages, a facade for the ui plug-in's images
- Add CopilotJobsImages, a facade for the jobs plug-in's images
- Improve plug-in instantiation
- Add unit tests to ensure the resources can be loaded
- Add fragment test bundle com.microsoft.copilot.eclipse.ui.jobs.test

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Migrate image loading and disposal to using image registries (microso…
cd4a030 
…ft#364)

Use image registries to access static images, no longer dispose them, since the registries handle Image disposals. Keep dynamic image loading and disposal as it was.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Use image registries to access shared images from other plug-ins (mic…
2e88e7e 
…rosoft#364)

Re-direct calls of PlatformUI.getWorkbench().getSharedImages().getImage(...) to the image registry.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Simplify WrappableIconLink to take only an Image parameter (microsoft…
45c02b3 
…#364)

Drop the icon-path/shared-image factory variants of WrappableIconLink in favor of a single create(...) that accepts an already-resolved Image. Callers now pass registry images (CopilotImages.IMG_INFORMATION) or shared workbench images directly, and the widget no longer owns or disposes the icon.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Remove unused buildImage(Descriptor)FromPngPath helpers (microsoft#364)
9427256 
All icon lookups now go through the image registries (CopilotImages / CopilotJobsImages), so the raw PNG-path helpers are no longer referenced. Drop them from the ui-bundle UiUtils and delete the jobs-bundle UiUtils, whose sole purpose was these two methods.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Document image registry usage and disposal rules in copilot instructi…
d7d4e2b 
…ons (microsoft#364)

Add an Image Registries subsection under Resource Management.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Improve javadoc: clarify image disposal
0d6ad79
@travkin79
Fix mixed image ownership in McpUtils.loadServerIcon
0a2880a 
Rename loadServerIcon to loadDisposableServerIcon and return null instead of the shared default icon when an icon cannot be loaded.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Fix stale SpinnerAnimator Javadoc on frame ownership
dc5f822 
Update docs about image registry usage and disposal responsibility.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
fix: Remove unused dependencies
d580fea
@travkin79
Remove unused image registry methods returning ImageDescriptor
980d83f
@travkin79
Complete Javadoc for public image registry APIs
e820a33 
Add missing @return and @param tags to newly introduced public methods.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Hide spinner frame constants
49bee87 
Make IMG_SPINNER_* keys and the SPINNER_FRAMES array private, since they are meant for internal use only. Expose only SPINNER_FRAME_COUNT and use it from SpinnerAnimator. Adapt test.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
Use shared registry image for NES ruler column icon
4cd7c6c
@travkin79
Add insert icon to copilot's image registry
4d0b6dc 
Register the insert icon (icons/full/elcl16/insert_template.png) from org.eclipse.ui.workbench.texteditor bundle in our own image registry, CopilotImages.
Remove image disposal (now handled by the registry) and unused code.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
travkin79 and others added 3 commits July 30, 2026 11:01
@travkin79
Remove unused method
c333bd7 
See review comment microsoft#365 (review)
@travkin79
Use the image registry in McpRuntimeLogger
31dfca0
@travkin79
Use the image registry for accessing the terminal icon
0f0f128 
Adapt the terminal SPI's API and both terminal implementations to retrieve the icon from an image registry instead of creating and
disposing own copies.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@travkin79
travkin79 force-pushed the fix-use-image-registry branch from 341c0b0 to 0f0f128 Compare July 30, 2026 09:01
@travkin79

travkin79 commented Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I rebased, but there is one test failing. I try to fix it quickly.

Update: Fixed now. I think, we're ready to merge. If you want me to squash commits, please let me know.

@travkin79
Provide a populated image registry to turn widget tests
9766881 
The image-registry refactor routes SourceViewerComposite's insert icon through CopilotUi.getPlugin().getImageRegistry(). The shared turn-widget test fixture mocked getPlugin() but not getImageRegistry(), so rendering a code block returned a null registry and NPE'd.

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

travkin79 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

It would be great if we could merge this PR soon and then go on an finish PRs #368 and #330. The latter is really huge and merge conflicts are likely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdneo jdneo jdneo left review comments
Copilot code review Copilot Copilot left review comments
@ethanyhou ethanyhou Awaiting requested review from ethanyhou ethanyhou is a code owner
@xinyi-gong xinyi-gong Awaiting requested review from xinyi-gong xinyi-gong is a code owner
@duzitong duzitong Awaiting requested review from duzitong duzitong is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@travkin79 @jdneo