Too long constructor

Discussion in 'Plugin Development' started by matejdro, Feb 20, 2011.

Thread Status:
Not open for further replies.
  1. Offline

    Acru

    What if someone is using a slightly dated server for one reason or another? Won't removing the constructor break the plugin for builds older than 45?

    As @The_Wrecker said, changing the plugin directory, for instance to one used by multiple plugins.

    The super() automatically loads the plugin's config.yml file, what if you have a config.yml file shared by multiple plugins!

    In my case I'm also loading a second yml file in the plugin constructor in the same way. (Though this could be moved to enable, I had it there to be consistant. Eg, not one settings file loaded on the super constructor, and the other on enable.)

    In addition, if I am not mistaken, Bukkit will eventually allow you to enable and disable plugins live, no? What if there are things you don't want to be done on each enable and disable, such as static memory allocation for the plugin?
    --- merged: Feb 21, 2011 11:04 PM ---
    More:

    I looked over the specific change, and I see that the stuff that was in the super constructor was moved to a function called initialize. (Along with the yml load.) Perhaps this can be overridden, though I'm still concerned about maintaining compatibility with a broad range of builds, if I can.
     
  2. Offline

    cjc343

    I still feel like there has to be a way to do it without using the long constructor. Perhaps Acru is heading down the right path with thinking about overloading initialize -- call the parent method, then do what you want.

    If you remove the constructor outright, builds older than 45 will not work. The easiest solution (imho) is to make a pre-45 and post-45 build available until 99% of people have transitioned to post-45 builds.
    I haven't played around with overriding initialize (although I've played around more with constructors since I said I hadn't fully tested) but it certainly sounds like a better place to be doing things that rely on information from JavaPlugin than the constructor.

    My main reason for believing that this stuff shouldn't go in the constructor is that is uses information provided by JavaPlugin, and the bukkit devs seem to be pushing plugin devs away from doing JavaPlugin related stuff in the constructor. Ideally, we'll get some input from then as to how they would like this to be done, and whether overriding initialize is the way to go about it.
     
  3. Offline

    The_Wrecker

    I'd be fine with overriding initialize... Seems it could be a solution. Just not sure if I should do that.

    I also suggested an onload and maybe an unload method before somewhere, but since initialize is already there...
     
  4. Offline

    cjc343

    Hopefully we'll hear from @EvilSeph or someone else on the dev team about whether this is a good solution or not.
     
  5. Offline

    Acru

    This is a sticking point for me, though. Have any idea on how to do something like;?
    if(build < jenkins#45) super(the long stuff)​

    I have a function that can get the build number if its available, (and it should always be available for jenkins builds after 35), but java requires that the super call be the first line in the constructor, and I can't think of an alternate way to call the super... *thinks more* *asks google*
     
  6. Offline

    cjc343

    I haven't tried this, so I don't know if it'd work. The first step would be determining if my initial assumption is true, either by testing or looking at bukkit's code.

    Initial assumption: If the (overridden) default constructor and TSLPC both exist, newer builds will call the default constructor.

    If that's the case, simply create two constructors. If it's not, then every other thought that flashed through my head isn't possible either.

    If TSLPC is still called by default... we're back where we started. Two versions and only support the new one, or one version and only support the new one...
     
  7. Offline

    Acru

    There is only the one constructor for JavaPlugin in both pre-45 and post-45.

    Pre-45 it is a required call, to init.

    Post-45, the init is done in the initialize function, jut before the call to onEnable, but if the constructor is called at all, it logs that error. (The super constructor logs the error.)

    What has me stumped is how to NOT call the super constructor on post-45 without removing the constructor and breaking pre-45...
    --- merged: Feb 22, 2011 3:03 AM ---
    @cjc343, In the code, it does this:
    Code:
    try {
         Constructor<? extends JavaPlugin> constructor = plugin.getConstructor(PluginLoader.class, Server.class, PluginDescriptionFile.class, File.class, File.class, ClassLoader.class);
         result = constructor.newInstance(this, server, description, dataFolder, file, loader);
    } catch (NoSuchMethodException ex) {
         Constructor<? extends JavaPlugin> constructor = plugin.getConstructor();
         result = constructor.newInstance();
    }
    result.initialize(this, server, description, dataFolder, file, loader);
    
    So it calls your long constructor if it exists, and it prints the warning message if your long constructor calls the super. So you NEED to call the super for pre-45 and NOT call the super for post-45.​


    --- merged: Feb 22, 2011 3:10 AM ---
    @EvilSeph, I'd like your feedback on the above too. I just want to make a plugin that will work for both pre and post build 45.

    --- merged: Feb 22, 2011 5:13 AM ---
    Ok, this is annoying.
    You can so easily get around the warning message and still use the constructor the same way by changing;
    Code:
    public Fillr(PluginLoader pluginLoader, Server  instance, PluginDescriptionFile desc, File folder, File plugin,  ClassLoader cLoader) {
         super(pluginLoader, instance, desc, folder, plugin, cLoader);
         // More code.
    }
    
    into;
    Code:
    public Fillr(PluginLoader pluginLoader, Server  instance, PluginDescriptionFile desc, File folder, File plugin,  ClassLoader cLoader) {
         super();
         // More code.
    }
    
    But all that does is break the plugin for pre-build-45.
    And doesn't solve what @EvilSeph put the change in there for.
    And the change prevents supporting pre-45 if you don't want the warning message showing!

    All in all, I would like to see the warning message removed, as I will not be updating the plugin until I think that most users have already updated past build jenkins 45... I mean, its one day old, only those on the cutting edge will have updated! You can put the message back in after a few weeks maybe?
    --- merged: Feb 22, 2011 5:42 AM ---
    I would suggest you revert the change for the above reason of pre-build-45 support.

    Or people with builds older than jenkins build 45 will get this error...
    --- merged: Feb 22, 2011 5:48 AM ---
    Also:
    Code:
    try {
         Constructor<? extends JavaPlugin> constructor = plugin.getConstructor(PluginLoader.class, Server.class, PluginDescriptionFile.class, File.class, File.class, ClassLoader.class);
         result = constructor.newInstance(this, server, description, dataFolder, file, loader);
    } catch (NoSuchMethodException ex) {
         Constructor<? extends JavaPlugin> constructor = plugin.getConstructor();
         result = constructor.newInstance();
    }
    result.initialize(this, server, description, dataFolder, file, loader);
    
    could be changed to
    Code:
    try {
         Constructor<? extends JavaPlugin> constructor = plugin.getConstructor();
         result = constructor.newInstance();
    } catch (NoSuchMethodException ex) {
         Constructor<? extends JavaPlugin> constructor = plugin.getConstructor(PluginLoader.class, Server.class, PluginDescriptionFile.class, File.class, File.class, ClassLoader.class);
         result = constructor.newInstance(this, server, description, dataFolder, file, loader);
    }
    result.initialize(this, server, description, dataFolder, file, loader);
    
    to allow this...
     
  8. Offline

    cjc343

    One explicitly defined constructor. The default constructor still exists, even if it doesn't work or isn't explicitly defined.

    I really think you're trying to overengineer this. Bukkit is not officially released yet. There have been, and will continue to be points where some, or all plugins are broken by a change. Plugins before the change will not be forwards compatible, plugins after will not be backwards compatible.

    For now, they've left in code that will use the constructor if it's present, which means forwards compatibility is present, but users will be presented with warnings that your plugin doesn't yet follow the latest standards. They could have removed the old constructor entirely and forced everyone to rebuild their plugins that way. Is that a preferrable route?

    That'd be because you're now calling the default constructor instead of the TSLPC.

    I'm guessing that in a few weeks we'll have already seen the first official release. The time simply isn't there to transition slowly, and as users and plugin developers, we should know that we are dealing with two unfinished products which can break our work whenever they wish.

    Yeah, except for the tiny problem that the default constructor is probably still implicitly defined.
     
  9. Offline

    Acru

    No, the default constructor isn't made if you define any other constructor, this is why just super(); throws an error in pre-45, there is no constructor JavaPlugin() just the long one.
     
  10. Offline

    cjc343

    I stand corrected.
     
  11. Offline

    Acru

    I'm just annoyed as the break was unnecessary at the transition from build 44 to 45.
    It was forced, and could have been delayed.
    Because at least one of the popular core plugins updated, it means everyone will have to update their servers.

    I support running builds back to b231, though the plugin won't enable unless you have at least b323.
    But it can print out a polite request to update your craftbukkit, rather than throw an exception which not helpful to the server operators.

    Meh, maybe I'll be better tomorrow but right now I'm just annoyed~ o3o
    Maybe @EvilSeph will reconsider, but won't know til I get his take on the big long merged chunk above.
    In any case I'll have to live with the message popping up at me for a while on post-45, if I want to print out the mentioned polite message to upgrade for those with older builds.
     
  12. Offline

    cjc343

    That's part of why I agree with your next statement. Also why I think multiple versions can be good. I have versions (of TS) posted back through 231, and a version that's rumored to work on 210. Versions previous to that should be supported by the original plugin, but that's not very publicly stated.

    Warnings are certainly a step below an exception, showing that they seem to be doing just that in this case. There are many other changes, which, since bukkit isn't finished, have previously broken plugins without warning. At one point, there were constructor changes that broke every plugin without warning.

    This is why I fully support multiple versions. Leave the old plugin up. Label it with known CB version numbers.

    Above it, put a prominent link to the new version, which doesn't throw warnings, but works with the newer versions of CB.

    For now, most users are going to be clicking the old link. Support is not required anymore. It's an old version.

    After a few more big ones migrate, the newer versions will become popular.

    E: also, I'm aware that TS is not yet updated for the latest builds. Really hoping a user will complain first, since it should be a non-breaking change.
     
  13. Offline

    Acru

    Okay, I did it! This won't be necessary after all~ I am happy again!

    If you have the following;
    Code:
    public Fillr(PluginLoader pluginLoader, Server instance, PluginDescriptionFile desc, File folder, File plugin, ClassLoader cLoader){
        super(pluginLoader, instance, desc, folder, plugin, cLoader);
    }
    
    you can change it to;
    Code:
    public Fillr(){}
    public Fillr(PluginLoader pluginLoader, Server instance, PluginDescriptionFile desc, File folder, File plugin, ClassLoader cLoader){
        super(pluginLoader, noNag(instance, true), desc, folder, plugin, cLoader);
        noNag(instance, false);
    }
    
    private static Server noNag(Server instance, boolean enable){
            try{
                Method        getlogger = Server.class.getMethod("getLogger", (Class[]) null);
                Logger        thelogger = (Logger) getlogger.invoke(instance, (Object[]) null);
    
                if(enable) thelogger.setLevel(Level.SEVERE);
                else thelogger.setLevel(null);
            }
            catch (NoSuchMethodException ex){}
            catch (IllegalArgumentException ex){}
            catch (IllegalAccessException ex){}
            catch (InvocationTargetException ex){}
            return(instance);
        }
    
    and the nag logger message will be blocked on all builds except 45-49 as those use println instead of a logger, but this works with builds 50+ while supporting the older jenkins and bamboo builds.

    Oh, you'll also need some extra imports;

    import java.util.logging.Logger;
    import java.util.logging.Level;
    import java.lang.reflect.Method;
    import java.lang.reflect.InvocationTargetException;

    Note that while I'm getting rid of the nag, I'm not doing anything that the nag was intended to stop (or at least bring attention to.) And I threw in a simple empty constructor too for later, as its not made by default.
    --- merged: Feb 22, 2011 12:01 PM ---
    Apparently not, tried to do it just now and noticed that its marked as final... D:
     
  14. Offline

    KaBob

    It turns out it was crashing for me because PluginDescriptionFile pdfFile = this.getDescription(); was up near the top (not inside the constructor either), once I moved it into onenable it worked.
     
  15. Offline

    Raphfrk

    Also, you can't set final variables now ... which is annoying.
     
Thread Status:
Not open for further replies.

Share This Page