From af8322617fd4dfa596840e833065747799ae9d31 Mon Sep 17 00:00:00 2001
From: Seulgi Kim <seulgi.kim.56@outlook.com>
Date: Tue, 30 Jul 2019 01:39:19 +0900
Subject: [PATCH] Restore user preferences (#456)

### 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 after `UnitConverterViewModel::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_isFirstTime`**
I noticed that we are calling `RestoreUserPreferences()`  twice when Calculator starts up, and the function is restoring the same value both times

The below happens when Calculator starts up
1) On startup, in `UnitConverterViewModel::InitializeView()`, `RestoreUserPreferences()` is called.
2) `RestoreUserPreferences()` in turn triggers `OnUnitChanged()`
3) During the first call to `OnUnitChanged()`, m_IsFirstTime is `True`, so we call `RestoreUserPreferences()` again while also setting `m_IsFirstTime` to `False`.
4) `RestoreUserPreference()` again triggers `OnUnitChanged()`
5) During the second call to `OnUnitChanged()`,  m_IsFirstTime is `False`, so we call `SaveUserPreferences()`

I think we should only call `SaveUserPreferences()` inside `OnUnitChanged()` 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 delete `m_isFirstTime`.


### How changes were validated:
Manually tested that units and the current category are properly selected when you quit and start Calculator.

![GifMaker_20190414182150911](https://user-images.githubusercontent.com/3166423/56102706-88f73400-5ee3-11e9-8bbf-7a0c8e051a5c.gif)

![GifMaker_20190414183403644](https://user-images.githubusercontent.com/3166423/56102763-f0ad7f00-5ee3-11e9-99ef-3b932f587393.gif)

## Fixes #445.
---
 src/CalcManager/UnitConverter.cpp            | 14 ++++++++++++--
 src/CalcViewModel/UnitConverterViewModel.cpp | 13 ++-----------
 src/CalcViewModel/UnitConverterViewModel.h   |  2 --
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/src/CalcManager/UnitConverter.cpp b/src/CalcManager/UnitConverter.cpp
index 983ef6d..eb361fa 100644
--- a/src/CalcManager/UnitConverter.cpp
+++ b/src/CalcManager/UnitConverter.cpp
@@ -791,17 +791,27 @@ void UnitConverter::InitializeSelectedUnits()
     vector<Unit> curUnits = itr->second;
     if (!curUnits.empty())
     {
+        // Units may already have been initialized through UnitConverter::RestoreUserPreferences().
+        // Check if they have been, and if so, do not override restored units.
+        bool isFromUnitValid = m_fromType != EMPTY_UNIT && find(curUnits.begin(), curUnits.end(), m_fromType) != curUnits.end();
+        bool isToUnitValid = m_toType != EMPTY_UNIT && find(curUnits.begin(), curUnits.end(), m_toType) != curUnits.end();
+
+        if (isFromUnitValid && isToUnitValid)
+        {
+            return;
+        }
+
         bool conversionSourceSet = false;
         bool conversionTargetSet = false;
         for (const Unit& cur : curUnits)
         {
-            if (!conversionSourceSet && cur.isConversionSource)
+            if (!conversionSourceSet && cur.isConversionSource && !isFromUnitValid)
             {
                 m_fromType = cur;
                 conversionSourceSet = true;
             }
 
-            if (!conversionTargetSet && cur.isConversionTarget)
+            if (!conversionTargetSet && cur.isConversionTarget && !isToUnitValid)
             {
                 m_toType = cur;
                 conversionTargetSet = true;
diff --git a/src/CalcViewModel/UnitConverterViewModel.cpp b/src/CalcViewModel/UnitConverterViewModel.cpp
index 71faf7d..e57b0dc 100644
--- a/src/CalcViewModel/UnitConverterViewModel.cpp
+++ b/src/CalcViewModel/UnitConverterViewModel.cpp
@@ -148,7 +148,6 @@ UnitConverterViewModel::UnitConverterViewModel(const shared_ptr<UCM::IUnitConver
     Unit2AutomationName = m_localizedOutputUnitName;
     IsDecimalEnabled = true;
 
-    m_IsFirstTime = true;
     m_model->Initialize();
     PopulateData();
 }
@@ -156,7 +155,6 @@ UnitConverterViewModel::UnitConverterViewModel(const shared_ptr<UCM::IUnitConver
 void UnitConverterViewModel::ResetView()
 {
     m_model->SendCommand(UCM::Command::Reset);
-    m_IsFirstTime = true;
     OnCategoryChanged(nullptr);
 }
 
@@ -239,15 +237,8 @@ void UnitConverterViewModel::OnUnitChanged(Object ^ parameter)
         // End timer to show results immediately
         m_supplementaryResultsTimer->Cancel();
     }
-    if (!m_IsFirstTime)
-    {
-        SaveUserPreferences();
-    }
-    else
-    {
-        RestoreUserPreferences();
-        m_IsFirstTime = false;
-    }
+
+    SaveUserPreferences();
 }
 
 void UnitConverterViewModel::OnSwitchActive(Platform::Object ^ unused)
diff --git a/src/CalcViewModel/UnitConverterViewModel.h b/src/CalcViewModel/UnitConverterViewModel.h
index 8eda417..6a8761a 100644
--- a/src/CalcViewModel/UnitConverterViewModel.h
+++ b/src/CalcViewModel/UnitConverterViewModel.h
@@ -311,8 +311,6 @@ namespace CalculatorApp
             std::wstring m_valueFromUnlocalized;
             std::wstring m_valueToUnlocalized;
             bool m_relocalizeStringOnSwitch;
-            // For Saving the User Preferences only if the Unit converter ViewModel is initialised for the first time
-            bool m_IsFirstTime;
 
             Platform::String ^ m_localizedValueFromFormat;
             Platform::String ^ m_localizedValueFromDecimalFormat;