Apply damage to original attack target #368

amir-arad · 2023-03-18T12:54:41Z

Add the possibility to inflict damage from an attack to the original target of the attack. The original target is determined at the time of the attack, so even if the atytacking player changed target the damage will go to the right actor.

Protected by applyDamageOption (preexisting) feature flag

fixes #287

dont show context menu or button to apply damage unless possible

wyrmisis

I haven't been able to test the changes, but everything here looks reasonable to me. I left some general code quality questions.

src/module/chat.ts

- extract `applyDamageToTarget` function - return early - improve ui errors to edge cases

wyrmisis · 2023-03-21T21:42:52Z

src/module/chat.ts

+    case CONFIG.OSE.apply_damage_options.selected:
+      return !!canvas.tokens?.controlled.length;
+    default: {
+      console.log('unknown setting');


I think we should make this more specific, and a UI warning. We shouldn't end up here, but if we do, it'd be helpful if a user could tell us how they got here!

wyrmisis · 2023-03-21T21:43:31Z

src/module/chat.ts

@bakbakbakbakbak Just a heads up, I forget if you had tests written for this file or the other stuff in this PR, but this might change things for you.

Thanks for the notice, I'll take a look at this

probably best to revisit in a later revision of 0.8.x and leave the test as-is until after merging. We're probably expecting a non-zero amount of other surprises after such a large merge

Yeah, I am having quite a bit on my plate right now either, and I have noticed that I procrastinate on this check here.

wyrmisis

I'm good with the changes here, but I'd appreciate if someone else could have a look!

Changes addressed; re-reviewed.

anthonyronda

The PR didn't seem to work as intended (got the new UI error message with each attack attempt) and in debugging I discovered a cause, shown below

anthonyronda · 2023-03-28T17:12:27Z

src/module/config.ts

+  apply_damage_options: {
+    selected : "selected",
+    targeted : "selected",
+    originalTarget : "selected",
+  },


this causes an error in the switch statement. It will always be the default case unless selected is the selected system setting

anthonyronda · 2023-03-28T17:16:07Z

src/templates/chat/roll-attack.html

-        <div class="chat-target">
-            vs {{result.victim}}
+        <div class="chat-target" data-id="{{result.victim.document.uuid}}">
+            vs {{result.victim.name}}


I like this :)

anthonyronda · 2023-03-28T17:16:27Z

src/module/chat.ts

+    case CONFIG.OSE.apply_damage_options.selected:
+      return !!canvas.tokens?.controlled.length;
+    default: {
+      ui.notifications?.error(`unexpected OSE setting applyDamageOption: ${applyDamageOption}`);


Since I'm requesting changes, I think it's appropriate to ask to use a localized string here

Add the string itself to en.json only, under the other error strings

- fix `apply_damage_options` mistake - make ui messages support multilanguage

anthonyronda

Confirmed working in Foundry

LGTM

anthonyronda · 2023-03-28T18:35:20Z

src/module/chat.ts

@@ -11,7 +11,10 @@ function canApplyDamage(html: JQuery) {
    case CONFIG.OSE.apply_damage_options.selected:
      return !!canvas.tokens?.controlled.length;
    default: {
-      ui.notifications?.error(`unexpected OSE setting applyDamageOption: ${applyDamageOption}`);
+      ui.notifications?.error(game.i18n.format("OSE.error.unexpectedSettings", {
+        configName: 'applyDamageOption',


Oh nice, good idea :D

anthonyronda · 2023-03-28T18:40:38Z

Thanks for your contribution @amir-arad 🎉

@all-contributors please add @amir-arad for code

allcontributors · 2023-03-28T18:40:47Z

@anthonyronda

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @amir-arad! 🎉

anthonyronda · 2023-03-28T18:42:11Z

@amir-arad please also take a look at #59 and tell me your preferred name and optional email for entering in the AUTHORS file :)

amir-arad added 3 commits March 17, 2023 11:46

add victim ID to attack chat message

7600edd

add setting to hit original target of attack

919788d

hit original target of attack

5615806

dont show context menu or button to apply damage unless possible

wyrmisis previously requested changes Mar 20, 2023

View reviewed changes

src/module/chat.ts Outdated Show resolved Hide resolved

src/module/chat.ts Outdated Show resolved Hide resolved

src/module/chat.ts Outdated Show resolved Hide resolved

amir-arad added 2 commits March 20, 2023 17:31

code review fixes

e651df8

- extract `applyDamageToTarget` function - return early - improve ui errors to edge cases

extract apply damage options enum to CONFIG.OSE

4c51f92

amir-arad requested a review from wyrmisis March 20, 2023 17:56

wyrmisis reviewed Mar 21, 2023

View reviewed changes

more helpful error message for unknown setting

36e610c

amir-arad requested review from wyrmisis and anthonyronda and removed request for wyrmisis and anthonyronda March 28, 2023 09:27

anthonyronda requested changes Mar 28, 2023

View reviewed changes

review fixes:

5576ec5

- fix `apply_damage_options` mistake - make ui messages support multilanguage

amir-arad requested a review from anthonyronda March 28, 2023 18:25

anthonyronda approved these changes Mar 28, 2023

View reviewed changes

anthonyronda merged commit 1459044 into vttred:main Mar 28, 2023

allcontributors bot mentioned this pull request Mar 28, 2023

docs: add amir-arad as a contributor for code #376

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply damage to original attack target #368

Apply damage to original attack target #368

amir-arad commented Mar 18, 2023

wyrmisis left a comment

wyrmisis Mar 21, 2023

amir-arad Mar 23, 2023

wyrmisis Mar 21, 2023

bakbakbakbakbak Mar 22, 2023

anthonyronda Mar 27, 2023

bakbakbakbakbak Mar 27, 2023

wyrmisis left a comment

anthonyronda left a comment

anthonyronda Mar 28, 2023

amir-arad Mar 28, 2023 •

edited

Loading

anthonyronda Mar 28, 2023

anthonyronda Mar 28, 2023

amir-arad Mar 28, 2023

anthonyronda left a comment

anthonyronda Mar 28, 2023

anthonyronda commented Mar 28, 2023

allcontributors bot commented Mar 28, 2023

anthonyronda commented Mar 28, 2023

Apply damage to original attack target #368

Apply damage to original attack target #368

Conversation

amir-arad commented Mar 18, 2023

wyrmisis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyrmisis left a comment

Choose a reason for hiding this comment

anthonyronda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amir-arad Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonyronda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonyronda commented Mar 28, 2023

allcontributors bot commented Mar 28, 2023

anthonyronda commented Mar 28, 2023

amir-arad Mar 28, 2023 •

edited

Loading