onChangeMap add-on compliance, output added #4318
Conversation
| ZoneRenderer currentZR = MapTool.getFrame().getCurrentZoneRenderer(); | ||
| try { | ||
| var libs = new LibraryManager().getLegacyEventTargets(ON_CHANGE_MAP_CALLBACK).get(); | ||
| if (!libs.isEmpty()) { |
There was a problem hiding this comment.
IMO it's better to exit the method if libs.isEmpty(). Generally it's better to handle the conditions you don't want first. Otherwise they tend to be forgotten. You also get less indent.
| null, | ||
| Collections.emptyMap()); | ||
| } catch (InterruptedException | ExecutionException e) { | ||
| throw new AssertionError("Error retrieving library namespace"); |
There was a problem hiding this comment.
You probably want to add the original exception as inner exception or at least log it. Otherwise it will be difficult to determine the reason for the AssertionError if it ever occurs in a log file.
| ZoneRenderer currentZR = MapTool.getFrame().getCurrentZoneRenderer(); | ||
| try { | ||
| var libs = new LibraryManager().getLegacyEventTargets(ON_CHANGE_MAP_CALLBACK).get(); | ||
| if (libs.isEmpty()) return; |
There was a problem hiding this comment.
Please add braces and indentation to this if for readability.
| EventMacroUtil.callEventHandler( | ||
| ON_CHANGE_MAP_CALLBACK, | ||
| libraryNamespace, | ||
| currentZR.getZone().getId().toString() + "," + currentZR.getZone().toString(), |
There was a problem hiding this comment.
I think we should remove the second argument as it's not needed - if the event handler does need it, it can get it based on the ID.
The result of Zone#toString() is also not very predictable: depending on whether it's a GM or player client, and whether the name and display name match, there could be several possible values. So if we keep the parameter, we should pick either getName() or getDisplayName() for it.
There was a problem hiding this comment.
I glanced at the code and don't see an answer — why is the zone name being used instead of the id in the first place? If it's just informational, why not just the zone id and let the listener query other attributes that it wants?
There was a problem hiding this comment.
I assumingly thought there would be added value to include the textual name as it was readily available.
I will remove it.
@Jmr3366 I don't see anything in this PR that will address that point. Is it just the addition of parameters to |
|
@kwvanderlinde I'm not certain what was causing the edit map-cancel event trigger issue. When I found that issue, I was also requested to test and make it add-on compliant. Once converted the issue I described was gone. That said, this function is victim to how a rename of a map is really a copy, paste, delete (thus causing this event to fire). This update does not change that. |
Identify the Bug or Feature request
Feature #1574
Initial creation of
onChangeMapwas not add-on compliant.This also resolves a false/positive event trigger when entering the menu Map - Edit Map dialog - Cancel (the event will still trigger if variables are changed and selecting OK).
Description of the Change
Compliance, tested with add-ons
Added output
mapID,mapNamePossible Drawbacks
No additional drawbacks from prior PR
Release Notes
Any macro named
onChangeMaplocated on a library token will execute when the map is changed.The macro will also receive the
mapIDandmapNamethroughmacro.args.This change is