[Feature]: Additional functions for Javascript Functions

ColdAnkles commented 1 year ago

Describe the Problem

MTScript has functions that can get/set token states, do easy isPC/isNPC and similar, yet nothing exists for the JS Token Object.

The Solution you'd like

Add additional functions to streamline token access/modification from JS.

Alternatives that you've considered.

Entirely possible to create your own JS Function that calls the relevant MTScript.evalMacro

Additional Context

Requesting, and submitting functions for some things I've found myself in need/desire of.

Baaaaaz commented 8 months ago

Tested in 1.15.0-rc.2 (see attached zipped token)

Test results: isPC() - okay. Noting it returns true/false but equivalent MTScript function returns 1/0. isNPC() - okay. Noting it returns true/false but equivalent MTScript function returns 1/0. setPC() - okay. setNPC() - okay. getType() - okay. Noting that there is not an equivalent MTScript function for parity's sake. Should there be? getState(stateName) - okay. setState(stateName, boolean) - okay. setAllStates(boolean) - okay. getActiveStates() - see comments below:

JSAPI: getActiveStates() vs MTScript: getTokenStates(tokenName) returns all the right array items, but not necessarily in the right order 🎹 ! I'm not sure what determines the array order for either or if this is even significant?
JSAPI: getActiveStates() vs MTScript: getTokenStates() can the JS function name match the MTScript function name (like other functions seemingly do?)

Wiki work: Suggest a new wiki page is created under https://wiki.rptools.info/index.php/js:MapTool to house the JSAPIToken functions listed here: https://github.com/RPTools/maptool/blob/develop/src/main/java/net/rptools/maptool/client/script/javascript/api/JSAPIToken.java as that seems to be a wiki gap at the moment?

4322 Dragon - 1.15.0-rc.2.zip

ColdAnkles commented 8 months ago

The wiki page for token object methods under JSAPIToken are documented here: https://wiki.rptools.info/index.php/js:Token

I don't really have any reason to keep it as getActiveStates and not change to match MTScript getTokenStates - it's what made sense to me at the time.

Baaaaaz commented 8 months ago

Thanks for the wiki link! I completely missed that... :) Maybe because the link at the bottom of https://wiki.rptools.info/index.php/js:MapTool has a link to that page displayed as the pluralised "JS: Tokens"

kwvanderlinde commented 8 months ago

Despite the discrepancy, I like the name getActiveStates() as it's clearly applied to a token. And MTScript's getTokenStates is an outlier there anyways.

If anything it could be shortened to getStates().

ColdAnkles commented 8 months ago

I think getActiveStates() is more explicitly only ones that are true, whereas getStates() might include all states and whether they're true or not.

Baaaaaz commented 8 months ago

Digging into it a bit more, MTScript getTokenStates() (with no token id argument) appears to behave differently to JSPAI getActiveStates() anyway, so actually a different name does seem more appropriate in the absence of any naming standards.

Maybe a design principle and a debate to had elsewhere, but should available JSAPI functions be a subset of equivalent MTscript functions? If so maybe there needs to be a standard that enforces some level of parity between the two, otherwise further divergence is a risk.

kwvanderlinde commented 8 months ago

Maybe a design principle and a debate to had elsewhere, but should available JSAPI functions be a subset of equivalent MTscript functions? If so maybe there needs to be a standard that enforces some level of parity between the two, otherwise further divergence is a risk.

No. There are plenty of examples in the MTScript functions where unfortunate decisions were made or had to be made, resulting in a lot of cruft and inconsistency over the years. It would not be good to carry that forward to the JS API just for parity.

This getTokenStates() function is a good example of inconsistency. Whereas most functions default to the current token when a token ID is not provided, getTokenStates() instead completely changes its behaviour to return any campaign states instead of active token states. It's better not to try and replicate that in a new API. Instead we should have different "spellings" for what are inherently different things.

Baaaaaz commented 8 months ago

Thanks for the insight about the cruft and inconsistency. What is apparent is that there are many forms of inconsistency here!

I wasn't strictly advocating replicating existing MTScript functions as is in JSAPI. Function parity in this particular case could be achieved if there was a new equivalent MTScript function created called getActiveStates() if that is the conventional and better way of doing it. If historic cruft and inconsistency is an issue, such a new function could start to provide a route to address specific consistency issues - assuming there is appetite to address this, and that inconsistent user-facing features can get deprecated.

Starting to document somewhere what the conventions are would be invaluable in building community knowledge, e.g. "MTScript token functions must default to the current token when a token ID is not supplied". This could provide new developers some initial guidance on required consistency, inform testers of general expected behaviours, simplify end-user experience, and also could be used as the basis to identify opportunities to address gaps and technical debt. Without any design principles or standards enforcing what is acceptable by constraining the solution there will always be the risk of more cruft and inconsistency going forward. There may be exceptions however, but those should be in the minority and need to be managed.

RPTools / maptool