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

Don't reuse identifier with registerAction2 #199744

Closed
7 tasks done
jrieken opened this issue Dec 1, 2023 · 6 comments · Fixed by #202291
Closed
7 tasks done

Don't reuse identifier with registerAction2 #199744

jrieken opened this issue Dec 1, 2023 · 6 comments · Fixed by #202291
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders menus Menu items and widget issues

Comments

@jrieken
Copy link
Member

jrieken commented Dec 1, 2023

I have noticed that some Action2 instances are registered that reuse an id. That's not OK because Action2 is a singleton model object, e.g it keybinding or menu registrations refer to it by id. This should be cleaned up and from then on it should be an error to reuse an id

@jrieken jrieken added the debt Code quality issues label Dec 1, 2023
@jrieken jrieken added this to the December 2023 milestone Dec 1, 2023
@jrieken
Copy link
Member Author

jrieken commented Dec 1, 2023

@sandy081 I believe this is all the same issue somewhere in views land?

@jrieken jrieken added the menus Menu items and widget issues label Dec 4, 2023
@sandy081
Copy link
Member

sandy081 commented Dec 4, 2023

This is how these actions are registered

id: `${viewDescriptor.id}.removeView`,

id: `${viewDescriptor.id}.toggleVisibility`,

May I know what is the suggested approach?

@jrieken
Copy link
Member Author

jrieken commented Dec 4, 2023

May I know what is the suggested approach?

Don't register them twice

@jrieken jrieken assigned Tyriar and lszomoru and unassigned sandy081 Jan 8, 2024
@jrieken
Copy link
Member Author

jrieken commented Jan 8, 2024

@sandy081 #202000 fixes duplication in view land by first disposing old action and only then creating new ones

@jrieken
Copy link
Member Author

jrieken commented Jan 8, 2024

@Tyriar @lszomoru you are new to this issue, there are ids that are reused for Action2 and those need to be fixed. Today, this prints a warning but it this become an error soon

@lszomoru lszomoru removed their assignment Jan 8, 2024
sandy081 added a commit that referenced this issue Jan 9, 2024
@sandy081 sandy081 removed their assignment Jan 9, 2024
sandy081 added a commit that referenced this issue Jan 9, 2024
#199744 do not register command twice
@jrieken
Copy link
Member Author

jrieken commented Jan 11, 2024

Adding @meganrogge for @Tyriar

meganrogge added a commit that referenced this issue Jan 11, 2024
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jan 11, 2024
jrieken added a commit that referenced this issue Jan 12, 2024
@aiday-mar aiday-mar added this to the December / January 2024 milestone Feb 6, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders menus Menu items and widget issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants