Skip to content

Add new untitled diff command#168533

Merged
bpasero merged 5 commits intomicrosoft:mainfrom
ste42:main
Dec 12, 2022
Merged

Add new untitled diff command#168533
bpasero merged 5 commits intomicrosoft:mainfrom
ste42:main

Conversation

@ste42
Copy link
Contributor

@ste42 ste42 commented Dec 9, 2022

Addresses #165123

This PR adds a new command called "New Untitled Diff" that opens a diff editor with two untitled sides.

I added the command in src/vs/workbench/contrib/files/browser/fileActions.ts because the new command is under the File category and similar commands (like Compare Active File with ... and New Untitled File) also appear under src/vs/workbench/contrib/files/browser/

Preview of the feature:

new_untitled_diff.mp4

@ste42
Copy link
Contributor Author

ste42 commented Dec 9, 2022

@microsoft-github-policy-service agree

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

This is already very good 👏

I just noticed how we then have these 2 entries:

image

But the menu calls this differently:

image

So I have a suggestion:

  • we rename New Untitled File to New Untitled Text File
  • we rename New Untitled Diff to Compare New Untitled Text Files

I think we should keep untitled in the label just to support existing users that may have found the command by typing untitled.

I would stay way from using Diff in the name because we mainly refer to the diff editor via compare labels it seems.

@bpasero bpasero added this to the January 2023 milestone Dec 9, 2022
@ste42
Copy link
Contributor Author

ste42 commented Dec 9, 2022

How about renaming New Text File in the menu bar to New Untitled File? The command's id is 'workbench.action.files.newUntitledFile', so changing the display name to New Untitled Text File puts it out of sync with the id (unless we change the id as well). It also looks like the term New Text File is only used in the menu bar.

Another benefit is that Compare New Untitled Files is a word shorter than Compare New Untitled Text Files!

@bpasero
Copy link
Member

bpasero commented Dec 10, 2022

No, we cannot. The current wording in the menu was part of an effort to make things more discoverable and understandable if I remember correctly.

@ste42
Copy link
Contributor Author

ste42 commented Dec 11, 2022

To clarify renaming New Untitled File to New Untitled Text File, a few questions:

  • Does this mean every instance where the term shows up, or only the command label?
  • How should variable names be handled (change NEW_UNTITLED_FILE_COMMAND_ID to NEW_UNTITLED_TEXT_FILE_COMMAND_ID, same with NEW_UNTITLED_FILE_LABEL)?
  • Do command ids also merit a change, like 'workbench.action.files.newUntitledFile'?

A few examples of other instances:

const newUntitledFile: WatermarkEntry = { text: nls.localize('watermark.newUntitledFile', "New Untitled File"), id: NEW_UNTITLED_FILE_COMMAND_ID };

name: 'New Untitled File args',

@bpasero
Copy link
Member

bpasero commented Dec 11, 2022

Yes, I suggest to change user facing labels, but we cannot change command IDs for backwards compatibility. I think leaving variable names as they are is also fine, but feel free to change them if you feel strong about it.

@ste42 ste42 requested a review from bpasero December 12, 2022 02:07
bpasero
bpasero previously approved these changes Dec 12, 2022
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Thanks!

@bpasero bpasero merged commit 9749e88 into microsoft:main Dec 12, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2023
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.

3 participants