[kwvanderlinde]

Identify the Bug or Feature request

Resolves #6070

Description of the Change

There's a few semi-independent things going on. Mostly, it's that cleanup in MapToolLineParser motivated clean up elsewhere. The bulk of the changes outside of MapToolLineParser, MapToolVariableResolver, and Token are just callers being updated to reflect the method changes.

MapToolLineParser

The bulk of the change is in here. Most method overloads have been removed since they did not do anything interesting beyond saving callers a few characters here and there. This applies to:

• #runMacro()
• #rollString()
• #expandRoll()
• #parseExpression()
• #enterContext() (overload that was only used in tests)
• #parseLine()

#parseLine() also has a couple other simplifications:

1. It requires the caller to pass a MapToolVariableResolver. Passing null is no longer an option.
2. With the changes in (1), it became clear that the tokenInContext parameter would always be the same as resolver.getTokenInContext(). So the parameter (which had to be plumbed around in many places) has been removed.

The class was generally cleaned up as well:

• Fields (static or otherwise) are all at the top instead of interleaved with other parts of the class.
• Unnecessary code was removed (unnecessary generics, length checks, initializers, etc)
• Unused code was removed
• Methods are made private if they aren't needed outside MapToolLineParser.

While updating dependent code, some other unnecessary or unused code was identified and removed as well.

Token

The Token class no longer has anything to do with property evaluation. It only does property lookup. The evaluation portions were moved into MapToolVariableResolver that only knows how to evaluate properties of its tokenInContext. Additionaly, MapToolVariableResolver does not permit arbitrary evaluations (as Token#evaluateProperty() did), instead only permitted properties to be looked up and evaluated by the property name.

The main thrust of this change is to avoid any possibility of one token being asked to evaluate its own properties with a resolver that was set up for a different token. And many callers already have their resolver + token available, and so it's simpler for them with the new approach.

MapToolVariableResolver

A quality of life change in MapToolVariableResolver is the new #spawn() method that creates a new MapToolVariableResolver with the same tokenInContext. This simplifies a handful of places that create resolvers, and allows these call sites to continue to be correct into the future if we ever add to what can be "in context".

Also, the delayed actions were never used, so they have been removed. That also means we no longer need to call MapToolVariableResolver#initialize() or MapToolVariableResolver#flush().

MapToolLineParser contexts

#enterContext() has been simplified so it no longer handles the context == null case. This case was only ever hit inside parseLine(), and it is clearer to create the new context there. All other callers provided a non-null context.

Since #enterContext() no longer has to return a MapToolMacroContext, it (and related methods) instead returns an AutoCloseable that can be used to exit the context. Most callers were already using try/finally (or at least try/catch) to consistently exit their contexts, and this allows to avoid the explicit finally logic. It also fixes some "bugs", though these aren't hit in practice:

• EventMacroUtil#callEventHandlerOld() could exit the wrong context if an error was thrown while setting variables. Not actually hit since it is only used for onCampaignLoad which does not set variables.
• ExecFunction#runExecFunction() would not exit its context if an unhandled exception was thrown, though it did at least catch ParserException.

Possible Drawbacks

The change to MapToolLineParser#enterContext() had one change of behaviour, but I'm not sure right now how to expose it. I'm 99% sure the original behaviour is unintended.

If #enterContext() received a null parameter with an empty context stack, it would always set #macroPathTrusted to false, even if the generated context would have been trusted (i.e., if the player is a GM). This is specific to being called the first time from #parseLine() as calls from #enterTrustedContext() and #enterUntrustedContext() do not hit this case even if they are run on an empty stack and would add an identical context to the stack. This had resulted in an inconsistency where entering a top-level trusted context on an empty stack might set #macroPathTrusted to true, but it might not. Along with the comment in #enterContext(), I'm pretty confident the new behaviour is the intended behaviour:

// First time through set our trusted path to same as first context.

Documentation Notes

N/A

Release Notes

• Cleaned up macro evaluation code.

---

This change is