Introduce TARGET versions of failure/success notifications. #3572
Conversation
This allows for fixing #3558 with a corresponding chsange to the Feudal Japan map setting the new property.
Codecov Report
@@ Coverage Diff @@
## master #3572 +/- ##
============================================
- Coverage 22.07% 22.07% -0.01%
Complexity 5937 5937
============================================
Files 838 838
Lines 71921 71938 +17
Branches 11546 11546
============================================
Hits 15880 15880
- Misses 53949 53966 +17
Partials 2092 2092
Continue to review full report at Codecov.
|
asvitkine
changed the title
| dontSendTo.add(player); | ||
|
|
||
| final Collection<PlayerID> otherPlayers = getData().getPlayerList().getPlayers(); | ||
| otherPlayers.remove(player); |
There was a problem hiding this comment.
Would it be better to move otherPlayers to after the first sendNotificationToPlayers call since it isn't used until the second call?
| if (hasMessage(actionKey, TARGET_NOTIFICATION_SUCCESS)) { | ||
| return getMessage(actionKey, TARGET_NOTIFICATION_SUCCESS); | ||
| } else { | ||
| return getNotificationSuccessOthers(actionKey); |
There was a problem hiding this comment.
Not following this logic. Shouldn't this return "NONE"?
There was a problem hiding this comment.
This is so maps that implement OTHER notifications continue to work as-is. That is, if TARGET is not specified then OTHER is sent to all other players, as is now.
There are other ways to do this, such as returning NONE and having logic in the caller to check for that and in the case of NONE, send OTHER to all players. This way is actually a bit cleaner since the caller doesn't need to have complicated logic to handle it.
But this way is also more flexible - for example if mapmaker wanted to only send notifications to players and others, but not target - they can do it with this set up (by setting TARGET to NONE). If we instead did it the other way, then this wouldn't be possible.
There was a problem hiding this comment.
Ok, I now see what you are doing here. I'm fine with that as I was just thinking about it the first way you describe.
| if (hasMessage(actionKey, TARGET_NOTIFICATION_FAILURE)) { | ||
| return getMessage(actionKey, TARGET_NOTIFICATION_FAILURE); | ||
| } else { | ||
| return getNotificationFailureOthers(actionKey); |
There was a problem hiding this comment.
Not following this logic. Shouldn't this return "NONE"?
|
@asvitkine Looks pretty good. Added a few comments. |
Introduce TARGET versions of failure/success notifications.
This allows for fixing #3558
with a corresponding change to the Feudal Japan map setting the
new properties.
Details:
TARGET_NOTIFICATION_SUCCESS will send to target(s) only.
Everyone else (not active player, not targets) will continue to receive OTHER_NOTIFICATION_SUCCESS, if it's value is not NONE (as is now).
If TARGET_NOTIFICATION_SUCCESS does not exist in the properties, then OTHER_NOTIFICATION_SUCCESS is used for everyone except active player.
This way, maps continue to work as-is unless updated.
The Feudal Japan map will be updated to add TARGET_NOTIFICATION_SUCCESS
and set OTHER_NOTIFICATION_SUCCESS to NONE.
Same thing for _FAILURE notifications.