-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fixed #4897: supported tab details to disambiguate identical names #5775
Conversation
Is this the intended behavior? Maybe a second opinion would be good. |
That sounds fine, I'm fine with having the same behavior as VSCode as well :) |
expect(pathMap.get(tabPaths[4])).to.be.equal('root1/aaa/bbb'); | ||
expect(pathMap.get(tabPaths[5])).to.be.equal('.../ccc'); | ||
// tslint:disable-next-line: no-unused-expression | ||
expect(pathMap.get(tabPaths[6])).to.be.undefined; |
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.
We can avoid the suppression of the tslint check by updating the code from:
expect(pathMap.get(tabPaths[6])).to.be.undefined;
To:
expect(pathMap.get(tabPaths[6])).to.be.equal(undefined);
import { TabBarRenderer } from './tab-bars'; | ||
|
||
describe('tab-bar', () => { | ||
describe('tab-name-disambiguation', () => { |
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.
Can we follow the pattern present in other tests?
Namely,
it('should...')
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.
Sure!
I open a multi-root workspace and I have the following file in each root folder:
Both are reported as : .theia/tasks.json |
Thanks for the feedback! I will test it and then address this issue |
I just rebased the PR to the latest master. Please let me know if it still does not work (it works on my side) ;) |
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.
Few minor comments
The test does not execute
Otherwise, code + execution looks nice, works well
@fangnx in order to fix the test: import { enableJSDOM } from '../test/jsdom';
let disableJSDOM = enableJSDOM();
import { expect } from 'chai';
import { Title, Widget } from '@phosphor/widgets';
import { TabBarRenderer } from './tab-bars';
disableJSDOM();
describe('tab bar', () => {
before(() => {
disableJSDOM = enableJSDOM();
});
after(() => {
disableJSDOM();
});
... We need to make sure to clean up after testing due to |
7f222bd
to
8a6cae4
Compare
…ical names - Supported additional tab details for tabs with identical titles. - If there existed multiple tabs with the same name, each tab would have its partial relative path displayed after the title. Signed-off-by: fangnx <[email protected]>
@lmcbout do you mind completing the review and merging if it's fine? |
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.
LGTM
Nice works @fangnx
What it does
Fixed #4897.
How to test
Review checklist
Reminder for reviewers