-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add openTabRenamer
action
#7462
Conversation
Sorry, there are two items that I don't know how to address:
For item 1 the only thing I can think of is to add a
But that doesn't seem very nice. Otherwise, is there is some way to only add the |
Hey sorry, just getting back from leave now and catching up on older PRs. As far as the focus issues are concerned, you've stumbled into one of the horrible bug factories of this project I'm afraid 😬 See #6680 for more details. Your proposed solution to issue 1 seems sensible to me. A bit hacky, but everything we're doing with focus is a tad bit hacky, and that doesn't seem any worse. I'm a tad bit worried about As far as issue 2 goes, since that's already existing behavior, I wouldn't worry about it too much during this PR. we can always punt that to a future bugfix. Other than that, there have been some settings changes recently, so you'll need to do a merge from master, but it should just be re-namespacing things. |
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.
Other than doing that keyup thing you mentioned earlier, and merging with master
, the rest of this looks great!
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
Thanks for the feedback on this. I'll make |
55afaf2
to
5757ec6
Compare
New misspellings found, please review:
To accept these changes, run the following commands
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
e446329
to
5b37c21
Compare
This has been rebased onto main and I've been able to try it out locally. Implementing the |
@Coridyn Thanks! I'll try and take a look at this tomorrow morning. In the meantime, don't worry about the linter error - it's being a tad aggressive and we're still trying to figure out the right balance here. We'll probably push an update to |
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.
This looks good to me, thanks for throwing this together!
Oh hey don't worry about the linter error - that's just the linter being a little over-zealous while we calibrate it the way we'd like. I'll take care of that. |
All our JSON files are _actually_ JSONC files - json with comments. A well-behaved application that accepts JSON should accept and ignore comments. However, `jsonlint` is not a well behaved application in this regard. So, to prevent the linter from complaining about our JSON comments, we need to disable it entirely. THAT'S RIGHT, there's not a setting to allow JSONC. See #8076 as an example of this working. This will also unblock #7462.
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.
This is lovely. Thanks for working on it, and sorry we took forever to review it!
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
👏 👏 |
Awesome, thanks for your help @zadjii-msft and @DHowett! |
🎉 Handy links: |
Adds a
ShortcutAction
to allow editing the tab title via the TextBox(just like double-clicking the tab, but triggered from a key binding or
command palette).
This implements "scenario 3" outlined in zadjii-msft's comment:
Add keybinding to rename tab #6557 (comment)
The
openTabRenamer
action name is taken from the discussion in thisPR:
Add
setTabColor
andopenTabColorPicker
actions #6567 (comment)Related to #6256 (but doesn't address pane renaming)