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

feat: dispose function for copy-paste plugin #2061

Closed
wants to merge 3 commits into from

Conversation

bartbutenaers
Copy link

The basics

The details

Resolves

Fixes issue #2058

Proposed Changes

Based on the tips from @alicialics in the above issue, I have added a dispose function to the plugin that executes the following actions:

  1. It removes the 2 contextmenu items that the plugin has been added in the init function (if available).
  2. It removes the 3 shortcut key actions, and replaces them by the 3 default shortcut key actions (cut/copy/paste).

Reason for Changes

In our application (i.e. a node for Node-RED) we re-create the workspace multiple times, which means we also need to dispose all the plugins that we have used.

Test Coverage

The plugin did not have any tests yet...

Documentation

I have added the dispose function to the example code snippet on the plugin's readme page.

Additional Information

@bartbutenaers bartbutenaers requested a review from a team as a code owner November 6, 2023 20:33
@bartbutenaers bartbutenaers requested review from maribethb and removed request for a team November 6, 2023 20:33
Copy link

google-cla bot commented Nov 6, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@bartbutenaers bartbutenaers changed the title dispose function for copy-paste plugin feat: dispose function for copy-paste plugin Nov 6, 2023
@alicialics
Copy link
Contributor

nice! you should probably run npm run format for this change.

@bartbutenaers
Copy link
Author

@alicialics,
Thanks for the tip! It is formatted now.
What a journey...

@BeksOmega BeksOmega assigned BeksOmega and unassigned maribethb Nov 20, 2023
@BeksOmega BeksOmega requested review from BeksOmega and removed request for maribethb November 20, 2023 16:57
@BeksOmega
Copy link
Contributor

@bartbutenaers Do you need to dispose of the copy paste plugin when injecting your new workspace? The plugin works globally on the page, so I don't think you should need to tear it down.

@bartbutenaers
Copy link
Author

@BeksOmega,
I have no tested it in the way you describe. So I cannot confirm at the moment if it works like that.

But personally I like to use all my plugins in the same way: I initialize all of them when I create my workspace, and I dispose all of them after stopping my workspace. That keeps my code a bit readable. Hope you understand.

@bartbutenaers
Copy link
Author

Perhaps I can add a comment to the code snippet on the readme page, to explain that it is not necessary to call the dispose function when the plugin is being initialized globally. Then people can choose how they want to use this plugin.

@BeksOmega
Copy link
Contributor

But personally I like to use all my plugins in the same way: I initialize all of them when I create my workspace, and I dispose all of them after stopping my workspace. That keeps my code a bit readable. Hope you understand.

I think it would be a bit misleading to add disposal for this plugin. Most plugins initialize themselves with a specific workspace (e.g. the backpack plugin), and therefore need to be disposed when the workspace is disposed. But this one is initialized globally, and therefore does not need to be disposed.

Similarly, this plugin only needs to be initialized once, and doesn't need to be initialized per-workspace.

If people think that the copy-paste plugin is a per-workspace thing like other plugins, it will actually cause them problems. For example, if you have two workspaces on a page, and then you tear down one of them + the copy paste plugin, the copy pasting will appear to break for the other workspace, because the copy-paste logic is actually global.

Does that make sense?

@bartbutenaers
Copy link
Author

Hi @BeksOmega,

Thanks for the explanation!
Yes indeed that makes sense.
I have updated my code to load the plugin globally and then it indeed still works.
So I will close this pull request.

BTW It would be more clear to understand if these kind of plugins would have a short explanation on their readme page, to explain that they need to be initialized globally so no dispose is required. Because now it looked - to me at least - that the developer of the node had forgotten to implement the dispose function...

Bart

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.

None yet

4 participants