Skip to content
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

open editors: sort by fullPath #133790

Merged
merged 4 commits into from
Jan 12, 2022
Merged

open editors: sort by fullPath #133790

merged 4 commits into from
Jan 12, 2022

Conversation

MaxGrekhov
Copy link
Contributor

Issue: #114470
I have added an option to sort editors by resource path.

@ghost
Copy link

ghost commented Sep 25, 2021

CLA assistant check
All CLA requirements met.

@@ -340,11 +340,12 @@ configurationRegistry.registerConfiguration({
},
'explorer.openEditors.sortOrder': {
'type': 'string',
'enum': ['editorOrder', 'alphabetical'],
'enum': ['editorOrder', 'alphabetical', 'fullPath'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename alphabetical to fileName, then the new option would be filePath. The code would need to be made backwards compatible with the old name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this but then reverted changes because editors are not equal to the files. There are other special editor types and generic 'alphabetical' is closer to reality.

return -1;
} else if (secondResource === undefined) {
return 1;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use comparers.ts's comparePaths here? I don't think we need to special case Schemas.file here or really compare schemas at all, did you see an example of the results being confusing without that schema comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I remember. There are several editor types that have different schema names and may not have a path. In my code schema participates in URI comparison therefore special editors can be placed before or after files without any explicit logic for the user so I placed them before all files because they are special in some way.
List of schemas
Dump of:

editors.forEach(x => {
  const r = x.editor?.resource;
  console.log(`A<${r?.authority}>F<${r?.fragment}>FS<${r?.fsPath}>P<${r?.path}>Q<${r?.query}>S<${r?.scheme}>`);
});
 "A<>F<>FS</home/max/dev/state-example/src/api/commentApi.ts>P</home/max/dev/state-example/src/api/commentApi.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/app/postItem.tsx>P</home/max/dev/state-example/src/app/postItem.tsx>Q<>S<file>"
 "A<undefined>F<undefined>FS<undefined>P<undefined>Q<undefined>S<undefined>"
 "A<>F<>FS<settingseditor>P<settingseditor>Q<>S<vscode-settings>"
 "A<vscode_getting_started_page>F<>FS<>P<>Q<>S<walkThrough>"
 "A<>F<>FS</home/max/dev/state-example/src/base/actionType.ts>P</home/max/dev/state-example/src/base/actionType.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/api/postApi.ts>P</home/max/dev/state-example/src/api/postApi.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/app/app.tsx>P</home/max/dev/state-example/src/app/app.tsx>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/base/httpClient.ts>P</home/max/dev/state-example/src/base/httpClient.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/core/globalState.ts>P</home/max/dev/state-example/src/core/globalState.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/core/http.ts>P</home/max/dev/state-example/src/core/http.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/core/loader.ts>P</home/max/dev/state-example/src/core/loader.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/core/types.ts>P</home/max/dev/state-example/src/core/types.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/global/usePosts.ts>P</home/max/dev/state-example/src/global/usePosts.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/shared/flex.tsx>P</home/max/dev/state-example/src/shared/flex.tsx>Q<>S<file>"
 "A<>F<>FS<Untitled-1>P<Untitled-1>Q<>S<untitled>"
 "A<>F<>FS</home/max/dev/state-example/src/shared/button.tsx>P</home/max/dev/state-example/src/shared/button.tsx>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/public/favicon.ico>P</home/max/dev/state-example/public/favicon.ico>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/public/logo192.png>P</home/max/dev/state-example/public/logo192.png>Q<>S<file>"
 "A<>F<>FS<Untitled-2>P<Untitled-2>Q<>S<untitled>"
 "A<>F<>FS<Untitled-3>P<Untitled-3>Q<>S<untitled>"
 "A<>F<>FS</home/max/dev/state-example/src/base/globalStateKey.ts>P</home/max/dev/state-example/src/base/globalStateKey.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/app/postList.tsx>P</home/max/dev/state-example/src/app/postList.tsx>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/base/actionType.ts>P</home/max/dev/state-example/src/base/actionType.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/core/globalState.ts>P</home/max/dev/state-example/src/core/globalState.ts>Q<>S<file>"
 "A<>F<>FS</home/max/dev/state-example/src/shared/button.tsx>P</home/max/dev/state-example/src/shared/button.tsx>Q<>S<file>"

For some editors, there are not so much data

 "A<vscode_getting_started_page>F<>FS<>P<>Q<>S<walkThrough>"
 "A<>F<>FS<Untitled-3>P<Untitled-3>Q<>S<untitled>"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no conditions non-file tabs go to the bottom of the list because they have vscode- prefix in the schema name.

image

current implementation.

image

Maybe we should sort all non-file tabs by editor name instead of random(for an end-user) schema name.

image

editors = editors.sort((first, second) => {
  //put non-file editors before files
  if (first.editor.resource?.scheme !== Schemas.file && second.editor.resource?.scheme !== Schemas.file) {
    return compareFileNamesDefault(first.editor.getName(), second.editor.getName());
  } else if (first.editor.resource?.scheme !== Schemas.file) {
    return -1;
  } else if (second.editor.resource?.scheme !== Schemas.file) {
    return 1;
  } else {
    return comparePaths(first.editor.resource?.fsPath ?? '', second.editor.resource?.fsPath ?? '');
  }
});

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like how the Untitled editors get split up in that last approach. The current method is fine

@JacksonKearl
Copy link
Contributor

I will be focusing on releasing the next vscode for the next week or so but will come back to reviewing this. Thanks for your contribution!

@MaxGrekhov
Copy link
Contributor Author

Hello, any updates?

'description': nls.localize({ key: 'openEditorsSortOrder', comment: ['Open is an adjective'] }, "Controls the sorting order of editors in the Open Editors pane."),
'enumDescriptions': [
nls.localize('sortOrder.editorOrder', 'Editors are ordered in the same order editor tabs are shown.'),
nls.localize('sortOrder.alphabetical', 'Editors are ordered in alphabetical order inside each editor group.')
nls.localize('sortOrder.alphabetical', 'Editors are ordered in alphabetical order inside each editor group.'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this to ..alphabetical order by {...} inside... to help explain the difference between this and the next option.

@JacksonKearl
Copy link
Contributor

Just one comment then looks good

'description': nls.localize({ key: 'openEditorsSortOrder', comment: ['Open is an adjective'] }, "Controls the sorting order of editors in the Open Editors pane."),
'enumDescriptions': [
nls.localize('sortOrder.editorOrder', 'Editors are ordered in the same order editor tabs are shown.'),
nls.localize('sortOrder.alphabetical', 'Editors are ordered in alphabetical order inside each editor group.')
nls.localize('sortOrder.alphabetical', 'Editors are ordered in alphabetical order by tab name inside each editor group.'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JacksonKearl
I've added ... by tab name ...

Jackson Kearl added 2 commits January 12, 2022 15:33
@JacksonKearl
Copy link
Contributor

Thanks!

@JacksonKearl JacksonKearl merged commit a7575fb into microsoft:main Jan 12, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants