Change Reduce HP to Modify HP #33

Malechus · 2022-01-24T02:15:16Z

Adds buttons and moves UI to allow user to modify HP in either direction by increments. Addresses sander1095#20.

…lete buttons into InfoTab.

sander1095 · 2022-01-24T06:54:29Z

Hello! Super cool to see you are still working on this! I will try to take a look at this in ~9 hours. If i don't have the time, it'll be Wednesday instead.

Thanks for your cool contribution!

sander1095 · 2022-01-24T15:38:12Z

OverviewForm.cs

@@ -167,7 +167,7 @@ private void SaveButton_Click(object sender, EventArgs e)

            short? maxHP = TryParseNullable(MaxHPInput.Text.Trim());
            short? tempHP = TryParseNullable(TempHPInput.Text.Trim());
-            short? reduceHP = TryParseNullable(ReduceHPInput.Text.Trim());
+            short? reduceHP = TryParseNullable(amountHPInput.Text.Trim());


I think calling it ModifyHPInput would be better (PascalCase)

sander1095 · 2022-01-24T15:38:36Z

OverviewForm.cs

@@ -227,7 +227,7 @@ private void SaveButton_Click(object sender, EventArgs e)
                    if (reduceHP.Value < 0)
                    {
                        errors = true;
-                        ReduceHPLabel.ForeColor = Color.Red;
+                        modifyHPLabel.ForeColor = Color.Red;


I would also call this ModifyHPLabel (PascalCase)

sander1095 · 2022-01-24T15:38:55Z

OverviewForm.cs

@@ -167,7 +167,7 @@ private void SaveButton_Click(object sender, EventArgs e)

            short? maxHP = TryParseNullable(MaxHPInput.Text.Trim());
            short? tempHP = TryParseNullable(TempHPInput.Text.Trim());
-            short? reduceHP = TryParseNullable(ReduceHPInput.Text.Trim());
+            short? reduceHP = TryParseNullable(amountHPInput.Text.Trim());


this variable is still called reduceHP, I think it should be called ModifyHP

take a look at my other comments first. I am not sure if this code still needs to exist here, but that will be more clear after the rest of the code comments :)

sander1095 · 2022-01-24T15:40:13Z

OverviewForm.cs

@@ -295,11 +295,11 @@ private void SaveButton_Click(object sender, EventArgs e)
                {
                    //Right now it is unclear if the user has entered wrong input (for example: "dsfsdf") or just didn't didnt enter anything into the Reduce HP range


this code comment still refers to "reduce HP" instead of modify

sander1095 · 2022-01-24T15:42:49Z

OverviewForm.cs

+            character.HP = (short)(character.HP + addAmt);
+
+            UpdateDetailsTab(character);
+        }


If you look at the README.md you can see that the picture is now outdated. Could you replace that picture (see Images folder) with the new version with your changes with the same data?

sander1095 · 2022-01-24T15:48:34Z

OverviewForm.cs

@@ -620,5 +620,27 @@ private void UpdateNextTab(Character character)
            short outValue;
            return short.TryParse(val, out outValue) ? (short?)outValue : null;
        }
+
+        private void reduceButton_Click(object sender, EventArgs e)


The names of the button and method are camelcase and not as clear as other buttons. I would propse to make them CamelCase:

ReduceHPButton
IncreaseHPButton

You can do this in the Properties window in visual studio and of course in here as well :)

sander1095 · 2022-01-24T15:50:57Z

OverviewForm.cs

@@ -620,5 +620,27 @@ private void UpdateNextTab(Character character)
            short outValue;
            return short.TryParse(val, out outValue) ? (short?)outValue : null;
        }
+
+        private void reduceButton_Click(object sender, EventArgs e)


Clicking on the - or + button when there is no text in the input results in an exception. That should not happen! :)

The label should become red and nothing should happen :)

This is probably because of short.Parse(amountHPInput.Text); .

Replacing that with the TryParseNullable() function combined with the current code on MASTER on rule 222 and 298 might help :)

sander1095 · 2022-01-24T15:55:44Z

OverviewForm.cs

+            character.HP = (short)(character.HP + addAmt);
+
+            UpdateDetailsTab(character);
+        }
    }
 }


Why did you move the delete/save buttons?

I quite liked them where they were :D

sander1095 · 2022-01-24T15:59:05Z

OverviewForm.cs

+            character.HP = (short)(character.HP + addAmt);
+
+            UpdateDetailsTab(character);
+        }
    }
 }


There are still some bugs:

If you give a user 44 HP, and you reduce it by 100, you would expect the character to die. this happens with existing code. This does not happen with yours; it updates the details tab in a weird way :)

If you increase someone's HP above their max HP, some weird stuff happens. I would expect that if you give someone 10 HP when they have 9 out of 10, it would just be set to 10 and stop there.

Changing the HP of a player that is not on the detail tab, changes the detail tab to show that player. That doesn't happen with the current setup and shouldn't happen with the new setup! :)

I'd like to hear your thoughts on this one. I believe that if you modify the HP of the current player, it should also modify the HP on the left. and in the list. Right now it doesn't update in the list or on the information tab. Take a look at the current behavior on MASTER and try to replicate that :). This means that the + and - buttons perform a SAVE call for you

sander1095 · 2022-01-24T16:03:29Z

OverviewForm.cs

@@ -620,5 +620,27 @@ private void UpdateNextTab(Character character)
            short outValue;
            return short.TryParse(val, out outValue) ? (short?)outValue : null;
        }
+
+        private void reduceButton_Click(object sender, EventArgs e)


I'd move these 2 new private methods up a bit, put them after the _clicked methods so they are all nice and close to eachother :)

sander1095 · 2022-01-24T16:13:23Z

I think it'd be good to do some more testing after you made it your changes. For example:

If you modify HP with -30 when the character has 10 HP, they should go down
f you modify HP with -30 when the character has 10 HP and max 10 HP, they should go down and die immediately because that is the rules of D&D 5e.
Increasing HP only increases until current MAX.
Modifying HP of a character whose turn it is currently not should not cause bugs or jump the player order around :)

Just mess about with the current features and try to not break any of them. I could have made this easier with unit tests but sadly I didn't write these years ago :)

sander1095 · 2022-01-24T16:16:50Z

@Malechus If I can help out with the PR or if it is overwhelming, let me know! We could even do this kind of stuff together if you'd like. Bottom line, I'd love to help you out! ;)

Malechus · 2022-01-30T03:32:52Z

I appreciate all the detailed feedback here. Give me a little while, and I will go through these and push some updates.

Adds buttons to OverviewForm to add or subtract HP. Moves Save and De…

19d9495

…lete buttons into InfoTab.

sander1095 requested changes Jan 24, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Reduce HP to Modify HP #33

Change Reduce HP to Modify HP #33

Malechus commented Jan 24, 2022

sander1095 commented Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 Jan 24, 2022

sander1095 commented Jan 24, 2022

sander1095 commented Jan 24, 2022

Malechus commented Jan 30, 2022

		@@ -295,11 +295,11 @@ private void SaveButton_Click(object sender, EventArgs e)
		{
		//Right now it is unclear if the user has entered wrong input (for example: "dsfsdf") or just didn't didnt enter anything into the Reduce HP range

Change Reduce HP to Modify HP #33

Are you sure you want to change the base?

Change Reduce HP to Modify HP #33

Conversation

Malechus commented Jan 24, 2022

sander1095 commented Jan 24, 2022

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

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

sander1095 commented Jan 24, 2022

sander1095 commented Jan 24, 2022

Malechus commented Jan 30, 2022