UnitConverter: Automatically flip units when users set one to the same type than the other #375
Conversation
|
Locking the PR since the associated Issue isn't approved yet. That way conversation on the problem being addressed is discussed in the Issue, so that future PR discussion can be limited to the implementation of the fix. |
|
rebase done |
ghost
added
the
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. Thank you for your contributions to Windows Calculator! |
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. Thank you for your contributions to Windows Calculator! |
| if ((m_valueFromUnlocalized != m_lastAnnouncedFrom | ||
| || m_valueToUnlocalized != m_lastAnnouncedTo) | ||
| && m_Unit1 != nullptr | ||
| && m_Unit2 != nullptr) |
There was a problem hiding this comment.
the method header mentions a validation against zero as well, does zero need to be added as a conditional to the if?
| { | ||
| m_lastAnnouncedFrom = m_valueFromUnlocalized; | ||
| m_lastAnnouncedTo = m_valueToUnlocalized; | ||
|
|
||
| Unit ^ unitFrom = Value1Active ? Unit1 : Unit2; | ||
| Unit ^ unitTo = (unitFrom == Unit1) ? Unit2 : Unit1; | ||
| Unit^ unitFrom = Value1Active ? m_Unit1 : m_Unit2; |
There was a problem hiding this comment.
nit: Could be out of scope but m_Unit1 and m_Unit2 don't feel like very descriptive variable names. New to the codebase so could be wrong though. Feel like something that better describes the unit may be better suited versus just numbering them.
| { | ||
| auto formerValue = m_Unit1; | ||
| m_Unit1 = value; | ||
| //Be sure m_Unit1 is set before setting Unit2 |
There was a problem hiding this comment.
Why is this validation the case? Is there some sequential logic that fires with setting these in a certain order? An extra comment line giving more context might be helpful.
| { | ||
| auto formerValue = m_Unit2; | ||
| m_Unit2 = value; | ||
| //Be sure m_Unit2 is set before setting Unit1 |
| { | ||
| m_lastAnnouncedFrom = m_valueFromUnlocalized; | ||
| m_lastAnnouncedTo = m_valueToUnlocalized; | ||
|
|
||
| Unit ^ unitFrom = Value1Active ? Unit1 : Unit2; | ||
| Unit ^ unitTo = (unitFrom == Unit1) ? Unit2 : Unit1; | ||
| Unit^ unitFrom = Value1Active ? m_Unit1 : m_Unit2; |
There was a problem hiding this comment.
Very cool that you can do null coalescing in cpp!!
| @@ -284,6 +284,8 @@ for (unsigned int k = 0; k < categoryList->Size; k++) | |||
| wstring unit1Name = vm->Unit1->Name->Data(); | |||
| for (unsigned int j = 0; j < unitList->Size; j++) | |||
| { | |||
| if (i == j) | |||
There was a problem hiding this comment.
a comment might help with understanding why i == j is a skip condition in this loop
|
We're converting Calculator to C# with a PR for the conversion coming in the next few days, so we're going to close this PR. If you want to revisit the issue, please open a new PR after the conversion has completed. |
Fixes #266
Description of the changes:
OBSERVABLE_PROPERTY_RWfor Unit1 and Unit2 by custom code.Unit1is equals to the current value ofUnit2, if it's the case, setUnit2to the former value ofUnit1.Unit1andUnit2bym_Unit1andm_Unit2when possibleHow changes were validated: