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

fix(plugin): Set preopen directories properly #723

Merged
merged 1 commit into from Sep 19, 2021

Conversation

tw4452852
Copy link
Contributor

Every plugin will have following two directories for its use:

./: Plugin's own data should be saved here, every plugin will have its own directory.
/global/: All plugins have access to this directory, Some shared data could be saved here.

Signed-off-by: Tw tw19881113@gmail.com

Copy link
Member

@TheLostLambda TheLostLambda left a comment

Choose a reason for hiding this comment

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

Another promising feature! I think we'll need to do a little more thinking about this though (it totally breaks strider for example, which relies on being able to access the current directory tree).

Additionally, while I think that having a private data directory for each plugin is good, I'm not really sure if we'd need / want a /global directory. It feels like that would primarily be for communicating between different plugins, but I think that's something that I think would be better implemented in another way.

I'd be happy with changing the plugin_own_data_dir to .join(path) so that all of your timer plugin instances can still share data like you need them to!

To summarize, how would you feel about:

  1. Changing . to /host and mounting the current directory there (this will also require Strider to be updated, but is better than just changing . back to the current directory, since that would conflict with /global in some cases)
  2. Changing /global to /data and naming that directory after the path of the loaded plugin somehow instead of the plugin_id (this way you can still share files between several instances of the same plugin)

@tw4452852
Copy link
Contributor Author

tw4452852 commented Sep 19, 2021

Hi @TheLostLambda

Very appreciate your good suggestions.

  1. Changing . to /host and mounting the current directory there (this will also require Strider to be updated, but is better than just changing . back to the current directory, since that would conflict with /global in some cases)

I'm fine with this.

I'd be happy with changing the plugin_own_data_dir to .join(path) so that all of your timer plugin instances can still share data like you need them to!
2. Changing /global to /data and naming that directory after the path of the loaded plugin somehow instead of the plugin_id (this way you can still share files between several instances of the same plugin)

Per my understanding, here join(path) means join(plugin_name), right?
Currently, we have two ways to specify a plugin, either by name or by absolute path of the wasm binary.
For the second way, shall we extract its from the loading path? Like /path/to/test.wasm => test

@TheLostLambda
Copy link
Member

@tw4452852

For the second way, shall we extract its from the loading path? Like /path/to/test.wasm => test

I'd actually be fine with either that or the full paths, you decide what you think works best! We can improve it to take into account versions and things in the future if we need :)

But yeah, plugin_name overall so plugins of the same type can share the folder like you need :)

@tw4452852
Copy link
Contributor Author

tw4452852 commented Sep 19, 2021

@TheLostLambda

I use the plugin name because of consistency.

@tw4452852
Copy link
Contributor Author

Hold on please, there's an issue when unloading the plugin.

@TheLostLambda
Copy link
Member

@tw4452852 Good catch! Let me know once things are working!

Every plugin will have following two directories for its use:

`./`: Plugin's own data should be saved here, every plugin will have its own directory.
`/global/`: All plugins have access to this directory, Some shared data could be saved here.

Signed-off-by: Tw <tw19881113@gmail.com>
@tw4452852
Copy link
Contributor Author

@TheLostLambda I'm done, but I leave a TODO for cleanup when unloading.

@TheLostLambda
Copy link
Member

@TheLostLambda I'm done, but I leave a TODO for cleanup when unloading.

Ah! To be honest, I was kinda thinking that the folders could just stick around. That way plugins can actually store information between Zellij sessions!

@tw4452852
Copy link
Contributor Author

Ah, yes.

@TheLostLambda
Copy link
Member

@tw4452852 If you happy with that, I can merge this and then make the Strider fixes? Do you reckon this is ready to merge?

@tw4452852
Copy link
Contributor Author

Yes, it's ready. Feel free to merge. Thanks.

@TheLostLambda TheLostLambda merged commit 4b792ca into zellij-org:main Sep 19, 2021
@tw4452852 tw4452852 deleted the plugin_path branch September 19, 2021 23:14
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

2 participants