First attempt at allowing dynamic portals #118
Conversation
Allow other plugins to create portals through the api.
|
@Maxopoly @ProgrammerDan Thoughts on how to make this better? |
| * portal type. | ||
| * @param plugin Your plugin's name. | ||
| */ | ||
| public void addPortalType(int id, String plugin) { |
There was a problem hiding this comment.
Those ids should be managed in BetterShards, what you are doing here will require all possible users of this API to coordinate with each other to avoid overlap, which is exactly the opposite of what you want in an API.
There was a problem hiding this comment.
No they won't because when they get the portal id it will be specific in that specific server set up.
So better shards can create 3 portals and they will be given id 0,1,2.
Then some other plugin can create a portal and it will be given the id of 3.
| return null; | ||
| } | ||
| return event.getPortal(); |
There was a problem hiding this comment.
Are you sure you want the API to offer changing the portal being loaded? It means that if I request a portal with name "XYZ" from the database, I might get anything else, because of listeners interferring. You should be careful to separate database storage and gameplay mechanics here.
There was a problem hiding this comment.
Other plugin listeners should only modify portals they care about. If a plugin is doing something they shouldn't there are additional problems anways.
| @@ -221,6 +227,11 @@ public Boolean call() { | |||
| + "inv_id INT NOT NULL," | |||
| + "last_upd TIMESTAMP NOT NULL DEFAULT NOW()," | |||
| + "PRIMARY KEY (uuid, inv_id));"); | |||
| this.db.registerMigration(3, false, "CREATE TABLE IF NOT EXISTS portalVersion(" | |||
There was a problem hiding this comment.
This is redundant with the portal_type column in the table "createPortalDataTable". It also doesnt convert over any preexisting portals
There was a problem hiding this comment.
It isn't. portal_type doesn't offer any unique way for other plugins to know what ids are taken by who.
In portals manager it registers the portals there.
| @@ -489,4 +493,31 @@ public void blockPlaceEvent(BlockPlaceEvent event) { | |||
| event.setCancelled(true); | |||
| } | |||
| } | |||
|
|
|||
| @EventHandler(priority = EventPriority.LOWEST) | |||
| public void portalLoadEvent(PortalLoadEvent event) { | |||
There was a problem hiding this comment.
Listening to your own events, especially when that event is only created in one place, is bad practice
There was a problem hiding this comment.
I believe it's only bad practice when you don't mention where to find that specific code which in fairness I didn't. I was also not sure how I should do this. I was thinking I could keep the code in the load method but then not call the event for BetterShards portals but then there is a state where the event is only called for other plugins making it inconsistent
| wb.setServerName(event.getServerName()); | ||
| event.setPortal(wb); | ||
| } | ||
| else if (db.getPortalID(BetterShardsPlugin.getInstance().getName(), 2) == specialId) { |
There was a problem hiding this comment.
There has to be an easier way than doing 3 separate sync db queries here.
There was a problem hiding this comment.
Yea I can. Will do.
|
Also self note need to work on making the create portal command compatible with these changes. |
Plugins register their portals with the BetterShardsAPI class. BetterShards builds the class and loads all the generic portal attributes, inside each sub Portal class there is a method where each plugin can specify how they want extra attributes loaded.
| PreparedStatement addPortalLoc = connection.prepareStatement(DatabaseManager.addPortalType)){ | ||
| addPortalLoc.setString(1, plugin); | ||
| addPortalLoc.setInt(2, id); | ||
| addPortalLoc.execute(); |
There was a problem hiding this comment.
It's worth noting that inserts / updates / deletes can silently fail. Always use executeUpdate() and check the value returned. If != to the number of rows you thought you'd be modifying, you've got a failure-state.
| * @param name The name of the portalType that will show up to players. | ||
| */ | ||
| public static <E extends Portal> void registerPortal(int plugin_id, String plugin, Class<E> portal, String name) { | ||
| BetterShardsPlugin.getDatabaseManager().addPortalType(plugin_id, plugin); |
There was a problem hiding this comment.
Given that this is called from startup and repeatedly elsewhere, shouldn't you be checking if this type exists before adding it? Even though your uniqueness pkey guarantee will probably prevent a new row from being generated, relying on an error is risky... better to explicitly check imho.
| } | ||
|
|
||
| public boolean isOnCurrentServer() { | ||
| return isOnCurrentServer; | ||
| } | ||
|
|
||
| public Portal setIsOnCurrentServer(boolean value) { | ||
| isOnCurrentServer = value; |
There was a problem hiding this comment.
Does dirty need to be set here?
| } | ||
|
|
||
| public Portal setFirstLocation(LocationWrapper loc) { | ||
| first = loc; |
There was a problem hiding this comment.
Does dirty need to be set here? I'd imagine so.
| } | ||
|
|
||
| public Portal setSecondLocation(LocationWrapper loc) { | ||
| second = loc; |
There was a problem hiding this comment.
Same -- I'd assume setting dirty is indicated.
Allow other plugins to create portals through the api.