Spinboxes now correct when rounding. #6748
Conversation
Signed-off-by: John Jones-Steele <82226755+jjjoness@users.noreply.github.com>
|
@AMZN-daimini I'll have a look and see. I'll create a PR if it can. |
| QString retValue = locale.toString(value, 'f', (numDecimals > 0) ? numToStringDecimals : 0); | ||
| // If we want to truncate, not round, we add extra decimal places to the formatting | ||
| // so we can remove the last values otherwise we allow rounding | ||
| QString retValue = locale.toString(value, 'f', (numDecimals > 0) ? (truncate ? numToStringDecimals : numDecimals) : 0); |
There was a problem hiding this comment.
suggestion: Could we replace the nested ternary statement here with an if statement (or immediately invoked lambda) It just gets pretty hard to parse as it is right now
| @@ -1460,7 +1460,7 @@ QString DoubleSpinBox::stringValue(double value, bool truncated) const | |||
| numDecimals = 0; | |||
| } | |||
|
|
|||
| return toString(value, numDecimals, locale(), isGroupSeparatorShown()); | |||
| return toString(value, numDecimals, locale(), isGroupSeparatorShown(), false); | |||
There was a problem hiding this comment.
question: This is likely intended it just seems odd on first glance that we don't pass the truncated parameter on line 1450 through to this call here. Are they different kind of truncation? Could the logic here be simplified if we just pass the bool through to toString?
There was a problem hiding this comment.
They are different. The value passed in means to either truncate to the number of decimals of to the numberofDisplayDecimals. I've reworked the logic so the term to pass is the round bool.
There was a problem hiding this comment.
Okay nice one, and thanks for the explanation 👍
| @@ -39,15 +39,15 @@ namespace AzQtComponents | |||
| return AZ::Color(static_cast<float>(rgb.redF()), static_cast<float>(rgb.greenF()), static_cast<float>(rgb.blueF()), static_cast<float>(rgb.alphaF())); | |||
| } | |||
|
|
|||
| QString toString(double value, int numDecimals, const QLocale& locale, bool showGroupSeparator) | |||
| QString toString(double value, int numDecimals, const QLocale& locale, bool showGroupSeparator, bool truncate) | |||
There was a problem hiding this comment.
suggestion: It might be useful to have a couple of quick sanity tests for this function to ensure we get the expected value. They could be really simple functions with no fixture, there should hopefully be a place for them in AzQtComponents I'd have thought. Could be a separate PR but would be great to capture the expected behavior of this function given various inputs
Signed-off-by: John Jones-Steele <82226755+jjjoness@users.noreply.github.com>
…igits-o3de#5298 Signed-off-by: John Jones-Steele <82226755+jjjoness@users.noreply.github.com>
| TEST(AzQtComponents, ToStringReturnsTruncatedString) | ||
| { | ||
| double testVal = 1.2399999; | ||
| QString result = AzQtComponents::toString(testVal, 3, QLocale(), false, false); | ||
| EXPECT_TRUE(result == "1.239"); | ||
| } | ||
|
|
||
| TEST(AzQtComponents, ToStringReturnsRoundedString) | ||
| { | ||
| double testVal = 1.2399999; | ||
| QString result = AzQtComponents::toString(testVal, 3, QLocale(), false, true); | ||
| EXPECT_TRUE(result == "1.24"); | ||
| } |
There was a problem hiding this comment.
Excellent! 😄 Thanks for adding these 🙂
Signed-off-by: John Jones-Steele <82226755+jjjoness@users.noreply.github.com>
* Spinboxes now correct when rounding. Signed-off-by: John Jones-Steele <82226755+jjjoness@users.noreply.github.com> * Changes from PR Signed-off-by: John Jones-Steele <82226755+jjjoness@users.noreply.github.com> * Fixed tests after change to rounding in spinbox Signed-off-by: John Jones-Steele <82226755+jjjoness@users.noreply.github.com> Signed-off-by: Chris Burel <burelc@amazon.com>
Signed-off-by: John Jones-Steele 82226755+jjjoness@users.noreply.github.com
Fixes #5298