Restore user preferences#456
Conversation
|
It really improves the user experience! Thank you for taking care of that! I added 2 NIT comments, let me know what you think. |
|
Hi, gentle bump on this. Thanks! |
|
Sorry for the delay, @TurtleShip. I am looking at this, but I'm also looking through the archived history of the code around this. The code wasn't exactly this way when it was initially written, and so I'm trying to review the history to see if there were other bugs/cases that were trying to be corrected along the way that we'd be regressing by taking this change. I should have feedback on this PR next week. Thanks for your patience. |
|
Hi, @HowardWolosky |
Hey there -- apologies for the delay. Preparation for this year's BUILD conference took up a bit more time than anticipated. I should have my investigation into the history around this code completed this week. I appreciate your continued patience here. |
|
Hi, a gentle bump on this... 🙂 @HowardWolosky |
|
You're totally right on this @TurtleShip -- I apologize for the delay and radio silence here. Other things came up preventing me from finishing that investigation. I'll be prioritizing this soon and hope that we can get this merged in within the next two weeks. I apologize again and appreciate your patience here. |
HowardWolosky
left a comment
HowardWolosky
left a comment
There was a problem hiding this comment.
@TurtleShip -- I really appreciate your patience here.
The code was first introduced back in November 2014. The broken logic that you uncovered was unfortunately introduced back in May 2017 when Currency Converter was being worked on (at the time, UnitConverter::Reset() got split into UnitConverter::Reset() and UnitConverter::InitializeSelectedUnits()).
When looking through the code, I had originally expected that it would save and restore all of the last-used units across all the converters, and would therefore need to keep that Restore during OnUnitsChanged, but upon looking how SaveUserPreferences works, it only acts on the current one, which makes needing to track m_IsFirstTime irrelevant (and buggy as we've seen).
I think if you make those minor requested updates, we can get this merged in and improve the experience for all of our converter users.
Thanks so much for looking into this!
| @@ -1,4 +1,4 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
Please revert the whitespace change here.
ghost
added
the
TurtleShip
force-pushed
the
restore-user-preferences
branch
2 times, most recently
from
438651f to
760e5ac
Compare
|
Hi, @HowardWolosky I've addressed your comments. |
|
Good job Seulgi! |
HowardWolosky
left a comment
HowardWolosky
left a comment
There was a problem hiding this comment.
Excellent! Thanks for this change, and for your patience during its investigation and validation! I look forward to seeing more contributions from you in the future.
5 participants
Fixes #445.
Description of the changes:
1) Do not set units to default values if they already have valid values
This fixes the actual issue.
UnitConverter::InitializeSelectedUnits()( this function resets all units to their default units if available for the current category ) gets called afterUnitConverterViewModel::RestoreUserPreferences()( this function restores user preferences ).So Calculator has been restoring saved values, and then overriding the restored values with default values.
I modified
InitializeSelectedUnits()so that we only initialize units only when they are not already set to valid units for the current category.2) Removed
m_isFirstTimeI noticed that we are calling
RestoreUserPreferences()twice when Calculator starts up, and the function is restoring the same value both timesThe below happens when Calculator starts up
UnitConverterViewModel::InitializeView(),RestoreUserPreferences()is called.RestoreUserPreferences()in turn triggersOnUnitChanged()OnUnitChanged(), m_IsFirstTime isTrue, so we callRestoreUserPreferences()again while also settingm_IsFirstTimetoFalse.RestoreUserPreference()again triggersOnUnitChanged()OnUnitChanged(), m_IsFirstTime isFalse, so we callSaveUserPreferences()I think we should only call
SaveUserPreferences()insideOnUnitChanged()since we already restored user preferences during view initialization. I can't really think of a reason to restore units after view has been initialized. This led me to just deletem_isFirstTime.How changes were validated:
Manually tested that units and the current category are properly selected when you quit and start Calculator.