Repository navigation
Fix Ctrl+F7 view switcher showing same name for multi-instance views - #4134
Philipp0205 wants to merge 1 commit into
Conversation
7837dac to
b566f4b
Compare
|
@Philipp0205 : I've rebased your branch on latest master state. Your state was very old. |
|
@iloveeclipse thanks, I forgot to update my fork 🙃 |
|
This works for Terminal views but doesn't work for Console / Search views - they also show exact same names in the tabs. I guess the way how Terminal (coming originally from CDT) manages its name differs from "regular" multi-instance views which were developed in the Platform. Ideally we should investigate what is the difference and provide a fix that works consistently for all views. |
|
okay, apparently I did not test this good enough. I also noticed that console and search views do not add numbers to their tab names if opened multiple times. Let me have another look at the other views. |
|
I have investigated this a bit further and I found that the problem can be reproduced for all these views:
As you pointed out @Philipp0205 , the Search and Console views does not add numbers to the tabs. Your fix does however work for those too. I tried adding similar logic (based on secondaryId if present as in TerminalsView::createPartControl) for updating the partName in ConsoleView and SearchView, and then I verified with your fix. I suggest that your fix should be accepted as it is and separate issues created if we want the names on the actual tab names to be changed for Search and Console views. I consider it to be different, although related topics. I am new to this community so bear with me if I am doing this in the wrong way :) |
|
@iloveeclipse, to summarize my findings above with respect to your concern: this fix is already generic. The switcher shows whatever part name a view sets, for all multi-instance views. Console and Search still show identical names only because those views never set a distinct part name per instance, unlike Terminal. Would you agree to track the Console/Search naming as a separate issue? If so, I can create that issue, and this PR could be merged as it is to close the original one. |
Remove the getPartName() override in ViewReference that always returned the static descriptor label from plugin.xml. The parent class WorkbenchPartReference.getPartName() returns part.getLocalizedLabel() which reflects the dynamic name set by views via setPartName(). This is a generic fix for all multi-instance views (Terminal, Console, Search) that customize their part name. Fixes eclipse-platform/eclipse.platform#2774
b566f4b to
9ab0818
Compare
|
Yes |
There was a problem hiding this comment.
🟡 Changes recommended
Existing tests for dynamic view-reference names remain disabled, leaving the regression uncovered.
1 open finding
What changed in this PR
Removes the static view-name lookup so Ctrl+F7 reflects dynamically assigned names for multi-instance views.
Changes:
- Inherits
getPartName()fromWorkbenchPartReference. - Uses the E4 model’s updated label.
| File | Description |
|---|---|
ViewReference.java |
Removes the descriptor-based name override. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return descriptor == null ? "" : descriptor.getLabel(); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| @Override |
|
@Philipp0205 or @ravnskjaer : please follow up on Copilot feedback. |
|
Happy to take this over. @Philipp0205, do you want to handle the Copilot finding yourself, or shall I? The finding is valid, the tests fail on master and passes with your fix. |
|
@ravnskjaer If you have time at the moment that would be awesome |
|
Thanks, @Philipp0205. I'll open a new PR with your fix plus the re-enabled tests, keeping you as the author of the fix, and link it here. |
|
New PR 4459 with enabled tests. This PR can now be closed. |
Change made: Removed the
getPartName()override fromViewReference.java.Views like Terminal call
setPartName("Terminal " + secondaryId)which triggersCompatibilityPartto update the E4 model viapart.setLabel(computeLabel()). The parent classWorkbenchPartReference.getPartName()returnspart.getLocalizedLabel()which reflects this dynamic name (e.g. Terminal 1, Terminal 2 instead of Terminal Terminal).The removed override was bypassing this by always returning the static plugin.xml descriptor label. Generic fix: Works for all multi-instance views (Terminal, Console, Search).
Fixes eclipse-platform/eclipse.platform#2774