Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make almost all of MacroButtonProperties fields non-null #4052

Conversation

kwvanderlinde
Copy link
Collaborator

@kwvanderlinde kwvanderlinde commented May 10, 2023

Identify the Bug or Feature request

Fixes #4051

Description of the Change

The direct cause of the bug was the use of nullable fields together with non-nullable DTO fields in MacroButtonProperties.toDto(). This PR is a general clean up of MacroButtonProperties with regards to the nullability of its fields, so that such things are less likely to occur in the future. In the end, all fields were made non-nullable, aside from the transient field tokenId.

The group of Boolean fields in the "comparison customization" section were in practice not null since readResolve() already handled them, there was special logic to fix them up, and callers never set them to something null (even though the setters did not enforce this). This has been reworked so that only readResolve() is responsible for replacing null with a valid value, while everything else can assume a non-null value. As a result, the getters and setters now deal in primitive booleans, and there is no more need for fixOldMacroSetCompare() anymore.

Another set of fields followed a pattern of allowing null values in the field itself, but getters were made responsible to update them when requested. While this guarded external callers from these invalid nulls, the class was still susceptible to unexpected nulls when directly accessing its own fields, e.g., as was done in toDto(). These fields have now been reworked to be given an initial value, to have the setters capture null values and replace them with something valid, and to have readResolve() likewise replace null values from deserializing. Ideally the setters would not even be called with null values, but it wasn't worth exhaustively checking if that's the case, so for now they will check.

After those changes, lots of redundant null checks could be removed in in MacroButtonProperties and elsewhere (some newly redundant cases, some already existing).

The hash codes have been changed as a result of not keeping the 0 case for null. But that should be immaterial unless we've gone off the rails already.

A couple miscellaneous changes are also included:

  • Some addition of braces to single line if
  • The transient field button was removed since it was only set and copied, but never otherwise referenced.
    • Its neighbour, tokenId is used and so was left as-is as the only remaining nullable field.
  • In modifySortString(), StringBuilder has replaced StringBuffer as it is lighterweight and there is no multithreading concerns.

Possible Drawbacks

Should be none.

Documentation Notes

N/A

Release Notes

  • Fixed a bug where old macros prevented campaigns from being opened.

This change is Reviewable

Most of the fields were either close to being non-nullable, or had getters that enforced non-nullability for callers. As
a result, most of them were readily made truly non-nullable, which enabled us to avoid the need for `null` checks
outside of setters and `readResolve()`.

`StringBuilder` is now used locally instead of `StringBuffer` in `modifySortString()` as it is lighterweight and there
is no multithreading concerns.
@kwvanderlinde kwvanderlinde force-pushed the bugfix/4051-macro-button-properties-toDto-npe-on-missing-tooltip branch from 8880946 to 53679f1 Compare May 10, 2023 22:31
@cwisniew cwisniew added this pull request to the merge queue May 10, 2023
@cwisniew cwisniew added the bug label May 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 10, 2023
@cwisniew cwisniew added this pull request to the merge queue May 11, 2023
Merged via the queue into RPTools:develop with commit d93687b May 11, 2023
@kwvanderlinde kwvanderlinde deleted the bugfix/4051-macro-button-properties-toDto-npe-on-missing-tooltip branch May 11, 2023 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

[Bug]: NPE when loading campaign with old macros.
2 participants