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

Add registerWindowTitleVariable command #225408

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

BABA983
Copy link
Contributor

@BABA983 BABA983 commented Aug 12, 2024

Close #202991

Usage in extension

vscode.commands.executeCommand('registerWindowTitleVariable', key, value)

settings.json

"window.title": "${windowTitleVariable.aa}"

window-title-variable-demo.mp4

@BABA983 BABA983 marked this pull request as draft August 12, 2024 14:36
@hediet hediet requested a review from benibenj August 13, 2024 09:00
@hediet hediet assigned benibenj and unassigned hediet Aug 13, 2024
@benibenj
Copy link
Contributor

@BABA983 any reason this is still a draft?

@BABA983 BABA983 marked this pull request as ready for review August 16, 2024 02:51
@BABA983
Copy link
Contributor Author

BABA983 commented Aug 16, 2024

@BABA983 any reason this is still a draft?

I was thinking should we separate the behaviour into 2 commands? e.g. registerWindowTitleVariable and setWindowTitleVariable. Maybe just 1 command is good enough for the simplicity.

@benibenj
Copy link
Contributor

Both options are not optimal, but having just one might be the better choice. If we have a register and a set command, we can easily detect if two extensions are creating the same variable with the same name and can log this, however, this would probably also mean we would have to support a way to unregister a variable as an extension might be enabled and disabled multiple times. I don't really want to have 3 commands for this, so I think having only a set command is good enough.

@benibenj
Copy link
Contributor

@bpasero can you have a glance at this PR?

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.

I think the title variable service thing is overkill unless we plan on using it somewhere else, so in that case we could just inline the command into the title bar part?

@BABA983 BABA983 force-pushed the feature/window-title-variable branch from 146b321 to bcb4fd8 Compare August 16, 2024 15:25
@BABA983 BABA983 force-pushed the feature/window-title-variable branch from 4717e83 to 5649053 Compare August 16, 2024 16:16
@BABA983
Copy link
Contributor Author

BABA983 commented Aug 16, 2024

Thanks for the feedback, I have addressed the issues mentioned in the previous review. Could you please take a moment to review the changes again?

benibenj
benibenj previously approved these changes Aug 19, 2024
@vs-code-engineering vs-code-engineering bot added this to the August 2024 milestone Aug 19, 2024
@benibenj benibenj removed the request for review from bpasero August 19, 2024 07:26
@benibenj
Copy link
Contributor

benibenj commented Aug 19, 2024

@BABA983 we decided that it would make sense for an extension to create the context key and then provide it to the registerWindowTitleVariable command. I made some changes to support this behaviour. Here is an example use case:

export function activate(context: vscode.ExtensionContext) {
    setInterval(() => 
        vscode.commands.executeCommand('setContext', 'timeNowContextKey', new Date().toLocaleTimeString()),
        1000
    );
    vscode.commands.executeCommand('registerWindowTitleVariable', 'timeNow', 'timeNowContextKey');
}

@benibenj benibenj requested a review from bpasero August 19, 2024 16:38
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.

I went ahead and did the changes I had in mind because I think we spend a bit too much time engineering this.

@benibenj benibenj merged commit b23a500 into microsoft:main Aug 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support referencing context key values in window.title template
5 participants