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

Cannot use addons as API #352

Closed
Rigner opened this issue Nov 22, 2018 · 11 comments
Closed

Cannot use addons as API #352

Rigner opened this issue Nov 22, 2018 · 11 comments
Assignees
Labels
Status: Under investigation Investigating the interest and the feasability of the issue.

Comments

@Rigner
Copy link

Rigner commented Nov 22, 2018

Description
We cannot use any code from the addons as an API. I tried to use the IslandLevelCalculatedEvent, but once i try, i get a NoClassDefError, prolly because only your custom ClassLoader knows about this class, and not the normal JVM one. Would you know how to get around this problem ? For now i moved the event from addon-level to bentobox to fix this issue locally so i can continue working.

Steps to reproduce the behavior:
Example code :

import bentobox.addon.level.event.IslandLevelCalculatedEvent;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;

import java.util.LinkedList;
import java.util.List;

public class IslandListener implements Listener
{
    @EventHandler
    public void onIslandLevelChange(IslandLevelCalculatedEvent event)
    {
        // Code
    }
}

Expected behavior
Be able to use the event

Screenshots
/

Server Information:

  • Database being used (Flat file, MySQL, Mongo): Flat file
  • OS: Debian 8
  • Java Version: Java 8 (191)
  • BentoBox version: 0.14.1
  • Addons installed? BSkyBlock 0.5.0-SNAPSHOT, BentoBox-Challenges 0.2.0-SNAPSHOT, Level 0.1.0-SNAPSHOT
  • Other plugins? Only the one i'm developing

Additional context
Note : This class is definitely loaded after Bentobox and its addons are fully enabled

@tastybento tastybento added Type: Bug Status: Under investigation Investigating the interest and the feasability of the issue. labels Nov 22, 2018
@tastybento tastybento self-assigned this Nov 22, 2018
@tastybento
Copy link
Member

This does not appear to be possible because of Java's visibility principle with class loaders. The parent class loader (in this case org.bukkit.plugin.java.PluginClassLoader) can only see classes it loads itself and not classes loaded by the custom class loader AddonClassLoader. So therefore, only other add-ons can see the event class. You will need to write an add-on to use this event right now.

Do you agree with this or do you see a way that this could be done?

@Poslovitch
Copy link
Member

Hmmm... If that's the case, that may cause a lot of problems.
I wonder if we could design an API that'd get its inspiration from Bukkit's ServiceProvider.

@tastybento
Copy link
Member

I just spent a few more hours looking at this and I cannot find a way change Bukkit's JavaPlugin class loader because it does not use the Thread class loader (like JNDI does) so it's not possible to switch it out to a custom one. So, in summary, class visibility is as follows:

Addons <> Addons - OK
Addons > Plugins - OK
Plugins <> Plugins - OK
Plugins > Addons - Not possible

The reason is because Plugins are "parents" in terms of class loading of Addons. Even BentoBox itself cannot see into Addons. BentoBox can only see what Addons tell it or store within it. This is seen as a feature in Java because it enables class isolation.

The only way to enable Addons to be visible to Plugins would be to remove the whole concept of Addons and just make them all Plugins. I'm not sure we want to do that, but it is a possible solution if there is a huge requirement that Plugins can see Add-on classes.

One work around is that if an author wants to talk to another add-on from a plugin, they write an add-on to do that and send the data to their plugin.

I'll leave this open for a few more comments, but there's not much to do here.

@Rigner
Copy link
Author

Rigner commented Nov 24, 2018

Alright, thanks for the help.

I was trying not to create an addon, but i guess i won't have the choice.
Gonna do that in the day, and tell you how it goes.

@Poslovitch
Copy link
Member

@tastybento Maybe we should just drop the idea of addons?

@Rigner
Copy link
Author

Rigner commented Nov 25, 2018

For now, I made my own addon to access these events. It just contains a simple listener which calls my own event. Kind of a loss of performance, but it's ok.

I think the concept of addons is not bad, but we would need a way to access them if needed. I think the events should be enough, and in that case, why not just moving all events / possible API features to BentoBox ?

tastybento added a commit that referenced this issue Nov 26, 2018
@tastybento
Copy link
Member

@Poslovitch Hmm, no, I don't think so. Other than this one issue, they work really well.

@PSNRigner It's not possible to move all possible event aspects to BentoBox because add ons are going to be independently developed and I can't predict what methods or data they will use, but I can add an a simple key-value map that addons can supply, i.e., like metadata.

I've made a pull request here for review: #354

I'm open to other ideas if there Is there a better way to do this.

@Poslovitch
Copy link
Member

Just merged the PR, it looks good.

@Rigner
Copy link
Author

Rigner commented Nov 26, 2018

Yeah I see, thanks. Looking at the commit, there's no way to "identify" the event ?

BentoBoxWorld/Level@5eea419#diff-002a5eaaf2e4fd81839ca16f872d12beR59

You're putting all the parameters, but a developer cannot know what kind of event it is, we can only know the contents of it, i think a string id would be necessary, like "IslandLevelCalculatedEvent" or something

@tastybento
Copy link
Member

@Poslovitch @PSNRigner Okay, I'll look at this again this weekend.

tastybento added a commit to BentoBoxWorld/Level that referenced this issue Dec 9, 2018
BentoBoxWorld/BentoBox#352

Rather than overriding the existing getEventName() method for Events, I
decided to keep it as a key-value reference.
@tastybento
Copy link
Member

Okay, I added another key-value pair to reference the underlying event. I did play with overriding the getEventName() method from Event, but that would cause the AddonEvent itself not to have a correct name. Anyway, this way the underlying event can be understood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Under investigation Investigating the interest and the feasability of the issue.
Projects
None yet
Development

No branches or pull requests

3 participants